Re: acquiring duplicate lock of same type: "vnode interlock"

On Thu, 12 Feb 2004, Jun Kuriyama wrote:

> Is this patch safe for locking?  This may remove warnings below:

Perhaps, but it has some style bugs.

> Index: ffs_snapshot.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/ufs/ffs/ffs_snapshot.c,v
> retrieving revision 1.77
> diff -u -r1.77 ffs_snapshot.c
> --- ffs_snapshot.c	4 Jan 2004 04:08:34 -0000	1.77
> +++ ffs_snapshot.c	12 Feb 2004 01:08:31 -0000
> _at__at_ -488,9 +488,10 _at__at_
>  	VI_LOCK(devvp);
>  	snaphead = &devvp->v_rdev->si_snapshots;
>  	if ((xp = TAILQ_FIRST(snaphead)) != NULL) {
> -		VI_LOCK(vp);
> -		vp->v_vnlock = ITOV(xp)->v_vnlock;
> +		struct lock *lkp = ITOV(xp)->v_vnlock;

(1) Nested declaration.
(2) Initialization in declaration.
(3) No blank line after declaration.

>  		VI_UNLOCK(devvp);
> +		VI_LOCK(vp);
> +		vp->v_vnlock = lkp;
>  	} else {
>  		struct lock *lkp;
>

However, (1) seems to be a normal style in this file.  It is used here in
similar code.  But (2) and (3) are not used here.


> _at__at_ -1793,9 +1794,10 _at__at_
>  		 */
>  		VI_LOCK(devvp);
>  		if ((xp = TAILQ_FIRST(snaphead)) != NULL) {
> -			VI_LOCK(vp);
> -			vp->v_vnlock = ITOV(xp)->v_vnlock;
> +			struct lock *lkp = ITOV(xp)->v_vnlock;
>  			VI_UNLOCK(devvp);
> +			VI_LOCK(vp);
> +			vp->v_vnlock = lkp;

As above.

>  		} else {
>  			struct lock *lkp;
>

As above.

The lkp local is now defined nested twice, so (1) is a larger style bug
than before; however, the functions are so large that the style bug is
more in the other direction -- they begin with a large list of declarations
and might benefit from more nested ones.  Anyway, following ther nearby
style is never wrong.

Bruce