Date: Wed, 31 May 2006 08:48:59 +0300 From: Diomidis Spinellis <dds@aueb.gr> To: Don Lewis <truckman@FreeBSD.ORG> Cc: cvs-src@FreeBSD.ORG, src-committers@FreeBSD.ORG, cvs-all@FreeBSD.ORG Subject: Re: cvs commit: src/sys/kern vnode_if.src Message-ID: <447D2E4B.5080602@aueb.gr> In-Reply-To: <200605292202.k4TM2mjt021917@gw.catspoiler.org> References: <200605292202.k4TM2mjt021917@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Don Lewis wrote: > On 28 May, Diomidis Spinellis wrote: >> dds 2006-05-28 07:24:12 UTC >> >> FreeBSD src repository >> >> Modified files: >> sys/kern vnode_if.src >> Log: >> Add missing % signs in the lock annotations of the functions: >> lookup, rename, strategy, islocked >> The missing % sign meant that the lines were processed as plain >> comments and the corresponding assertions were never generated. >> >> Revision Changes Path >> 1.80 +8 -8 src/sys/kern/vnode_if.src > > [appearing from the void] > > The additional sanity checking generated by this patch made my system > unbootable with DEBUG_VFS_LOCKS enabled. > > ASSERT_VI_UNLOCKED() calls are added to VOP_ISLOCKED_APV(), but there > is an explicit call to VOP_ISLOCKED() in vput() with the vnode > interlock held. This could be fixed by re-ordering the code in > vput(), but there are also a number of places in lower level code that > are called with the vnode interlock held that contain calls to > ASSERT_VOP_LOCKED(), which in turn calls VOP_ISLOCKED(). Possible > fixes: > Specify the locking value as "-" for vop_islocked() in vnode_if.src > > Introduce a new locking value, similar to "=", but which skips the > interlock check. Note: vnode_if.awk does not implement the "=" > vnode lock assertion. > > The fdvp and fvp initial vnode lock state assertions can't be checked > by the generated wrapper. If fdvp and fvp happen to be references to > tdvp and/or tvp, which must be locked, then fdvp and/or fvp will also > be locked. They should only be unlocked if they are different vnodes > than tdvp and tvp. The proper sanity checking appears to be done in > vop_rename_pre(). The initial lock value for fdvp and fvp should > probably be set to "-" in vnode_if.src, or to another value that only > generates the interlock assertion. > > VOP_RENAME_APV() calls ASSERT_VI_UNLOCKED() on tvp before and after > the vop->vop_rename() call, even though tvp may be NULL. vnode_if.src > specifies the initial lock state as "X", which indicates that the > check should be skipped if the vnode pointer is NULL, but vnode_if.awk > does not handle the "X" value at all, and adds an unconditional > interlock check. vop_rename_pre() does the conditional vnode lock > check, so maybe the interlock check could be done there as well and > omitted from the wrapper. The final state is specified as "U", though > this should also be a conditional check, but there is no vnode locking > value that generates a conditional unlock assertion. > vop_rename_post() does not do any lock state sanity checks. > > BTW, the VOP_ISLOCKED(9) man page does not document that the return > value is the shared/exclusive lock type. > > [disappearing into the void] I committed a stopgap measure (I disabled the erroneous checks), and will look at how I can correctly enable them again. Do you think that booting with DEBUG_VFS_LOCKS and performing a kernel build would be enough to gain confidence that a particular assertion is correctly generated? Any proposals for additional tests? (The system I use for testing has unfortunately only a single processor). Diomidis
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?447D2E4B.5080602>