CFR: Fix panic in zfs_umount() due to dropped root vnode refcount

[Will Andrews]

Sounds good to me.  I tried your approach, and it also fixes the problem.
 If you'd like, I'll commit it for you.

Thanks,
--Will.


On Wed, Mar 13, 2013 at 3:57 PM, Andriy Gapon <avg at freebsd.org> wrote:

> on 13/03/2013 23:21 Will Andrews said the following:
> > Hi,
> >
> > Diff:
> > http://people.freebsd.org/~will/zfs/zfs_vfsops.c-fix-unmount-panic.diff
> >
> > Change 660985 by willa at willa_SpectraBSD on 2013/03/08 17:43:49
> >
> >         Fix the zfs unmount panic that is due to a missed refcount drop.
> >
> >         When checking for activity in a non-forced unmount, remember that
> >         we called vflush() releasing the refcount that zfs_mount() placed
> >         on the filesystem's root vnode using VFS_ROOT, and restore it.
> >
> >         Without this change, the next time around, the missing refcount
> >         led to a second release of the root vnode when its refcount was
> >         already zero, and thus the panic occurred while trying to grab
> >         the vnode's interlock, which had been destroyed when the vnode's
> >         refcount dropped to zero.
> >
> >         While I'm here, refactor the unmount failure code to ensure that
> >         the zfs ctldir (i.e. .zfs/.snapshots) is always restored, along
> > with,
> >         if needed, zfsvfs->z_unmounted = B_FALSE.  In short, if the
> unmount
> >         fails, restore all original functionality prior to unmounting.
> >
> > This particular panic manifests easily if you have active NFS I/O on a
> ZFS
> > while trying to do a non-forced unmount.  We've also seen it happen on
> > shutdown occasionally.
> >
> > I would like to commit this or a version of it, to FreeBSD/head next
> week.
>
> I propose to put the whole troublesome if (!(fflag & MS_FORCE)) {... }
> block
> under #ifdef sun -- that's what I have in my tree.
> Solaris uses its own mechanism to decide whether unmounting is allowed, but
> FreeBSD has its own -- vflush success is sufficient.
>
> References:
>
> http://code.metager.de/source/xref/freebsd/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c#1942
>
> http://code.metager.de/source/search?q=&defs=&refs=mnt_ref&path=%2Ffreebsd%2F&hist=&project=freebsd
>
> http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_vfsops.c#1866
>
> Thank you for catching this edge case.
> --
> Andriy Gapon
>