Re: [PATCH 25/26] xfs: rework unreferenced inode lookups

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-xfs@vger.kernel.org, linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
	"Paul E. McKenney" <paulmck@kernel.org>
Subject: Re: [PATCH 25/26] xfs: rework unreferenced inode lookups
Date: Sat, 12 Oct 2019 10:38:27 +1100	[thread overview]
Message-ID: <20191011233827.GP16973@dread.disaster.area> (raw)
In-Reply-To: <20191011125522.GA13167@infradead.org>

On Fri, Oct 11, 2019 at 05:55:22AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 09, 2019 at 02:21:23PM +1100, Dave Chinner wrote:
> > 	4. it xfs_ilock_nowait() fails until the rcu grace period
> 
> Should this be:
> 
> > 	4. if xfs_ilock_nowait() fails before the rcu grace period
> 
> ?
> 
> > +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >  	kmem_zone_free(xfs_inode_zone, ip);
> >  }
> >  
> > @@ -131,6 +132,7 @@ xfs_inode_free(
> >  	 * free state. The ip->i_flags_lock provides the barrier against lookup
> >  	 * races.
> >  	 */
> > +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> 
> This introduceѕ a non-owner unlock of an exclusively held rwsem.  As-is
> this will make lockdep very unhappy.  We have a non-owner unlock version
> of up_read, but not of up_write currently.  I'm also not sure if those
> are allowed from RCU callback, which IIRC can run from softirq context.
> 
> That being said this scheme of only unlocking the inode in the rcu free
> callback makes totaly sense to me, so I wish we can accomodate it
> somehow.

AFAICT it is safe to do this. Lockdep just needs to be bashed about
the head a bit to make it shut up.

> > @@ -312,7 +327,8 @@ xfs_iget_cache_hit(
> >  			rcu_read_lock();
> >  			spin_lock(&ip->i_flags_lock);
> >  			wake = !!__xfs_iflags_test(ip, XFS_INEW);
> > -			ip->i_flags &= ~(XFS_INEW | XFS_IRECLAIM);
> > +			ip->i_flags &= ~XFS_INEW | XFS_IRECLAIM;
> 
> This change looks wrong to me, or did I miss something?  We now
> clear all bits that are not XFS_I_NEW and XFS_IRECLAIM, which
> already is set in ~XFS_INEW.  So if that was the intent just:
> 
> 		ip->i_flags &= ~XFS_INEW;

Nah, I screwed up backing out a change. This line should not be
modified at all.

> 
> > + * This requires code that requires such pins to do the following under a single
> 
> This adds an > 80 char line.  (there are a few more below.
> 
> > +		/* push the AIL to clean dirty reclaimable inodes */
> > +		xfs_ail_push_all(mp->m_ail);
> > +
> > +		/* push the AIL to clean dirty reclaimable inodes */
> > +		xfs_ail_push_all(mp->m_ail);
> > +
> 
> This looks spurious vs the rest of the patch.

Looks like rebase failure fallout. I must have missed it on
cleanup. I'll sort that out.

> > +			if (__xfs_iflags_test(ip, XFS_ISTALE)) {
> > +				spin_unlock(&ip->i_flags_lock);
> > +				if (ip != free_ip)
> >  					xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > -				}
> > +				rcu_read_unlock();
> > +				continue;
> 
> This unlock out of order.  Should be harmless, but also pointless.
> 
> I think this code would be a lot easier to understand if we fatored
> this inner loop into a new helper.  Untested patch that does, and
> also removes a no incorrect comment below:

*nod*

I'll put a refacting patch at the start of the series to split this
into separate code movement and algorithm modification patches....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com