svn commit: r186276 - head/sys/kern

[Kostik Belousov]

On Wed, Mar 18, 2009 at 12:15:22PM -0400, John Baldwin wrote:
> On Friday 13 March 2009 5:22:29 pm Kostik Belousov wrote:
> > On Fri, Mar 13, 2009 at 02:15:34PM -0400, John Baldwin wrote:
> > > Konstantin Belousov wrote:
> > > >Author: kib
> > > >Date: Thu Dec 18 11:58:12 2008
> > > >New Revision: 186276
> > > >URL: http://svn.freebsd.org/changeset/base/186276
> > > >
> > > >Log:
> > > >  Do not return success and doomed vnode from lookup. LK_UPGRADE allows
> > > >  the vnode to be reclaimed.
> > > >  
> > > >  Tested by:	pho
> > > >  MFC after:	1 month
> > > 
> > > Would EBADF be more appropriate?  That is what other places that check 
> > > VI_DOOMED return for this type of check (e.g. in cache_lookup()).
> > 
> > I do not think so. When we do namei lookup, there is actually no
> > file descriptor to be invalid. The fact the we lost the race with
> > forced unmount actually means that there is no more node with
> > supplied name.
> 
> Hmm, I think a few places need to be fixed to ENOENT instead of EBADF then:
> 
> --- //depot/user/jhb/lock/kern/vfs_cache.c
> +++ /home/jhb/work/p4/lock/kern/vfs_cache.c
> @@ -315,7 +315,7 @@
>   * (negative cacheing), a status of ENOENT is returned. If the lookup
>   * fails, a status of zero is returned.  If the directory vnode is
>   * recycled out from under us due to a forced unmount, a status of
> - * EBADF is returned.
> + * ENOENT is returned.
>   *
>   * vpp is locked and ref'd on return.  If we're looking up DOTDOT, dvp is
>   * unlocked.  If we're looking up . an extra ref is taken, but the lock is
> @@ -467,7 +467,7 @@
>  					/* forced unmount */
>  					vrele(*vpp);
>  					*vpp = NULL;
> -					return (EBADF);
> +					return (ENOENT);
>  				}
>  			} else
>  				vn_lock(*vpp, LK_DOWNGRADE | LK_RETRY);
> @@ -974,7 +974,7 @@
>  		if (vp->v_vflag & VV_ROOT) {
>  			if (vp->v_iflag & VI_DOOMED) {	/* forced unmount */
>  				CACHE_RUNLOCK();
> -				error = EBADF;
> +				error = ENOENT;
>  				break;
>  			}
>  			vp = vp->v_mount->mnt_vnodecovered;
> --- //depot/user/jhb/lock/kern/vfs_lookup.c
> +++ /home/jhb/work/p4/lock/kern/vfs_lookup.c
> @@ -602,7 +602,7 @@
>  			if ((dp->v_vflag & VV_ROOT) == 0)
>  				break;
>  			if (dp->v_iflag & VI_DOOMED) {	/* forced unmount */
> -				error = EBADF;
> +				error = ENOENT;
>  				goto bad;
>  			}
>  			tdp = dp;
> @@ -764,9 +764,11 @@
>  	     *ndp->ni_next == '/')) {
>  		cnp->cn_flags |= ISSYMLINK;
>  		if (dp->v_iflag & VI_DOOMED) {
> -			/* We can't know whether the directory was mounted with
> -			 * NOSYMFOLLOW, so we can't follow safely. */
> -			error = EBADF;
> +			/*
> +			 * We can't know whether the directory was mounted with
> +			 * NOSYMFOLLOW, so we can't follow safely.
> +			 */
> +			error = ENOENT;
>  			goto bad2;
>  		}
>  		if (dp->v_mount->mnt_flag & MNT_NOSYMFOLLOW) {
> 

There is one place in lookup() that does MNT_UNION handling, it switches
to the covered directory when VOP_LOOKUP returns ENOENT. In current code,
EBADF from VOP_LOOKUP would cause namei to return, but with the change,
it will continue. I doubt that this can cause a problem.

Overall, it looks good to me.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20090318/77c7ca6f/attachment.pgp