From owner-freebsd-current@FreeBSD.ORG  Thu Feb 12 11:16:26 2004
Return-Path: <owner-freebsd-current@FreeBSD.ORG>
Delivered-To: freebsd-current@freebsd.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP id CD48C16A4CE
	for <freebsd-current@freebsd.org>;
	Thu, 12 Feb 2004 11:16:26 -0800 (PST)
Received: from mailout1.pacific.net.au (mailout1.pacific.net.au [61.8.0.84])
	by mx1.FreeBSD.org (Postfix) with ESMTP id 53C5443D2F
	for <freebsd-current@freebsd.org>;
	Thu, 12 Feb 2004 11:16:26 -0800 (PST)	(envelope-from bde@zeta.org.au)
Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au
	[61.8.0.87])i1CJGPLE004884;	Fri, 13 Feb 2004 06:16:25 +1100
Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246])
	i1CJGMEJ021592;	Fri, 13 Feb 2004 06:16:23 +1100
Date: Fri, 13 Feb 2004 06:16:21 +1100 (EST)
From: Bruce Evans <bde@zeta.org.au>
X-X-Sender: bde@gamplex.bde.org
To: Jun Kuriyama <kuriyama@imgsrc.co.jp>
In-Reply-To: <7m65ed2j50.wl@black.imgsrc.co.jp>
Message-ID: <20040213060939.Q24610@gamplex.bde.org>
References: <7m65ed2j50.wl@black.imgsrc.co.jp>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
cc: Kirk McKusick <mckusick@mckusick.com>
cc: Current <freebsd-current@freebsd.org>
Subject: Re: acquiring duplicate lock of same type: "vnode interlock"
X-BeenThere: freebsd-current@freebsd.org
X-Mailman-Version: 2.1.1
Precedence: list
List-Id: Discussions about the use of FreeBSD-current
	<freebsd-current.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-current>,
	<mailto:freebsd-current-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-current>
List-Post: <mailto:freebsd-current@freebsd.org>
List-Help: <mailto:freebsd-current-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-current>,
	<mailto:freebsd-current-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 12 Feb 2004 19:16:26 -0000

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
> @@ -488,9 +488,10 @@
>  	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.


> @@ -1793,9 +1794,10 @@
>  		 */
>  		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