xfs
[Top] [All Lists]

Re: [PATCH 4/4] xfs: open code inc_inode_iversion when logging an inode

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/4] xfs: open code inc_inode_iversion when logging an inode
From: Ben Myers <bpm@xxxxxxx>
Date: Tue, 1 Oct 2013 18:04:34 -0500
Cc: Eric Sandeen <sandeen@xxxxxxxxxxx>, Jean Noel Cordenner <jean-noel.cordenner@xxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131001111236.GQ12541@dastard>
References: <1380497826-13474-1-git-send-email-david@xxxxxxxxxxxxx> <1380497826-13474-5-git-send-email-david@xxxxxxxxxxxxx> <5249FA36.1070609@xxxxxxxxxxx> <20130930223946.GQ1935@xxxxxxx> <20131001111236.GQ12541@dastard>
User-agent: Mutt/1.5.20 (2009-06-14)
Hi Dave,

On Tue, Oct 01, 2013 at 09:12:36PM +1000, Dave Chinner wrote:
> On Mon, Sep 30, 2013 at 05:39:46PM -0500, Ben Myers wrote:
> > On Mon, Sep 30, 2013 at 05:24:54PM -0500, Eric Sandeen wrote:
> > > On 9/29/13 6:37 PM, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > > > 
> > > > Michael L Semon reported that generic/069 runtime increased on v5
> > > > superblocks by 100% compared to v4 superblocks. his perf-based
> > > > analysis pointed directly at the timestamp updates being done by the
> > > > write path in this workload. The append writers are doing 4-byte
> > > > writes, so there are lots of timestamp updates occurring.
> ...
> > > > diff --git a/fs/xfs/xfs_trans_inode.c b/fs/xfs/xfs_trans_inode.c
> > > > index 53dfe46..e6601c1 100644
> > > > --- a/fs/xfs/xfs_trans_inode.c
> > > > +++ b/fs/xfs/xfs_trans_inode.c
> > > > @@ -118,8 +118,7 @@ xfs_trans_log_inode(
> > > >          */
> > > >         if (!(ip->i_itemp->ili_item.li_desc->lid_flags & XFS_LID_DIRTY) 
> > > > &&
> > > >             IS_I_VERSION(VFS_I(ip))) {
> > > > -               inode_inc_iversion(VFS_I(ip));
> > > > -               ip->i_d.di_changecount = VFS_I(ip)->i_version;
> > > 
> > > comment about the reason for the open-code might be good, too?
> 
> Sure, I can add that.
> 
> > > otherwise some semantic patcher might "fix" it for you again later...
> > > 
> > > -Eric
> > > 
> > > > +               ip->i_d.di_changecount = ++VFS_I(ip)->i_version;
> > > >                 flags |= XFS_ILOG_CORE;
> > > >         }
> > > >  
> > > > 
> > 
> > Adding a comment strikes me as a good idea too... But isn't that lock there 
> > for
> > a reason?  I suspect that will break i_version like i_size on 32 bit 
> > systems.
> > Jean added this function, hopefully he can shed some light.
> 
> I can't see how there's a 32 bit issue here - i_version is always
> read unlocked, and so if you're worried about a 32 bit system doing
> 2 32 bit reads to read the 64 bit value and seeing values on
> different sides of the increment, then we've already got that
> problem *everywhere*.

I think if we had the 32 bit issues with i_size, the same is likely true here.
You're not making it any worse, AFAICT.

> i.e. the only place that i_version is
> protected by i_lock is in inode_inc_iversion() - nowhere else is
> that lock used at all when reading or writing i_version.

Seems like if nobody is taking the i_lock when reading i_version, it's not
really providing the protection that was intended.  Weird.

> A quick grep points out that ext2/3/4 directory code all update and
> read i_version without using the i_lock - they are all serialised by
> the directory locks that are held. Ceph, exofs, ocfs2, ecryptfs,
> affs, fat, etc all do similar things with inode->i_version. 
> 
> So if the intention is to make i_version safe on 32 bit systems,
> then it's failed.

Agreed.

> The only thing it does in inode_inc_iversion is
> serialise other updates that aren't done under some exclusive inode
> locks, and all the XFS updates are done either under the i_mutex
> and/or the i_ilock, so I don't think there is any problem with
> racing occurring here...

I'll take another look at it with that in mind.

Thanks,
        Ben

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