[PATCH] Fix 'implicit declaration' warning and update vgone(9)
Kostik Belousov
kostikbel at gmail.com
Wed Oct 27 15:35:14 UTC 2010
On Wed, Oct 27, 2010 at 10:59:56AM -0400, Benjamin Kaduk wrote:
> On Wed, 27 Oct 2010, John Baldwin wrote:
>
> >On Wednesday, October 27, 2010 7:33:13 am Sergey Kandaurov wrote:
> >>On 27 October 2010 10:23, Lars Hartmann <lars at chaotika.org> wrote:
> >>>The vgonel function isnt declarated in any header, the vgonel prototype
> >>>in vgone(9) isnt correct - found by Ben Kaduk <kaduk at mit.edu>
> >>
> >>Hi.
> >>
> >>I'm afraid it's just an overlooked man page after many VFS changes in 5.x.
> >>As vgonel() is a static (i.e. private and not visible from outside)
> >>function
> >>IMO it should be removed from vgone(9) man page.
> >
> >Agreed. It certainly should not be added to vnode.h. I'm curious how the
> >reporter is getting a warning since there is a static prototype for
> >vgonel()
> >in vfs_subr.c.
>
> It's for a third-party kernel module, for the OpenAFS filesystem (which
> has been unloved for FreeBSD since the 4.X days). The AFS code is
> currently using unlocked accesses to v_usecount (which, unsurprisingly,
> led to a race condition that caused an invariant check to fail), and I was
> going through and adding the interlock around them. At the place that I
> suspect to be the main cause of this race [1], the usecount was checked to
> be nonpositive along with a couple other conditions, and a little later
> vgone() was called. Holding the interlock across both of these calls (and
> therefore using vgonel()) seems to have closed the race condition I was
> seeing. (Other checks of v_usecount were changed to grab the interlock,
> but drop it before doing anything else.)
> However, looking at the commit message for vfs_subr.c:1.631, I guess this
> is not the locking strategy I'm supposed to be using?
>
> I saw a warning of implicit declaration when compiling the kernel module,
> but the kernel linker was happy to load it. I forget whether it matters
> that vgonel is only declared static at its declaration but not its
> definition.
>
> -Ben Kaduk
>
>
> [1] The old (racy) function is osi_TryEvictVCache, here:
> http://git.openafs.org/?p=openafs.git;a=blob;f=src/afs/FBSD/osi_vcache.c;h=c2060c74f0155a610d2ea94f3c7f508e8ca4373a;hb=HEAD
The function looks very strange for much more serious reasons. Why do you
try to manage the vnode revocation in the filesystem module at all ?
VFS will (assumedly in a right way) revoke and destroy non-referenced
vnodes when needed.
Anyway, you need to hold vnode lock before checking for the vnode refcounter.
See the vfs_subr.c:vlrureclaim() for the correct dance with vholdl()/vn_lock()
sequence that does the revocation I mentioned in the race-free way.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20101027/389fe205/attachment.pgp
More information about the freebsd-hackers
mailing list