[PATCH 0/3] btrfs: a couple fixes for record_root_in_trans()

[PATCH 0/3] btrfs: a couple fixes for record_root_in_trans()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Fix a couple issues in record_root_in_trans / btrfs_record_root_in_trans()
detected by Gemini, plus add a lockdep assertion.

Filipe Manana (3):
  btrfs: clear BTRFS_ROOT_IN_TRANS_SETUP on early exit from record_root_in_trans()
  btrfs: fix barrier usage in btrfs_record_root_in_trans()
  btrfs: assert reloc mutex is held in record_root_in_trans()

 fs/btrfs/transaction.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.47.2

[PATCH 1/3] btrfs: clear BTRFS_ROOT_IN_TRANS_SETUP on early exit from record_root_in_trans()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we exit early because the transaction that last used the root already
matches the current transaction, we leave the BTRFS_ROOT_IN_TRANS_SETUP
bit set in the root (which we just set right before the exit). While this
does not cause any functional issue, it makes callers of
btrfs_record_root_in_trans() always lock fs_info->reloc_mutex and call
record_root_in_trans() for nothing, causing unnecessary lock contention.
One caller of btrfs_record_root_in_trans() is start_transaction(), used to
start new transaction or joining an existing one, which is a hot path.

So clear BTRFS_ROOT_IN_TRANS_SETUP on early exit.

