Re: [PATCH v2 1/4] btrfs: zoned: only change active zone counter on successful (de)activation

[Wang Yugui <wangyugui@e16-tech.com>]

Hi,

> btrfs_zone_activate() unconditionally decrements the reserved_active_zones
> counter regardless if btrfs_dev_set_active_zone() fails to set the zone
> active, i.e. because it raced with another call that already set the bit
> in the bitmask, or not.
> 
> This can lead to a double decrement of the counter in case the bit has
> already been set.
> 
> Only decrement the counter *iff* btrfs_dev_set_active_zone() successfully
> marked the zone in the bitmap.
> 
> Mirror this behaviour when clearing the bit again.
> 
> Fixes: a7e1ac7bdc5a ("btrfs: zoned: reserve zones for an active metadata/system block group")
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/zoned.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 9f562cc34e38..d68b91008e76 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1126,11 +1126,13 @@ u64 btrfs_find_allocatable_zones(struct btrfs_device *device, u64 hole_start,
>  	return pos;
>  }
>  
> -static bool btrfs_dev_set_active_zone(struct btrfs_device *device, u64 pos)
> +static bool btrfs_dev_set_active_zone(struct btrfs_device *device, u64 pos, bool *new)
>  {
>  	struct btrfs_zoned_device_info *zone_info = device->zone_info;
>  	unsigned int zno = (pos >> zone_info->zone_size_shift);
>  
> +	*new = false;
> +
>  	/* We can use any number of zones */
>  	if (zone_info->max_active_zones == 0)
>  		return true;
> @@ -1142,23 +1144,30 @@ static bool btrfs_dev_set_active_zone(struct btrfs_device *device, u64 pos)
>  		if (test_and_set_bit(zno, zone_info->active_zones)) {
>  			/* Someone already set the bit */
>  			atomic_inc(&zone_info->active_zones_left);
> +		} else {
> +			*new = true;
>  		}
>  	}
>  
>  	return true;
>  }
>  
> -static void btrfs_dev_clear_active_zone(struct btrfs_device *device, u64 pos)
> +static bool btrfs_dev_clear_active_zone(struct btrfs_device *device, u64 pos)
>  {
>  	struct btrfs_zoned_device_info *zone_info = device->zone_info;
>  	unsigned int zno = (pos >> zone_info->zone_size_shift);
>  
> +
>  	/* We can use any number of zones */
>  	if (zone_info->max_active_zones == 0)
> -		return;
> +		return false;
>  
> -	if (test_and_clear_bit(zno, zone_info->active_zones))
> +	if (test_and_clear_bit(zno, zone_info->active_zones)) {
>  		atomic_inc(&zone_info->active_zones_left);
> +		return true;
> +	}
> +
> +	return false;
>  }
>  
>  int btrfs_reset_device_zone(struct btrfs_device *device, u64 physical,
> @@ -2411,6 +2420,7 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group)
>  	u64 physical;
>  	const bool is_data = (block_group->flags & BTRFS_BLOCK_GROUP_DATA);
>  	bool ret;
> +	bool new;

'new' is not a good var name because it is a keyword in C++/Java.

'is_new' just like above 'is_data'  maybe a better var name.

Best  Regards
Wang Yugui