xfs
[Top] [All Lists]

Re: [PATCH 07/27] xfs: always log timestamp updates in xfs_setattr_size

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: [PATCH 07/27] xfs: always log timestamp updates in xfs_setattr_size
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 30 Jun 2011 12:10:33 +1000
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <1309385603.8649.37.camel@doink>
References: <20110629140109.003209430@xxxxxxxxxxxxxxxxxxxxxx> <20110629140337.876449102@xxxxxxxxxxxxxxxxxxxxxx> <1309385603.8649.37.camel@doink>
User-agent: Mutt/1.5.20 (2009-06-14)
On Wed, Jun 29, 2011 at 05:13:23PM -0500, Alex Elder wrote:
> On Wed, 2011-06-29 at 10:01 -0400, Christoph Hellwig wrote:
> > plain text document attachment (xfs-simplify-setattr)
> > Get rid of the special case where we use unlogged timestamp updates for
> > a truncate to the current inode size, and just call xfs_setattr_nonsize
> > for it to treat it like a utimes calls.
> 
> I'm probably just missing something, but I have a
> question below.  If it does in fact call out a
> problem, it will be easily fixed.
> 
> Reviewed-by: Alex Elder <aelder@xxxxxxx>
> 
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Index: xfs/fs/xfs/linux-2.6/xfs_iops.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/linux-2.6/xfs_iops.c    2011-06-21 11:12:11.008794493 
> > +0200
> > +++ xfs/fs/xfs/linux-2.6/xfs_iops.c 2011-06-21 11:12:22.398793917 +0200
> > @@ -773,14 +773,16 @@ xfs_setattr_size(
> >      */
> >     if (iattr->ia_size == 0 &&
> >         ip->i_size == 0 && ip->i_d.di_nextents == 0) {
> > -           xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > -           lock_flags &= ~XFS_ILOCK_EXCL;
> > -           if (mask & ATTR_CTIME) {
> > -                   inode->i_mtime = inode->i_ctime =
> > -                                   current_fs_time(inode->i_sb);
> > -                   xfs_mark_inode_dirty_sync(ip);
> > -           }
> > -           goto out_unlock;
> > +           if (!(mask & ATTR_CTIME))
> > +                   goto out_unlock;
> > +
> > +           /*
> > +            * Use the regular setattr path to update the timestamps.
> > +            */
> > +           xfs_iunlock(ip, lock_flags);
> > +           iattr->ia_valid &= ~ATTR_SIZE;
> > +           iattr->ia_valid |= ATTR_MTIME;
> 
> How/where does iattr->ia_mtime get initialized to the current
> value (I believe it would just match iattr->ia_ctime)?

notify_change() always sets both the ia_ctime and ia_mtime to the
current time unless the caller has specified ATTR_MTIME_SET, in
which case they've set a specific ia_mtime themselves. IOWs, I'm
pretty sure we can rely on both ia_mtime and ia_ctime being correct
through the ATTR_SIZE truncate path without having to touch it
here...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>