[Top] [All Lists]

Re: [PATCH 07/16] xfs: convert inode cache lookups to use RCU locking

To: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH 07/16] xfs: convert inode cache lookups to use RCU locking
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 9 Nov 2010 16:04:17 +1100
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx, eric.dumazet@xxxxxxxxx
In-reply-to: <20101109033628.GN4032@xxxxxxxxxxxxxxxxxx>
References: <1289206519-18377-1-git-send-email-david@xxxxxxxxxxxxx> <1289206519-18377-8-git-send-email-david@xxxxxxxxxxxxx> <20101108230929.GA13299@xxxxxxxxxxxxx> <20101109033628.GN4032@xxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Mon, Nov 08, 2010 at 07:36:28PM -0800, Paul E. McKenney wrote:
> On Mon, Nov 08, 2010 at 06:09:29PM -0500, Christoph Hellwig wrote:
> > This patch generally looks good to me, but with so much RCU magic I'd prefer
> > if Paul & Eric could look over it.
> Is there a git tree, tarball, or whatever? 

git://git.kernel.org/pub/scm/linux/kernel/git/dgc/xfsdev.git working

contains the series that this patch is in.

> For example, I don't see
> how this patch handles the case of an inode being freed just as an RCU
> reader gains a reference to it,

XFS_IRECLAIM flag is set on inodes as they transition into the
reclaim state long before they are freed. The XFS_IRECLAIM flag is left there 
freed. Hence lookups in xfs_iget_cache_hit() will see this.

If the inode has been reallocated, the inode number will not yet be
set, or the inode state will have changed to XFS_INEW, both of which
xfs_iget_cache_hit() will also reject.

> but then reallocated as some other inode
> (so that ->ino is nonzero) before the RCU reader gets a chance to actually
> look at the inode.

XFS_INEW is not cleared until well after a new ->i_ino is set, so
the lookup should find trip over XFS_INEW in that case. I think that
I may need to move the inode number check under the i_flags_lock
after validating the flags - more to check that we've got the
correct inode than to validate we have a freed inode.

> But such a check might well be in the code that this
> patch didn't change...

Yeah, most of the XFS code is already in a form compatible with such
RCU use because inodes have always had a quiescent "reclaimable"
state between active and reclaim (XFS_INEW -> active ->
XFS_IRECLAIMABLE -> XFS_IRECLAIM) where the inode can be reused
before being freed. The result is that lookups have always had to
handle races with inodes that have just transitioned into the
XFS_IRECLAIM state and hence cannot be immediately reused...


Dave Chinner

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