From owner-cvs-src@FreeBSD.ORG Mon May 29 22:03:15 2006 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id A0E7216B367; Mon, 29 May 2006 22:03:15 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from nccn1.nccn.net (nccn1.nccn.net [12.165.58.11]) by mx1.FreeBSD.org (Postfix) with ESMTP id AB14143D70; Mon, 29 May 2006 22:03:14 +0000 (GMT) (envelope-from truckman@FreeBSD.org) Received: from gw.catspoiler.org (66-53-242-201.stkn.mdsg-pacwest.com [66.53.242.201] (may be forged)) by nccn1.nccn.net (8.12.8/8.12.8/*SLiM* NO UCE! [V13]) with ESMTP id k4TM2xMG026198; Mon, 29 May 2006 15:03:02 -0700 Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.13.3/8.13.3) with ESMTP id k4TM2mjt021917; Mon, 29 May 2006 15:02:51 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <200605292202.k4TM2mjt021917@gw.catspoiler.org> Date: Mon, 29 May 2006 15:02:48 -0700 (PDT) From: Don Lewis To: dds@FreeBSD.org In-Reply-To: <200605280724.k4S7OCl5083567@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii X-MailScanner-Information: Please contact the ISP for more information X-MailScanner: Not scanned: please contact your Internet E-Mail Service Provider for details 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-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 May 2006 22:03:30 -0000 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]