Re: [PATCH against 2.6.14] truncate() or ftruncate shouldn't change mtime if size doesn't change.

[Anton Altaparmakov <aia21@cam.ac.uk>]

On Mon, 2005-10-31 at 21:30 +1100, Neil Brown wrote:
> On Monday October 31, aia21@cam.ac.uk wrote:
> > > 
> > > 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...
> > 
> > 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?
> 
> I think the 'goto' is fine as presumably an error from vmtruncate
> means that nothing was changed, and as there are no other attr bits,
> there is no need to mark_inode_dirty.

But is it not "and" it is "or", so when there is no error, i.e. changes
have happened, and no other attr bits are set, the inode is not marked
dirty.  And if a filesystem does not mark it dirty itself, the inode
will not get marked dirty at all and strange things may result...

> However, there always will be other ATTR bits as whenever we set
> ATTR_SIZE (do_truncate and nfsd_setattr) we also set ATTR_CTIME,
> so this "optimisation" is just a waste of space.
> Best make it:
> 
> >  	if (ia_valid & ATTR_SIZE)
> > 		error = vmtruncate(inode, attr->ia_size);
> 
> and assume that the filesystem's ->truncate or ->setattr will still
> set mtime if the size doesn't change (->truncate would have a hard
> time knowing if it has changed or not, so it has to set mtime
> unconditionally if it exists .. ->setattr... probably does the right
> thing)

That is possible.  It does change the behaviour though.  Now you always
get the inode marked dirty where as before you did not.  Mind you I
think it is possibly wrong now so changing it as you suggest would
definitely make it right.

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/