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