diff mbox series

[16/21] xfs: don't lose solo superblock counter update transactions

Message ID 173258398074.4032920.16314140758572044747.stgit@frogsfrogsfrogs (mailing list archive)
State Deferred, archived
Headers show
Series [01/21] xfs: fix off-by-one error in fsmap's end_daddr usage | expand

Commit Message

Darrick J. Wong Nov. 26, 2024, 1:28 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Superblock counter updates are tracked via per-transaction counters in
the xfs_trans object.  These changes are then turned into dirty log
items in xfs_trans_apply_sb_deltas just prior to commiting the log items
to the CIL.

However, updating the per-transaction counter deltas do not cause
XFS_TRANS_DIRTY to be set on the transaction.  In other words, a pure sb
counter update will be silently discarded if there are no other dirty
log items attached to the transaction.

This is currently not the case anywhere in the filesystem because sb
counter updates always dirty at least one other metadata item, but let's
not leave a logic bomb.

Cc: <stable@vger.kernel.org> # v2.6.35
Fixes: 0924378a689ccb ("xfs: split out iclog writing from xfs_trans_commit()")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_trans.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Nov. 26, 2024, 5:14 a.m. UTC | #1 
On Mon, Nov 25, 2024 at 05:28:53PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Superblock counter updates are tracked via per-transaction counters in
> the xfs_trans object.  These changes are then turned into dirty log
> items in xfs_trans_apply_sb_deltas just prior to commiting the log items
> to the CIL.
> 
> However, updating the per-transaction counter deltas do not cause
> XFS_TRANS_DIRTY to be set on the transaction.  In other words, a pure sb
> counter update will be silently discarded if there are no other dirty
> log items attached to the transaction.
> 
> This is currently not the case anywhere in the filesystem because sb
> counter updates always dirty at least one other metadata item, but let's
> not leave a logic bomb.

xfs_trans_mod_sb is the only place that sets XFS_TRANS_SB_DIRTY, and
always forces XFS_TRANS_DIRTY.  So this seems kinda intentional, even
if this new version is much cleaner.  So the change looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But I don't think the Fixes tag is really warranted.
Darrick J. Wong Nov. 26, 2024, 6:23 p.m. UTC | #2 
On Mon, Nov 25, 2024 at 09:14:07PM -0800, Christoph Hellwig wrote:
> On Mon, Nov 25, 2024 at 05:28:53PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Superblock counter updates are tracked via per-transaction counters in
> > the xfs_trans object.  These changes are then turned into dirty log
> > items in xfs_trans_apply_sb_deltas just prior to commiting the log items
> > to the CIL.
> > 
> > However, updating the per-transaction counter deltas do not cause
> > XFS_TRANS_DIRTY to be set on the transaction.  In other words, a pure sb
> > counter update will be silently discarded if there are no other dirty
> > log items attached to the transaction.
> > 
> > This is currently not the case anywhere in the filesystem because sb
> > counter updates always dirty at least one other metadata item, but let's
> > not leave a logic bomb.
> 
> xfs_trans_mod_sb is the only place that sets XFS_TRANS_SB_DIRTY, and
> always forces XFS_TRANS_DIRTY.  So this seems kinda intentional, even
> if this new version is much cleaner.  So the change looks fine:

<nod> I don't think this one was absolutely necessary, it just seemed
like a cleanup to put anything that set TRANS_DIRTY before the
TRANS_DIRTY check.

> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But I don't think the Fixes tag is really warranted.

Dropped.

--D
diff mbox series

Patch

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 26bb2343082af4..427a8ba0ab99e2 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -860,6 +860,13 @@  __xfs_trans_commit(
 
 	trace_xfs_trans_commit(tp, _RET_IP_);
 
+	/*
+	 * Commit per-transaction changes that are not already tracked through
+	 * log items.  This can add dirty log items to the transaction.
+	 */
+	if (tp->t_flags & XFS_TRANS_SB_DIRTY)
+		xfs_trans_apply_sb_deltas(tp);
+
 	error = xfs_trans_run_precommits(tp);
 	if (error)
 		goto out_unreserve;
@@ -890,8 +897,6 @@  __xfs_trans_commit(
 	/*
 	 * If we need to update the superblock, then do it now.
 	 */
-	if (tp->t_flags & XFS_TRANS_SB_DIRTY)
-		xfs_trans_apply_sb_deltas(tp);
 	xfs_trans_apply_dquot_deltas(tp);
 
 	xlog_cil_commit(log, tp, &commit_seq, regrant);