fix for remove for NFS through nullfs

[Gleb Kurtsou]

On (18/07/2010 22:40), Rick Macklem wrote:
> Mikolaj Golub submitted the attached patch that fixes a problem w.r.t.
> a nullfs mounted NFS mount point for remove. The problem is that,
> without this patch, NFS does not see that a file is still open
> (v_usecount > 1) when removed and removes it instead of silly renaming
> it. This patch increments the v_usecount of the lower level vnode
> during the remove call, so that silly rename works. kib@ has noted
> that this may be "racy" and result in silly rename happening when it
> isn't required but, imho, that is less of a problem than it never
> working. (I have tested it a bit for NFS and UFS and it seems to
> work for those file systems under a nullfs mount.)
> 
> Why I am posting is that I am wondering if anyone knows of a file
> system type where this extra v_usecount on the vnode at the time of remove
> will/might cause problems?
It seems to be easily reproducible only with stacked filesystems. But
the problem is that v_usecount can be different from number of open
descriptors for the vnode, and it generally is.

IMHO using v_usecount is racy for all filesystems. Grep for vref and
VREF, it's used all over the place.

The issue was discussed some time ago already:
http://marc.info/?l=freebsd-hackers&m=125675165319186&w=4

I think the better solution would be to add a means of getting number of
opened file descriptors, but not misuse v_usecount, e.g. the patch looks
a pure hack for me.

Thanks,
Gleb.

> 
> Thanks in advance for looking at this, rick
> --- submitted patch for nullfs ---
> --- fs/nullfs/null_vnops.c.sav	2010-07-18 19:33:00.000000000 -0400
> +++ fs/nullfs/null_vnops.c	2010-07-18 19:35:25.000000000 -0400
> @@ -499,6 +499,29 @@
>   }
> 
>   /*
> + * Increasing refcount of lower vnode is needed at least for the case
> + * when lower FS is NFS to do sillyrename if the file is in use.
> + */
> +static int
> +null_remove(struct vop_remove_args *ap)
> +{
> +	int retval;
> +	struct vnode *lvp;
> +	boolean_t vreleit;
> +
> +	if (ap->a_vp->v_usecount > 1) {
> +		lvp = NULLVPTOLOWERVP(ap->a_vp);
> +		VREF(lvp);
> +		vreleit = TRUE;
> +	} else
> +		vreleit = FALSE;
> +	retval = null_bypass(&ap->a_gen);
> +	if (vreleit)
> +		vrele(lvp);
> +	return (retval);
> +}
> +
> +/*
>    * We handle this to eliminate null FS to lower FS
>    * file moving. Don't know why we don't allow this,
>    * possibly we should.
> @@ -809,6 +832,7 @@
>   	.vop_open =		null_open,
>   	.vop_print =		null_print,
>   	.vop_reclaim =		null_reclaim,
> +	.vop_remove =		null_remove,
>   	.vop_rename =		null_rename,
>   	.vop_setattr =		null_setattr,
>   	.vop_strategy =		VOP_EOPNOTSUPP,
> _______________________________________________
> freebsd-fs at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe at freebsd.org"