[PATCHSET v2 0/2] xfs: random fixes for 5.19-rc4

[PATCHSET v2 0/2] xfs: random fixes for 5.19-rc4

[Darrick J. Wong]

Hi all,

Fix some unmount hangs during recovery of larp xattr operations.

v2: clean up function prologue, fix bugs in v1 patch

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-fixes-5.19
---
 fs/xfs/xfs_attr_item.c |   43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

[PATCH 1/2] xfs: always free xattri_leaf_bp when cancelling a deferred op

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

While running the following fstest with logged xattrs DISabled, I
noticed the following:

# FSSTRESS_AVOID="-z -f unlink=1 -f rmdir=1 -f creat=2 -f mkdir=2 -f
getfattr=3 -f listfattr=3 -f attr_remove=4 -f removefattr=4 -f
setfattr=20 -f attr_set=60" ./check generic/475

INFO: task u9:1:40 blocked for more than 61 seconds.
      Tainted: G           O      5.19.0-rc2-djwx #rc2
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:u9:1            state:D stack:12872 pid:   40 ppid:     2 flags:0x00004000
Workqueue: xfs-cil/dm-0 xlog_cil_push_work [xfs]
Call Trace:
 <TASK>
 __schedule+0x2db/0x1110
 schedule+0x58/0xc0
 schedule_timeout+0x115/0x160
 __down_common+0x126/0x210
 down+0x54/0x70
 xfs_buf_lock+0x2d/0xe0 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
 xfs_buf_item_unpin+0x227/0x3a0 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
 xfs_trans_committed_bulk+0x18e/0x320 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
 xlog_cil_committed+0x2ea/0x360 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
 xlog_cil_push_work+0x60f/0x690 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
 process_one_work+0x1df/0x3c0
 worker_thread+0x53/0x3b0
 kthread+0xea/0x110
 ret_from_fork+0x1f/0x30
 </TASK>

This appears to be the result of shortform_to_leaf creating a new leaf
buffer as part of adding an xattr to a file.  The new leaf buffer is
held and attached to the xfs_attr_intent structure, but then the
filesystem shuts down.  Instead of the usual path (which adds the attr
to the held leaf buffer which releases the hold), we instead cancel the
entire deferred operation.

Unfortunately, xfs_attr_cancel_item doesn't release any attached leaf
buffers, so we leak the locked buffer.  The CIL cannot do anything
about that, and hangs.  Fix this by teaching it to release leaf buffers,
and make XFS a little more careful about not leaving a dangling
reference.

