lkml.org 
[lkml]   [1998]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subject[PATCH] bug in nfsd (incorrect handling of truncate)
	Bug in nfsd: nfsd_setattr() doesn't do vmtruncate() and doesn't
touch the semaphore on inode it's truncating. I rewrote it (monkeyed up
from nfsd_truncate and do_truncate). As the result we've got the following
situation: foo->i_op->truncate being called _only_ after vmtruncate()
preceeded by notify_change() with ATTR_SIZE set and all calls of
notify_change() with ATTR_SIZE set are followed by this sequence
(nfsd_setattr was the last offender). Thus I've moved vmtruncate()/
foo->i_op->truncate() calls in the end of notify_change().
Affected functions: notify_change(), do_truncate(), nfsd_setattr(),
nfsd_truncate(). From the filesystem drivers' POV nothing changed.
Patch (against 2.1.128-pre1, but relevant files didn't change
between 2.1.128-pre1 and 2.1.128) attached.

If nobody will object I'll do the following: move the vmtruncate
and i_op->truncate calls into inode_set_attr(). It will mean that
1) nfs, coda, ncpfs and smbfs will have to call vmtruncate() by hands _or_
we'll do it from notify_change() for them - they can be distinguished from
those who call inode_setattr() since they are only ones who have
i_op->notify_change and don't have i_op->truncate() (yes, ugly hack, but...)

2) umsdos handling of notify_change method will be slightly modified (locking).

3) we'll be able to do truncate right, i.e. make it return meaningful error
codes when something fails and avoid the situation when truncate fails but
the i_size remains changed. That will mean that truncate becomes
int (*truncate)(struct inode*, off_t size) instead of void (*truncate)(struct
inode*). Yes, it means changing the interface, but the changes to filesystem
drivers would be minimal - to emulate current behaviour one has to replace
void frobfs_truncate(struct inode *inode)
{
bar();
}
with
int frobfs_truncate(struct inode *inode, off_t size)
{
inode->i_size = size;
bar();
return 0;
}
We should do it for weird filesystems anyway - since we are dealing with
FAT and its ugly brethren we have situations when size-increasing truncate
is impossible. Right now sys_truncate happily returns 0 in such cases. And
there always are IO errors, even for sane filesystems.
Cheers,
Al
PS: Apologies for missed definitions in minix_fs.h ;-<<
--
There are no "civil aviation for dummies" books out there and most of
you would probably be scared and spend a lot of your time looking up
if there was one. :-) Jordan Hubbard in c.u.b.f.m

--- fs/attr.c.old Fri Nov 13 05:05:29 1998
+++ fs/attr.c Fri Nov 13 05:10:21 1998
@@ -87,6 +87,7 @@
int error;
time_t now = CURRENT_TIME;
unsigned int ia_valid = attr->ia_valid;
+ unsigned long size = attr->ia_size;

attr->ia_ctime = now;
if (!(ia_valid & ATTR_ATIME_SET))
@@ -101,6 +102,11 @@
error = inode_change_ok(inode, attr);
if (!error)
inode_setattr(inode, attr);
+ }
+ if (!error && (ia_valid & ATTR_SIZE)) {
+ vmtruncate(inode, size);
+ if (inode->i_op && inode->i_op->truncate)
+ inode->i_op->truncate(inode);
}
return error;
}
--- fs/open.c.old Fri Nov 13 05:07:37 1998
+++ fs/open.c Fri Nov 13 05:11:10 1998
@@ -87,12 +87,6 @@
newattrs.ia_size = length;
newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
error = notify_change(dentry, &newattrs);
- if (!error) {
- /* truncate virtual mappings of this file */
- vmtruncate(inode, length);
- if (inode->i_op && inode->i_op->truncate)
- inode->i_op->truncate(inode);
- }
up(&inode->i_sem);
return error;
}
--- fs/nfsd/vfs.c.old Fri Nov 13 02:24:39 1998
+++ fs/nfsd/vfs.c Fri Nov 13 05:13:57 1998
@@ -232,29 +232,6 @@
dentry = fhp->fh_dentry;
inode = dentry->d_inode;

- /* The size case is special... */
- if (iap->ia_valid & ATTR_SIZE) {
-if (!S_ISREG(inode->i_mode))
-printk("nfsd_setattr: size change??\n");
- if (iap->ia_size < inode->i_size) {
- err = nfsd_permission(fhp->fh_export, dentry, MAY_TRUNC);
- if (err != 0)
- goto out;
- }
- err = get_write_access(inode);
- if (err)
- goto out_nfserr;
- /* N.B. Should we update the inode cache here? */
- inode->i_size = iap->ia_size;
- if (inode->i_op && inode->i_op->truncate)
- inode->i_op->truncate(inode);
- mark_inode_dirty(inode);
- put_write_access(inode);
- iap->ia_valid &= ~ATTR_SIZE;
- iap->ia_valid |= ATTR_MTIME;
- iap->ia_mtime = CURRENT_TIME;
- }
-
imode = inode->i_mode;
if (iap->ia_valid & ATTR_MODE) {
iap->ia_mode &= S_IALLUGO;
@@ -273,11 +250,31 @@
iap->ia_mode = imode &= ~S_ISGID;
}

+ /* The size case is special... */
+ if (iap->ia_valid & ATTR_SIZE) {
+if (!S_ISREG(inode->i_mode))
+printk("nfsd_setattr: size change??\n");
+ if (iap->ia_size < inode->i_size) {
+ err = nfsd_permission(fhp->fh_export, dentry, MAY_TRUNC);
+ if (err != 0)
+ goto out;
+ }
+ iap->ia_valid |= ATTR_MTIME;
+ }
+
/* Change the attributes. */
if (iap->ia_valid) {
iap->ia_valid |= ATTR_CTIME;
- iap->ia_ctime = CURRENT_TIME;
- err = notify_change(dentry, iap);
+ if (iap->ia_valid & ATTR_SIZE) {
+ err = get_write_access(inode);
+ if (err)
+ goto out_nfserr;
+ fh_lock(fhp);
+ err = notify_change(dentry, iap);
+ fh_unlock(fhp);
+ put_write_access(inode);
+ } else
+ err = notify_change(dentry, iap);
if (err)
goto out_nfserr;
if (EX_ISSYNC(fhp->fh_export))
@@ -737,11 +734,6 @@
newattrs.ia_size = size;
newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
err = notify_change(dentry, &newattrs);
- if (!err) {
- vmtruncate(inode, size);
- if (inode->i_op && inode->i_op->truncate)
- inode->i_op->truncate(inode);
- }
put_write_access(inode);
DQUOT_DROP(inode);
fh_unlock(fhp);
\
 
 \ /
  Last update: 2005-03-22 13:45    [W:0.021 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site