Kill zp->z_xattr_parent to prevent pinning

zp->z_xattr_parent will pin the parent. This will cause huge issue when unlink a file with xattr. Because the unlinked file is pinned, it will never get purged immediately. And because of that, the xattr stuff will never be marked as unlinked. So the whole unlinked stuff will stay there until shrink cache or umount. Signed-off-by: Chunwei Chen <[email protected]>
openzfs · Jul 11, 2016 · 47c3348 · 47c3348
1 parent 79322dd
commit 47c3348
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 56 deletions.
diff --git a/include/sys/zfs_znode.h b/include/sys/zfs_znode.h
@@ -196,7 +196,6 @@ typedef struct znode {
 	zfs_acl_t	*z_acl_cached;	/* cached acl */
 	krwlock_t	z_xattr_lock;	/* xattr data lock */
 	nvlist_t	*z_xattr_cached; /* cached xattrs */
-	struct znode	*z_xattr_parent; /* xattr parent znode */
 	list_node_t	z_link_node;	/* all znodes in fs link */
 	sa_handle_t	*z_sa_hdl;	/* handle to sa data */
 	boolean_t	z_is_sa;	/* are we native sa? */

diff --git a/module/zfs/zfs_acl.c b/module/zfs/zfs_acl.c
@@ -2471,53 +2471,33 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
 {
 	uint32_t	working_mode;
 	int		error;
+	int		is_attr;
 	boolean_t	check_privs;
+	znode_t		*xzp;
 	znode_t		*check_zp = zp;
 	mode_t		needed_bits;
 	uid_t		owner;
 
+	is_attr = ((zp->z_pflags & ZFS_XATTR) && S_ISDIR(ZTOI(zp)->i_mode));
+
 	/*
 	 * If attribute then validate against base file
 	 */
-	if ((zp->z_pflags & ZFS_XATTR) && S_ISDIR(ZTOI(zp)->i_mode)) {
+	if (is_attr) {
 		uint64_t	parent;
 
-		rw_enter(&zp->z_xattr_lock, RW_READER);
-		if (zp->z_xattr_parent) {
-			check_zp = zp->z_xattr_parent;
-			rw_exit(&zp->z_xattr_lock);
-
-			/*
-			 * Verify a lookup yields the same znode.
-			 */
-			ASSERT3S(sa_lookup(zp->z_sa_hdl, SA_ZPL_PARENT(
-			    ZTOZSB(zp)), &parent, sizeof (parent)), ==, 0);
-			ASSERT3U(check_zp->z_id, ==, parent);
-		} else {
-			rw_exit(&zp->z_xattr_lock);
-
-			error = sa_lookup(zp->z_sa_hdl, SA_ZPL_PARENT(
-			    ZTOZSB(zp)), &parent, sizeof (parent));
-			if (error)
-				return (error);
+		if ((error = sa_lookup(zp->z_sa_hdl,
+		    SA_ZPL_PARENT(ZTOZSB(zp)), &parent,
+		    sizeof (parent))) != 0)
+			return (error);
 
-			/*
-			 * Cache the lookup on the parent file znode as
-			 * zp->z_xattr_parent and hold a reference.  This
-			 * effectively pins the parent in memory until all
-			 * child xattr znodes have been destroyed and
-			 * release their references in zfs_inode_destroy().
-			 */
-			error = zfs_zget(ZTOZSB(zp), parent, &check_zp);
-			if (error)
-				return (error);
-
-			rw_enter(&zp->z_xattr_lock, RW_WRITER);
-			if (zp->z_xattr_parent == NULL)
-				zp->z_xattr_parent = check_zp;
-			rw_exit(&zp->z_xattr_lock);
+		if ((error = zfs_zget(ZTOZSB(zp),
+		    parent, &xzp)) != 0)	{
+			return (error);
 		}
 
+		check_zp = xzp;
+
 		/*
 		 * fixup mode to map to xattr perms
 		 */
@@ -2559,11 +2539,15 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
 
 	if ((error = zfs_zaccess_common(check_zp, mode, &working_mode,
 	    &check_privs, skipaclchk, cr)) == 0) {
+		if (is_attr)
+			iput(ZTOI(xzp));
 		return (secpolicy_vnode_access2(cr, ZTOI(zp), owner,
 		    needed_bits, needed_bits));
 	}
 
 	if (error && !check_privs) {
+		if (is_attr)
+			iput(ZTOI(xzp));
 		return (error);
 	}
 
@@ -2624,6 +2608,8 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
 		    needed_bits, needed_bits);
 	}
 
+	if (is_attr)
+		iput(ZTOI(xzp));
 	return (error);
 }
 

diff --git a/module/zfs/zfs_znode.c b/module/zfs/zfs_znode.c
@@ -119,7 +119,6 @@ zfs_znode_cache_constructor(void *buf, void *arg, int kmflags)
 	zp->z_dirlocks = NULL;
 	zp->z_acl_cached = NULL;
 	zp->z_xattr_cached = NULL;
-	zp->z_xattr_parent = NULL;
 	zp->z_moved = 0;
 	return (0);
 }
@@ -141,7 +140,6 @@ zfs_znode_cache_destructor(void *buf, void *arg)
 	ASSERT(zp->z_dirlocks == NULL);
 	ASSERT(zp->z_acl_cached == NULL);
 	ASSERT(zp->z_xattr_cached == NULL);
-	ASSERT(zp->z_xattr_parent == NULL);
 }
 
 static int
@@ -433,11 +431,6 @@ zfs_inode_destroy(struct inode *ip)
 		zp->z_xattr_cached = NULL;
 	}
 
-	if (zp->z_xattr_parent) {
-		zfs_iput_async(ZTOI(zp->z_xattr_parent));
-		zp->z_xattr_parent = NULL;
-	}
-
 	kmem_cache_free(znode_cache, zp);
 }
 
@@ -603,7 +596,6 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
 	ASSERT(zp->z_dirlocks == NULL);
 	ASSERT3P(zp->z_acl_cached, ==, NULL);
 	ASSERT3P(zp->z_xattr_cached, ==, NULL);
-	ASSERT3P(zp->z_xattr_parent, ==, NULL);
 	zp->z_moved = 0;
 	zp->z_sa_hdl = NULL;
 	zp->z_unlinked = 0;
@@ -646,14 +638,6 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
 	ip->i_generation = (uint32_t)tmp_gen;
 	set_nlink(ip, (uint32_t)tmp_z_links);
 
-	/*
-	 * xattr znodes hold a reference on their unique parent
-	 */
-	if (dip && zp->z_pflags & ZFS_XATTR) {
-		igrab(dip);
-		zp->z_xattr_parent = ITOZ(dip);
-	}
-
 	ip->i_ino = obj;
 	zfs_inode_update_new(zp);
 	zfs_inode_set_ops(zsb, ip);
@@ -1202,11 +1186,6 @@ zfs_rezget(znode_t *zp)
 		nvlist_free(zp->z_xattr_cached);
 		zp->z_xattr_cached = NULL;
 	}
-
-	if (zp->z_xattr_parent) {
-		zfs_iput_async(ZTOI(zp->z_xattr_parent));
-		zp->z_xattr_parent = NULL;
-	}
 	rw_exit(&zp->z_xattr_lock);
 
 	ASSERT(zp->z_sa_hdl == NULL);