Skip to content

Commit

Permalink
Kill zp->z_xattr_parent to prevent pinning
Browse files Browse the repository at this point in the history 
zp->z_xattr_parent will pin the parent. This will cause huge issue when unlink
a file with xattr. Because the unlinked file is pinned, it will never get
purged immediately. And because of that, the xattr stuff will never be marked
as unlinked. So the whole unlinked stuff will stay there until shrink cache or
umount.

Signed-off-by: Chunwei Chen <[email protected]>
  • Loading branch information
Chunwei Chen committed Jul 8, 2016
1 parent 2ec87c0 commit 5479e64
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 56 deletions.
1 change: 0 additions & 1 deletion include/sys/zfs_znode.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,6 @@ 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
54 changes: 20 additions & 34 deletions module/zfs/zfs_acl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2471,53 +2471,33 @@ 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 ((zp->z_pflags & ZFS_XATTR) && S_ISDIR(ZTOI(zp)->i_mode)) {
if (is_attr) {
uint64_t parent;

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);

/*
* 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);
if ((error = sa_lookup(zp->z_sa_hdl,
SA_ZPL_PARENT(ZTOZSB(zp)), &parent,
sizeof (parent))) != 0)
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);
if ((error = zfs_zget(ZTOZSB(zp),
parent, &xzp)) != 0) {
return (error);
}

check_zp = xzp;

/*
* fixup mode to map to xattr perms
*/
Expand Down Expand Up @@ -2559,11 +2539,15 @@ 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 @@ -2624,6 +2608,8 @@ 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
21 changes: 0 additions & 21 deletions module/zfs/zfs_znode.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ 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 @@ -141,7 +140,6 @@ 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);
}

static int
Expand Down Expand Up @@ -433,11 +431,6 @@ zfs_inode_destroy(struct inode *ip)
zp->z_xattr_cached = NULL;
}

if (zp->z_xattr_parent) {
zfs_iput_async(ZTOI(zp->z_xattr_parent));
zp->z_xattr_parent = NULL;
}

kmem_cache_free(znode_cache, zp);
}

Expand Down Expand Up @@ -603,7 +596,6 @@ 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 @@ -645,14 +637,6 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
zp->z_mode = mode;
ip->i_generation = (uint32_t)tmp_gen;

/*
* xattr znodes hold a reference on their unique parent
*/
if (dip && zp->z_pflags & ZFS_XATTR) {
igrab(dip);
zp->z_xattr_parent = ITOZ(dip);
}

This comment has been minimized.

Sign in to view

Copy link
@chrisrd

chrisrd Jul 12, 2016

Contributor

dip is now unused: should be remove from arg list and callers
ip->i_ino = obj;
zfs_inode_update_new(zp);
zfs_inode_set_ops(zsb, ip);
Expand Down Expand Up @@ -1200,11 +1184,6 @@ zfs_rezget(znode_t *zp)
nvlist_free(zp->z_xattr_cached);
zp->z_xattr_cached = NULL;
}

if (zp->z_xattr_parent) {
zfs_iput_async(ZTOI(zp->z_xattr_parent));
zp->z_xattr_parent = NULL;
}
rw_exit(&zp->z_xattr_lock);

ASSERT(zp->z_sa_hdl == NULL);
Expand Down

0 comments on commit 5479e64

Please sign in to comment.