[PATCH] btrfs: simplify how first hit is passed to __btrfs_abort_transaction()

[PATCH] btrfs: simplify how first hit is passed to __btrfs_abort_transaction()

[David Sterba]

Optimize the btrfs_abort_transaction() for size as it (by our
convention) must be put right after the error condition is detected.
The exact file:line is reported so there's a portion that must be
inlined. As this is cold code it bloats functions. In previous patch
"btrfs: move transaction abort message to __btrfs_abort_transaction()"
the error message was moved to the common helper, saving like 20KiB of
btrfs.ko and several instructions per call site and some stack space.

There's little left to be optimized, we need to keep the atomic
test_and_set_bit() and to convey that as 'first hit' to
__btrfs_abort_transaction().

Right now it's a bool, which takes 8 bytes on stack for each call but
it's 1 bit of information. We can encode that to some of the other
parameters.

For that let's use the 'error' parameter, by convention it's negative
errno so we can reliably detect if it's the first hit or a later error.
Also the negation is usually implemented by a single instruction (NEG on
x86_64) so the resulting object code is kept short.

This reduces btrfs.ko by 8K and stack in several functions by 8 bytes.

Cumulative effect with the other commit is -30K of btrfs.ko. While the
encoding is an implementation detail, it's contained within the API.
Making the transaction abort calls very light is desired.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/transaction.c | 13 ++++++++++++-
 fs/btrfs/transaction.h | 15 +++++++++------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 01f66328966f..bbef3ae86ac4 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2723,12 +2723,23 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_fs_info *fs_info)
  *
  * We'll complete the cleanup in btrfs_end_transaction and
  * btrfs_commit_transaction.
+ *
+ * Note: the parameter @error encodes whether the transactin abort was first hit
+ *       (setting the FS_ERROR state bit in btrfs_abort_transaction())
+ *       - positive number - first hit
+ *       - negative number - abort after it was already done
  */
 void __cold __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 				      const char *function,
-				      unsigned int line, int error, bool first_hit)
+				      unsigned int line, int error)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
+	bool first_hit = false;
+
+	if (error > 0) {
+		error = -error;
+		first_hit = true;
+	}
 
 	WRITE_ONCE(trans->aborted, error);
 	WRITE_ONCE(trans->transaction->aborted, error);
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index f1cb05460cec..e1d4e7330198 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -243,20 +243,23 @@ static inline bool btrfs_abort_should_print_stack(int error)
 }
 
 /*
- * Call btrfs_abort_transaction as early as possible when an error condition is
- * detected, that way the exact stack trace is reported for some errors.
+ * Call btrfs_abort_transaction() as early as possible when an error condition
+ * is detected, that way the exact stack trace is reported for some errors.
+ *
+ * Error number must be negative as it encodes wheather it's the first abort.
  */
 #define btrfs_abort_transaction(trans, error)		\
 do {								\
-	bool __first = false;					\
+	int __error = (error);					\
+								\
 	/* Report first abort since mount */			\
 	if (!test_and_set_bit(BTRFS_FS_STATE_TRANS_ABORTED,	\
 			&((trans)->fs_info->fs_state))) {	\
-		__first = true;					\
+		__error = -__error;				\
 		WARN_ON(btrfs_abort_should_print_stack(error));	\
 	}							\
 	__btrfs_abort_transaction((trans), __func__,		\
-				  __LINE__, (error), __first);	\
+				  __LINE__, __error);		\
 } while (0)
 
 int btrfs_end_transaction(struct btrfs_trans_handle *trans);
@@ -294,7 +297,7 @@ void btrfs_add_dropped_root(struct btrfs_trans_handle *trans,
 void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans);
 void __cold __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 				      const char *function,
-				      unsigned int line, int error, bool first_hit);
+				      unsigned int line, int error);
 
 int __init btrfs_transaction_init(void);
 void __cold btrfs_transaction_exit(void);
-- 
2.51.0

Re: [PATCH] btrfs: simplify how first hit is passed to __btrfs_abort_transaction()

[Qu Wenruo]

在 2026/4/30 23:48, David Sterba 写道:
> Optimize the btrfs_abort_transaction() for size as it (by our
> convention) must be put right after the error condition is detected.
> The exact file:line is reported so there's a portion that must be
> inlined. As this is cold code it bloats functions. In previous patch
> "btrfs: move transaction abort message to __btrfs_abort_transaction()"
> the error message was moved to the common helper, saving like 20KiB of
> btrfs.ko and several instructions per call site and some stack space.
> 
> There's little left to be optimized, we need to keep the atomic
> test_and_set_bit() and to convey that as 'first hit' to
> __btrfs_abort_transaction().
> 
> Right now it's a bool, which takes 8 bytes on stack for each call but
> it's 1 bit of information. We can encode that to some of the other
> parameters.
> 
> For that let's use the 'error' parameter, by convention it's negative
> errno so we can reliably detect if it's the first hit or a later error.
> Also the negation is usually implemented by a single instruction (NEG on
> x86_64) so the resulting object code is kept short.
> 
> This reduces btrfs.ko by 8K and stack in several functions by 8 bytes.
> 
> Cumulative effect with the other commit is -30K of btrfs.ko. While the
> encoding is an implementation detail, it's contained within the API.
> Making the transaction abort calls very light is desired.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/transaction.c | 13 ++++++++++++-
>   fs/btrfs/transaction.h | 15 +++++++++------
>   2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 01f66328966f..bbef3ae86ac4 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -2723,12 +2723,23 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_fs_info *fs_info)
>    *
>    * We'll complete the cleanup in btrfs_end_transaction and
>    * btrfs_commit_transaction.
> + *
> + * Note: the parameter @error encodes whether the transactin abort was first hit
> + *       (setting the FS_ERROR state bit in btrfs_abort_transaction())
> + *       - positive number - first hit
> + *       - negative number - abort after it was already done
>    */
>   void __cold __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
>   				      const char *function,
> -				      unsigned int line, int error, bool first_hit)
> +				      unsigned int line, int error)
>   {
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	bool first_hit = false;
> +
> +	if (error > 0) {
> +		error = -error;
> +		first_hit = true;
> +	}
>   
>   	WRITE_ONCE(trans->aborted, error);
>   	WRITE_ONCE(trans->transaction->aborted, error);
> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> index f1cb05460cec..e1d4e7330198 100644
> --- a/fs/btrfs/transaction.h
> +++ b/fs/btrfs/transaction.h
> @@ -243,20 +243,23 @@ static inline bool btrfs_abort_should_print_stack(int error)
>   }
>   
>   /*
> - * Call btrfs_abort_transaction as early as possible when an error condition is
> - * detected, that way the exact stack trace is reported for some errors.
> + * Call btrfs_abort_transaction() as early as possible when an error condition
> + * is detected, that way the exact stack trace is reported for some errors.
> + *
> + * Error number must be negative as it encodes wheather it's the first abort.

What about an ASSERT() for it?

It's not that rare to forgot the minus sign for immediately error code 
like "-EIO".

>    */
>   #define btrfs_abort_transaction(trans, error)		\

And I do not like to use macros when inlined function can be used.
There is no special requirement for things like #name, thus this can be 
converted to a regular static inline function.

Not sure how an static inlined version would affect the text section 
size though.

Thanks,
Qu
>   do {								\
> -	bool __first = false;					\
> +	int __error = (error);					\
> +								\
>   	/* Report first abort since mount */			\
>   	if (!test_and_set_bit(BTRFS_FS_STATE_TRANS_ABORTED,	\
>   			&((trans)->fs_info->fs_state))) {	\
> -		__first = true;					\
> +		__error = -__error;				\
>   		WARN_ON(btrfs_abort_should_print_stack(error));	\
>   	}							\
>   	__btrfs_abort_transaction((trans), __func__,		\
> -				  __LINE__, (error), __first);	\
> +				  __LINE__, __error);		\
>   } while (0)
>   
>   int btrfs_end_transaction(struct btrfs_trans_handle *trans);
> @@ -294,7 +297,7 @@ void btrfs_add_dropped_root(struct btrfs_trans_handle *trans,
>   void btrfs_trans_release_chunk_metadata(struct btrfs_trans_handle *trans);
>   void __cold __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
>   				      const char *function,
> -				      unsigned int line, int error, bool first_hit);
> +				      unsigned int line, int error);
>   
>   int __init btrfs_transaction_init(void);
>   void __cold btrfs_transaction_exit(void);

Re: [PATCH] btrfs: simplify how first hit is passed to __btrfs_abort_transaction()

[David Sterba]

On Fri, May 01, 2026 at 07:54:19AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2026/4/30 23:48, David Sterba 写道:
> > Optimize the btrfs_abort_transaction() for size as it (by our
> > convention) must be put right after the error condition is detected.
> > The exact file:line is reported so there's a portion that must be
> > inlined. As this is cold code it bloats functions. In previous patch
> > "btrfs: move transaction abort message to __btrfs_abort_transaction()"
> > the error message was moved to the common helper, saving like 20KiB of
> > btrfs.ko and several instructions per call site and some stack space.
> > 
> > There's little left to be optimized, we need to keep the atomic
> > test_and_set_bit() and to convey that as 'first hit' to
> > __btrfs_abort_transaction().
> > 
> > Right now it's a bool, which takes 8 bytes on stack for each call but
> > it's 1 bit of information. We can encode that to some of the other
> > parameters.
> > 
> > For that let's use the 'error' parameter, by convention it's negative
> > errno so we can reliably detect if it's the first hit or a later error.
> > Also the negation is usually implemented by a single instruction (NEG on
> > x86_64) so the resulting object code is kept short.
> > 
> > This reduces btrfs.ko by 8K and stack in several functions by 8 bytes.
> > 
> > Cumulative effect with the other commit is -30K of btrfs.ko. While the
> > encoding is an implementation detail, it's contained within the API.
> > Making the transaction abort calls very light is desired.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/transaction.c | 13 ++++++++++++-
> >   fs/btrfs/transaction.h | 15 +++++++++------
> >   2 files changed, 21 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > index 01f66328966f..bbef3ae86ac4 100644
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -2723,12 +2723,23 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_fs_info *fs_info)
> >    *
> >    * We'll complete the cleanup in btrfs_end_transaction and
> >    * btrfs_commit_transaction.
> > + *
> > + * Note: the parameter @error encodes whether the transactin abort was first hit
> > + *       (setting the FS_ERROR state bit in btrfs_abort_transaction())
> > + *       - positive number - first hit
> > + *       - negative number - abort after it was already done
> >    */
> >   void __cold __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
> >   				      const char *function,
> > -				      unsigned int line, int error, bool first_hit)
> > +				      unsigned int line, int error)
> >   {
> >   	struct btrfs_fs_info *fs_info = trans->fs_info;
> > +	bool first_hit = false;
> > +
> > +	if (error > 0) {
> > +		error = -error;
> > +		first_hit = true;
> > +	}
> >   
> >   	WRITE_ONCE(trans->aborted, error);
> >   	WRITE_ONCE(trans->transaction->aborted, error);
> > diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
> > index f1cb05460cec..e1d4e7330198 100644
> > --- a/fs/btrfs/transaction.h
> > +++ b/fs/btrfs/transaction.h
> > @@ -243,20 +243,23 @@ static inline bool btrfs_abort_should_print_stack(int error)
> >   }
> >   
> >   /*
> > - * Call btrfs_abort_transaction as early as possible when an error condition is
> > - * detected, that way the exact stack trace is reported for some errors.
> > + * Call btrfs_abort_transaction() as early as possible when an error condition
> > + * is detected, that way the exact stack trace is reported for some errors.
> > + *
> > + * Error number must be negative as it encodes wheather it's the first abort.
> 
> What about an ASSERT() for it?
> 
> It's not that rare to forgot the minus sign for immediately error code 
> like "-EIO".

Yes I can add it, but ASSERT is evaluated at run time so this would not
catch "EIO". I have figured out how to do it at compile time, will be in
in v2.

> >    */
> >   #define btrfs_abort_transaction(trans, error)		\
> 
> And I do not like to use macros when inlined function can be used.
> There is no special requirement for things like #name, thus this can be 
> converted to a regular static inline function.

No it can't, static inline changes how __LINE__ and __func__ (and
__FILE__ for that matter) work so we'd get reports from the same
function where the function is defined. While with macro we have the
exact location, this is a standard trick. It's been like that since
forever, 49b25e0540904b ("btrfs: enhance transaction abort
infrastructure").