Re: [PATCH 1/4] btrfs: zoned: consolidate zone finish function

[David Sterba <dsterba@suse.cz>]

On Fri, Apr 29, 2022 at 12:02:15AM +0900, Naohiro Aota wrote:
>  1 file changed, 54 insertions(+), 73 deletions(-)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 997a96d7a3d5..9cddafe78fb1 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1879,20 +1879,14 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group)
>  	return ret;
>  }
>  
> -int btrfs_zone_finish(struct btrfs_block_group *block_group)
> +static int __btrfs_zone_finish(struct btrfs_block_group *block_group, bool nowait)

Can you please avoid using __ for functions? Eg. the main function can
be btrfs_zone_finish(taking 2 parameters) and the exported one being
btrfs_zone_finish_nowait reflecting the preset parameter and also making
the 'nowait' semantics clear.

>  {
>  	struct btrfs_fs_info *fs_info = block_group->fs_info;
>  	struct map_lookup *map;
> -	struct btrfs_device *device;
> -	u64 physical;
> +	bool need_zone_finish;
>  	int ret = 0;
>  	int i;
>  
> -	if (!btrfs_is_zoned(fs_info))
> -		return 0;
> -
> -	map = block_group->physical_map;
> -
>  	spin_lock(&block_group->lock);
>  	if (!block_group->zone_is_active) {
>  		spin_unlock(&block_group->lock);
> @@ -1906,36 +1900,42 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group)
>  		spin_unlock(&block_group->lock);
>  		return -EAGAIN;
>  	}
> -	spin_unlock(&block_group->lock);
>  
> -	ret = btrfs_inc_block_group_ro(block_group, false);
> -	if (ret)
> -		return ret;
> +	if (!nowait) {
> +		spin_unlock(&block_group->lock);
>  
> -	/* Ensure all writes in this block group finish */
> -	btrfs_wait_block_group_reservations(block_group);
> -	/* No need to wait for NOCOW writers. Zoned mode does not allow that. */
> -	btrfs_wait_ordered_roots(fs_info, U64_MAX, block_group->start,
> -				 block_group->length);
> +		ret = btrfs_inc_block_group_ro(block_group, false);
> +		if (ret)
> +			return ret;
>  
> -	spin_lock(&block_group->lock);
> +		/* Ensure all writes in this block group finish */
> +		btrfs_wait_block_group_reservations(block_group);
> +		/* No need to wait for NOCOW writers. Zoned mode does not allow that. */
> +		btrfs_wait_ordered_roots(fs_info, U64_MAX, block_group->start,
> +					 block_group->length);
>  
> -	/*
> -	 * Bail out if someone already deactivated the block group, or
> -	 * allocated space is left in the block group.
> -	 */
> -	if (!block_group->zone_is_active) {
> -		spin_unlock(&block_group->lock);
> -		btrfs_dec_block_group_ro(block_group);
> -		return 0;
> -	}
> +		spin_lock(&block_group->lock);
>  
> -	if (block_group->reserved) {
> -		spin_unlock(&block_group->lock);
> -		btrfs_dec_block_group_ro(block_group);
> -		return -EAGAIN;
> +		/*
> +		 * Bail out if someone already deactivated the block group, or
> +		 * allocated space is left in the block group.
> +		 */
> +		if (!block_group->zone_is_active) {
> +			spin_unlock(&block_group->lock);
> +			btrfs_dec_block_group_ro(block_group);
> +			return 0;
> +		}
> +
> +		if (block_group->reserved) {
> +			spin_unlock(&block_group->lock);
> +			btrfs_dec_block_group_ro(block_group);
> +			return -EAGAIN;
> +		}
>  	}
>  
> +	/* There is unwritten space left. Need to finish the underlying zones. */
> +	need_zone_finish = (block_group->zone_capacity - block_group->alloc_offset) > 0;

This could be simplified to 'bg->zone_capacity > bg->alloc_offset',
but maybe should be behind a helper as the expression appears more
times.

> +
>  	block_group->zone_is_active = 0;
>  	block_group->alloc_offset = block_group->zone_capacity;
>  	block_group->free_space_ctl->free_space = 0;
> @@ -1943,24 +1943,29 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group)
>  	btrfs_clear_data_reloc_bg(block_group);
>  	spin_unlock(&block_group->lock);
>  
> +	map = block_group->physical_map;
>  	for (i = 0; i < map->num_stripes; i++) {
> -		device = map->stripes[i].dev;
> -		physical = map->stripes[i].physical;
> +		struct btrfs_device *device = map->stripes[i].dev;
> +		const u64 physical = map->stripes[i].physical;
>  
>  		if (device->zone_info->max_active_zones == 0)
>  			continue;
>  
> -		ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
> -				       physical >> SECTOR_SHIFT,
> -				       device->zone_info->zone_size >> SECTOR_SHIFT,
> -				       GFP_NOFS);
> +		if (need_zone_finish) {
> +			ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH,
> +					       physical >> SECTOR_SHIFT,
> +					       device->zone_info->zone_size >> SECTOR_SHIFT,
> +					       GFP_NOFS);
>  
> -		if (ret)
> -			return ret;
> +			if (ret)
> +				return ret;
> +		}
>  
>  		btrfs_dev_clear_active_zone(device, physical);
>  	}
> -	btrfs_dec_block_group_ro(block_group);
> +
> +	if (!nowait)
> +		btrfs_dec_block_group_ro(block_group);
>  
>  	spin_lock(&fs_info->zone_active_bgs_lock);
>  	ASSERT(!list_empty(&block_group->active_bg_list));