Directory xattr znodes hold a reference on their parent

Unlike normal file or directory znodes, an xattr znode is
guaranteed to only have a single parent.  Therefore, we can
take a refernce on that parent if it is provided at create
time and cache it.  Additionally, we take care to cache it
on any subsequent zfs_zaccess() where the parent is provided
as an optimization.

This allows us to avoid needing to do a zfs_zget() when
setting up the SELinux security xattr in the create path.
This is critical because a hash lookup on the directory
will deadlock since it is locked.

The zpl_xattr_security_init() call has also been moved up
to the zpl layer to ensure TXs to create the required
xattrs are performed after the create TX.  Otherwise we
run the risk of deadlocking on the open create TX.

Ideally the security xattr should be fully constructed
before the new inode is unlocked.  However, doing so would
require far more extensive changes to ZFS.

This change may also have the benefitial side effect of
ensuring xattr directory znodes are evicted from the cache
before normal file or directory znodes due to the extra
reference.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #671

diff --git a/include/sys/zfs_znode.h b/include/sys/zfs_znode.h
index 8af5798ba8f18479407b40833c3a1ce435c2ea52..0b75d52955ae95b82b94231f5bcc7b8f729e47be 100644 (file)
--- a/include/sys/zfs_znode.h
+++ b/include/sys/zfs_znode.h
@@ -209,6 +209,7 @@ 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 98c0019503cd579443ba693c1a8a0cd95f719203..db6331ea595c665dd6d6d5b876a4e5cabed352ba 100644 (file)
--- a/module/zfs/zfs_acl.c
+++ b/module/zfs/zfs_acl.c
@@ -2453,32 +2453,52 @@ 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 (is_attr) {
+       if ((zp->z_pflags & ZFS_XATTR) && S_ISDIR(ZTOI(zp)->i_mode)) {
                uint64_t        parent;
 
-               if ((error = sa_lookup(zp->z_sa_hdl,
-                   SA_ZPL_PARENT(ZTOZSB(zp)), &parent,
-                   sizeof (parent))) != 0)
-                       return (error);
+               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);
 
-               if ((error = zfs_zget(ZTOZSB(zp),
-                   parent, &xzp)) != 0)        {
-                       return (error);
-               }
+                       /*
+                        * 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);
 
-               check_zp = xzp;
+                       error = sa_lookup(zp->z_sa_hdl, SA_ZPL_PARENT(
+                           ZTOZSB(zp)), &parent, sizeof (parent));
+                       if (error)
+                               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);
+               }
 
                /*
                 * fixup mode to map to xattr perms
@@ -2521,15 +2541,11 @@ 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);
        }
 
@@ -2590,10 +2606,6 @@ 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 885d2245907dcd5c020fbbfcb8f1c52dba85f280..8074f1d008c4c7d1d5505845955b2ccbb2210831 100644 (file)
--- a/module/zfs/zfs_znode.c
+++ b/module/zfs/zfs_znode.c
@@ -116,6 +116,7 @@ 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);
 }
@@ -138,6 +139,7 @@ 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);
 }
 
 void
@@ -286,6 +288,11 @@ zfs_inode_destroy(struct inode *ip)
                zp->z_xattr_cached = NULL;
        }
 
+       if (zp->z_xattr_parent) {
+               iput(ZTOI(zp->z_xattr_parent));
+               zp->z_xattr_parent = NULL;
+       }
+
        kmem_cache_free(znode_cache, zp);
 }
 
@@ -359,6 +366,7 @@ 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;
@@ -394,6 +402,14 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
                goto error;
        }
 
+       /*
+        * 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);
@@ -401,12 +417,8 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
        if (insert_inode_locked(ip))
                goto error;
 
-       if (dentry) {
-               if (zpl_xattr_security_init(ip, dip, &dentry->d_name))
-                       goto error;
-
+       if (dentry)
                d_instantiate(dentry, ip);
-       }
 
        mutex_enter(&zsb->z_znodes_lock);
        list_insert_tail(&zsb->z_all_znodes, zp);

diff --git a/module/zfs/zpl_inode.c b/module/zfs/zpl_inode.c
index bb389f8375ea25b6de94495588de7ed84546d094..6175c2e93f8e4c2fe168f86a651a6764b3f8fc57 100644 (file)
--- a/module/zfs/zpl_inode.c
+++ b/module/zfs/zpl_inode.c
@@ -92,8 +92,12 @@ zpl_create(struct inode *dir, struct dentry *dentry, zpl_umode_t mode,
        vap = kmem_zalloc(sizeof(vattr_t), KM_SLEEP);
        zpl_vap_init(vap, dir, dentry, mode, cr);
 
-       error = -zfs_create(dir, (char *)dentry->d_name.name,
-           vap, 0, mode, &ip, cr, 0, NULL);
+       error = -zfs_create(dir, dname(dentry), vap, 0, mode, &ip, cr, 0, NULL);
+       if (error == 0) {
+               error = zpl_xattr_security_init(ip, dir, &dentry->d_name);
+               VERIFY3S(error, ==, 0);
+       }
+
        kmem_free(vap, sizeof(vattr_t));
        crfree(cr);
        ASSERT3S(error, <=, 0);