From e89260a1c8851ce05ea04b23606ba438b271d890 Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Thu, 29 Nov 2012 16:10:03 -0800 Subject: [PATCH] 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 Closes #671 --- include/sys/zfs_znode.h | 1 + module/zfs/zfs_acl.c | 56 +++++++++++++++++++++++++---------------- module/zfs/zfs_znode.c | 22 ++++++++++++---- module/zfs/zpl_inode.c | 8 ++++-- 4 files changed, 58 insertions(+), 29 deletions(-) diff --git a/include/sys/zfs_znode.h b/include/sys/zfs_znode.h index 8af5798ba8..0b75d52955 100644 --- 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 98c0019503..db6331ea59 100644 --- a/module/zfs/zfs_acl.c +++ b/module/zfs/zfs_acl.c @@ -2453,33 +2453,53 @@ 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); + + 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); } - check_zp = xzp; - /* * 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 885d224590..8074f1d008 100644 --- 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 bb389f8375..6175c2e93f 100644 --- 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);