xfs
[Top] [All Lists]

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

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 1/7] xfs: use a normal shrinker for the dquot freelist
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 9 Feb 2012 17:56:26 -0500
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20120209220320.GL7762@xxxxxxx>
References: <20120201135719.202171828@xxxxxxxxxxxxxxxxxxxxxx> <20120201140039.011990931@xxxxxxxxxxxxxxxxxxxxxx> <20120209220320.GL7762@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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.

> 
> > This also fixes an bug in the previous lock ordering, where we would take
> > the hash and dqlist locks inside of the freelist lock against the normal
> > lock ordering.  This is only solvable by introducing the dispose list,
> > and thus not when using direct reclaim of unused dquots for new allocations.
> 
> FWICS this fixes a possible deadlock, xfs_qm_dqget vs xfs_qm_dqreclaim
> one.

Yes.

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

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

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

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