Date: Thu, 28 Oct 2010 16:20:40 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Benjamin Kaduk <kaduk@mit.edu> Cc: freebsd-hackers@freebsd.org Subject: Re: [PATCH] Fix 'implicit declaration' warning and update vgone(9) Message-ID: <20101028132040.GV2392@deviant.kiev.zoral.com.ua> In-Reply-To: <alpine.GSO.1.10.1010272322280.19200@multics.mit.edu> References: <1288160610.4280.18.camel@apollon> <AANLkTimCN%2BY4OsJRNvdmXidLu8XPHz5RydAhe0phvgnH@mail.gmail.com> <201010270912.47076.jhb@freebsd.org> <alpine.GSO.1.10.1010271043550.19200@multics.mit.edu> <20101027153402.GS2392@deviant.kiev.zoral.com.ua> <alpine.GSO.1.10.1010272322280.19200@multics.mit.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Wed, Oct 27, 2010 at 11:57:31PM -0400, Benjamin Kaduk wrote: > On Wed, 27 Oct 2010, Kostik Belousov wrote: > > >On Wed, Oct 27, 2010 at 10:59:56AM -0400, Benjamin Kaduk wrote: > >>[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 ? > > I am still becoming familiar with the AFS code, but I think this is > largely due to a difference in the vfs structure that AFS has been using > and the FreeBSD standard. E.g. vop_inactive/vop_reclaim do not actually > free filesystem-specific resources, instead keeping a free list of > "vcache" entries. So, the original authors of this AFS code were > approaching the problem in a somewhat different way. > Therefore, are somewhat-orthogonal pools of vcaches and vnodes (with some > intersection). If the vcaches are all in use in use, there is a routine > which tries to "shake some loose"; if it can free up vcaches, their > associated vnodes also need to be cleaned up in some fashion. It may be > that no additional code is actually needed to do this, though -- I am not > sure. I think the best would be to move the code to vop_reclaim(), possibly calling vgone() from vop_inactivate(). > > >VFS will (assumedly in a right way) revoke and destroy non-referenced > >vnodes when needed. > > > > You are, in essence, suggesting that the body of the conditional should > be just "return 1;"? (In my incomplete testing the vref() call was always > made.) See above. > > >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. > > Unless I am mistaken, the path you refer to still uses vgonel(), which is > not available to me. Is it safe to drop and reaquire the interlock > between the vgone and vdrop? vgonel() use is an optimization to avoid dropping/reacquiring the vnode interlock. Correct code can call vdrop() after vgone() call, without doing anything with interlock. Please note that vnode lock should be dropped between vgone and vdrop. [-- Attachment #2 --] -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (FreeBSD) iEYEARECAAYFAkzJeKYACgkQC3+MBN1Mb4hsrgCdFQG74NlbbWc11/wK8dqwOrRq 1rcAoINyikHIyz7RRAPGQOdFSXlncjPE =cEbt -----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101028132040.GV2392>