Prevent duplicated xattr between SA and dir

When replacing an xattr would cause overflowing in SA, we would fallback
to xattr dir. However, current implementation don't clear the one in SA,
so we would end up with duplicated SA.

For example, running the following script on an xattr=sa filesystem
would cause duplicated "user.1".

-- dup_xattr.sh begin --
randbase64()
{
        dd if=/dev/urandom bs=1 count=$1 2>/dev/null | openssl enc -a -A
}

file=$1
touch $file
setfattr -h -n user.1 -v `randbase64 5000` $file
setfattr -h -n user.2 -v `randbase64 20000` $file
setfattr -h -n user.3 -v `randbase64 20000` $file
setfattr -h -n user.1 -v `randbase64 20000` $file
getfattr -m. -d $file
-- dup_xattr.sh end --

Also, when a filesystem is switch from xattr=sa to xattr=on, it will
never modify those in SA. This would cause strange behavior like, you
cannot delete an xattr, or setxattr would cause duplicate and the result
would not match when you getxattr.

For example, the following shell sequence.

-- shell begin --
$ sudo zfs set xattr=sa pp/fs0
$ touch zzz
$ setfattr -n user.test -v asdf zzz
$ sudo zfs set xattr=on pp/fs0
$ setfattr -x user.test zzz
setfattr: zzz: No such attribute
$ getfattr -d zzz
user.test="asdf"
$ setfattr -n user.test -v zxcv zzz
$ getfattr -d zzz
user.test="asdf"
user.test="asdf"
-- shell end --

We fix this behavior, by first finding where the xattr resides before
setxattr. Then, after we successfully updated the xattr in one location,
we will clear the other location. Note that, because update and clear
are not in single tx, we could still end up with duplicated xattr. But
by doing setxattr again, it can be fixed.

Signed-off-by: Chunwei Chen <[email protected]>
Closes #3472
Closes #4153

1 files changed, 68 insertions, 5 deletions

diff --git a/module/zfs/zpl_xattr.c b/module/zfs/zpl_xattr.c
index 253ea9661..e39d94eaa 100644
--- a/module/zfs/zpl_xattr.c
+++ b/module/zfs/zpl_xattr.c
@@ -337,6 +337,45 @@ out:
 	return (error);
 }
 
+#define	XATTR_NOENT	0x0
+#define	XATTR_IN_SA	0x1
+#define	XATTR_IN_DIR	0x2
+/* check where the xattr resides */
+static int
+__zpl_xattr_where(struct inode *ip, const char *name, int *where, cred_t *cr)
+{
+	znode_t *zp = ITOZ(ip);
+	zfs_sb_t *zsb = ZTOZSB(zp);
+	int error;
+
+	ASSERT(where);
+	ASSERT(RW_LOCK_HELD(&zp->z_xattr_lock));
+
+	*where = XATTR_NOENT;
+	if (zsb->z_use_sa && zp->z_is_sa) {
+		error = zpl_xattr_get_sa(ip, name, NULL, 0);
+		if (error >= 0)
+			*where |= XATTR_IN_SA;
+		else if (error != -ENOENT)
+			return (error);
+	}
+
+	error = zpl_xattr_get_dir(ip, name, NULL, 0, cr);
+	if (error >= 0)
+		*where |= XATTR_IN_DIR;
+	else if (error != -ENOENT)
+		return (error);
+
+	if (*where == (XATTR_IN_SA|XATTR_IN_DIR))
+		cmn_err(CE_WARN, "ZFS: inode %p has xattr \"%s\""
+		    " in both SA and dir", ip, name);
+	if (*where == XATTR_NOENT)
+		error = -ENODATA;
+	else
+		error = 0;
+	return (error);
+}
+
 static int
 zpl_xattr_get(struct inode *ip, const char *name, void *value, size_t size)
 {
@@ -449,7 +488,15 @@ zpl_xattr_set_sa(struct inode *ip, const char *name, const void *value,
 	znode_t *zp = ITOZ(ip);
 	nvlist_t *nvl;
 	size_t sa_size;
-	int error;
+	int error = 0;
+
+	mutex_enter(&zp->z_lock);
+	if (zp->z_xattr_cached == NULL)
+		error = -zfs_sa_get_xattr(zp);
+	mutex_exit(&zp->z_lock);
+
+	if (error)
+		return (error);
 
 	ASSERT(zp->z_xattr_cached);
 	nvl = zp->z_xattr_cached;
@@ -501,6 +548,7 @@ zpl_xattr_set(struct inode *ip, const char *name, const void *value,
 	zfs_sb_t *zsb = ZTOZSB(zp);
 	cred_t *cr = CRED();
 	fstrans_cookie_t cookie;
+	int where;
 	int error;
 
 	crhold(cr);
@@ -514,12 +562,14 @@ zpl_xattr_set(struct inode *ip, const char *name, const void *value,
 	 *
 	 *   XATTR_CREATE: fail if xattr already exists
 	 *   XATTR_REPLACE: fail if xattr does not exist
+	 *
+	 * We also want to know if it resides in sa or dir, so we can make
+	 * sure we don't end up with duplicate in both places.
 	 */
-	error = __zpl_xattr_get(ip, name, NULL, 0, cr);
+	error = __zpl_xattr_where(ip, name, &where, cr);
 	if (error < 0) {
 		if (error != -ENODATA)
 			goto out;
-
 		if (flags & XATTR_REPLACE)
 			goto out;
 
@@ -534,13 +584,26 @@ zpl_xattr_set(struct inode *ip, const char *name, const void *value,
 	}
 
 	/* Preferentially store the xattr as a SA for better performance */
-	if (zsb->z_use_sa && zsb->z_xattr_sa && zp->z_is_sa) {
+	if (zsb->z_use_sa && zp->z_is_sa &&
+	    (zsb->z_xattr_sa || (value == NULL && where & XATTR_IN_SA))) {
 		error = zpl_xattr_set_sa(ip, name, value, size, flags, cr);
-		if (error == 0)
+		if (error == 0) {
+			/*
+			 * Successfully put into SA, we need to clear the one
+			 * in dir.
+			 */
+			if (where & XATTR_IN_DIR)
+				zpl_xattr_set_dir(ip, name, NULL, 0, 0, cr);
 			goto out;
+		}
 	}
 
 	error = zpl_xattr_set_dir(ip, name, value, size, flags, cr);
+	/*
+	 * Successfully put into dir, we need to clear the one in SA.
+	 */
+	if (error == 0 && (where & XATTR_IN_SA))
+		zpl_xattr_set_sa(ip, name, NULL, 0, 0, cr);
 out:
 	rw_exit(&ITOZ(ip)->z_xattr_lock);
 	rrm_exit(&(zsb)->z_teardown_lock, FTAG);