Re: [PATCH 28/28] xfs: rework unreferenced inode lookups

[Brian Foster <bfoster@redhat.com>]

On Fri, Nov 01, 2019 at 10:46:18AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Looking up an unreferenced inode in the inode cache is a bit hairy.
> We do this for inode invalidation and writeback clustering purposes,
> which is all invisible to the VFS. Hence we can't take reference
> counts to the inode and so must be very careful how we do it.
> 
> There are several different places that all do the lookups and
> checks slightly differently. Fundamentally, though, they are all
> racy and inode reclaim has to block waiting for the inode lock if it
> loses the race. This is not very optimal given all the work we;ve
> already done to make reclaim non-blocking.
> 
> We can make the reclaim process nonblocking with a couple of simple
> changes. If we define the unreferenced lookup process in a way that
> will either always grab an inode in a way that reclaim will notice
> and skip, or will notice a reclaim has grabbed the inode so it can
> skip the inode, then there is no need for reclaim to need to cycle
> the inode ILOCK at all.
> 
> Selecting an inode for reclaim is already non-blocking, so if the
> ILOCK is held the inode will be skipped. If we ensure that reclaim
> holds the ILOCK until the inode is freed, then we can do the same
> thing in the unreferenced lookup to avoid inodes in reclaim. We can
> do this simply by holding the ILOCK until the RCU grace period
> expires and the inode freeing callback is run. As all unreferenced
> lookups have to hold the rcu_read_lock(), we are guaranteed that
> a reclaimed inode will be noticed as the trylock will fail.
> 
...
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/mrlock.h     |  27 +++++++++
>  fs/xfs/xfs_icache.c |  88 +++++++++++++++++++++--------
>  fs/xfs/xfs_inode.c  | 131 +++++++++++++++++++++-----------------------
>  3 files changed, 153 insertions(+), 93 deletions(-)
> 
> diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
> index 79155eec341b..1752a2592bcc 100644
> --- a/fs/xfs/mrlock.h
> +++ b/fs/xfs/mrlock.h
...
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 11bf4768d491..45ee3b5cd873 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -106,6 +106,7 @@ xfs_inode_free_callback(
>  		ip->i_itemp = NULL;
>  	}
>  
> +	mrunlock_excl_non_owner(&ip->i_lock);
>  	kmem_zone_free(xfs_inode_zone, ip);
>  }
>  
> @@ -132,6 +133,7 @@ xfs_inode_free(
>  	 * free state. The ip->i_flags_lock provides the barrier against lookup
>  	 * races.
>  	 */
> +	mrupdate_non_owner(&ip->i_lock);

Can we tie these into the proper locking interface using flags? For
example, something like xfs_ilock(ip, XFS_ILOCK_EXCL|XFS_ILOCK_NONOWNER)
or xfs_ilock(ip, XFS_ILOCK_EXCL_NONOWNER) perhaps?

>  	spin_lock(&ip->i_flags_lock);
>  	ip->i_flags = XFS_IRECLAIM;
>  	ip->i_ino = 0;
> @@ -295,11 +297,24 @@ xfs_iget_cache_hit(
>  		}
>  
>  		/*
> -		 * We need to set XFS_IRECLAIM to prevent xfs_reclaim_inode
> -		 * from stomping over us while we recycle the inode. Remove it
> -		 * from the LRU straight away so we can re-init the VFS inode.
> +		 * Before we reinitialise the inode, we need to make sure
> +		 * reclaim does not pull it out from underneath us. We already
> +		 * hold the i_flags_lock, and because the XFS_IRECLAIM is not
> +		 * set we know the inode is still on the LRU. However, the LRU
> +		 * code may have just selected this inode to reclaim, so we need
> +		 * to ensure we hold the i_flags_lock long enough for the
> +		 * trylock in xfs_inode_reclaim_isolate() to fail. We do this by
> +		 * removing the inode from the LRU, which will spin on the LRU
> +		 * list locks until reclaim stops walking, at which point we
> +		 * know there is no possible race between reclaim isolation and
> +		 * this lookup.
> +		 *

Somewhat related to my question about the lru_lock on the earlier patch.

> +		 * We also set the XFS_IRECLAIM flag here while trying to do the
> +		 * re-initialisation to prevent multiple racing lookups on this
> +		 * inode from all landing here at the same time.
>  		 */
>  		ip->i_flags |= XFS_IRECLAIM;
> +		list_lru_del(&mp->m_inode_lru, &inode->i_lru);
>  		spin_unlock(&ip->i_flags_lock);
>  		rcu_read_unlock();
>  
...
> @@ -1022,19 +1076,7 @@ xfs_dispose_inode(
>  	spin_unlock(&pag->pag_ici_lock);
>  	xfs_perag_put(pag);
>  
> -	/*
> -	 * Here we do an (almost) spurious inode lock in order to coordinate
> -	 * with inode cache radix tree lookups.  This is because the lookup
> -	 * can reference the inodes in the cache without taking references.
> -	 *
> -	 * We make that OK here by ensuring that we wait until the inode is
> -	 * unlocked after the lookup before we go ahead and free it.
> -	 *
> -	 * XXX: need to check this is still true. Not sure it is.
> -	 */
> -	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_qm_dqdetach(ip);
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);

Ok, so I'm staring at this a bit more and think I'm missing something.
If we put aside the change to hold ilock until the inode is freed, we
basically have the following (simplified) flow as the inode goes from
isolation to disposal:

	ilock	(isolate)
	iflock
	set XFS_IRECLAIM
	ifunlock (disposal)
	iunlock
	radix delete
	ilock cycle (drain)
	rcu free

What we're trying to eliminate is the ilock cycle to drain any
concurrent unreferenced lookups from accessing the inode once it is
freed. The free itself is still RCU protected.

Looking over at the ifree path, we now have something like this:

	rcu_read_lock()
	radix lookup
	check XFS_IRECLAIM
	ilock
	if XFS_ISTALE, skip
	set XFS_ISTALE
	rcu_read_unlock()
	iflock
	/* return locked down inode */

Given that we set XFS_IRECLAIM under ilock, would we still need either
the ilock cycle or to hold ilock through the RCU free if the ifree side
(re)checked XFS_IRECLAIM after it has the ilock (but before it drops the
rcu read lock)? ISTM we should either have a non-reclaim inode with
ilock protection or a reclaim inode with RCU protection (so we can skip
it before it frees), but I could easily be missing something here..

>  
>  	__xfs_inode_free(ip);
>  }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 33edb18098ca..5c0be82195fc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2538,60 +2538,63 @@ xfs_ifree_get_one_inode(
>  	if (!ip)
>  		goto out_rcu_unlock;
>  
> +

Extra whitespace here.

> +	spin_lock(&ip->i_flags_lock);
> +	if (!ip->i_ino || ip->i_ino != inum ||
> +	    __xfs_iflags_test(ip, XFS_IRECLAIM))
> +		goto out_iflags_unlock;
> +
>  	/*
> -	 * because this is an RCU protected lookup, we could find a recently
> -	 * freed or even reallocated inode during the lookup. We need to check
> -	 * under the i_flags_lock for a valid inode here. Skip it if it is not
> -	 * valid, the wrong inode or stale.
> +	 * We've got the right inode and it isn't in reclaim but it might be
> +	 * locked by someone else.  In that case, we retry the inode rather than
> +	 * skipping it completely as we have to process it with the cluster
> +	 * being freed.
>  	 */
> -	spin_lock(&ip->i_flags_lock);
> -	if (ip->i_ino != inum || __xfs_iflags_test(ip, XFS_ISTALE)) {
> +	if (ip != free_ip && !xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
>  		spin_unlock(&ip->i_flags_lock);
> -		goto out_rcu_unlock;
> +		rcu_read_unlock();
> +		delay(1);
> +		goto retry;
>  	}
> -	spin_unlock(&ip->i_flags_lock);
>  
>  	/*
> -	 * Don't try to lock/unlock the current inode, but we _cannot_ skip the
> -	 * other inodes that we did not find in the list attached to the buffer
> -	 * and are not already marked stale. If we can't lock it, back off and
> -	 * retry.
> +	 * Inode is now pinned against reclaim until we unlock it. If the inode
> +	 * is already marked stale, then it has already been flush locked and
> +	 * attached to the buffer so we don't need to do anything more here.
>  	 */
> -	if (ip != free_ip) {
> -		if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
> -			rcu_read_unlock();
> -			delay(1);
> -			goto retry;
> -		}
> -
> -		/*
> -		 * Check the inode number again in case we're racing with
> -		 * freeing in xfs_reclaim_inode().  See the comments in that
> -		 * function for more information as to why the initial check is
> -		 * not sufficient.
> -		 */
> -		if (ip->i_ino != inum) {
> +	if (__xfs_iflags_test(ip, XFS_ISTALE)) {

Is there a correctness reason for why we move the stale check to under
ilock (in both iflush/ifree)?

> +		if (ip != free_ip)
>  			xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -			goto out_rcu_unlock;
> -		}
> +		goto out_iflags_unlock;
>  	}
> +	__xfs_iflags_set(ip, XFS_ISTALE);
> +	spin_unlock(&ip->i_flags_lock);
>  	rcu_read_unlock();
>  
> +	/*
> +	 * The flush lock will now hold off inode reclaim until the buffer
> +	 * completion routine runs the xfs_istale_done callback and unlocks the
> +	 * flush lock. Hence the caller can safely drop the ILOCK when it is
> +	 * done attaching the inode to the cluster buffer.
> +	 */
>  	xfs_iflock(ip);
> -	xfs_iflags_set(ip, XFS_ISTALE);
>  
>  	/*
> -	 * We don't need to attach clean inodes or those only with unlogged
> -	 * changes (which we throw away, anyway).
> +	 * We don't need to attach clean inodes to the buffer - they are marked
> +	 * stale in memory now and will need to be re-initialised by inode
> +	 * allocation before they can be reused.
>  	 */
>  	if (!ip->i_itemp || xfs_inode_clean(ip)) {
>  		ASSERT(ip != free_ip);
>  		xfs_ifunlock(ip);
> -		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +		if (ip != free_ip)
> +			xfs_iunlock(ip, XFS_ILOCK_EXCL);

There's an assert against this case just above, though I suppose there's
nothing wrong with just keeping it and making the functional code more
cautious.

Brian

>  		goto out_no_inode;
>  	}
>  	return ip;
>  
> +out_iflags_unlock:
> +	spin_unlock(&ip->i_flags_lock);
>  out_rcu_unlock:
>  	rcu_read_unlock();
>  out_no_inode:
> @@ -3519,44 +3522,40 @@ xfs_iflush_cluster(
>  			continue;
>  
>  		/*
> -		 * because this is an RCU protected lookup, we could find a
> -		 * recently freed or even reallocated inode during the lookup.
> -		 * We need to check under the i_flags_lock for a valid inode
> -		 * here. Skip it if it is not valid or the wrong inode.
> +		 * See xfs_dispose_inode() for an explanation of the
> +		 * tests here to avoid inode reclaim races.
>  		 */
>  		spin_lock(&cip->i_flags_lock);
>  		if (!cip->i_ino ||
> -		    __xfs_iflags_test(cip, XFS_ISTALE)) {
> +		    __xfs_iflags_test(cip, XFS_IRECLAIM)) {
>  			spin_unlock(&cip->i_flags_lock);
>  			continue;
>  		}
>  
> -		/*
> -		 * Once we fall off the end of the cluster, no point checking
> -		 * any more inodes in the list because they will also all be
> -		 * outside the cluster.
> -		 */
> +		/* ILOCK will pin the inode against reclaim */
> +		if (!xfs_ilock_nowait(cip, XFS_ILOCK_SHARED)) {
> +			spin_unlock(&cip->i_flags_lock);
> +			continue;
> +		}
> +
> +		if (__xfs_iflags_test(cip, XFS_ISTALE)) {
> +			xfs_iunlock(cip, XFS_ILOCK_SHARED);
> +			spin_unlock(&cip->i_flags_lock);
> +			continue;
> +		}
> +
> +		/* Lookup can find inodes outside the cluster being flushed. */
>  		if ((XFS_INO_TO_AGINO(mp, cip->i_ino) & mask) != first_index) {
> +			xfs_iunlock(cip, XFS_ILOCK_SHARED);
>  			spin_unlock(&cip->i_flags_lock);
>  			break;
>  		}
>  		spin_unlock(&cip->i_flags_lock);
>  
>  		/*
> -		 * Do an un-protected check to see if the inode is dirty and
> -		 * is a candidate for flushing.  These checks will be repeated
> -		 * later after the appropriate locks are acquired.
> -		 */
> -		if (xfs_inode_clean(cip) && xfs_ipincount(cip) == 0)
> -			continue;
> -
> -		/*
> -		 * Try to get locks.  If any are unavailable or it is pinned,
> +		 * If we can't get the flush lock now or the inode is pinned,
>  		 * then this inode cannot be flushed and is skipped.
>  		 */
> -
> -		if (!xfs_ilock_nowait(cip, XFS_ILOCK_SHARED))
> -			continue;
>  		if (!xfs_iflock_nowait(cip)) {
>  			xfs_iunlock(cip, XFS_ILOCK_SHARED);
>  			continue;
> @@ -3567,22 +3566,9 @@ xfs_iflush_cluster(
>  			continue;
>  		}
>  
> -
>  		/*
> -		 * Check the inode number again, just to be certain we are not
> -		 * racing with freeing in xfs_reclaim_inode(). See the comments
> -		 * in that function for more information as to why the initial
> -		 * check is not sufficient.
> -		 */
> -		if (!cip->i_ino) {
> -			xfs_ifunlock(cip);
> -			xfs_iunlock(cip, XFS_ILOCK_SHARED);
> -			continue;
> -		}
> -
> -		/*
> -		 * arriving here means that this inode can be flushed.  First
> -		 * re-check that it's dirty before flushing.
> +		 * Arriving here means that this inode can be flushed. First
> +		 * check that it's dirty before flushing.
>  		 */
>  		if (!xfs_inode_clean(cip)) {
>  			int	error;
> @@ -3596,6 +3582,7 @@ xfs_iflush_cluster(
>  			xfs_ifunlock(cip);
>  		}
>  		xfs_iunlock(cip, XFS_ILOCK_SHARED);
> +		/* unsafe to reference cip from here */
>  	}
>  
>  	if (clcount) {
> @@ -3634,7 +3621,11 @@ xfs_iflush_cluster(
>  
>  	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
>  
> -	/* abort the corrupt inode, as it was not attached to the buffer */
> +	/*
> +	 * Abort the corrupt inode, as it was not attached to the buffer. It is
> +	 * unlocked, but still pinned against reclaim by the flush lock so it is
> +	 * safe to reference here until after the flush abort completes.
> +	 */
>  	xfs_iflush_abort(cip, false);
>  	kmem_free(cilist);
>  	xfs_perag_put(pag);
> -- 
> 2.24.0.rc0
>