xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/7] xfs: use a normal shrinker for the dquot freelist
From: Ben Myers <bpm@xxxxxxx>
Date: Thu, 9 Feb 2012 17:13:46 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20120209225626.GA844@xxxxxxxxxxxxx>
References: <20120201135719.202171828@xxxxxxxxxxxxxxxxxxxxxx> <20120201140039.011990931@xxxxxxxxxxxxxxxxxxxxxx> <20120209220320.GL7762@xxxxxxx> <20120209225626.GA844@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
Hey Christoph,

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:
> > I've been messing with this and haven't gotten it to call us with
> > nr_to_scan other than 0 or -1 yet.  Maybe I need more dquots.
> > (time passes)  Ok, I have it going now.  Comments below.
> 
> To actually hit this I hade to use a VM with very little memory assigned
> to it, and then creat lots of dquots and causes memory pressure.
> 
> I have about 20.000 users on it, and I did a quota report for all of
> them while catting one block device into another using buffered I/O.

Ah, I see.

> > > + 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.

That's a good point, but the shrinker interface has documented that
you're supposed to return -1 in this situation... and that you aren't
allowed to return -1 when nr_to_scan == 0.

> > > -
> > > - return B_TRUE;
> > > + while (!list_empty(&dispose_list)) {
> > > +         dqp = list_first_entry(&dispose_list, struct xfs_dquot,
> > > +                                q_freelist);
> > > +         list_del_init(&dqp->q_freelist);
> > > +         xfs_qm_dqfree_one(dqp);
> > > + }
> > > +out:
> > > + return (xfs_Gqm->qm_dqfrlist_cnt / 100) * sysctl_vfs_cache_pressure;
> > 
> > return atomic_read(&xfs_Gqm->qm_totaldquots);
> > 
> > This works well for me and seems to be closer to the shrinker interface
> > as documented:
> 
> It's pointless - we can only apply pressure to dquots that are on the
> freelist.  No amount of shaking will allow us to reclaim a referenced
> dquot.

Sure... then it should be:

return atomic_read(&xfs_Gqm->qm_frlist_cnt);

What is the value of the additional calculation?

> >  * 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.

No it isn't... if nr_to_scan == 0 we would have jumped to 'out' and
returned the count.

Thanks,
        Ben

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