From owner-freebsd-stable@FreeBSD.ORG Fri Apr 16 21:12:03 2010 Return-Path: Delivered-To: freebsd-stable@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6BD49106564A for ; Fri, 16 Apr 2010 21:12:03 +0000 (UTC) (envelope-from matthew.fleming@isilon.com) Received: from seaxch09.isilon.com (seaxch09.isilon.com [74.85.160.25]) by mx1.freebsd.org (Postfix) with ESMTP id 50B238FC0A for ; Fri, 16 Apr 2010 21:12:03 +0000 (UTC) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Date: Fri, 16 Apr 2010 14:12:01 -0700 Message-ID: <06D5F9F6F655AD4C92E28B662F7F853E039387FE@seaxch09.desktop.isilon.com> In-Reply-To: <20100416204117.GM2415@deviant.kiev.zoral.com.ua> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: panic in vget() Thread-Index: AcrdpT4Q18qB/Bs+Rdyt5bRkFfeypAAA1Drw References: <06D5F9F6F655AD4C92E28B662F7F853E039387EF@seaxch09.desktop.isilon.com> <20100416204117.GM2415@deviant.kiev.zoral.com.ua> From: "Matthew Fleming" To: "Kostik Belousov" Cc: freebsd-stable@freebsd.org Subject: RE: panic in vget() X-BeenThere: freebsd-stable@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Production branch of FreeBSD source code List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Apr 2010 21:12:03 -0000 > -----Original Message----- > From: Kostik Belousov [mailto:kostikbel@gmail.com] > Sent: Friday, April 16, 2010 1:41 PM > To: Matthew Fleming > Cc: freebsd-stable@freebsd.org > Subject: Re: panic in vget() >=20 > On Fri, Apr 16, 2010 at 01:23:17PM -0700, Matthew Fleming wrote: > > I'm looking at this panic in vget() on stable/7: > > > > if (vp->v_iflag & VI_DOOMED && (flags & LK_RETRY) =3D=3D 0) > > panic("vget: vn_lock failed to return ENOENT\n"); > > > > It seems to me that this is not a correct assertion, because if the > > caller passed in no lock flags (i.e. just checking the vnode for > > validity) then there is a window between the VI_UNLOCK() in > > _vn_lock(9) and the subsequent VI_LOCK() in vget() where another > > thread could have set VI_DOOMED. > > > > This isn't a problem on CURRENT because the code has been changed to > > not allow an empty lock flags. > > > > I believe the following is a potential fix is: > > > > vholdl(vp); > > if ((error =3D vn_lock(vp, flags | LK_INTERLOCK, td)) !=3D 0) { > > vdrop(vp); > > return (error); > > } > > VI_LOCK(vp); > > + /* > > + * Deal with a timing window when the interlock is not held > > + * and VI_DOOMED can be set, since we only have a holdcnt, > > + * not a usecount. > > + */ > > + if (vp->v_iflag & VI_DOOMED && (flags & LK_RETRY) =3D=3D 0) { > > + KASSERT((flags & LK_TYPE_MASK) =3D=3D 0, ("Unexpected flags > > %x", flags)); > > + vdropl(vp); > > + return (ENOENT); > > + } > > /* Upgrade our holdcnt to a usecount. */ > > v_upgrade_usecount(vp); > > - if (vp->v_iflag & VI_DOOMED && (flags & LK_RETRY) =3D=3D 0) > > - panic("vget: vn_lock failed to return ENOENT\n"); > > if (oweinact) { > > if (vp->v_iflag & VI_OWEINACT) > > vinactive(vp, td); > > VI_UNLOCK(vp); > > if ((oldflags & LK_TYPE_MASK) =3D=3D 0) >=20 > Both the analysis and the patch look good. >=20 > Did you considered locking the vnode even when no locking flags were > given, as is done for VI_OWEINACT handling ? Your solution is better, esp. > for old lockmgr, but acquiring vnode lock might be safer. For our systems, the vnode lock is distributed across the entire cluster, so we prefer not to take it unless required. The code path that produced this panic is one such; it is using other mechanisms to guarantee the data is correct. (As a side note, splitting the vnode lock into a lock on the vnode struct and a "file" lock would be really great, since the VOP_LOCK uses seem split between serializing the file contents and serializing some of the members of struct vnode itself, and we only need a distributed lock for the file contents). Thanks, matthew