From owner-cvs-all@FreeBSD.ORG Wed May 31 05:49:03 2006 Return-Path: X-Original-To: cvs-all@FreeBSD.ORG Delivered-To: cvs-all@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 292BD16A53A; Wed, 31 May 2006 05:49:03 +0000 (UTC) (envelope-from dds@aueb.gr) Received: from mx-out-01.forthnet.gr (mx-out.forthnet.gr [193.92.150.30]) by mx1.FreeBSD.org (Postfix) with ESMTP id 60FA543D46; Wed, 31 May 2006 05:49:01 +0000 (GMT) (envelope-from dds@aueb.gr) Received: from mx-av-03.forthnet.gr (mx-av.forthnet.gr [193.92.150.27]) by mx-out-01.forthnet.gr (8.13.6/8.13.6) with ESMTP id k4V5mxt2018062; Wed, 31 May 2006 08:48:59 +0300 Received: from mx-in-02.forthnet.gr (mx-in-02.forthnet.gr [193.92.150.185]) by mx-av-03.forthnet.gr (8.13.6/8.13.6) with ESMTP id k4V5mxvh006202; Wed, 31 May 2006 08:48:59 +0300 Received: from [192.168.136.16] (ppp76-181.adsl.forthnet.gr [195.74.246.181]) by mx-in-02.forthnet.gr (8.13.6/8.13.6) with ESMTP id k4V5mwfX025831; Wed, 31 May 2006 08:48:59 +0300 Authentication-Results: mx-in-02.forthnet.gr from=dds@aueb.gr; sender-id=neutral; spf=neutral Message-ID: <447D2E4B.5080602@aueb.gr> Date: Wed, 31 May 2006 08:48:59 +0300 From: Diomidis Spinellis User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060404 SeaMonkey/1.0.1 MIME-Version: 1.0 To: Don Lewis References: <200605292202.k4TM2mjt021917@gw.catspoiler.org> In-Reply-To: <200605292202.k4TM2mjt021917@gw.catspoiler.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: cvs-src@FreeBSD.ORG, src-committers@FreeBSD.ORG, cvs-all@FreeBSD.ORG Subject: Re: cvs commit: src/sys/kern vnode_if.src X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 May 2006 05:49:03 -0000 Don Lewis wrote: > On 28 May, Diomidis Spinellis wrote: >> dds 2006-05-28 07:24:12 UTC >> >> FreeBSD src repository >> >> Modified files: >> sys/kern vnode_if.src >> Log: >> Add missing % signs in the lock annotations of the functions: >> lookup, rename, strategy, islocked >> The missing % sign meant that the lines were processed as plain >> comments and the corresponding assertions were never generated. >> >> Revision Changes Path >> 1.80 +8 -8 src/sys/kern/vnode_if.src > > [appearing from the void] > > The additional sanity checking generated by this patch made my system > unbootable with DEBUG_VFS_LOCKS enabled. > > ASSERT_VI_UNLOCKED() calls are added to VOP_ISLOCKED_APV(), but there > is an explicit call to VOP_ISLOCKED() in vput() with the vnode > interlock held. This could be fixed by re-ordering the code in > vput(), but there are also a number of places in lower level code that > are called with the vnode interlock held that contain calls to > ASSERT_VOP_LOCKED(), which in turn calls VOP_ISLOCKED(). Possible > fixes: > Specify the locking value as "-" for vop_islocked() in vnode_if.src > > Introduce a new locking value, similar to "=", but which skips the > interlock check. Note: vnode_if.awk does not implement the "=" > vnode lock assertion. > > The fdvp and fvp initial vnode lock state assertions can't be checked > by the generated wrapper. If fdvp and fvp happen to be references to > tdvp and/or tvp, which must be locked, then fdvp and/or fvp will also > be locked. They should only be unlocked if they are different vnodes > than tdvp and tvp. The proper sanity checking appears to be done in > vop_rename_pre(). The initial lock value for fdvp and fvp should > probably be set to "-" in vnode_if.src, or to another value that only > generates the interlock assertion. > > VOP_RENAME_APV() calls ASSERT_VI_UNLOCKED() on tvp before and after > the vop->vop_rename() call, even though tvp may be NULL. vnode_if.src > specifies the initial lock state as "X", which indicates that the > check should be skipped if the vnode pointer is NULL, but vnode_if.awk > does not handle the "X" value at all, and adds an unconditional > interlock check. vop_rename_pre() does the conditional vnode lock > check, so maybe the interlock check could be done there as well and > omitted from the wrapper. The final state is specified as "U", though > this should also be a conditional check, but there is no vnode locking > value that generates a conditional unlock assertion. > vop_rename_post() does not do any lock state sanity checks. > > BTW, the VOP_ISLOCKED(9) man page does not document that the return > value is the shared/exclusive lock type. > > [disappearing into the void] I committed a stopgap measure (I disabled the erroneous checks), and will look at how I can correctly enable them again. Do you think that booting with DEBUG_VFS_LOCKS and performing a kernel build would be enough to gain confidence that a particular assertion is correctly generated? Any proposals for additional tests? (The system I use for testing has unfortunately only a single processor). Diomidis