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@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
