diff mbox series

[15/21] xfs: remove recursion in __xfs_trans_commit

Message ID 173258398059.4032920.3998675004204277948.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>

Currently, __xfs_trans_commit calls xfs_defer_finish_noroll, which calls
__xfs_trans_commit again on the same transaction.  In other words,
there's function recursion that has caused minor amounts of confusion in
the past.  There's no reason to keep this around, since there's only one
place where we actually want the xfs_defer_finish_noroll, and that is in
the top level xfs_trans_commit call.

Fixes: 98719051e75ccf ("xfs: refactor internal dfops initialization")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_trans.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Comments

Christoph Hellwig Nov. 26, 2024, 5:11 a.m. UTC | #1 
On Mon, Nov 25, 2024 at 05:28:37PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Currently, __xfs_trans_commit calls xfs_defer_finish_noroll, which calls
> __xfs_trans_commit again on the same transaction.  In other words,
> there's function recursion that has caused minor amounts of confusion in
> the past.  There's no reason to keep this around, since there's only one
> place where we actually want the xfs_defer_finish_noroll, and that is in
> the top level xfs_trans_commit call.

Hmm, I don't think the current version is a recursion, because the
is keyed off the regrant argument.  That being said the new version is
a lot cleaner, but maybe adjust the commit log and drop the fixes tag?
Christoph Hellwig Nov. 26, 2024, 5:11 a.m. UTC | #2 
On Mon, Nov 25, 2024 at 09:11:06PM -0800, Christoph Hellwig wrote:
> On Mon, Nov 25, 2024 at 05:28:37PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Currently, __xfs_trans_commit calls xfs_defer_finish_noroll, which calls
> > __xfs_trans_commit again on the same transaction.  In other words,
> > there's function recursion that has caused minor amounts of confusion in
> > the past.  There's no reason to keep this around, since there's only one
> > place where we actually want the xfs_defer_finish_noroll, and that is in
> > the top level xfs_trans_commit call.
> 
> Hmm, I don't think the current version is a recursion, because the
> is keyed off the regrant argument.  That being said the new version is
> a lot cleaner, but maybe adjust the commit log and drop the fixes tag?

With that:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Nov. 26, 2024, 6:20 p.m. UTC | #3 
On Mon, Nov 25, 2024 at 09:11:23PM -0800, Christoph Hellwig wrote:
> On Mon, Nov 25, 2024 at 09:11:06PM -0800, Christoph Hellwig wrote:
> > On Mon, Nov 25, 2024 at 05:28:37PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Currently, __xfs_trans_commit calls xfs_defer_finish_noroll, which calls
> > > __xfs_trans_commit again on the same transaction.  In other words,
> > > there's function recursion that has caused minor amounts of confusion in
> > > the past.  There's no reason to keep this around, since there's only one
> > > place where we actually want the xfs_defer_finish_noroll, and that is in
> > > the top level xfs_trans_commit call.
> > 
> > Hmm, I don't think the current version is a recursion, because the
> > is keyed off the regrant argument.  That being said the new version is
> > a lot cleaner, but maybe adjust the commit log and drop the fixes tag?

How about:

"xfs: avoid nested calls to __xfs_trans_commit

"Currently, __xfs_trans_commit calls xfs_defer_finish_noroll, which
calls __xfs_trans_commit again on the same transaction.  In other words,
there's a nested function call (albeit with slightly different
arguments) that has caused minor amounts of confusion in the past.
There's no reason to keep this around, since there's only one place
where we actually want the xfs_defer_finish_noroll, and that is in the
top level xfs_trans_commit call.

"This also reduces stack usage a little bit."

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

Thanks!

--D
Christoph Hellwig Nov. 27, 2024, 5:44 a.m. UTC | #4 
On Tue, Nov 26, 2024 at 10:20:52AM -0800, Darrick J. Wong wrote:
> How about:
> 
> "xfs: avoid nested calls to __xfs_trans_commit
> 
> "Currently, __xfs_trans_commit calls xfs_defer_finish_noroll, which
> calls __xfs_trans_commit again on the same transaction.  In other words,
> there's a nested function call (albeit with slightly different
> arguments) that has caused minor amounts of confusion in the past.
> There's no reason to keep this around, since there's only one place
> where we actually want the xfs_defer_finish_noroll, and that is in the
> top level xfs_trans_commit call.
> 
> "This also reduces stack usage a little bit."

Sounds good.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 4a517250efc911..26bb2343082af4 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -860,18 +860,6 @@  __xfs_trans_commit(
 
 	trace_xfs_trans_commit(tp, _RET_IP_);
 
-	/*
-	 * Finish deferred items on final commit. Only permanent transactions
-	 * should ever have deferred ops.
-	 */
-	WARN_ON_ONCE(!list_empty(&tp->t_dfops) &&
-		     !(tp->t_flags & XFS_TRANS_PERM_LOG_RES));
-	if (!regrant && (tp->t_flags & XFS_TRANS_PERM_LOG_RES)) {
-		error = xfs_defer_finish_noroll(&tp);
-		if (error)
-			goto out_unreserve;
-	}
-
 	error = xfs_trans_run_precommits(tp);
 	if (error)
 		goto out_unreserve;
@@ -950,6 +938,20 @@  int
 xfs_trans_commit(
 	struct xfs_trans	*tp)
 {
+	/*
+	 * Finish deferred items on final commit. Only permanent transactions
+	 * should ever have deferred ops.
+	 */
+	WARN_ON_ONCE(!list_empty(&tp->t_dfops) &&
+		     !(tp->t_flags & XFS_TRANS_PERM_LOG_RES));
+	if (tp->t_flags & XFS_TRANS_PERM_LOG_RES) {
+		int error = xfs_defer_finish_noroll(&tp);
+		if (error) {
+			xfs_trans_cancel(tp);
+			return error;
+		}
+	}
+
 	return __xfs_trans_commit(tp, false);
 }