Date: Sat, 17 Apr 2010 00:36:12 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Matthew Fleming <matthew.fleming@isilon.com> Cc: freebsd-stable@freebsd.org Subject: Re: panic in vget() Message-ID: <20100416213612.GO2415@deviant.kiev.zoral.com.ua> In-Reply-To: <06D5F9F6F655AD4C92E28B662F7F853E039387FE@seaxch09.desktop.isilon.com> References: <06D5F9F6F655AD4C92E28B662F7F853E039387EF@seaxch09.desktop.isilon.com> <20100416204117.GM2415@deviant.kiev.zoral.com.ua> <06D5F9F6F655AD4C92E28B662F7F853E039387FE@seaxch09.desktop.isilon.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--WxVlef7wfy3nngmb
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Fri, Apr 16, 2010 at 02:12:01PM -0700, Matthew Fleming wrote:
> > -----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.
>=20
> 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.
Ok.
>=20
> (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).
Shameless plug:
did you see http://wiki.freebsd.org/VM6 ?
--WxVlef7wfy3nngmb
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (FreeBSD)
iEYEARECAAYFAkvI2EsACgkQC3+MBN1Mb4iPQwCglrrLhaIGi1BYXrd0ZfdfPwnJ
xMAAn3A3yzD3Gq4l/niHcHBoJEkb5R1+
=kLFe
-----END PGP SIGNATURE-----
--WxVlef7wfy3nngmb--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100416213612.GO2415>