On Mon, May 21, 2007 at 01:46:26PM -0400, Blair Sadewitz wrote:

> This patch does what ad@ suggested on netbsd-bugs.
> I have tested it with lfs and ffs, and I haven't had any more deadlocks 
> thusfar.
>
> +	if ((*vpp = ufs_ihashget(dev, ino, 0)) != NULL) {

Nit: don't need the *vpp = here. :-)

> --- sys/ufs/ufs/ufs_ihash.c	27 Feb 2007 16:11:51 -0000	1.22
> +++ sys/ufs/ufs/ufs_ihash.c	21 May 2007 16:47:47 -0000
> @@ -145,11 +145,13 @@
>  	LIST_FOREACH(ip, ipp, i_hash) {
>  		if (inum == ip->i_number && dev == ip->i_dev) {
>  			vp = ITOV(ip);
> -			simple_lock(&vp->v_interlock);
>  			mutex_exit(&ufs_ihash_lock);
> -			if (vget(vp, flags | LK_INTERLOCK))
> -				goto loop;
> -			return (vp);
> +			if (flags != 0) {
> +				simple_lock(&vp->v_interlock);
> +				if (vget(vp, flags | LK_INTERLOCK))
> +					goto loop;
> +			}

You need to hold ufs_ihash_lock while locking vp->v_interlock, to ensure
that the vnode doesn't disappear off the list as soon as you unlock. It does
not make a difference right now since this is all covered by the kernel_lock
but to be correct it's needed. Otherwise the changes are pretty much as I
described.

There are a few other file systems that need similar modifications. The
inode hash is something that could made abstracted and made a function of
genfs, since the ufs code has been cut-n-pasted all over the place. It could
also do with being per-mount. All of those are other projects though. :-)

Thanks,
Andrew