On Fri, Feb 10, 2012 at 12:49:47PM +1100, Dave Chinner wrote:
> 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...
Ok... so it'll be sorted out in a different patch. ;)