cvs commit: src/sys/kern vnode_if.src

[Diomidis Spinellis]

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