The prologue of xfs_attri_item_recover is (in this author's opinion) a
little hard to figure out, so I'll clean that up in the next patch.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_attr_item.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 4a28c2d77070..76a23283ec65 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -455,6 +455,8 @@ static inline void
 xfs_attr_free_item(
 	struct xfs_attr_intent		*attr)
 {
+	ASSERT(attr->xattri_leaf_bp == NULL);
+
 	if (attr->xattri_da_state)
 		xfs_da_state_free(attr->xattri_da_state);
 	xfs_attri_log_nameval_put(attr->xattri_nameval);
@@ -509,6 +511,10 @@ xfs_attr_cancel_item(
 	struct xfs_attr_intent		*attr;
 
 	attr = container_of(item, struct xfs_attr_intent, xattri_list);
+	if (attr->xattri_leaf_bp) {
+		xfs_buf_relse(attr->xattri_leaf_bp);
+		attr->xattri_leaf_bp = NULL;
+	}
 	xfs_attr_free_item(attr);
 }
 
@@ -667,9 +673,21 @@ xfs_attri_item_recover(
 	error = xfs_defer_ops_capture_and_commit(tp, capture_list);
 
 out_unlock:
-	if (attr->xattri_leaf_bp)
+	if (attr->xattri_leaf_bp) {
 		xfs_buf_relse(attr->xattri_leaf_bp);
 
+		/*
+		 * If there's more work to do to complete the attr intent, the
+		 * defer capture structure will have taken its own reference to
+		 * the attr leaf buffer and will give that to the continuation
+		 * transaction.  The attr intent struct drives the continuation
+		 * work, so release our refcount on the attr leaf buffer but
+		 * retain the pointer in the intent structure.
+		 */
+		if (ret != -EAGAIN)
+			attr->xattri_leaf_bp = NULL;
+	}
+
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	xfs_irele(ip);
 out:

[PATCH 2/2] xfs: clean up the end of xfs_attri_item_recover

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The end of this function could use some cleanup -- the EAGAIN
conditionals make it harder to figure out what's going on with the
disposal of xattri_leaf_bp, and the dual error/ret variables aren't
needed.  Turn the EAGAIN case into a separate block documenting all the
subtleties of recovering in the middle of an xattr update chain, which
makes the rest of the prologue much simpler.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_attr_item.c |   45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 76a23283ec65..0561c142b711 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -582,7 +582,7 @@ xfs_attri_item_recover(
 	struct xfs_trans_res		tres;
 	struct xfs_attri_log_format	*attrp;
 	struct xfs_attri_log_nameval	*nv = attrip->attri_nameval;
-	int				error, ret = 0;
+	int				error;
 	int				total;
 	int				local;
 	struct xfs_attrd_log_item	*done_item = NULL;
@@ -658,13 +658,31 @@ xfs_attri_item_recover(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
-	ret = xfs_xattri_finish_update(attr, done_item);
-	if (ret == -EAGAIN) {
-		/* There's more work to do, so add it to this transaction */
+	error = xfs_xattri_finish_update(attr, done_item);
+	if (error == -EAGAIN) {
+		/*
+		 * There's more work to do, so add the intent item to this
+		 * transaction so that we can continue it later.
+		 */
 		xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_ATTR, &attr->xattri_list);
-	} else
-		error = ret;
+		error = xfs_defer_ops_capture_and_commit(tp, capture_list);
+		if (error)
+			goto out_unlock;
 
+		/*
+		 * The defer capture structure took its own reference to the
+		 * attr leaf buffer and will give that to the continuation
+		 * transaction.  The attr intent struct drives the continuation
+		 * work, so release our refcount on the attr leaf buffer but
+		 * retain the pointer in the intent structure.
+		 */
+		if (attr->xattri_leaf_bp)
+			xfs_buf_relse(attr->xattri_leaf_bp);
+
+		xfs_iunlock(ip, XFS_ILOCK_EXCL);
+		xfs_irele(ip);
+		return 0;
+	}
 	if (error) {
 		xfs_trans_cancel(tp);
 		goto out_unlock;
@@ -675,24 +693,13 @@ xfs_attri_item_recover(
 out_unlock:
 	if (attr->xattri_leaf_bp) {
 		xfs_buf_relse(attr->xattri_leaf_bp);
-
-		/*
-		 * If there's more work to do to complete the attr intent, the
-		 * defer capture structure will have taken its own reference to
-		 * the attr leaf buffer and will give that to the continuation
-		 * transaction.  The attr intent struct drives the continuation
-		 * work, so release our refcount on the attr leaf buffer but
-		 * retain the pointer in the intent structure.
-		 */
-		if (ret != -EAGAIN)
-			attr->xattri_leaf_bp = NULL;
+		attr->xattri_leaf_bp = NULL;
 	}
 
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	xfs_irele(ip);
 out:
-	if (ret != -EAGAIN)
-		xfs_attr_free_item(attr);
+	xfs_attr_free_item(attr);
 	return error;
 }
 

Re: [PATCH 1/2] xfs: always free xattri_leaf_bp when cancelling a deferred op

[Dave Chinner]

On Thu, Jun 23, 2022 at 01:40:01PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> While running the following fstest with logged xattrs DISabled, I
> noticed the following:
> 
> # FSSTRESS_AVOID="-z -f unlink=1 -f rmdir=1 -f creat=2 -f mkdir=2 -f
> getfattr=3 -f listfattr=3 -f attr_remove=4 -f removefattr=4 -f
> setfattr=20 -f attr_set=60" ./check generic/475
> 
> INFO: task u9:1:40 blocked for more than 61 seconds.
>       Tainted: G           O      5.19.0-rc2-djwx #rc2
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:u9:1            state:D stack:12872 pid:   40 ppid:     2 flags:0x00004000
> Workqueue: xfs-cil/dm-0 xlog_cil_push_work [xfs]
> Call Trace:
>  <TASK>
>  __schedule+0x2db/0x1110
>  schedule+0x58/0xc0
>  schedule_timeout+0x115/0x160
>  __down_common+0x126/0x210
>  down+0x54/0x70
>  xfs_buf_lock+0x2d/0xe0 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
>  xfs_buf_item_unpin+0x227/0x3a0 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
>  xfs_trans_committed_bulk+0x18e/0x320 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
>  xlog_cil_committed+0x2ea/0x360 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
>  xlog_cil_push_work+0x60f/0x690 [xfs 0532c1cb1d67dd81d15cb79ac6e415c8dec58f73]
>  process_one_work+0x1df/0x3c0
>  worker_thread+0x53/0x3b0
>  kthread+0xea/0x110
>  ret_from_fork+0x1f/0x30
>  </TASK>
> 
> This appears to be the result of shortform_to_leaf creating a new leaf
> buffer as part of adding an xattr to a file.  The new leaf buffer is
> held and attached to the xfs_attr_intent structure, but then the
> filesystem shuts down.  Instead of the usual path (which adds the attr
> to the held leaf buffer which releases the hold), we instead cancel the
> entire deferred operation.
> 
> Unfortunately, xfs_attr_cancel_item doesn't release any attached leaf
> buffers, so we leak the locked buffer.  The CIL cannot do anything
> about that, and hangs.  Fix this by teaching it to release leaf buffers,
> and make XFS a little more careful about not leaving a dangling
> reference.
> 
> The prologue of xfs_attri_item_recover is (in this author's opinion) a
> little hard to figure out, so I'll clean that up in the next patch.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_attr_item.c |   20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: clean up the end of xfs_attri_item_recover

[Dave Chinner]

On Thu, Jun 23, 2022 at 01:40:06PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The end of this function could use some cleanup -- the EAGAIN
> conditionals make it harder to figure out what's going on with the
> disposal of xattri_leaf_bp, and the dual error/ret variables aren't
> needed.  Turn the EAGAIN case into a separate block documenting all the
> subtleties of recovering in the middle of an xattr update chain, which
> makes the rest of the prologue much simpler.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Decent cleanup. :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com