Messages in this thread Patch in this message | | | Subject | Re: [PATCH against 2.6.14] truncate() or ftruncate shouldn't change mtime if size doesn't change. | From | Anton Altaparmakov <> | Date | Mon, 31 Oct 2005 10:16:59 +0000 |
| |
On Mon, 2005-10-31 at 09:48 +0000, Anton Altaparmakov wrote: > On Sun, 2005-10-30 at 23:48 -0800, Andrew Morton wrote: > > NeilBrown <neilb@suse.de> wrote: > > > According to Posix and SUS, truncate(2) and ftruncate(2) only update > > > ctime and mtime if the size actually changes. Linux doesn't currently > > > obey this. > > > > > > There is no need to test the size under i_sem, as loosing any race > > > will not make a noticable different the mtime or ctime. > > > > Well if there's a race then the file may end up not being truncated after > > this patch is applied. But that could have happened anwyay, so I don't see > > a need for i_sem synchronisation either. > > > > > (According to SUS, truncate and ftruncate 'may' clear setuid/setgid > > > as well, currently we don't. Should we? > > > ) > > > > > > > > > Signed-off-by: Neil Brown <neilb@suse.de> > > > > > > ### Diffstat output > > > ./fs/open.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff ./fs/open.c~current~ ./fs/open.c > > > --- ./fs/open.c~current~ 2005-10-31 16:22:44.000000000 +1100 > > > +++ ./fs/open.c 2005-10-31 16:22:44.000000000 +1100 > > > @@ -260,7 +260,8 @@ static inline long do_sys_truncate(const > > > goto dput_and_out; > > > > > > error = locks_verify_truncate(inode, NULL, length); > > > - if (!error) { > > > + if (!error && > > > + length != i_size_read(dentry->d_inode)) { > > > > Odd code layout? > > > > > DQUOT_INIT(inode); > > > error = do_truncate(nd.dentry, length); > > > } > > > @@ -313,7 +314,8 @@ static inline long do_sys_ftruncate(unsi > > > goto out_putf; > > > > > > error = locks_verify_truncate(inode, file, length); > > > - if (!error) > > > + if (!error && > > > + length != i_size_read(dentry->d_inode)) > > > error = do_truncate(dentry, length); > > > out_putf: > > > fput(file); > > > > This partially obsoletes the similar optimisation in inode_setattr(). I > > guess the optimisation there retains some usefulness for O_TRUNC opens of > > zero-length files, but for symettry and micro-efficiency, perhaps we should > > remvoe the inode_setattr() test and check for i_size==0 in may_open()? > > Sounds like a good idea. That does simplify inode_setattr() nicely...
D'oh! I forgot to append the may_open diff! Below is full patch...
> Signed-off-by: Anton Altaparmakov <aia21@cantab.net> > > --- linux-2.6/fs/attr.c.old 2005-10-31 09:29:38.000000000 +0000 > +++ linux-2.6/fs/attr.c 2005-10-31 09:30:39.000000000 +0000 > @@ -70,19 +70,10 @@ int inode_setattr(struct inode * inode, > int error = 0; > > if (ia_valid & ATTR_SIZE) { > - if (attr->ia_size != i_size_read(inode)) { > - error = vmtruncate(inode, attr->ia_size); > - if (error || (ia_valid == ATTR_SIZE)) > - goto out; > - } else { > - /* > - * We skipped the truncate but must still update > - * timestamps > - */ > - ia_valid |= ATTR_MTIME|ATTR_CTIME; > - } > + error = vmtruncate(inode, attr->ia_size); > + if (error || (ia_valid == ATTR_SIZE)) > + goto out; > } > - > if (ia_valid & ATTR_UID) > inode->i_uid = attr->ia_uid; > if (ia_valid & ATTR_GID) > > btw. Is it actually correct that we "goto out;" when "ia_valid == > ATTR_SIZE"? That way we skip the mark_inode_dirty() call just before > the "out" label... > > For ntfs at least that is fine because ntfs does an > "inode_update_time(inode, 1)" unconditionally in ntfs_truncate() even > when the size has not changed which calls mark_inode_dirty_sync() and > when the size changes it also does a "__mark_inode_dirty(inode, > I_DIRTY_SYNC | I_DIRTY_DATASYNC);" but I am not sure all filesystems are > fine in that respect?
Best regards,
Anton -- Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @) Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
---
From: Anton Altaparmakov <aia21@cantab.net>
In may_open() only truncate when O_TRUNC is set and the file size is not zero.
And in inode_setattr() always do the truncate as we catch the file size not changing case earlier. (And on a race condition the filesystem will notice that the size is not changing...)
Signed-off-by: Anton Altaparmakov <aia21@cantab.net>
--- linux-2.6/fs/attr.c.old 2005-10-31 09:29:38.000000000 +0000 +++ linux-2.6/fs/attr.c 2005-10-31 09:30:39.000000000 +0000 @@ -70,19 +70,10 @@ int inode_setattr(struct inode * inode, int error = 0; if (ia_valid & ATTR_SIZE) { - if (attr->ia_size != i_size_read(inode)) { - error = vmtruncate(inode, attr->ia_size); - if (error || (ia_valid == ATTR_SIZE)) - goto out; - } else { - /* - * We skipped the truncate but must still update - * timestamps - */ - ia_valid |= ATTR_MTIME|ATTR_CTIME; - } + error = vmtruncate(inode, attr->ia_size); + if (error || (ia_valid == ATTR_SIZE)) + goto out; } - if (ia_valid & ATTR_UID) inode->i_uid = attr->ia_uid; if (ia_valid & ATTR_GID) --- linux-2.6/fs/namei.c.old 2005-10-31 09:28:38.000000000 +0000 +++ linux-2.6/fs/namei.c 2005-10-31 09:30:39.000000000 +0000 @@ -1447,7 +1447,7 @@ int may_open(struct nameidata *nd, int a if (error) return error; - if (flag & O_TRUNC) { + if (flag & O_TRUNC && i_size_read(inode)) { error = get_write_access(inode); if (error) return error;
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |