public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: "Darrick J . Wong" <djwong@kernel.org>
Cc: Leah Rumancik <leah.rumancik@gmail.com>,
	Chandan Babu R <chandan.babu@oracle.com>,
	linux-xfs@vger.kernel.org, fstests@vger.kernel.org,
	Brian Foster <bfoster@redhat.com>
Subject: [PATCH 5.10 CANDIDATE 7/9] xfs: remove dead stale buf unpin handling code
Date: Tue, 26 Jul 2022 11:21:23 +0200	[thread overview]
Message-ID: <20220726092125.3899077-8-amir73il@gmail.com> (raw)
In-Reply-To: <20220726092125.3899077-1-amir73il@gmail.com>

From: Brian Foster <bfoster@redhat.com>

commit e53d3aa0b605c49d780e1b2fd0b49dba4154f32b upstream.

This code goes back to a time when transaction commits wrote
directly to iclogs. The associated log items were pinned, written to
the log, and then "uncommitted" if some part of the log write had
failed. This uncommit sequence called an ->iop_unpin_remove()
handler that was eventually folded into ->iop_unpin() via the remove
parameter. The log subsystem has since changed significantly in that
transactions commit to the CIL instead of direct to iclogs, though
log items must still be aborted in the event of an eventual log I/O
error. However, the context for a log item abort is now asynchronous
from transaction commit, which means the committing transaction has
been freed by this point in time and the transaction uncommit
sequence of events is no longer relevant.

Further, since stale buffers remain locked at transaction commit
through unpin, we can be certain that the buffer is not associated
with any transaction when the unpin callback executes. Remove this
unused hunk of code and replace it with an assertion that the buffer
is disassociated from transaction context.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/xfs_buf_item.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 452af57731ed..a3d5ecccfc2c 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -435,28 +435,11 @@ xfs_buf_item_unpin(
 		ASSERT(xfs_buf_islocked(bp));
 		ASSERT(bp->b_flags & XBF_STALE);
 		ASSERT(bip->__bli_format.blf_flags & XFS_BLF_CANCEL);
+		ASSERT(list_empty(&lip->li_trans));
+		ASSERT(!bp->b_transp);
 
 		trace_xfs_buf_item_unpin_stale(bip);
 
-		if (remove) {
-			/*
-			 * If we are in a transaction context, we have to
-			 * remove the log item from the transaction as we are
-			 * about to release our reference to the buffer.  If we
-			 * don't, the unlock that occurs later in
-			 * xfs_trans_uncommit() will try to reference the
-			 * buffer which we no longer have a hold on.
-			 */
-			if (!list_empty(&lip->li_trans))
-				xfs_trans_del_item(lip);
-
-			/*
-			 * Since the transaction no longer refers to the buffer,
-			 * the buffer should no longer refer to the transaction.
-			 */
-			bp->b_transp = NULL;
-		}
-
 		/*
 		 * If we get called here because of an IO error, we may or may
 		 * not have the item on the AIL. xfs_trans_ail_delete() will
-- 
2.25.1


  parent reply	other threads:[~2022-07-26  9:21 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-26  9:21 [PATCH 5.10 CANDIDATE 0/9] xfs stable candidate patches for 5.10.y (from v5.13+) Amir Goldstein
2022-07-26  9:21 ` [PATCH 5.10 CANDIDATE 1/9] xfs: refactor xfs_file_fsync Amir Goldstein
2022-07-26  9:21 ` [PATCH 5.10 CANDIDATE 2/9] xfs: xfs_log_force_lsn isn't passed a LSN Amir Goldstein
2022-07-26  9:21 ` [PATCH 5.10 CANDIDATE 3/9] xfs: prevent UAF in xfs_log_item_in_current_chkpt Amir Goldstein
2022-07-26  9:21 ` [PATCH 5.10 CANDIDATE 4/9] xfs: fix log intent recovery ENOSPC shutdowns when inactivating inodes Amir Goldstein
2022-07-26  9:21 ` [PATCH 5.10 CANDIDATE 5/9] xfs: force the log offline when log intent item recovery fails Amir Goldstein
2022-07-26  9:21 ` [PATCH 5.10 CANDIDATE 6/9] xfs: hold buffer across unpin and potential shutdown processing Amir Goldstein
2022-07-26  9:21 ` Amir Goldstein [this message]
2022-07-26  9:21 ` [PATCH 5.10 CANDIDATE 8/9] xfs: logging the on disk inode LSN can make it go backwards Amir Goldstein
2022-07-26  9:21 ` [PATCH 5.10 CANDIDATE 9/9] xfs: Enforce attr3 buffer recovery order Amir Goldstein
2022-07-27 19:17 ` [PATCH 5.10 CANDIDATE 0/9] xfs stable candidate patches for 5.10.y (from v5.13+) Amir Goldstein
2022-07-28  2:01   ` Darrick J. Wong
2022-07-28  2:07     ` Darrick J. Wong
2022-07-28  9:39       ` Amir Goldstein
2022-07-29 16:15         ` Amir Goldstein

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220726092125.3899077-8-amir73il@gmail.com \
    --to=amir73il@gmail.com \
    --cc=bfoster@redhat.com \
    --cc=chandan.babu@oracle.com \
    --cc=djwong@kernel.org \
    --cc=fstests@vger.kernel.org \
    --cc=leah.rumancik@gmail.com \
    --cc=linux-xfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox