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 <david.chen@osnexus.com>
Closes #3472
Closes #4153

diff --git a/module/zfs/zpl_xattr.c b/module/zfs/zpl_xattr.c
index 253ea966179a92bc3699b2d0f90e48c7eec4766e..e39d94eaaacc39da0b6ad0128d2bf7ff5147ccf8 100644 (file)
--- 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);