> On Feb 17, 2020, at 11:19 PM, Andrew Doran <ad%netbsd.org@localhost> 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 <ad%netbsd.org@localhost> 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 - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig (Germany)
Attachment:
signature.asc
Description: Message signed with OpenPGP