Assisted-by: LLM
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index ca114235bbe1..c1555621ae4e 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -432,6 +432,7 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
 		spin_lock(&fs_info->fs_roots_radix_lock);
 		if (btrfs_get_root_last_trans(root) == trans->transid && !force) {
 			spin_unlock(&fs_info->fs_roots_radix_lock);
+			clear_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state);
 			return 0;
 		}
 		radix_tree_tag_set(&fs_info->fs_roots_radix,
-- 
2.47.2

Re: [PATCH 1/3] btrfs: clear BTRFS_ROOT_IN_TRANS_SETUP on early exit from record_root_in_trans()

[Boris Burkov]

On Fri, Sep 18, 2026 at 01:28:09PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we exit early because the transaction that last used the root already
> matches the current transaction, we leave the BTRFS_ROOT_IN_TRANS_SETUP
> bit set in the root (which we just set right before the exit). While this
> does not cause any functional issue, it makes callers of
> btrfs_record_root_in_trans() always lock fs_info->reloc_mutex and call

I found "always" kind of confusing here. It's until one succeeds and
clears the bit right? It kind of makes it sound like it leaks it
forever.

> record_root_in_trans() for nothing, causing unnecessary lock contention.
> One caller of btrfs_record_root_in_trans() is start_transaction(), used to
> start new transaction or joining an existing one, which is a hot path.
> 
> So clear BTRFS_ROOT_IN_TRANS_SETUP on early exit.
> 
> Assisted-by: LLM
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/transaction.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index ca114235bbe1..c1555621ae4e 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -432,6 +432,7 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
>  		spin_lock(&fs_info->fs_roots_radix_lock);
>  		if (btrfs_get_root_last_trans(root) == trans->transid && !force) {
>  			spin_unlock(&fs_info->fs_roots_radix_lock);
> +			clear_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state);
>  			return 0;
>  		}
>  		radix_tree_tag_set(&fs_info->fs_roots_radix,
> -- 
> 2.47.2
> 

Re: [PATCH 1/3] btrfs: clear BTRFS_ROOT_IN_TRANS_SETUP on early exit from record_root_in_trans()

[Filipe Manana]

On Fri, Sep 18, 2026 at 6:18 PM Boris Burkov <boris@bur.io> wrote:
>
> On Fri, Sep 18, 2026 at 01:28:09PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > If we exit early because the transaction that last used the root already
> > matches the current transaction, we leave the BTRFS_ROOT_IN_TRANS_SETUP
> > bit set in the root (which we just set right before the exit). While this
> > does not cause any functional issue, it makes callers of
> > btrfs_record_root_in_trans() always lock fs_info->reloc_mutex and call
>
> I found "always" kind of confusing here. It's until one succeeds and
> clears the bit right? It kind of makes it sound like it leaks it
> forever.

Yes, I can reword the sentence to:

"While this does not cause any functional issue, it makes callers of
  btrfs_record_root_in_trans() lock fs_info->reloc_mutex and call
  record_root_in_trans() for nothing, causing unnecessary lock contention,
  until one of them clears the bit in record_root_in_trans()."

Thanks.

>
> > record_root_in_trans() for nothing, causing unnecessary lock contention.
> > One caller of btrfs_record_root_in_trans() is start_transaction(), used to
> > start new transaction or joining an existing one, which is a hot path.
> >
> > So clear BTRFS_ROOT_IN_TRANS_SETUP on early exit.
> >
> > Assisted-by: LLM
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >  fs/btrfs/transaction.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > index ca114235bbe1..c1555621ae4e 100644
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -432,6 +432,7 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
> >               spin_lock(&fs_info->fs_roots_radix_lock);
> >               if (btrfs_get_root_last_trans(root) == trans->transid && !force) {
> >                       spin_unlock(&fs_info->fs_roots_radix_lock);
> > +                     clear_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state);
> >                       return 0;
> >               }
> >               radix_tree_tag_set(&fs_info->fs_roots_radix,
> > --
> > 2.47.2
> >

Re: [PATCH 1/3] btrfs: clear BTRFS_ROOT_IN_TRANS_SETUP on early exit from record_root_in_trans()

[Boris Burkov]

On Fri, Sep 18, 2026 at 06:32:05PM +0100, Filipe Manana wrote:
> On Fri, Sep 18, 2026 at 6:18 PM Boris Burkov <boris@bur.io> wrote:
> >
> > On Fri, Sep 18, 2026 at 01:28:09PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > If we exit early because the transaction that last used the root already
> > > matches the current transaction, we leave the BTRFS_ROOT_IN_TRANS_SETUP
> > > bit set in the root (which we just set right before the exit). While this
> > > does not cause any functional issue, it makes callers of
> > > btrfs_record_root_in_trans() always lock fs_info->reloc_mutex and call
> >
> > I found "always" kind of confusing here. It's until one succeeds and
> > clears the bit right? It kind of makes it sound like it leaks it
> > forever.
> 
> Yes, I can reword the sentence to:
> 
> "While this does not cause any functional issue, it makes callers of
>   btrfs_record_root_in_trans() lock fs_info->reloc_mutex and call
>   record_root_in_trans() for nothing, causing unnecessary lock contention,
>   until one of them clears the bit in record_root_in_trans()."
> 
> Thanks.
> 

That version sounds great!

> >
> > > record_root_in_trans() for nothing, causing unnecessary lock contention.
> > > One caller of btrfs_record_root_in_trans() is start_transaction(), used to
> > > start new transaction or joining an existing one, which is a hot path.
> > >
> > > So clear BTRFS_ROOT_IN_TRANS_SETUP on early exit.
> > >
> > > Assisted-by: LLM
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > >  fs/btrfs/transaction.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > > index ca114235bbe1..c1555621ae4e 100644
> > > --- a/fs/btrfs/transaction.c
> > > +++ b/fs/btrfs/transaction.c
> > > @@ -432,6 +432,7 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
> > >               spin_lock(&fs_info->fs_roots_radix_lock);
> > >               if (btrfs_get_root_last_trans(root) == trans->transid && !force) {
> > >                       spin_unlock(&fs_info->fs_roots_radix_lock);
> > > +                     clear_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state);
> > >                       return 0;
> > >               }
> > >               radix_tree_tag_set(&fs_info->fs_roots_radix,
> > > --
> > > 2.47.2
> > >

Re: [PATCH 1/3] btrfs: clear BTRFS_ROOT_IN_TRANS_SETUP on early exit from record_root_in_trans()

[Qu Wenruo]

在 2026/9/18 21:58, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we exit early because the transaction that last used the root already
> matches the current transaction, we leave the BTRFS_ROOT_IN_TRANS_SETUP
> bit set in the root (which we just set right before the exit). While this
> does not cause any functional issue, it makes callers of
> btrfs_record_root_in_trans() always lock fs_info->reloc_mutex and call
> record_root_in_trans() for nothing, causing unnecessary lock contention.
> One caller of btrfs_record_root_in_trans() is start_transaction(), used to
> start new transaction or joining an existing one, which is a hot path.
> 
> So clear BTRFS_ROOT_IN_TRANS_SETUP on early exit.
> 
> Assisted-by: LLM
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/transaction.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index ca114235bbe1..c1555621ae4e 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -432,6 +432,7 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
>   		spin_lock(&fs_info->fs_roots_radix_lock);
>   		if (btrfs_get_root_last_trans(root) == trans->transid && !force) {
>   			spin_unlock(&fs_info->fs_roots_radix_lock);
> +			clear_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state);
>   			return 0;
>   		}
>   		radix_tree_tag_set(&fs_info->fs_roots_radix,

[PATCH 2/3] btrfs: fix barrier usage in btrfs_record_root_in_trans()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The barrier usage in btrfs_record_root_in_trans() is wrong, as the writer
side, in record_root_in_trans(), sets BTRFS_ROOT_IN_TRANS_SETUP, does a
write barrier and then sets the root's last transaction. This means the
reader side must check the root's last transaction, issue a read barrier
and then check for BTRFS_ROOT_IN_TRANS_SETUP. However, currently we issue
a read barrier and then check the root's last transaction and the bit
BTRFS_ROOT_IN_TRANS_SETUP, which can be problematic because the CPU is
free to reorder the checks and the following can happen:

1) Before reading the root's last_trans, it checks that
   BTRFS_ROOT_IN_TRANS_SETUP is not set.

2) A writer sets BTRFS_ROOT_IN_TRANS_SETUP, does smp_wmb() and updates
   the root's last_trans.

3) The reader then sees the root's last_trans matches the current
   transaction and falsely concludes the root setup is completes and
   returns without waiting for the writer task to complete the setup
   (calling btrfs_init_reloc_root()).

So fix the reading ordered as previously described: check the root's
last_trans, issue read barrier and then check BTRFS_ROOT_IN_TRANS_SETUP
(the reverse of what the writer side does).

Assisted-by: LLM
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index c1555621ae4e..13203ea9e116 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -511,10 +511,11 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans,
 	 * see record_root_in_trans for comments about IN_TRANS_SETUP usage
 	 * and barriers
 	 */
-	smp_rmb();
-	if (btrfs_get_root_last_trans(root) == trans->transid &&
-	    !test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
-		return 0;
+	if (btrfs_get_root_last_trans(root) == trans->transid) {
+		smp_rmb();
+		if (!test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
+			return 0;
+	}
 
 	mutex_lock(&fs_info->reloc_mutex);
 	ret = record_root_in_trans(trans, root, false);
-- 
2.47.2

Re: [PATCH 2/3] btrfs: fix barrier usage in btrfs_record_root_in_trans()

[Boris Burkov]

On Fri, Sep 18, 2026 at 01:28:10PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The barrier usage in btrfs_record_root_in_trans() is wrong, as the writer
> side, in record_root_in_trans(), sets BTRFS_ROOT_IN_TRANS_SETUP, does a
> write barrier and then sets the root's last transaction. This means the
> reader side must check the root's last transaction, issue a read barrier
> and then check for BTRFS_ROOT_IN_TRANS_SETUP. However, currently we issue
> a read barrier and then check the root's last transaction and the bit
> BTRFS_ROOT_IN_TRANS_SETUP, which can be problematic because the CPU is
> free to reorder the checks and the following can happen:
> 
> 1) Before reading the root's last_trans, it checks that
>    BTRFS_ROOT_IN_TRANS_SETUP is not set.
> 
> 2) A writer sets BTRFS_ROOT_IN_TRANS_SETUP, does smp_wmb() and updates
>    the root's last_trans.
> 
> 3) The reader then sees the root's last_trans matches the current
>    transaction and falsely concludes the root setup is completes and
>    returns without waiting for the writer task to complete the setup
>    (calling btrfs_init_reloc_root()).
> 
> So fix the reading ordered as previously described: check the root's
> last_trans, issue read barrier and then check BTRFS_ROOT_IN_TRANS_SETUP
> (the reverse of what the writer side does).

I think a comment on why we can't use release/acquire (u64) but that the
non-atomicity is ok (we only check equality?) might be nice. Otherwise
we are supposed to have some code like i_size_read(), right?

Not blocking at all, just an observation, since those helpers are
supposed to prevent this kind of bug.

> 
> Assisted-by: LLM
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/transaction.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index c1555621ae4e..13203ea9e116 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -511,10 +511,11 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans,
>  	 * see record_root_in_trans for comments about IN_TRANS_SETUP usage
>  	 * and barriers
>  	 */
> -	smp_rmb();
> -	if (btrfs_get_root_last_trans(root) == trans->transid &&
> -	    !test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> -		return 0;
> +	if (btrfs_get_root_last_trans(root) == trans->transid) {
> +		smp_rmb();
> +		if (!test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> +			return 0;
> +	}
>  
>  	mutex_lock(&fs_info->reloc_mutex);
>  	ret = record_root_in_trans(trans, root, false);
> -- 
> 2.47.2
> 

Re: [PATCH 2/3] btrfs: fix barrier usage in btrfs_record_root_in_trans()

[Filipe Manana]

On Fri, Sep 18, 2026 at 6:36 PM Boris Burkov <boris@bur.io> wrote:
>
> On Fri, Sep 18, 2026 at 01:28:10PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > The barrier usage in btrfs_record_root_in_trans() is wrong, as the writer
> > side, in record_root_in_trans(), sets BTRFS_ROOT_IN_TRANS_SETUP, does a
> > write barrier and then sets the root's last transaction. This means the
> > reader side must check the root's last transaction, issue a read barrier
> > and then check for BTRFS_ROOT_IN_TRANS_SETUP. However, currently we issue
> > a read barrier and then check the root's last transaction and the bit
> > BTRFS_ROOT_IN_TRANS_SETUP, which can be problematic because the CPU is
> > free to reorder the checks and the following can happen:
> >
> > 1) Before reading the root's last_trans, it checks that
> >    BTRFS_ROOT_IN_TRANS_SETUP is not set.
> >
> > 2) A writer sets BTRFS_ROOT_IN_TRANS_SETUP, does smp_wmb() and updates
> >    the root's last_trans.
> >
> > 3) The reader then sees the root's last_trans matches the current
> >    transaction and falsely concludes the root setup is completes and
> >    returns without waiting for the writer task to complete the setup
> >    (calling btrfs_init_reloc_root()).
> >
> > So fix the reading ordered as previously described: check the root's
> > last_trans, issue read barrier and then check BTRFS_ROOT_IN_TRANS_SETUP
> > (the reverse of what the writer side does).
>
> I think a comment on why we can't use release/acquire (u64) but that the
> non-atomicity is ok (we only check equality?) might be nice. Otherwise
> we are supposed to have some code like i_size_read(), right?
>
> Not blocking at all, just an observation, since those helpers are
> supposed to prevent this kind of bug.

I'm not sure what you mean. If you are mentioning the helpers for
last_trans use READ/WRITE_ONCE and that that prevents the bug being
fixed here, then that is not correct, because READ/WRITE_ONCE does not
prevent a CPU from reordering intructions (just compiler level
reordering, load/store tearing and a few other things).


>
> >
> > Assisted-by: LLM
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >  fs/btrfs/transaction.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > index c1555621ae4e..13203ea9e116 100644
> > --- a/fs/btrfs/transaction.c
> > +++ b/fs/btrfs/transaction.c
> > @@ -511,10 +511,11 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans,
> >        * see record_root_in_trans for comments about IN_TRANS_SETUP usage
> >        * and barriers
> >        */
> > -     smp_rmb();
> > -     if (btrfs_get_root_last_trans(root) == trans->transid &&
> > -         !test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> > -             return 0;
> > +     if (btrfs_get_root_last_trans(root) == trans->transid) {
> > +             smp_rmb();
> > +             if (!test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> > +                     return 0;
> > +     }
> >
> >       mutex_lock(&fs_info->reloc_mutex);
> >       ret = record_root_in_trans(trans, root, false);
> > --
> > 2.47.2
> >

Re: [PATCH 2/3] btrfs: fix barrier usage in btrfs_record_root_in_trans()

[Boris Burkov]

On Fri, Sep 18, 2026 at 06:43:54PM +0100, Filipe Manana wrote:
> On Fri, Sep 18, 2026 at 6:36 PM Boris Burkov <boris@bur.io> wrote:
> >
> > On Fri, Sep 18, 2026 at 01:28:10PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > The barrier usage in btrfs_record_root_in_trans() is wrong, as the writer
> > > side, in record_root_in_trans(), sets BTRFS_ROOT_IN_TRANS_SETUP, does a
> > > write barrier and then sets the root's last transaction. This means the
> > > reader side must check the root's last transaction, issue a read barrier
> > > and then check for BTRFS_ROOT_IN_TRANS_SETUP. However, currently we issue
> > > a read barrier and then check the root's last transaction and the bit
> > > BTRFS_ROOT_IN_TRANS_SETUP, which can be problematic because the CPU is
> > > free to reorder the checks and the following can happen:
> > >
> > > 1) Before reading the root's last_trans, it checks that
> > >    BTRFS_ROOT_IN_TRANS_SETUP is not set.
> > >
> > > 2) A writer sets BTRFS_ROOT_IN_TRANS_SETUP, does smp_wmb() and updates
> > >    the root's last_trans.
> > >
> > > 3) The reader then sees the root's last_trans matches the current
> > >    transaction and falsely concludes the root setup is completes and
> > >    returns without waiting for the writer task to complete the setup
> > >    (calling btrfs_init_reloc_root()).
> > >
> > > So fix the reading ordered as previously described: check the root's
> > > last_trans, issue read barrier and then check BTRFS_ROOT_IN_TRANS_SETUP
> > > (the reverse of what the writer side does).
> >
> > I think a comment on why we can't use release/acquire (u64) but that the
> > non-atomicity is ok (we only check equality?) might be nice. Otherwise
> > we are supposed to have some code like i_size_read(), right?
> >
> > Not blocking at all, just an observation, since those helpers are
> > supposed to prevent this kind of bug.
> 
> I'm not sure what you mean. If you are mentioning the helpers for
> last_trans use READ/WRITE_ONCE and that that prevents the bug being
> fixed here, then that is not correct, because READ/WRITE_ONCE does not
> prevent a CPU from reordering intructions (just compiler level
> reordering, load/store tearing and a few other things).
> 
> 

Sorry for being unclear. No, what I mean is I think we should
justify/document why we are not using smp_store_release/smp_load_acquire
since those are exactly this pattern, and if we had used them, it would
have prevented the bug.

> >
> > >
> > > Assisted-by: LLM
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > >  fs/btrfs/transaction.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > > index c1555621ae4e..13203ea9e116 100644
> > > --- a/fs/btrfs/transaction.c
> > > +++ b/fs/btrfs/transaction.c
> > > @@ -511,10 +511,11 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans,
> > >        * see record_root_in_trans for comments about IN_TRANS_SETUP usage
> > >        * and barriers
> > >        */
> > > -     smp_rmb();
> > > -     if (btrfs_get_root_last_trans(root) == trans->transid &&
> > > -         !test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> > > -             return 0;
> > > +     if (btrfs_get_root_last_trans(root) == trans->transid) {
> > > +             smp_rmb();
> > > +             if (!test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> > > +                     return 0;
> > > +     }
> > >
> > >       mutex_lock(&fs_info->reloc_mutex);
> > >       ret = record_root_in_trans(trans, root, false);
> > > --
> > > 2.47.2
> > >

Re: [PATCH 2/3] btrfs: fix barrier usage in btrfs_record_root_in_trans()

[Filipe Manana]

On Fri, Sep 18, 2026 at 6:47 PM Boris Burkov <boris@bur.io> wrote:
>
> On Fri, Sep 18, 2026 at 06:43:54PM +0100, Filipe Manana wrote:
> > On Fri, Sep 18, 2026 at 6:36 PM Boris Burkov <boris@bur.io> wrote:
> > >
> > > On Fri, Sep 18, 2026 at 01:28:10PM +0100, fdmanana@kernel.org wrote:
> > > > From: Filipe Manana <fdmanana@suse.com>
> > > >
> > > > The barrier usage in btrfs_record_root_in_trans() is wrong, as the writer
> > > > side, in record_root_in_trans(), sets BTRFS_ROOT_IN_TRANS_SETUP, does a
> > > > write barrier and then sets the root's last transaction. This means the
> > > > reader side must check the root's last transaction, issue a read barrier
> > > > and then check for BTRFS_ROOT_IN_TRANS_SETUP. However, currently we issue
> > > > a read barrier and then check the root's last transaction and the bit
> > > > BTRFS_ROOT_IN_TRANS_SETUP, which can be problematic because the CPU is
> > > > free to reorder the checks and the following can happen:
> > > >
> > > > 1) Before reading the root's last_trans, it checks that
> > > >    BTRFS_ROOT_IN_TRANS_SETUP is not set.
> > > >
> > > > 2) A writer sets BTRFS_ROOT_IN_TRANS_SETUP, does smp_wmb() and updates
> > > >    the root's last_trans.
> > > >
> > > > 3) The reader then sees the root's last_trans matches the current
> > > >    transaction and falsely concludes the root setup is completes and
> > > >    returns without waiting for the writer task to complete the setup
> > > >    (calling btrfs_init_reloc_root()).
> > > >
> > > > So fix the reading ordered as previously described: check the root's
> > > > last_trans, issue read barrier and then check BTRFS_ROOT_IN_TRANS_SETUP
> > > > (the reverse of what the writer side does).
> > >
> > > I think a comment on why we can't use release/acquire (u64) but that the
> > > non-atomicity is ok (we only check equality?) might be nice. Otherwise
> > > we are supposed to have some code like i_size_read(), right?
> > >
> > > Not blocking at all, just an observation, since those helpers are
> > > supposed to prevent this kind of bug.
> >
> > I'm not sure what you mean. If you are mentioning the helpers for
> > last_trans use READ/WRITE_ONCE and that that prevents the bug being
> > fixed here, then that is not correct, because READ/WRITE_ONCE does not
> > prevent a CPU from reordering intructions (just compiler level
> > reordering, load/store tearing and a few other things).
> >
> >
>
> Sorry for being unclear. No, what I mean is I think we should
> justify/document why we are not using smp_store_release/smp_load_acquire
> since those are exactly this pattern, and if we had used them, it would
> have prevented the bug.

It should work, but I don't see an advantage of one method over the
other (perhaps smp_store_release and and_load_acquire are a bit easier
to read for some).

I have no idea why that code (really old now) was written using smp_wmb/rmb.
Perhaps the macros for smp_store_release and and_load_acquire did not
exist back then, and that explains why we don't use them anywhere in
btrfs and always use smp_wmb/rmb.


>
> > >
> > > >
> > > > Assisted-by: LLM
> > > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > > ---
> > > >  fs/btrfs/transaction.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > > > index c1555621ae4e..13203ea9e116 100644
> > > > --- a/fs/btrfs/transaction.c
> > > > +++ b/fs/btrfs/transaction.c
> > > > @@ -511,10 +511,11 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans,
> > > >        * see record_root_in_trans for comments about IN_TRANS_SETUP usage
> > > >        * and barriers
> > > >        */
> > > > -     smp_rmb();
> > > > -     if (btrfs_get_root_last_trans(root) == trans->transid &&
> > > > -         !test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> > > > -             return 0;
> > > > +     if (btrfs_get_root_last_trans(root) == trans->transid) {
> > > > +             smp_rmb();
> > > > +             if (!test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> > > > +                     return 0;
> > > > +     }
> > > >
> > > >       mutex_lock(&fs_info->reloc_mutex);
> > > >       ret = record_root_in_trans(trans, root, false);
> > > > --
> > > > 2.47.2
> > > >

Re: [PATCH 2/3] btrfs: fix barrier usage in btrfs_record_root_in_trans()

[Boris Burkov]

On Fri, Sep 18, 2026 at 07:05:24PM +0100, Filipe Manana wrote:
> On Fri, Sep 18, 2026 at 6:47 PM Boris Burkov <boris@bur.io> wrote:
> >
> > On Fri, Sep 18, 2026 at 06:43:54PM +0100, Filipe Manana wrote:
> > > On Fri, Sep 18, 2026 at 6:36 PM Boris Burkov <boris@bur.io> wrote:
> > > >
> > > > On Fri, Sep 18, 2026 at 01:28:10PM +0100, fdmanana@kernel.org wrote:
> > > > > From: Filipe Manana <fdmanana@suse.com>
> > > > >
> > > > > The barrier usage in btrfs_record_root_in_trans() is wrong, as the writer
> > > > > side, in record_root_in_trans(), sets BTRFS_ROOT_IN_TRANS_SETUP, does a
> > > > > write barrier and then sets the root's last transaction. This means the
> > > > > reader side must check the root's last transaction, issue a read barrier
> > > > > and then check for BTRFS_ROOT_IN_TRANS_SETUP. However, currently we issue
> > > > > a read barrier and then check the root's last transaction and the bit
> > > > > BTRFS_ROOT_IN_TRANS_SETUP, which can be problematic because the CPU is
> > > > > free to reorder the checks and the following can happen:
> > > > >
> > > > > 1) Before reading the root's last_trans, it checks that
> > > > >    BTRFS_ROOT_IN_TRANS_SETUP is not set.
> > > > >
> > > > > 2) A writer sets BTRFS_ROOT_IN_TRANS_SETUP, does smp_wmb() and updates
> > > > >    the root's last_trans.
> > > > >
> > > > > 3) The reader then sees the root's last_trans matches the current
> > > > >    transaction and falsely concludes the root setup is completes and
> > > > >    returns without waiting for the writer task to complete the setup
> > > > >    (calling btrfs_init_reloc_root()).
> > > > >
> > > > > So fix the reading ordered as previously described: check the root's
> > > > > last_trans, issue read barrier and then check BTRFS_ROOT_IN_TRANS_SETUP
> > > > > (the reverse of what the writer side does).
> > > >
> > > > I think a comment on why we can't use release/acquire (u64) but that the
> > > > non-atomicity is ok (we only check equality?) might be nice. Otherwise
> > > > we are supposed to have some code like i_size_read(), right?
> > > >
> > > > Not blocking at all, just an observation, since those helpers are
> > > > supposed to prevent this kind of bug.
> > >
> > > I'm not sure what you mean. If you are mentioning the helpers for
> > > last_trans use READ/WRITE_ONCE and that that prevents the bug being
> > > fixed here, then that is not correct, because READ/WRITE_ONCE does not
> > > prevent a CPU from reordering intructions (just compiler level
> > > reordering, load/store tearing and a few other things).
> > >
> > >
> >
> > Sorry for being unclear. No, what I mean is I think we should
> > justify/document why we are not using smp_store_release/smp_load_acquire
> > since those are exactly this pattern, and if we had used them, it would
> > have prevented the bug.
> 
> It should work, but I don't see an advantage of one method over the
> other (perhaps smp_store_release and and_load_acquire are a bit easier
> to read for some).
> 
> I have no idea why that code (really old now) was written using smp_wmb/rmb.
> Perhaps the macros for smp_store_release and and_load_acquire did not
> exist back then, and that explains why we don't use them anywhere in
> btrfs and always use smp_wmb/rmb.
> 
> 

OK, all good. Thanks for the discussion and the fix, and you don't need
to bother with any additional justification or explanation.

> >
> > > >
> > > > >
> > > > > Assisted-by: LLM
> > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > > > ---
> > > > >  fs/btrfs/transaction.c | 9 +++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> > > > > index c1555621ae4e..13203ea9e116 100644
> > > > > --- a/fs/btrfs/transaction.c
> > > > > +++ b/fs/btrfs/transaction.c
> > > > > @@ -511,10 +511,11 @@ int btrfs_record_root_in_trans(struct btrfs_trans_handle *trans,
> > > > >        * see record_root_in_trans for comments about IN_TRANS_SETUP usage
> > > > >        * and barriers
> > > > >        */
> > > > > -     smp_rmb();
> > > > > -     if (btrfs_get_root_last_trans(root) == trans->transid &&
> > > > > -         !test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> > > > > -             return 0;
> > > > > +     if (btrfs_get_root_last_trans(root) == trans->transid) {
> > > > > +             smp_rmb();
> > > > > +             if (!test_bit(BTRFS_ROOT_IN_TRANS_SETUP, &root->state))
> > > > > +                     return 0;
> > > > > +     }
> > > > >
> > > > >       mutex_lock(&fs_info->reloc_mutex);
> > > > >       ret = record_root_in_trans(trans, root, false);
> > > > > --
> > > > > 2.47.2
> > > > >

[PATCH 3/3] btrfs: assert reloc mutex is held in record_root_in_trans()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The fs_info->reloc_mutex is supposed to be held when record_root_in_trans()
is called and we mention that in a comment inside the function. Add a
lockdep assertion to check the lock is held.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 13203ea9e116..0a3913b61f16 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -413,6 +413,8 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	int ret = 0;
 
+	lockdep_assert_held(&fs_info->reloc_mutex);
+
 	if ((test_bit(BTRFS_ROOT_SHAREABLE, &root->state) &&
 	    btrfs_get_root_last_trans(root) < trans->transid) || force) {
 		WARN_ON(!force && root->commit_root != root->node);
-- 
2.47.2

Re: [PATCH 3/3] btrfs: assert reloc mutex is held in record_root_in_trans()

[Qu Wenruo]

在 2026/9/18 21:58, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The fs_info->reloc_mutex is supposed to be held when record_root_in_trans()
> is called and we mention that in a comment inside the function. Add a
> lockdep assertion to check the lock is held.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/transaction.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 13203ea9e116..0a3913b61f16 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -413,6 +413,8 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
>   	struct btrfs_fs_info *fs_info = root->fs_info;
>   	int ret = 0;
>   
> +	lockdep_assert_held(&fs_info->reloc_mutex);
> +
>   	if ((test_bit(BTRFS_ROOT_SHAREABLE, &root->state) &&
>   	    btrfs_get_root_last_trans(root) < trans->transid) || force) {
>   		WARN_ON(!force && root->commit_root != root->node);

Re: [PATCH 0/3] btrfs: a couple fixes for record_root_in_trans()

[Boris Burkov]

On Fri, Sep 18, 2026 at 01:28:08PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Fix a couple issues in record_root_in_trans / btrfs_record_root_in_trans()
> detected by Gemini, plus add a lockdep assertion.

Some simple nits/questions on the patches but this LGTM, please feel free to add
Reviewed-by: Boris Burkov <boris@bur.io>

Thanks,
Boris

> 
> Filipe Manana (3):
>   btrfs: clear BTRFS_ROOT_IN_TRANS_SETUP on early exit from record_root_in_trans()
>   btrfs: fix barrier usage in btrfs_record_root_in_trans()
>   btrfs: assert reloc mutex is held in record_root_in_trans()
> 
>  fs/btrfs/transaction.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> -- 
> 2.47.2
>