Re: [PATCH v2 1/3] btrfs: move __TRANS_* and TRANS_* flags out of transaction.h

[Qu Wenruo <wqu@suse.com>]

在 2026/9/29 03:57, Filipe Manana 写道:
> On Thu, Sep 24, 2026 at 7:00 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> Among all those flags, only __TRANS_DUMMY is used outside of
>> transaction.[ch], and there are only two places using it:
>>
>> - find_parent_nodes()
>>    Introduce a helper, btrfs_trans_is_dummy(), for this call site.
>>
>> - btrfs_init_dummy_trans()
>>    Move the function into transaction.[ch], and hide it behind
>>    CONFIG_BTRFS_FS_RUN_SANITY_TESTS.
>>
>> With the above changes, we can hide __TRANS_* and TRANS_* flags inside
>> transaction.c. This allows us to modify those flags in the future
>> without causing any changes to existing callers.
>>
>> After the flags relocation, now we can also hide __TRANS_DUMMY behind
>> CONFIG_BTRFS_FS_RUN_SANITY_TESTS.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/backref.c           |  2 +-
>>   fs/btrfs/tests/btrfs-tests.c |  9 ---------
>>   fs/btrfs/tests/btrfs-tests.h |  2 --
>>   fs/btrfs/transaction.c       | 36 ++++++++++++++++++++++++++++++++++++
>>   fs/btrfs/transaction.h       | 29 +++++++++++------------------
>>   5 files changed, 48 insertions(+), 30 deletions(-)
>>
>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>> index 1be632c742bd..939511629ecc 100644
>> --- a/fs/btrfs/backref.c
>> +++ b/fs/btrfs/backref.c
>> @@ -1432,7 +1432,7 @@ static int find_parent_nodes(struct btrfs_backref_walk_ctx *ctx,
>>                  goto out;
>>          }
>>
>> -       if (ctx->trans && likely(ctx->trans->type != __TRANS_DUMMY) &&
>> +       if (ctx->trans && likely(!btrfs_trans_is_dummy(ctx->trans)) &&
>>              ctx->time_seq != BTRFS_SEQ_LAST) {
>>                  /*
>>                   * We have a specific time_seq we care about and trans which
>> diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
>> index 6287d940323d..cebc9a17b94d 100644
>> --- a/fs/btrfs/tests/btrfs-tests.c
>> +++ b/fs/btrfs/tests/btrfs-tests.c
>> @@ -247,15 +247,6 @@ void btrfs_init_dummy_transaction(struct btrfs_transaction *trans, struct btrfs_
>>          spin_lock_init(&trans->delayed_refs.lock);
>>   }
>>
>> -void btrfs_init_dummy_trans(struct btrfs_trans_handle *trans,
>> -                           struct btrfs_fs_info *fs_info)
>> -{
>> -       memset(trans, 0, sizeof(*trans));
>> -       trans->transid = 1;
>> -       trans->type = __TRANS_DUMMY;
>> -       trans->fs_info = fs_info;
>> -}
>> -
>>   int btrfs_run_sanity_tests(void)
>>   {
>>          int ret, i;
>> diff --git a/fs/btrfs/tests/btrfs-tests.h b/fs/btrfs/tests/btrfs-tests.h
>> index cea58fe84a6d..a16855138105 100644
>> --- a/fs/btrfs/tests/btrfs-tests.h
>> +++ b/fs/btrfs/tests/btrfs-tests.h
>> @@ -59,8 +59,6 @@ btrfs_alloc_dummy_block_group(struct btrfs_fs_info *fs_info, unsigned long lengt
>>   void btrfs_free_dummy_block_group(struct btrfs_block_group *cache);
>>   DEFINE_FREE(btrfs_free_dummy_block_group, struct btrfs_block_group *,
>>              btrfs_free_dummy_block_group(_T));
>> -void btrfs_init_dummy_trans(struct btrfs_trans_handle *trans,
>> -                           struct btrfs_fs_info *fs_info);
>>   void btrfs_init_dummy_transaction(struct btrfs_transaction *trans, struct btrfs_fs_info *fs_info);
>>   struct btrfs_device *btrfs_alloc_dummy_device(struct btrfs_fs_info *fs_info);
>>
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index ca114235bbe1..4181f3897a30 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -38,6 +38,26 @@
>>
>>   static struct kmem_cache *btrfs_trans_handle_cachep;
>>
>> +enum {
>> +       ENUM_BIT(__TRANS_FREEZABLE),
>> +       ENUM_BIT(__TRANS_START),
>> +       ENUM_BIT(__TRANS_ATTACH),
>> +       ENUM_BIT(__TRANS_JOIN),
>> +       ENUM_BIT(__TRANS_JOIN_NOLOCK),
>> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>> +       ENUM_BIT(__TRANS_DUMMY),
>> +#endif
>> +       ENUM_BIT(__TRANS_JOIN_NOSTART),
>> +};
>> +
>> +#define TRANS_START            (__TRANS_START | __TRANS_FREEZABLE)
>> +#define TRANS_ATTACH           (__TRANS_ATTACH)
>> +#define TRANS_JOIN             (__TRANS_JOIN | __TRANS_FREEZABLE)
>> +#define TRANS_JOIN_NOLOCK      (__TRANS_JOIN_NOLOCK)
>> +#define TRANS_JOIN_NOSTART     (__TRANS_JOIN_NOSTART)
>> +
>> +#define TRANS_EXTWRITERS       (__TRANS_START | __TRANS_ATTACH)
>> +
>>   /*
>>    * Transaction states and transitions
>>    *
>> @@ -139,6 +159,22 @@ static const unsigned int btrfs_blocked_trans_types[TRANS_STATE_MAX] = {
>>                                             __TRANS_JOIN_NOSTART),
>>   };
>>
>> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>> +bool btrfs_trans_is_dummy(const struct btrfs_trans_handle *trans)
>> +{
> 
> So most of our exported transaction functions have names like:
> 
> btrfs_end_transaction()
> btrfs_commit_transaction()
> btrfs_start_transaction()
> btrfs_abort_transaction()
> etc
> 
> So I would suggest, for consistency, naming it as:  btrfs_is_dummy_transaction()

That sounds good.
> 
>> +       return trans->type == __TRANS_DUMMY;
>> +}
>> +
>> +void btrfs_init_dummy_trans(struct btrfs_trans_handle *trans,
>> +                           struct btrfs_fs_info *fs_info)
>> +{
> 
> And here; btrfs_init_dummy_transaction()

Unfortunately we already have one in the selftests with the same name, 
but using btrfs_transaction as the first parameter.

Thus I'm afraid I have to keep this name.

Thanks,
Qu

> 
> Otherwise:
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Thanks.
> 
>> +       memset(trans, 0, sizeof(*trans));
>> +       trans->transid = 1;
>> +       trans->type = __TRANS_DUMMY;
>> +       trans->fs_info = fs_info;
>> +}
>> +#endif
>> +
>>   void btrfs_put_transaction(struct btrfs_transaction *transaction)
>>   {
>>          if (refcount_dec_and_test(&transaction->use_count)) {
>> diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
>> index 89153cd22596..9eb92b1ad54d 100644
>> --- a/fs/btrfs/transaction.h
>> +++ b/fs/btrfs/transaction.h
>> @@ -119,24 +119,6 @@ struct btrfs_transaction {
>>          wait_queue_head_t pending_wait;
>>   };
>>
>> -enum {
>> -       ENUM_BIT(__TRANS_FREEZABLE),
>> -       ENUM_BIT(__TRANS_START),
>> -       ENUM_BIT(__TRANS_ATTACH),
>> -       ENUM_BIT(__TRANS_JOIN),
>> -       ENUM_BIT(__TRANS_JOIN_NOLOCK),
>> -       ENUM_BIT(__TRANS_DUMMY),
>> -       ENUM_BIT(__TRANS_JOIN_NOSTART),
>> -};
>> -
>> -#define TRANS_START            (__TRANS_START | __TRANS_FREEZABLE)
>> -#define TRANS_ATTACH           (__TRANS_ATTACH)
>> -#define TRANS_JOIN             (__TRANS_JOIN | __TRANS_FREEZABLE)
>> -#define TRANS_JOIN_NOLOCK      (__TRANS_JOIN_NOLOCK)
>> -#define TRANS_JOIN_NOSTART     (__TRANS_JOIN_NOSTART)
>> -
>> -#define TRANS_EXTWRITERS       (__TRANS_START | __TRANS_ATTACH)
>> -
>>   /*
>>    * Number of extent buffers a transaction handle tracks for writeback
>>    * inhibition. The CLOCK reference bits pack into a u32 so this must not exceed
>> @@ -305,6 +287,17 @@ do {                                                               \
>>                                    __LINE__, __error);           \
>>   } while (0)
>>
>> +#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>> +bool btrfs_trans_is_dummy(const struct btrfs_trans_handle *trans);
>> +void btrfs_init_dummy_trans(struct btrfs_trans_handle *trans,
>> +                           struct btrfs_fs_info *fs_info);
>> +#else
>> +static inline bool btrfs_trans_is_dummy(const struct btrfs_trans_handle *trans)
>> +{
>> +       return false;
>> +}
>> +#endif
>> +
>>   int btrfs_end_transaction(struct btrfs_trans_handle *trans);
>>   struct btrfs_trans_handle *btrfs_start_transaction(struct btrfs_root *root,
>>                                                     unsigned int num_items);
>> --
>> 2.55.0
>>
>>