[PATCH][X][SRU] UBUNTU: SAUCE: (noup) Update zfs to 0.6.5.6-0ubuntu28

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][X][SRU] UBUNTU: SAUCE: (noup) Update zfs to 0.6.5.6-0ubuntu28

Colin Ian King-2
From: Colin Ian King <[hidden email]>

BugLink: https://launchpad.net/bugs/1839521

Sync ZFS 0.6.5.6-0ubuntu28 to Xenial:

Fix shrinker deadlock with xattrs (LP: #1839521)

Upstream ZFS fix 31b6111fd92a ("Kill zp->z_xattr_parent to prevent pinning")
and ddae16a9cf0b ("xattr dir doesn't get purged during iput") fix a deadlock
in shrinker path when a xattr directory inode and its xattr inode are in the
same disposal list and the xattr dir inode is evicted before the xattr inode.

Signed-off-by: Colin Ian King <[hidden email]>
---
 zfs/META                    |  2 +-
 zfs/include/sys/zfs_znode.h |  1 -
 zfs/module/zfs/zfs_acl.c    | 59 ++++++++++++++++++---------------------------
 zfs/module/zfs/zfs_dir.c    |  3 ++-
 zfs/module/zfs/zfs_znode.c  | 29 +++-------------------
 5 files changed, 29 insertions(+), 65 deletions(-)

diff --git a/zfs/META b/zfs/META
index 0ec36d1..2574764 100644
--- a/zfs/META
+++ b/zfs/META
@@ -2,7 +2,7 @@ Meta:         1
 Name:         zfs
 Branch:       1.0
 Version:      0.6.5.6
-Release:      0ubuntu26
+Release:      0ubuntu28
 Release-Tags: relext
 License:      CDDL
 Author:       OpenZFS on Linux
diff --git a/zfs/include/sys/zfs_znode.h b/zfs/include/sys/zfs_znode.h
index c03bef5..abeb2ee 100644
--- a/zfs/include/sys/zfs_znode.h
+++ b/zfs/include/sys/zfs_znode.h
@@ -209,7 +209,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? */
diff --git a/zfs/module/zfs/zfs_acl.c b/zfs/module/zfs/zfs_acl.c
index a208dea..bbb0193 100644
--- a/zfs/module/zfs/zfs_acl.c
+++ b/zfs/module/zfs/zfs_acl.c
@@ -2473,53 +2473,33 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
 {
  uint32_t working_mode;
  int error;
- boolean_t check_privs;
- znode_t *check_zp = zp;
+ 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
  */
@@ -2561,11 +2541,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);
  }
 
@@ -2626,6 +2610,9 @@ 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/zfs/module/zfs/zfs_dir.c b/zfs/module/zfs/zfs_dir.c
index c1eadd0..b0c8e36 100644
--- a/zfs/module/zfs/zfs_dir.c
+++ b/zfs/module/zfs/zfs_dir.c
@@ -593,7 +593,7 @@ zfs_purgedir(znode_t *dzp)
  if (error)
  skipped += 1;
  dmu_tx_commit(tx);
-
+ set_nlink(ZTOI(xzp), xzp->z_links);
  zfs_iput_async(ZTOI(xzp));
  }
  zap_cursor_fini(&zc);
@@ -694,6 +694,7 @@ zfs_rmnode(znode_t *zp)
  mutex_enter(&xzp->z_lock);
  xzp->z_unlinked = B_TRUE; /* mark xzp for deletion */
  xzp->z_links = 0; /* no more links to it */
+ set_nlink(ZTOI(xzp), 0); /* this will let iput purge us */
  VERIFY(0 == sa_update(xzp->z_sa_hdl, SA_ZPL_LINKS(zsb),
     &xzp->z_links, sizeof (xzp->z_links), tx));
  mutex_exit(&xzp->z_lock);
diff --git a/zfs/module/zfs/zfs_znode.c b/zfs/module/zfs/zfs_znode.c
index 860354b..dc42d26 100644
--- a/zfs/module/zfs/zfs_znode.c
+++ b/zfs/module/zfs/zfs_znode.c
@@ -120,7 +120,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);
 }
@@ -143,7 +142,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
@@ -435,11 +433,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);
 }
 
@@ -501,8 +494,7 @@ zfs_inode_set_ops(zfs_sb_t *zsb, struct inode *ip)
  */
 static znode_t *
 zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
-    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl,
-    struct inode *dip)
+    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl)
 {
  znode_t *zp;
  struct inode *ip;
@@ -521,7 +513,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;
@@ -560,14 +551,6 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
 
  zp->z_mode = mode;
 
- /*
- * 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);
@@ -913,8 +896,7 @@ zfs_mknode(znode_t *dzp, vattr_t *vap, dmu_tx_t *tx, cred_t *cr,
  VERIFY(sa_replace_all_by_template(sa_hdl, sa_attrs, cnt, tx) == 0);
 
  if (!(flag & IS_ROOT_NODE)) {
- *zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl,
-    ZTOI(dzp));
+ *zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl);
  VERIFY(*zpp != NULL);
  VERIFY(dzp != NULL);
  } else {
@@ -1124,7 +1106,7 @@ again:
  * bonus buffer.
  */
  zp = zfs_znode_alloc(zsb, db, doi.doi_data_block_size,
-    doi.doi_bonus_type, obj_num, NULL, NULL);
+    doi.doi_bonus_type, obj_num, NULL);
  if (zp == NULL) {
  err = SET_ERROR(ENOENT);
  } else {
@@ -1162,11 +1144,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);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH][X][SRU] UBUNTU: SAUCE: (noup) Update zfs to 0.6.5.6-0ubuntu28

Kleber Souza
On 09.08.19 12:29, Colin King wrote:

> From: Colin Ian King <[hidden email]>
>
> BugLink: https://launchpad.net/bugs/1839521
>
> Sync ZFS 0.6.5.6-0ubuntu28 to Xenial:
>
> Fix shrinker deadlock with xattrs (LP: #1839521)
>
> Upstream ZFS fix 31b6111fd92a ("Kill zp->z_xattr_parent to prevent pinning")
> and ddae16a9cf0b ("xattr dir doesn't get purged during iput") fix a deadlock
> in shrinker path when a xattr directory inode and its xattr inode are in the
> same disposal list and the xattr dir inode is evicted before the xattr inode.
>
> Signed-off-by: Colin Ian King <[hidden email]>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

> ---
>  zfs/META                    |  2 +-
>  zfs/include/sys/zfs_znode.h |  1 -
>  zfs/module/zfs/zfs_acl.c    | 59 ++++++++++++++++++---------------------------
>  zfs/module/zfs/zfs_dir.c    |  3 ++-
>  zfs/module/zfs/zfs_znode.c  | 29 +++-------------------
>  5 files changed, 29 insertions(+), 65 deletions(-)
>
> diff --git a/zfs/META b/zfs/META
> index 0ec36d1..2574764 100644
> --- a/zfs/META
> +++ b/zfs/META
> @@ -2,7 +2,7 @@ Meta:         1
>  Name:         zfs
>  Branch:       1.0
>  Version:      0.6.5.6
> -Release:      0ubuntu26
> +Release:      0ubuntu28
>  Release-Tags: relext
>  License:      CDDL
>  Author:       OpenZFS on Linux
> diff --git a/zfs/include/sys/zfs_znode.h b/zfs/include/sys/zfs_znode.h
> index c03bef5..abeb2ee 100644
> --- a/zfs/include/sys/zfs_znode.h
> +++ b/zfs/include/sys/zfs_znode.h
> @@ -209,7 +209,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? */
> diff --git a/zfs/module/zfs/zfs_acl.c b/zfs/module/zfs/zfs_acl.c
> index a208dea..bbb0193 100644
> --- a/zfs/module/zfs/zfs_acl.c
> +++ b/zfs/module/zfs/zfs_acl.c
> @@ -2473,53 +2473,33 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
>  {
>   uint32_t working_mode;
>   int error;
> - boolean_t check_privs;
> - znode_t *check_zp = zp;
> + 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
>   */
> @@ -2561,11 +2541,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);
>   }
>  
> @@ -2626,6 +2610,9 @@ 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/zfs/module/zfs/zfs_dir.c b/zfs/module/zfs/zfs_dir.c
> index c1eadd0..b0c8e36 100644
> --- a/zfs/module/zfs/zfs_dir.c
> +++ b/zfs/module/zfs/zfs_dir.c
> @@ -593,7 +593,7 @@ zfs_purgedir(znode_t *dzp)
>   if (error)
>   skipped += 1;
>   dmu_tx_commit(tx);
> -
> + set_nlink(ZTOI(xzp), xzp->z_links);
>   zfs_iput_async(ZTOI(xzp));
>   }
>   zap_cursor_fini(&zc);
> @@ -694,6 +694,7 @@ zfs_rmnode(znode_t *zp)
>   mutex_enter(&xzp->z_lock);
>   xzp->z_unlinked = B_TRUE; /* mark xzp for deletion */
>   xzp->z_links = 0; /* no more links to it */
> + set_nlink(ZTOI(xzp), 0); /* this will let iput purge us */
>   VERIFY(0 == sa_update(xzp->z_sa_hdl, SA_ZPL_LINKS(zsb),
>      &xzp->z_links, sizeof (xzp->z_links), tx));
>   mutex_exit(&xzp->z_lock);
> diff --git a/zfs/module/zfs/zfs_znode.c b/zfs/module/zfs/zfs_znode.c
> index 860354b..dc42d26 100644
> --- a/zfs/module/zfs/zfs_znode.c
> +++ b/zfs/module/zfs/zfs_znode.c
> @@ -120,7 +120,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);
>  }
> @@ -143,7 +142,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
> @@ -435,11 +433,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);
>  }
>  
> @@ -501,8 +494,7 @@ zfs_inode_set_ops(zfs_sb_t *zsb, struct inode *ip)
>   */
>  static znode_t *
>  zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
> -    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl,
> -    struct inode *dip)
> +    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl)
>  {
>   znode_t *zp;
>   struct inode *ip;
> @@ -521,7 +513,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;
> @@ -560,14 +551,6 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
>  
>   zp->z_mode = mode;
>  
> - /*
> - * 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);
> @@ -913,8 +896,7 @@ zfs_mknode(znode_t *dzp, vattr_t *vap, dmu_tx_t *tx, cred_t *cr,
>   VERIFY(sa_replace_all_by_template(sa_hdl, sa_attrs, cnt, tx) == 0);
>  
>   if (!(flag & IS_ROOT_NODE)) {
> - *zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl,
> -    ZTOI(dzp));
> + *zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl);
>   VERIFY(*zpp != NULL);
>   VERIFY(dzp != NULL);
>   } else {
> @@ -1124,7 +1106,7 @@ again:
>   * bonus buffer.
>   */
>   zp = zfs_znode_alloc(zsb, db, doi.doi_data_block_size,
> -    doi.doi_bonus_type, obj_num, NULL, NULL);
> +    doi.doi_bonus_type, obj_num, NULL);
>   if (zp == NULL) {
>   err = SET_ERROR(ENOENT);
>   } else {
> @@ -1162,11 +1144,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);
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH][X][SRU] UBUNTU: SAUCE: (noup) Update zfs to 0.6.5.6-0ubuntu28

Stefan Bader-2
In reply to this post by Colin Ian King-2
On 09.08.19 12:29, Colin King wrote:

> From: Colin Ian King <[hidden email]>
>
> BugLink: https://launchpad.net/bugs/1839521
>
> Sync ZFS 0.6.5.6-0ubuntu28 to Xenial:
>
> Fix shrinker deadlock with xattrs (LP: #1839521)
>
> Upstream ZFS fix 31b6111fd92a ("Kill zp->z_xattr_parent to prevent pinning")
> and ddae16a9cf0b ("xattr dir doesn't get purged during iput") fix a deadlock
> in shrinker path when a xattr directory inode and its xattr inode are in the
> same disposal list and the xattr dir inode is evicted before the xattr inode.
>
> Signed-off-by: Colin Ian King <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>

> ---
>  zfs/META                    |  2 +-
>  zfs/include/sys/zfs_znode.h |  1 -
>  zfs/module/zfs/zfs_acl.c    | 59 ++++++++++++++++++---------------------------
>  zfs/module/zfs/zfs_dir.c    |  3 ++-
>  zfs/module/zfs/zfs_znode.c  | 29 +++-------------------
>  5 files changed, 29 insertions(+), 65 deletions(-)
>
> diff --git a/zfs/META b/zfs/META
> index 0ec36d1..2574764 100644
> --- a/zfs/META
> +++ b/zfs/META
> @@ -2,7 +2,7 @@ Meta:         1
>  Name:         zfs
>  Branch:       1.0
>  Version:      0.6.5.6
> -Release:      0ubuntu26
> +Release:      0ubuntu28
>  Release-Tags: relext
>  License:      CDDL
>  Author:       OpenZFS on Linux
> diff --git a/zfs/include/sys/zfs_znode.h b/zfs/include/sys/zfs_znode.h
> index c03bef5..abeb2ee 100644
> --- a/zfs/include/sys/zfs_znode.h
> +++ b/zfs/include/sys/zfs_znode.h
> @@ -209,7 +209,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? */
> diff --git a/zfs/module/zfs/zfs_acl.c b/zfs/module/zfs/zfs_acl.c
> index a208dea..bbb0193 100644
> --- a/zfs/module/zfs/zfs_acl.c
> +++ b/zfs/module/zfs/zfs_acl.c
> @@ -2473,53 +2473,33 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
>  {
>   uint32_t working_mode;
>   int error;
> - boolean_t check_privs;
> - znode_t *check_zp = zp;
> + 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
>   */
> @@ -2561,11 +2541,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);
>   }
>  
> @@ -2626,6 +2610,9 @@ 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/zfs/module/zfs/zfs_dir.c b/zfs/module/zfs/zfs_dir.c
> index c1eadd0..b0c8e36 100644
> --- a/zfs/module/zfs/zfs_dir.c
> +++ b/zfs/module/zfs/zfs_dir.c
> @@ -593,7 +593,7 @@ zfs_purgedir(znode_t *dzp)
>   if (error)
>   skipped += 1;
>   dmu_tx_commit(tx);
> -
> + set_nlink(ZTOI(xzp), xzp->z_links);
>   zfs_iput_async(ZTOI(xzp));
>   }
>   zap_cursor_fini(&zc);
> @@ -694,6 +694,7 @@ zfs_rmnode(znode_t *zp)
>   mutex_enter(&xzp->z_lock);
>   xzp->z_unlinked = B_TRUE; /* mark xzp for deletion */
>   xzp->z_links = 0; /* no more links to it */
> + set_nlink(ZTOI(xzp), 0); /* this will let iput purge us */
>   VERIFY(0 == sa_update(xzp->z_sa_hdl, SA_ZPL_LINKS(zsb),
>      &xzp->z_links, sizeof (xzp->z_links), tx));
>   mutex_exit(&xzp->z_lock);
> diff --git a/zfs/module/zfs/zfs_znode.c b/zfs/module/zfs/zfs_znode.c
> index 860354b..dc42d26 100644
> --- a/zfs/module/zfs/zfs_znode.c
> +++ b/zfs/module/zfs/zfs_znode.c
> @@ -120,7 +120,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);
>  }
> @@ -143,7 +142,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
> @@ -435,11 +433,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);
>  }
>  
> @@ -501,8 +494,7 @@ zfs_inode_set_ops(zfs_sb_t *zsb, struct inode *ip)
>   */
>  static znode_t *
>  zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
> -    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl,
> -    struct inode *dip)
> +    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl)
>  {
>   znode_t *zp;
>   struct inode *ip;
> @@ -521,7 +513,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;
> @@ -560,14 +551,6 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
>  
>   zp->z_mode = mode;
>  
> - /*
> - * 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);
> @@ -913,8 +896,7 @@ zfs_mknode(znode_t *dzp, vattr_t *vap, dmu_tx_t *tx, cred_t *cr,
>   VERIFY(sa_replace_all_by_template(sa_hdl, sa_attrs, cnt, tx) == 0);
>  
>   if (!(flag & IS_ROOT_NODE)) {
> - *zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl,
> -    ZTOI(dzp));
> + *zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl);
>   VERIFY(*zpp != NULL);
>   VERIFY(dzp != NULL);
>   } else {
> @@ -1124,7 +1106,7 @@ again:
>   * bonus buffer.
>   */
>   zp = zfs_znode_alloc(zsb, db, doi.doi_data_block_size,
> -    doi.doi_bonus_type, obj_num, NULL, NULL);
> +    doi.doi_bonus_type, obj_num, NULL);
>   if (zp == NULL) {
>   err = SET_ERROR(ENOENT);
>   } else {
> @@ -1162,11 +1144,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);
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

APPLIED: [PATCH][X][SRU] UBUNTU: SAUCE: (noup) Update zfs to 0.6.5.6-0ubuntu28

Khalid Elmously
In reply to this post by Colin Ian King-2
On 2019-08-09 11:29:17 , Colin King wrote:

> From: Colin Ian King <[hidden email]>
>
> BugLink: https://launchpad.net/bugs/1839521
>
> Sync ZFS 0.6.5.6-0ubuntu28 to Xenial:
>
> Fix shrinker deadlock with xattrs (LP: #1839521)
>
> Upstream ZFS fix 31b6111fd92a ("Kill zp->z_xattr_parent to prevent pinning")
> and ddae16a9cf0b ("xattr dir doesn't get purged during iput") fix a deadlock
> in shrinker path when a xattr directory inode and its xattr inode are in the
> same disposal list and the xattr dir inode is evicted before the xattr inode.
>
> Signed-off-by: Colin Ian King <[hidden email]>
> ---
>  zfs/META                    |  2 +-
>  zfs/include/sys/zfs_znode.h |  1 -
>  zfs/module/zfs/zfs_acl.c    | 59 ++++++++++++++++++---------------------------
>  zfs/module/zfs/zfs_dir.c    |  3 ++-
>  zfs/module/zfs/zfs_znode.c  | 29 +++-------------------
>  5 files changed, 29 insertions(+), 65 deletions(-)
>
> diff --git a/zfs/META b/zfs/META
> index 0ec36d1..2574764 100644
> --- a/zfs/META
> +++ b/zfs/META
> @@ -2,7 +2,7 @@ Meta:         1
>  Name:         zfs
>  Branch:       1.0
>  Version:      0.6.5.6
> -Release:      0ubuntu26
> +Release:      0ubuntu28
>  Release-Tags: relext
>  License:      CDDL
>  Author:       OpenZFS on Linux
> diff --git a/zfs/include/sys/zfs_znode.h b/zfs/include/sys/zfs_znode.h
> index c03bef5..abeb2ee 100644
> --- a/zfs/include/sys/zfs_znode.h
> +++ b/zfs/include/sys/zfs_znode.h
> @@ -209,7 +209,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? */
> diff --git a/zfs/module/zfs/zfs_acl.c b/zfs/module/zfs/zfs_acl.c
> index a208dea..bbb0193 100644
> --- a/zfs/module/zfs/zfs_acl.c
> +++ b/zfs/module/zfs/zfs_acl.c
> @@ -2473,53 +2473,33 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
>  {
>   uint32_t working_mode;
>   int error;
> - boolean_t check_privs;
> - znode_t *check_zp = zp;
> + 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
>   */
> @@ -2561,11 +2541,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);
>   }
>  
> @@ -2626,6 +2610,9 @@ 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/zfs/module/zfs/zfs_dir.c b/zfs/module/zfs/zfs_dir.c
> index c1eadd0..b0c8e36 100644
> --- a/zfs/module/zfs/zfs_dir.c
> +++ b/zfs/module/zfs/zfs_dir.c
> @@ -593,7 +593,7 @@ zfs_purgedir(znode_t *dzp)
>   if (error)
>   skipped += 1;
>   dmu_tx_commit(tx);
> -
> + set_nlink(ZTOI(xzp), xzp->z_links);
>   zfs_iput_async(ZTOI(xzp));
>   }
>   zap_cursor_fini(&zc);
> @@ -694,6 +694,7 @@ zfs_rmnode(znode_t *zp)
>   mutex_enter(&xzp->z_lock);
>   xzp->z_unlinked = B_TRUE; /* mark xzp for deletion */
>   xzp->z_links = 0; /* no more links to it */
> + set_nlink(ZTOI(xzp), 0); /* this will let iput purge us */
>   VERIFY(0 == sa_update(xzp->z_sa_hdl, SA_ZPL_LINKS(zsb),
>      &xzp->z_links, sizeof (xzp->z_links), tx));
>   mutex_exit(&xzp->z_lock);
> diff --git a/zfs/module/zfs/zfs_znode.c b/zfs/module/zfs/zfs_znode.c
> index 860354b..dc42d26 100644
> --- a/zfs/module/zfs/zfs_znode.c
> +++ b/zfs/module/zfs/zfs_znode.c
> @@ -120,7 +120,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);
>  }
> @@ -143,7 +142,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
> @@ -435,11 +433,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);
>  }
>  
> @@ -501,8 +494,7 @@ zfs_inode_set_ops(zfs_sb_t *zsb, struct inode *ip)
>   */
>  static znode_t *
>  zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
> -    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl,
> -    struct inode *dip)
> +    dmu_object_type_t obj_type, uint64_t obj, sa_handle_t *hdl)
>  {
>   znode_t *zp;
>   struct inode *ip;
> @@ -521,7 +513,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;
> @@ -560,14 +551,6 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
>  
>   zp->z_mode = mode;
>  
> - /*
> - * 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);
> @@ -913,8 +896,7 @@ zfs_mknode(znode_t *dzp, vattr_t *vap, dmu_tx_t *tx, cred_t *cr,
>   VERIFY(sa_replace_all_by_template(sa_hdl, sa_attrs, cnt, tx) == 0);
>  
>   if (!(flag & IS_ROOT_NODE)) {
> - *zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl,
> -    ZTOI(dzp));
> + *zpp = zfs_znode_alloc(zsb, db, 0, obj_type, obj, sa_hdl);
>   VERIFY(*zpp != NULL);
>   VERIFY(dzp != NULL);
>   } else {
> @@ -1124,7 +1106,7 @@ again:
>   * bonus buffer.
>   */
>   zp = zfs_znode_alloc(zsb, db, doi.doi_data_block_size,
> -    doi.doi_bonus_type, obj_num, NULL, NULL);
> +    doi.doi_bonus_type, obj_num, NULL);
>   if (zp == NULL) {
>   err = SET_ERROR(ENOENT);
>   } else {
> @@ -1162,11 +1144,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);
> --
> 2.7.4
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team