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

[Pawel Jakub Dawidek]

On Sat, Jun 06, 2009 at 08:14:07PM +0200, Nick Barkas wrote:
> 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. [...]

The ufsdirhash_destroy() function does this:

	TAILQ_REMOVE(&ufsdirhash_list, dh, dh_list);

Once you return from ufsdirhash_destroy(), dh is no longer on the queue,
so how this is suppose to work: dh = TAILQ_NEXT(dh, dh_list)?

TAILQ_NEXT() should panic on you when called for element which is not on
the queue, but we don't have such strict checks compiled-in by default.
Try to define QUEUE_MACRO_DEBUG somewhere in sys/queue.h to get the debug.

All in all, if you traverse the queue and want to remove current element
from inside the loop, TAILQ_FOREACH_SAFE() is for you.

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

-- 
Pawel Jakub Dawidek                       http://www.wheel.pl
pjd at FreeBSD.org                           http://www.FreeBSD.org
FreeBSD committer                         Am I Evil? Yes, I Am!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-head/attachments/20090606/600833f3/attachment.pgp