Date: Fri, 16 Apr 2010 23:41:17 +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: <20100416204117.GM2415@deviant.kiev.zoral.com.ua> In-Reply-To: <06D5F9F6F655AD4C92E28B662F7F853E039387EF@seaxch09.desktop.isilon.com> References: <06D5F9F6F655AD4C92E28B662F7F853E039387EF@seaxch09.desktop.isilon.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--P+ru8oodRbeKwk31
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Fri, Apr 16, 2010 at 01:23:17PM -0700, Matthew Fleming wrote:
> I'm looking at this panic in vget() on stable/7:
>=20
> if (vp->v_iflag & VI_DOOMED && (flags & LK_RETRY) =3D=3D 0)
> panic("vget: vn_lock failed to return ENOENT\n");
>=20
> 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.
>=20
> This isn't a problem on CURRENT because the code has been changed to not
> allow an empty lock flags.
>=20
> I believe the following is a potential fix is:
>=20
> 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)
Both the analysis and the patch look good.
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.
--P+ru8oodRbeKwk31
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (FreeBSD)
iEYEARECAAYFAkvIy20ACgkQC3+MBN1Mb4jD7gCgh1fiQISRHQEwmULKIjqmdGtL
BS0AoJ2zEfvuq2FFSzDPaEygDNfLPvwu
=9WCG
-----END PGP SIGNATURE-----
--P+ru8oodRbeKwk31--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100416204117.GM2415>