Date: Wed, 27 Oct 2010 10:59:56 -0400 (EDT) From: Benjamin Kaduk <kaduk@MIT.EDU> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-hackers@freebsd.org, Sergey Kandaurov <pluknet@gmail.com>, Lars Hartmann <lars@chaotika.org> Subject: Re: [PATCH] Fix 'implicit declaration' warning and update vgone(9) Message-ID: <alpine.GSO.1.10.1010271043550.19200@multics.mit.edu> In-Reply-To: <201010270912.47076.jhb@freebsd.org> References: <1288160610.4280.18.camel@apollon> <AANLkTimCN%2BY4OsJRNvdmXidLu8XPHz5RydAhe0phvgnH@mail.gmail.com> <201010270912.47076.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@chaotika.org> wrote: >>> The vgonel function isnt declarated in any header, the vgonel prototype >>> in vgone(9) isnt correct - found by Ben Kaduk <kaduk@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.GSO.1.10.1010271043550.19200>