Re: CVS commit: src/sys/kern

[Andrew Doran <ad%netbsd.org@localhost>]

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