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