svn commit: r193375 - head/sys/ufs/ufs

[Nick Barkas]

On Wed, Jun 03, 2009 at 11:06:52PM +0200, Pawel Jakub Dawidek wrote:
> On Wed, Jun 03, 2009 at 09:44:22AM +0000, Sean Nicholas Barkas wrote:
> > Author: snb
> > Date: Wed Jun  3 09:44:22 2009
> > New Revision: 193375
> > URL: http://svn.freebsd.org/changeset/base/193375
> > 
> > Log:
> >   Add vm_lowmem event handler for dirhash. This will cause dirhashes to be
> >   deleted when the system is low on memory. This ought to allow an increase to
> >   vfs.ufs.dirhash_maxmem on machines that have lots of memory, without
> >   degrading performance by having too much memory reserved for dirhash when
> >   other things need it. The default value for dirhash_maxmem is being kept at
> >   2MB for now, though.
> >   
> >   This work was mostly done during the 2008 Google Summer of Code.
> >   
> >   Approved by:	dwmalone (mentor), re
> >   MFC after:	3 months
> [...]
> > +static int
> > +ufsdirhash_destroy(struct dirhash *dh)
> > +{
> [...]
> > +	/* Remove it from the list and detach its memory. */
> > +	TAILQ_REMOVE(&ufsdirhash_list, dh, dh_list);
> [...]
> > +static void
> > +ufsdirhash_lowmem()
> > +{
> [...]
> > +	/* 
> > +	 * Delete dirhashes not used for more than ufs_dirhashreclaimage 
> > +	 * seconds. If we can't get a lock on the dirhash, it will be skipped.
> > +	 */
> > +	for (dh = TAILQ_FIRST(&ufsdirhash_list); dh != NULL; dh = 
> > +	    TAILQ_NEXT(dh, dh_list)) {
> > +		if (!sx_try_xlock(&dh->dh_lock))
> > +			continue;
> > +		if (time_second - dh->dh_lastused > ufs_dirhashreclaimage)
> > +			memfreed += ufsdirhash_destroy(dh);
> > +		/* Unlock if we didn't delete the dirhash */
> > +		else
> > +			ufsdirhash_release(dh);
> > +	}
> > +
> > +	/* 
> > +	 * If not enough memory was freed, keep deleting hashes from the head 
> > +	 * of the dirhash list. The ones closest to the head should be the 
> > +	 * oldest. 
> > +	 */
> > +	for (dh = TAILQ_FIRST(&ufsdirhash_list); memfreed < memwanted &&
> > +	    dh !=NULL; dh = TAILQ_NEXT(dh, dh_list)) {
> > +		if (!sx_try_xlock(&dh->dh_lock))
> > +			continue;
> > +		memfreed += ufsdirhash_destroy(dh);
> > +	}
> > +	DIRHASHLIST_UNLOCK();
> > +}
> 
> I don't see how that works. If you remove dh from the tailq in
> ufsdirhash_destroy(), you can't do 'dh = TAILQ_NEXT(dh, dh_list)' at the
> end of the loop.
> 
> You should use TAILQ_FOREACH_SAFE(3). In the second case you also need to
> move this extra check into the loop, probably.

Yes, I think you are right. Thanks for catching that. I think that, as
written, both loops will only try to delete the first hash they can lock
in ufsdirhash_list. I'll try to correct that. 

> In addition you drop DIRHASHLIST lock in ufsdirhash_destroy() during the
> loop. Can't the tailq be modified from elsewhere? Or even from parallel
> call to ufsdirhash_lowmem() (I don't think we serialize those)?

The lock is held on the tailq while we are doing TAILQ_REMOVE(). It is
only released for freeing data structures contained in dirhashes that
have been removed from the tailq. I don't see how this would be a
problem, but I certainly could be missing something. But, I don't really
understand why the lock is dropped within ufsdirhash_destroy(), anyway.
Perhaps it is not necessary to do so. 

I mostly just copied the code needed out of ufsdirhash_recycle() to
write ufsdirhash_destroy(). ufsdirhash_recycle() I think could be
concurrently called (by ufsdirhash_build()) previously, with the lock on
ufsdirhash_list lock dropped in the same place, and I don't think this
caused any problems.

Nick