Skip to content

Commit

Permalink
Directory xattr znodes hold a reference on their parent
Browse files Browse the repository at this point in the history
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 <[email protected]>
Closes #671
  • Loading branch information
behlendorf committed Dec 3, 2012
1 parent 645fb9c commit e89260a
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 29 deletions.
1 change: 1 addition & 0 deletions include/sys/zfs_znode.h
Original file line number Diff line number Diff line change
Expand Up @@ -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? */
Expand Down
56 changes: 34 additions & 22 deletions module/zfs/zfs_acl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand Down
22 changes: 17 additions & 5 deletions module/zfs/zfs_znode.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -394,19 +402,23 @@ 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);

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);
Expand Down
8 changes: 6 additions & 2 deletions module/zfs/zpl_inode.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit e89260a

Please sign in to comment.