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

[Christoph Hellwig <hch@infradead.org>]

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.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs