move vnode creation from zfs_znode_cache_constructor to zfs_znode_alloc

Andriy Gapon avg at FreeBSD.org
Sun Feb 3 17:32:00 UTC 2013


Please review the following proposed change.
Thank you.

commit db54bca1cc711e5632b26da3f9591ca62a9d7c87
Author: Andriy Gapon <avg at icyb.net.ua>
Date:   Fri Nov 16 11:54:04 2012 +0200

    zfs: move vnode creation from zfs_znode_cache_constructor to zfs_znode_alloc

    All other places where a znode is allocated do not need z_vnode at all.
    These are:
    - zfs_create_share_dir
    - zfs_create_fs

    This chnage ensures two things:
    - VN_LOCK_ASHARE is not erroneously called for VFIFO vnodes
    - vn_lock is called on a fully constructed vnode with correct v_ops

    The change also allows to make zfs_znode_cache_constructor a normal kmem_cache
    constructor again (as it is in upstream).
    This allows to avoid a potential problem where zfs_znode_cache_destructor
    may be called on un-constructed znodes.

    With the new arrangement we can call getnewvnode after all the necessary
    znode checks thus removing a need for zfs_vnode_forget.
    This change required moving vnode manipulations from zfs_znode_sa_init to
    zfs_znode_alloc.

diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c
b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c
index 7d7b20a..c2ea9ef 100644
--- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_znode.c
@@ -113,37 +113,12 @@ extern struct vop_vector zfs_vnodeops;
 extern struct vop_vector zfs_fifoops;
 extern struct vop_vector zfs_shareops;

-/*
- * XXX: We cannot use this function as a cache constructor, because
- *      there is one global cache for all file systems and we need
- *      to pass vfsp here, which is not possible, because argument
- *      'cdrarg' is defined at kmem_cache_create() time.
- */
-/*ARGSUSED*/
 static int
 zfs_znode_cache_constructor(void *buf, void *arg, int kmflags)
 {
 	znode_t *zp = buf;
-	vnode_t *vp;
-	vfs_t *vfsp = arg;
-	int error;

 	POINTER_INVALIDATE(&zp->z_zfsvfs);
-	ASSERT(!POINTER_IS_VALID(zp->z_zfsvfs));
-
-	if (vfsp != NULL) {
-		error = getnewvnode("zfs", vfsp, &zfs_vnodeops, &vp);
-		if (error != 0 && (kmflags & KM_NOSLEEP))
-			return (-1);
-		ASSERT(error == 0);
-		vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
-		zp->z_vnode = vp;
-		vp->v_data = (caddr_t)zp;
-		VN_LOCK_AREC(vp);
-		VN_LOCK_ASHARE(vp);
-	} else {
-		zp->z_vnode = NULL;
-	}

 	list_link_init(&zp->z_link_node);

@@ -158,7 +133,9 @@ zfs_znode_cache_constructor(void *buf, void *arg, int kmflags)

 	zp->z_dirlocks = NULL;
 	zp->z_acl_cached = NULL;
+	zp->z_vnode = NULL;
 	zp->z_moved = 0;
+
 	return (0);
 }

@@ -377,7 +354,7 @@ zfs_znode_init(void)
 	rw_init(&zfsvfs_lock, NULL, RW_DEFAULT, NULL);
 	ASSERT(znode_cache == NULL);
 	znode_cache = kmem_cache_create("zfs_znode_cache",
-	    sizeof (znode_t), 0, /* zfs_znode_cache_constructor */ NULL,
+	    sizeof (znode_t), 0, zfs_znode_cache_constructor,
 	    zfs_znode_cache_destructor, NULL, NULL, NULL, 0);
 	kmem_cache_set_move(znode_cache, zfs_znode_move);
 }
@@ -501,7 +478,9 @@ zfs_create_share_dir(zfsvfs_t *zfsvfs, dmu_tx_t *tx)
 	zfs_acl_ids_t acl_ids;
 	vattr_t vattr;
 	znode_t *sharezp;
-	vnode_t *vp, vnode;
+#ifdef sun
+	vnode_t *vp;
+#endif
 	znode_t *zp;
 	int error;

@@ -512,7 +491,6 @@ zfs_create_share_dir(zfsvfs_t *zfsvfs, dmu_tx_t *tx)
 	vattr.va_gid = crgetgid(kcred);

 	sharezp = kmem_cache_alloc(znode_cache, KM_SLEEP);
-	zfs_znode_cache_constructor(sharezp, zfsvfs->z_parent->z_vfs, 0);
 	ASSERT(!POINTER_IS_VALID(sharezp->z_zfsvfs));
 	sharezp->z_moved = 0;
 	sharezp->z_unlinked = 0;
@@ -520,11 +498,10 @@ zfs_create_share_dir(zfsvfs_t *zfsvfs, dmu_tx_t *tx)
 	sharezp->z_zfsvfs = zfsvfs;
 	sharezp->z_is_sa = zfsvfs->z_use_sa;

-	sharezp->z_vnode = &vnode;
-	vnode.v_data = sharezp;
-
+#ifdef sun
 	vp = ZTOV(sharezp);
 	vp->v_type = VDIR;
+#endif

 	VERIFY(0 == zfs_acl_ids_create(sharezp, IS_ROOT_NODE, &vattr,
 	    kcred, NULL, &acl_ids));
@@ -536,12 +513,7 @@ zfs_create_share_dir(zfsvfs_t *zfsvfs, dmu_tx_t *tx)
 	zfsvfs->z_shares_dir = sharezp->z_id;

 	zfs_acl_ids_free(&acl_ids);
-	ZTOV(sharezp)->v_data = NULL;
-	ZTOV(sharezp)->v_count = 0;
-	ZTOV(sharezp)->v_holdcnt = 0;
-	zp->z_vnode = NULL;
 	sa_handle_destroy(sharezp->z_sa_hdl);
-	sharezp->z_vnode = NULL;
 	kmem_cache_free(znode_cache, sharezp);

 	return (error);
@@ -608,11 +580,13 @@ zfs_znode_sa_init(zfsvfs_t *zfsvfs, znode_t *zp,

 	zp->z_is_sa = (obj_type == DMU_OT_SA) ? B_TRUE : B_FALSE;

+#ifdef sun
 	/*
 	 * Slap on VROOT if we are the root znode
 	 */
 	if (zp->z_id == zfsvfs->z_root)
 		ZTOV(zp)->v_flag |= VROOT;
+#endif

 	mutex_exit(&zp->z_lock);
 	vn_exists(ZTOV(zp));
@@ -629,17 +603,6 @@ zfs_znode_dmu_fini(znode_t *zp)
 	zp->z_sa_hdl = NULL;
 }

-static void
-zfs_vnode_forget(vnode_t *vp)
-{
-
-	/* copied from insmntque_stddtr */
-	vp->v_data = NULL;
-	vp->v_op = &dead_vnodeops;
-	vgone(vp);
-	vput(vp);
-}
-
 /*
  * Construct a new znode/vnode and intialize.
  *
@@ -657,9 +620,9 @@ zfs_znode_alloc(zfsvfs_t *zfsvfs, dmu_buf_t *db, int blksz,
 	uint64_t parent;
 	sa_bulk_attr_t bulk[9];
 	int count = 0;
+	int error;

 	zp = kmem_cache_alloc(znode_cache, KM_SLEEP);
-	zfs_znode_cache_constructor(zp, zfsvfs->z_parent->z_vfs, 0);

 	ASSERT(zp->z_dirlocks == NULL);
 	ASSERT(!POINTER_IS_VALID(zp->z_zfsvfs));
@@ -678,8 +641,6 @@ zfs_znode_alloc(zfsvfs_t *zfsvfs, dmu_buf_t *db, int blksz,
 	zp->z_seq = 0x7A4653;
 	zp->z_sync_cnt = 0;

-	vp = ZTOV(zp);
-
 	zfs_znode_sa_init(zfsvfs, zp, db, obj_type, hdl);

 	SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_MODE(zfsvfs), NULL, &mode, 8);
@@ -701,12 +662,27 @@ zfs_znode_alloc(zfsvfs_t *zfsvfs, dmu_buf_t *db, int blksz,
 	if (sa_bulk_lookup(zp->z_sa_hdl, bulk, count) != 0 || zp->z_gen == 0) {
 		if (hdl == NULL)
 			sa_handle_destroy(zp->z_sa_hdl);
-		zfs_vnode_forget(vp);
-		zp->z_vnode = NULL;
 		kmem_cache_free(znode_cache, zp);
 		return (NULL);
 	}

+	error = getnewvnode("zfs", zfsvfs->z_parent->z_vfs, &zfs_vnodeops, &vp);
+	ASSERT(error == 0);
+	if (error != 0) {	/* hm, just in case */
+		kmem_cache_free(znode_cache, zp);
+		return (NULL);
+	}
+	zp->z_vnode = vp;
+	vp->v_data = zp;
+
+#ifdef sun
+	/*
+	 * Slap on VROOT if we are the root znode
+	 */
+	if (zp->z_id == zfsvfs->z_root)
+		vp->v_flag |= VROOT;
+#endif
+
 	zp->z_mode = mode;

 	vp->v_type = IFTOVT((mode_t)mode);
@@ -749,8 +725,6 @@ zfs_znode_alloc(zfsvfs_t *zfsvfs, dmu_buf_t *db, int blksz,
 		break;
 #endif	/* sun */
 	}
-	if (vp->v_type != VFIFO)
-		VN_LOCK_ASHARE(vp);

 	mutex_enter(&zfsvfs->z_znodes_lock);
 	list_insert_tail(&zfsvfs->z_all_znodes, zp);
@@ -762,6 +736,14 @@ zfs_znode_alloc(zfsvfs_t *zfsvfs, dmu_buf_t *db, int blksz,
 	zp->z_zfsvfs = zfsvfs;
 	mutex_exit(&zfsvfs->z_znodes_lock);

+	/*
+	 * Acquire vnode lock before making it available to the world.
+	 */
+	vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
+	VN_LOCK_AREC(vp);
+	if (vp->v_type != VFIFO)
+		VN_LOCK_ASHARE(vp);
+
 	VFS_HOLD(zfsvfs->z_vfs);
 	return (zp);
 }
@@ -1839,7 +1821,9 @@ zfs_create_fs(objset_t *os, cred_t *cr, nvlist_t
*zplprops, dmu_tx_t *tx)
 	int		error;
 	int		i;
 	znode_t		*rootzp = NULL;
-	vnode_t		vnode;
+#ifdef sun
+	vnode_t		*vp;
+#endif
 	vattr_t		vattr;
 	znode_t		*zp;
 	zfs_acl_ids_t	acl_ids;
@@ -1918,16 +1902,17 @@ zfs_create_fs(objset_t *os, cred_t *cr, nvlist_t
*zplprops, dmu_tx_t *tx)
 	bzero(&zfsvfs, sizeof (zfsvfs_t));

 	rootzp = kmem_cache_alloc(znode_cache, KM_SLEEP);
-	zfs_znode_cache_constructor(rootzp, NULL, 0);
 	ASSERT(!POINTER_IS_VALID(rootzp->z_zfsvfs));
 	rootzp->z_moved = 0;
 	rootzp->z_unlinked = 0;
 	rootzp->z_atime_dirty = 0;
 	rootzp->z_is_sa = USE_SA(version, os);

-	vnode.v_type = VDIR;
-	vnode.v_data = rootzp;
-	rootzp->z_vnode = &vnode;
+#ifdef sun
+	vp = ZTOV(rootzp);
+	vn_reinit(vp);
+	vp->v_type = VDIR;
+#endif

 	zfsvfs.z_os = os;
 	zfsvfs.z_parent = &zfsvfs;
@@ -1966,7 +1951,6 @@ zfs_create_fs(objset_t *os, cred_t *cr, nvlist_t
*zplprops, dmu_tx_t *tx)
 	POINTER_INVALIDATE(&rootzp->z_zfsvfs);

 	sa_handle_destroy(rootzp->z_sa_hdl);
-	rootzp->z_vnode = NULL;
 	kmem_cache_free(znode_cache, rootzp);

 	/*

-- 
Andriy Gapon


More information about the zfs-devel mailing list