From owner-cvs-src@FreeBSD.ORG Wed May 31 09:20:32 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 A588E16A420; Wed, 31 May 2006 09:20:32 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from gw.catspoiler.org (adsl-75-1-14-242.dsl.scrm01.sbcglobal.net [75.1.14.242]) by mx1.FreeBSD.org (Postfix) with ESMTP id 584D043D53; Wed, 31 May 2006 09:20:32 +0000 (GMT) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.13.3/8.13.3) with ESMTP id k4V9KGEn001337; Wed, 31 May 2006 02:20:23 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <200605310920.k4V9KGEn001337@gw.catspoiler.org> Date: Wed, 31 May 2006 02:20:16 -0700 (PDT) From: Don Lewis To: dds@aueb.gr In-Reply-To: <447D2E4B.5080602@aueb.gr> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii 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: Wed, 31 May 2006 09:20:32 -0000 On 31 May, Diomidis Spinellis wrote: > 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). That should be sufficient. The VOP_ISLOCKED() bug gets tripped pretty quickly, when the root file system gets mounted as I recall. The VOP_RENAME() bugs got triggered during the transition to multi-user mode. Once I disabled the tests that I mentioned, I was able to build a bunch of ports.