cvs commit: src/sys/kern vnode_if.src

[Don Lewis]

On 31 May, Diomidis Spinellis wrote:
> 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).

That should be sufficient.  The VOP_ISLOCKED() bug gets tripped pretty
quickly, when the root file system gets mounted as I recall.  The
VOP_RENAME() bugs got triggered during the transition to multi-user
mode.   Once I disabled the tests that I mentioned, I was able to build
a bunch of ports.