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.

This change partially reverts e89260a.  This is safe because only the
zp->z_xattr_parent optimization is removed, zpl_xattr_security_init()
is still called from the zpl outside the inode lock.

Signed-off-by: Chunwei Chen <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Signed-off-by: Chris Dunlop <[email protected]>
Issue #4359
Issue #3508
Issue #4413
Issue #4827

3 files changed, 26 insertions, 63 deletions

diff --git a/include/sys/zfs_znode.h b/include/sys/zfs_znode.h
index 0511c1c51..32ffe07d2 100644
--- a/include/sys/zfs_znode.h
+++ b/include/sys/zfs_znode.h
@@ -209,7 +209,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
index a208dea15..bbb01930f 100644
--- a/module/zfs/zfs_acl.c
+++ b/module/zfs/zfs_acl.c
@@ -2473,53 +2473,33 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
 {
 	uint32_t	working_mode;
 	int		error;
-	boolean_t	check_privs;
-	znode_t		*check_zp = zp;
+	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
 		 */
@@ -2561,11 +2541,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);
 	}
 
@@ -2626,6 +2610,9 @@ 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
index c478b36f1..eac9ce486 100644
--- a/module/zfs/zfs_znode.c
+++ b/module/zfs/zfs_znode.c
@@ -118,7 +118,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);
 }
@@ -140,7 +139,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
@@ -432,11 +430,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);
 }
 
@@ -498,8 +491,7 @@ zfs_inode_set_ops(zfs_sb_t *zsb, struct inode *ip)
  */
 static znode_t *
 zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
-    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl,
-    struct inode *dip)
+    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl)
 {
 	znode_t	*zp;
 	struct inode *ip;
@@ -518,7 +510,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;
@@ -559,14 +550,6 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
 
 	zp->z_mode = mode;
 
-	/*
-	 * 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(zp);
 	zfs_inode_set_ops(zsb, ip);
@@ -912,8 +895,7 @@ zfs_mknode(znode_t *dzp, vattr_t *vap, dmu_tx_t *tx, cred_t *cr,
 	VERIFY(sa_replace_all_by_template(sa_hdl, sa_attrs, cnt, tx) == 0);
 
 	if (!(flag & IS_ROOT_NODE)) {
-		*zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl,
-		    ZTOI(dzp));
+		*zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl);
 		VERIFY(*zpp != NULL);
 		VERIFY(dzp != NULL);
 	} else {
@@ -1123,7 +1105,7 @@ again:
 	 * bonus buffer.
 	 */
 	zp = zfs_znode_alloc(zsb, db, doi.doi_data_block_size,
-	    doi.doi_bonus_type, obj_num, NULL, NULL);
+	    doi.doi_bonus_type, obj_num, NULL);
 	if (zp == NULL) {
 		err = SET_ERROR(ENOENT);
 	} else {
@@ -1171,11 +1153,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);