Messages in this thread Patch in this message | | | Date | Fri, 13 Nov 1998 11:09:38 -0500 (EST) | From | Alexander Viro <> | 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); | |