xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/7] xfs: use a normal shrinker for the dquot freelist
From: Ben Myers <bpm@xxxxxxx>
Date: Fri, 10 Feb 2012 10:56:46 -0600
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20120210014947.GE12836@dastard>
References: <20120201135719.202171828@xxxxxxxxxxxxxxxxxxxxxx> <20120201140039.011990931@xxxxxxxxxxxxxxxxxxxxxx> <20120209220320.GL7762@xxxxxxx> <20120209225626.GA844@xxxxxxxxxxxxx> <20120210014947.GE12836@dastard>
User-agent: Mutt/1.5.18 (2008-05-17)
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

<Prev in Thread] Current Thread [Next in Thread>