> On Feb 17, 2020, at 11:19 PM, Andrew Doran <a...@netbsd.org> wrote:
> 
> Hi,
> 
> On Thu, Feb 06, 2020 at 06:33:55PM +0100, J. Hannken-Illjes wrote:
> 
>>> On 12. Jan 2020, at 18:49, Andrew Doran <a...@netbsd.org> wrote:
>>> 
>>> Module Name:        src
>>> Committed By:       ad
>>> Date:               Sun Jan 12 17:49:17 UTC 2020
>>> 
>>> Modified Files:
>>>     src/sys/kern: vfs_vnode.c
>>> 
>>> Log Message:
>>> vput(): don't drop the vnode lock, carry the hold over into vrelel() which
>>> might need it anyway.
>>> 
>>> 
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.106 -r1.107 src/sys/kern/vfs_vnode.c
>>> 
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>> 
>> 
>> vput(vnode_t *vp)
>> {
>> +       int lktype;
>> 
>> -       VOP_UNLOCK(vp);
>> -       vrele(vp);
>> +       if ((vp->v_vflag & VV_LOCKSWORK) == 0) {
>> +               lktype = LK_EXCLUSIVE;
>> +       } else {
>> +               lktype = VOP_ISLOCKED(vp);
>> +               KASSERT(lktype != LK_NONE);
>> +       }
>> +       mutex_enter(vp->v_interlock);
>> +       vrelel(vp, 0, lktype);
>> }
>> 
>> This is quite wrong, from the manual:
>> 
>>     VOP_ISLOCKED(vp)
>>              Test if the vnode vp is locked.  Possible return values are
>>              LK_EXCLUSIVE, LK_SHARED or 0 for lock held exclusively by the
>>              calling thread, shared lock held by anyone or unlocked,
>>              respectively.
>> 
>>              This function must never be used to make locking decisions at
>>              run time: it is provided only for diagnostic purposes.
>> 
>> I suppose you cannot determine if the current thread holds
>> a shared lock.
> 
> The intention of that last sentence was to dissuade people from doing error
> prone, complicated stuff with locks.  To my mind that idea of the locking
> primitives is to take something difficult (concurrency) and package it up in
> a way that's much easier to think about and work with - and yes it's still
> complicated.
> 
> There are limited cases when I think it makes sense to infer lock ownership,
> for example when known for sure that a RW lock is held but the type of hold
> needs to be known - like this case.  The failure case there is the lock
> being unheld, which would be caught by LOCKDEBUG in this case.  Consider
> that a rw_exit() with the lock unheld by the caller will produce what you
> might charitably call "undefined behaviour" in a non-LOCKDEBUG kernel.
> 
> Andrew

Yes, I think it is safe here but it deserves a comment like the text above.

--
J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)

Attachment: signature.asc
Description: Message signed with OpenPGP

Reply via email to