Re: [PATCH v4 06/10] btrfs-progs: support byte length for zone resetting

[David Sterba <dsterba@suse.cz>]

On Wed, May 29, 2024 at 04:13:21PM +0900, Naohiro Aota wrote:
> Even with "mkfs.btrfs -b", mkfs.btrfs resets all the zones on the device.
> Limit the reset target within the specified length.
> 
> Also, we need to check that there is no active zone outside of the FS
> range. Having an active zone outside FS reduces the number of zones btrfs
> can write simultaneously. Technically, we can still scan all the device
> zones and keep active zones outside FS intact and try to live with the
> limited active zones. But, that will make btrfs operations harder.
> 
> It is generally bad idea to use "-b" on a non-test usage on a device with
> active zone limit in the first place. You really need to take care that FS
> and outside the FS goes over the limit. That means you'll never be able to
> use zones outside the FS anyway.
> 
> So, until there is a strong request for that, I don't think it's worthwhile
> to do so.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> ---
>  common/device-utils.c | 17 ++++++++++++-----
>  kernel-shared/zoned.c | 23 ++++++++++++++++++++++-
>  kernel-shared/zoned.h |  7 ++++---
>  3 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/common/device-utils.c b/common/device-utils.c
> index 86942e0c7041..7df7d9ce39d8 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -254,16 +254,23 @@ int btrfs_prepare_device(int fd, const char *file, u64 *byte_count_ret,
>  
>  		if (!zinfo->emulated) {
>  			if (opflags & PREP_DEVICE_VERBOSE)
> -				printf("Resetting device zones %s (%u zones) ...\n",
> -				       file, zinfo->nr_zones);
> +				printf("Resetting device zones %s (%llu zones) ...\n",
> +				       file, byte_count / zinfo->zone_size);
>  			/*
>  			 * We cannot ignore zone reset errors for a zoned block
>  			 * device as this could result in the inability to write
>  			 * to non-empty sequential zones of the device.
>  			 */
> -			if (btrfs_reset_all_zones(fd, zinfo)) {
> -				error("zoned: failed to reset device '%s' zones: %m",
> -				      file);
> +			ret = btrfs_reset_zones(fd, zinfo, byte_count);
> +			if (ret) {
> +				if (ret == EBUSY) {
> +					error("zoned: device '%s' contains an active zone outside of the FS range",
> +					      file);
> +					error("zoned: btrfs needs full control of active zones");
> +				} else {
> +					error("zoned: failed to reset device '%s' zones: %m",
> +					      file);
> +				}
>  				goto err;
>  			}
>  		}
> diff --git a/kernel-shared/zoned.c b/kernel-shared/zoned.c
> index fb1e1388804e..b4244966ca36 100644
> --- a/kernel-shared/zoned.c
> +++ b/kernel-shared/zoned.c
> @@ -395,16 +395,24 @@ static int report_zones(int fd, const char *file,
>   * Discard blocks in the zones of a zoned block device. Process this with zone
>   * size granularity so that blocks in conventional zones are discarded using
>   * discard_range and blocks in sequential zones are reset though a zone reset.
> + *
> + * We need to ensure that zones outside of the FS is not active, so that
> + * the FS can use all the active zones. Return EBUSY if there is an active
> + * zone.
>   */
> -int btrfs_reset_all_zones(int fd, struct btrfs_zoned_device_info *zinfo)
> +int btrfs_reset_zones(int fd, struct btrfs_zoned_device_info *zinfo, u64 byte_count)
>  {
>  	unsigned int i;
>  	int ret = 0;
>  
>  	ASSERT(zinfo);
> +	ASSERT(IS_ALIGNED(byte_count, zinfo->zone_size));
>  
>  	/* Zone size granularity */
>  	for (i = 0; i < zinfo->nr_zones; i++) {
> +		if (byte_count == 0)
> +			break;
> +
>  		if (zinfo->zones[i].type == BLK_ZONE_TYPE_CONVENTIONAL) {
>  			ret = device_discard_blocks(fd,
>  					     zinfo->zones[i].start << SECTOR_SHIFT,
> @@ -419,7 +427,20 @@ int btrfs_reset_all_zones(int fd, struct btrfs_zoned_device_info *zinfo)
>  
>  		if (ret)
>  			return ret;
> +
> +		byte_count -= zinfo->zone_size;
>  	}
> +	for (; i < zinfo->nr_zones; i++) {
> +		const enum blk_zone_cond cond = zinfo->zones[i].cond;
> +
> +		if (zinfo->zones[i].type == BLK_ZONE_TYPE_CONVENTIONAL)
> +			continue;
> +		if (cond == BLK_ZONE_COND_IMP_OPEN ||
> +		    cond == BLK_ZONE_COND_EXP_OPEN ||
> +		    cond == BLK_ZONE_COND_CLOSED)
> +			return EBUSY;

Should this return -EBUSY? It should not matter for this case but by
convention it would be better to use only negative errnos. I found
another one that's in the same call chain that still returns plain
errno, discard_range(). This should be fixed, possibly separately, so
I'll keep your patch as is.