[PATCH 16/18] xfs: serialise inode reclaim within an AG

[Dave Chinner]

On Sat, Sep 25, 2010 at 07:49:29PM -0400, Christoph Hellwig wrote:
> I really don't like the way the "trylock" variable is overloaded here.
> Just add a new skipped variable for restarting the scan and otherwise
> use (flags & SYNC_TRYLOCK) directly.
> 
> > +	int			trylock = !!(flags & SYNC_TRYLOCK);
> >  
> > +restart:
> >  	ag = 0;
> >  	while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_RECLAIM_TAG))) {
> >  		unsigned long	first_index = 0;
> > @@ -837,6 +839,17 @@ xfs_reclaim_inodes_ag(
> >  
> >  		ag = pag->pag_agno + 1;
> >  
> > +		if (!mutex_trylock(&pag->pag_ici_reclaim_lock)) {
> > +			if (trylock) {
> > +				trylock++;
> > +				continue;
> > +			}
> > +			mutex_lock(&pag->pag_ici_reclaim_lock);
> > +		}
> > +
> > +		if (trylock)
> > +			first_index = pag->pag_ici_reclaim_cursor;
> 
> Also this could be made more clear by:
> 
> 	if (flags & SYNC_TRYLOCK) {
> 		if (!mutex_trylock(&pag->pag_ici_reclaim_lock)) {
> 			skipped++;
> 			continue;
> 		}
> 
> 		first_index = pag->pag_ici_reclaim_cursor;
> 	} else {
> 		mutex_lock(&pag->pag_ici_reclaim_lock);
> 	}

I'll leave the trylock variable, but add a skipped variable. The
trylock two-pass algorithm (where the second pass goes into full
blocking reclaim mode) is intentional to ensure that we block
shrinker calls when there are more shrinkers than AGs rather than
spinning just trying to get per-ag reclaim locks. Regardless, I'll
clean up the code like you suggest because it is neater.

> 
> > +
> >  		do {
> >  			struct xfs_inode *batch[XFS_LOOKUP_BATCH];
> >  			int	i;
> > @@ -889,8 +902,19 @@ xfs_reclaim_inodes_ag(
> >  
> >  		} while (nr_found && !done && *nr_to_scan > 0);
> >  
> > +		pag->pag_ici_reclaim_cursor = (done || !trylock) ? 0 : first_index;
> 
> 		if ((flags & SYNC_TRYLOCK) && !done)
> 			pag->pag_ici_reclaim_cursor = first_index;
> 		else
> 			pag->pag_ici_reclaim_cursor = 0;
> 
> > +	/*
> > +	 * if we skipped any AG, and we still have scan count remaining, do
> > +	 * another pass this time waiting on the reclaim locks.
> > +	 */
> > +	if (trylock > 1 && *nr_to_scan) {
> > +		trylock = 0;
> > +		goto restart;
> > +	}
> 
> In addition to waiting on the lock this also ignores the reclaim cursor.

Fixed the comment to indicate this is intentіonal.

Cheers,

Dave.

-- 
Dave Chinner
david at fromorbit.com