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

[John Baldwin]

On Saturday 22 November 2008 08:11:11 am Konstantin Belousov wrote:
> Author: kib
> Date: Sat Nov 22 13:11:11 2008
> New Revision: 185170
> URL: http://svn.freebsd.org/changeset/base/185170
> 
> Log:
>   Busy ufs filesystem around block of code that does ".." lookup. Since
>   mnt_lock is before lock of any vnode on the mp, it uses LK_NOWAIT. Since
>   MNTK_UNMOUNT may be transient, pdp lock is dropped when vfs_busy()
>   failed, and operation is retried after some time. This way, ffs_vget()
>   is not called on the mp that may be in the process of being destroyed by
>   unmount.
>   
>   Check for the VI_DOOMED flag on pdp after its lock is reacquired, to
>   better detect some situations where directory containing ".."
>   entry is removed during the lookup.

I'm not really sure it matters if the parent directory goes away because it 
will have deadfs vops so any subsequent operations will already fail, yes?

Also, do you really need to grab the VI_LOCK just to check VI_DOOMED?   Other 
places in the kernel check that flag while holding the vnode lock w/o 
acquiring the interlock.  Since you are just doing a single atomic read the 
interlock doesn't actually close any races anyway.  I think it just adds 
overhead.

>   Reviewed by:	tegge, attilio (previous version)
>   Tested by:	pho
>   MFC after:	1 month
> 
> Modified:
>   head/sys/ufs/ufs/ufs_lookup.c
> 
> Modified: head/sys/ufs/ufs/ufs_lookup.c
> 
==============================================================================
> --- head/sys/ufs/ufs/ufs_lookup.c	Sat Nov 22 12:36:15 2008	(r185169)
> +++ head/sys/ufs/ufs/ufs_lookup.c	Sat Nov 22 13:11:11 2008	(r185170)
> @@ -157,6 +157,8 @@ ufs_lookup(ap)
>  	int nameiop = cnp->cn_nameiop;
>  	ino_t ino;
>  	int ltype;
> +	int pdoomed;
> +	struct mount *mp;
>  
>  	bp = NULL;
>  	slotoffset = -1;
> @@ -578,9 +580,32 @@ found:
>  	pdp = vdp;
>  	if (flags & ISDOTDOT) {
>  		ltype = VOP_ISLOCKED(pdp);
> +		mp = pdp->v_mount;
> +		for (;;) {
> +			error = vfs_busy(mp, MBF_NOWAIT);
> +			if (error == 0)
> +				break;
> +			VOP_UNLOCK(pdp, 0);
> +			pause("ufs_dd", 1);
> +			vn_lock(pdp, ltype | LK_RETRY);
> +			VI_LOCK(pdp);
> +			pdoomed = pdp->v_iflag & VI_DOOMED;
> +			VI_UNLOCK(pdp);
> +			if (pdoomed)
> +				return (ENOENT);
> +		}
>  		VOP_UNLOCK(pdp, 0);	/* race to get the inode */
> -		error = VFS_VGET(pdp->v_mount, ino, cnp->cn_lkflags, &tdp);
> +		error = VFS_VGET(mp, ino, cnp->cn_lkflags, &tdp);
> +		vfs_unbusy(mp);
>  		vn_lock(pdp, ltype | LK_RETRY);
> +		VI_LOCK(pdp);
> +		pdoomed = pdp->v_iflag & VI_DOOMED;
> +		VI_UNLOCK(pdp);
> +		if (pdoomed) {
> +			if (error == 0)
> +				vput(tdp);
> +			error = ENOENT;
> +		}
>  		if (error)
>  			return (error);
>  		*vpp = tdp;
> 



-- 
John Baldwin