Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 5 May 2015 11:01:06 +0000 (UTC)
From:      Andriy Gapon <avg@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r282475 - in head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs: . sys
Message-ID:  <201505051101.t45B16eD012277@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: avg
Date: Tue May  5 11:01:06 2015
New Revision: 282475
URL: https://svnweb.freebsd.org/changeset/base/282475

Log:
  zfs: do not hold an extra reference on a root vnode while a filesystem is mounted
  
  At present zfs_domount() acquires a reference on the filesystem's root vnode
  and that reference is kept until zfs_umount.
  The latter calls vflush(rootrefs = 1) to dispose of the extra reference.
  
  There is no explanation of why that reference is kept - what problem it
  solves or what behavior it improves.
  Also, that logic is FreeBSD specific.
  
  There is one real problem with that reference, though.
  zfs recv -F may receive a full, non-incremental stream to a mounted filesystem.
  In that case the received root object is likely to have a different z_gen
  attribute value. Because of that, zfs_rezget will leave the previous root znode
  and vnode disassociated from the actual object (z_sa_hdl == NULL).
  Thus, future calls to VFS_ROOT() -> zfs_root() will produce a new vnode-znode
  pair, while the old one will be kept alive by the outstanding reference.
  So, the outstanding reference will not actually be for the new root vnode
  (or, more precisely, vnodes - because a root vnode may be recycled and a newer
  one can be created).
  As a result, when vflush(rootrefs = 1) s called there will be two problems:
  
  - a leaked reference on the old root vnode preventing a graceful unmount
  - insufficient references on the actual root vnode leading to a crash upon
    access to the vnode after it is destroyed by vgone() + vdrop()
  
  The second issue will actually override the first one.
  
  Differential Revision:	https://reviews.freebsd.org/D2353
  Reviewed by:		delphij, kib, smh
  MFC after:	17 days

Modified:
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_znode.h
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_znode.h
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_znode.h	Tue May  5 11:00:50 2015	(r282474)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/sys/zfs_znode.h	Tue May  5 11:01:06 2015	(r282475)
@@ -261,10 +261,6 @@ VTOZ(vnode_t *vp)
 		} \
 	}
 
-/* Called on entry to each ZFS vnode and vfs operation that can not return EIO */
-#define	ZFS_ENTER_NOERROR(zfsvfs) \
-	rrm_enter(&(zfsvfs)->z_teardown_lock, RW_READER, FTAG)
-
 /* Must be called before exiting the vop */
 #define	ZFS_EXIT(zfsvfs) rrm_exit(&(zfsvfs)->z_teardown_lock, FTAG)
 

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c	Tue May  5 11:00:50 2015	(r282474)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c	Tue May  5 11:01:06 2015	(r282475)
@@ -1239,9 +1239,6 @@ zfs_domount(vfs_t *vfsp, char *osname)
 	}
 
 	vfs_mountedfrom(vfsp, osname);
-	/* Grab extra reference. */
-	VERIFY(VFS_ROOT(vfsp, LK_EXCLUSIVE, &vp) == 0);
-	VOP_UNLOCK(vp, 0);
 
 	if (!zfsvfs->z_issnap)
 		zfsctl_create(zfsvfs);
@@ -1819,7 +1816,7 @@ zfs_root(vfs_t *vfsp, int flags, vnode_t
 	znode_t *rootzp;
 	int error;
 
-	ZFS_ENTER_NOERROR(zfsvfs);
+	ZFS_ENTER(zfsvfs);
 
 	error = zfs_zget(zfsvfs, zfsvfs->z_root, &rootzp);
 	if (error == 0)
@@ -1994,7 +1991,7 @@ zfs_umount(vfs_t *vfsp, int fflag)
 	/*
 	 * Flush all the files.
 	 */
-	ret = vflush(vfsp, 1, (fflag & MS_FORCE) ? FORCECLOSE : 0, td);
+	ret = vflush(vfsp, 0, (fflag & MS_FORCE) ? FORCECLOSE : 0, td);
 	if (ret != 0) {
 		if (!zfsvfs->z_issnap) {
 			zfsctl_create(zfsvfs);

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201505051101.t45B16eD012277>