Re: kern/41374: getnewvnode deadlock

[Andrew Doran <ad%netbsd.org@localhost>]

On Sun, May 10, 2009 at 09:44:41AM +0000, YAMAMOTO Takashi wrote:

> > How about making XLOCK
> > a flag bit in v_useconut?
> 
> it sounds like a good idea.

On second thought what you propose looks fine to me. Putting a flag bit into
v_usecount would have the advantage that no memory barrier would be required
in vtryget().

> @@ -1783,8 +1794,8 @@ vclean(vnode_t *vp, int flags)
>       KASSERT(vp->v_usecount != 0);
>  
>       /* If cleaning is already in progress wait until done and return. */
> -     if (vp->v_iflag & VI_XLOCK) {
> -             vwait(vp, VI_XLOCK);
> +     if (vp->v_iflag & VI_CLEANING) {
> +             vwait(vp, VI_CLEANING);
>               return;
>       }

Can't this allow a concurrent vclean(), e.g. via revoke or forced unmount?

> @@ -464,6 +464,15 @@ cache_lookup_raw(struct vnode *dvp, stru
>               return ENOENT;
>       }
>       if (vtryget(vp)) {
> +             membar_consumer();
> +             if ((vp->v_iflag & VI_XLOCK) != 0) {
> +                     mutex_exit(&ncp->nc_lock);
> +                     mutex_exit(&cpup->cpu_lock);
> +                     vrele(vp);
> +                     COUNT(cpup->cpu_stats, ncs_falsehits);
> +                     *vpp = NULL;
> +                     return -1;
> +             }
>               mutex_exit(&ncp->nc_lock);
>               mutex_exit(&cpup->cpu_lock);

I spent a while today trying to remember why vget() does not do vtryget().
Of course it's due to VI_FREEING. I wonder if we could change the order of
events in ufs_inactive() to this, allowing us to remove VI_FREEING (I don't
know what ZFS will require):

        UFS_UPDATE(vp, ...);
        ufs_ihashrem(ip);
        UFS_VFREE(vp, ...);

I'm not yet sure if fsck can handle it the other way around.