Re: [PATCH 1/7] xfs: use a normal shrinker for the dquot freelist

[Dave Chinner <david@fromorbit.com>]

On Thu, Feb 09, 2012 at 05:56:26PM -0500, Christoph Hellwig wrote:
> On Thu, Feb 09, 2012 at 04:03:20PM -0600, Ben Myers wrote:
> > > +	LIST_HEAD		(dispose_list);
> > > +	struct xfs_dquot	*dqp;
> > >  
> > > -	if (nfree <= ndqused && nfree < ndquot)
> > > +	if ((sc->gfp_mask & (__GFP_FS|__GFP_WAIT)) != (__GFP_FS|__GFP_WAIT))
> > >  		return 0;
> > > +	if (!nr_to_scan)
> > > +		goto out;
> > 
> > I suggest something more like:
> > 
> > 	if (!nr_to_scan)
> > 		goto out;
> >         if ((sc->gfp_mask...
> > 		return -1;
> 
> Why?  Counting the number of objects when we can't actually do anything
> is just a waste of time, and -1 vs 0 for the sizing pass seem to be
> treateds the same in the calling code.

.....

> >  * The callback must not return -1 if nr_to_scan is zero.
> 
> this is against your suggestion of using -1 for the estimation pass
> above, btw.

Technically, if the shrinker cannot make progress or the gfp mask
means it cannot enter the filesystem code, then it should return -1,
not zero. Yes, the calc code treats 0 and -1 the same because it is
defensive - for the calculation a shrinker can validly return 0 to
mean "I have no work to do" rather than "I cannot do any work in
this context", but both mean the same thing - don't try to run the
shrinker here.

However, the later shrinker callout to do work (i.e. nr_to_scan !=
0) relies on this distinction to break out of the shrink loop early
whenteh shrinker says "can't do any work". If you just keep
returning zero there then it will just looping uselessly until the
scan count runs out.

The interface is a piece of shit, and I need to get back to my patch
series that fixes this all up by separating the calculation callback
from the work callback...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs