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

[Boris Burkov <boris@bur.io>]

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
> > >