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