Date: Mon, 19 Jul 2010 10:40:50 +0300 From: Gleb Kurtsou <gleb.kurtsou@gmail.com> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: freebsd-fs@freebsd.org Subject: Re: fix for remove for NFS through nullfs Message-ID: <20100719074050.GA2640@tops> In-Reply-To: <Pine.GSO.4.63.1007182231590.21767@muncher.cs.uoguelph.ca> References: <Pine.GSO.4.63.1007182231590.21767@muncher.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
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@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100719074050.GA2640>