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) == 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 = vn_lock(vp, flags | LK_INTERLOCK, td)) != 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) == 0) {
> + KASSERT((flags & LK_TYPE_MASK) == 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) == 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) == 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.
pgpdnqu5rpluO.pgp
Description: PGP signature
