Re: [PATCH 1/3] btrfs: zoned: get rid of relocation_bg_lock

[Damien Le Moal <dlemoal@kernel.org>]

On 6/24/25 1:56 AM, Johannes Thumshirn wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Lockstat analysis of benchmark workloads shows a very high contention of
> the relocation_bg_lock. But the relocation_bg_lock only protects a single

s/of the/on
s/But the/But or s/But the/But the lock

> field in 'struct btrfs_fs_info', namely 'u64 data_reloc_bg'.
> 
> Use READ_ONCE()/WRITE_ONCE() to access 'btrfs_fs_info::data_reloc_bg'.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[...]

> @@ -3908,8 +3903,8 @@ static int do_allocation_zoned(struct btrfs_block_group *block_group,
>  	       block_group->start == fs_info->treelog_bg ||
>  	       fs_info->treelog_bg == 0);
>  	ASSERT(!ffe_ctl->for_data_reloc ||
> -	       block_group->start == fs_info->data_reloc_bg ||
> -	       fs_info->data_reloc_bg == 0);
> +	       block_group->start == data_reloc_bytenr ||
> +	       data_reloc_bytenr == 0);

Shouldn't all these use of data_reloc_bytenr be replaced with
READ_ONCE(fs_info->data_reloc_bg) ? That is, if data_reloc_bg can change while
this function is running.

> @@ -3957,8 +3952,8 @@ static int do_allocation_zoned(struct btrfs_block_group *block_group,
>  		fs_info->treelog_bg = block_group->start;
>  
>  	if (ffe_ctl->for_data_reloc) {
> -		if (!fs_info->data_reloc_bg)
> -			fs_info->data_reloc_bg = block_group->start;
> +		if (READ_ONCE(fs_info->data_reloc_bg) == 0)
> +			WRITE_ONCE(fs_info->data_reloc_bg, block_group->start);

See ! You did it here :)

> @@ -4304,10 +4298,10 @@ static int prepare_allocation_zoned(struct btrfs_fs_info *fs_info,
>  			ffe_ctl->hint_byte = fs_info->treelog_bg;
>  		spin_unlock(&fs_info->treelog_bg_lock);
>  	} else if (ffe_ctl->for_data_reloc) {
> -		spin_lock(&fs_info->relocation_bg_lock);
> -		if (fs_info->data_reloc_bg)
> -			ffe_ctl->hint_byte = fs_info->data_reloc_bg;
> -		spin_unlock(&fs_info->relocation_bg_lock);
> +		u64 data_reloc_bg = READ_ONCE(fs_info->data_reloc_bg);
> +
> +		if (data_reloc_bg)
> +			ffe_ctl->hint_byte = data_reloc_bg;

Why not drop the data_reloc_bg variable and use
READ_ONCE(fs_info->data_reloc_bg) directly ?

> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index a4a309050b67..f58bd85ddf5a 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -2495,11 +2495,10 @@ void btrfs_schedule_zone_finish_bg(struct btrfs_block_group *bg,
>  void btrfs_clear_data_reloc_bg(struct btrfs_block_group *bg)
>  {
>  	struct btrfs_fs_info *fs_info = bg->fs_info;
> +	u64 data_reloc_bg = READ_ONCE(fs_info->data_reloc_bg);
>  
> -	spin_lock(&fs_info->relocation_bg_lock);
> -	if (fs_info->data_reloc_bg == bg->start)
> -		fs_info->data_reloc_bg = 0;
> -	spin_unlock(&fs_info->relocation_bg_lock);
> +	if (data_reloc_bg == bg->start)
> +		WRITE_ONCE(fs_info->data_reloc_bg, 0);

data_reloc_bg variable is not needed.


-- 
Damien Le Moal
Western Digital Research