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

[Pawel Jakub Dawidek]

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.

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)?

-- 
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-all/attachments/20090603/76ad5ef7/attachment.pgp