cvs commit: src/sys/kern vnode_if.src
dds at aueb.gr
Tue May 30 22:49:03 PDT 2006
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
>> 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
> 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).
More information about the cvs-src