Re: [PATCH v9 04/41] btrfs: Get zone information of zoned block devices

[David Sterba <dsterba@suse.cz>]

On Fri, Oct 30, 2020 at 10:51:11PM +0900, Naohiro Aota wrote:
> +static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos,
> +			       struct blk_zone *zones, unsigned int *nr_zones)
> +{
> +	int ret;
> +
> +	if (!*nr_zones)
> +		return 0;
> +
> +	ret = blkdev_report_zones(device->bdev, pos >> SECTOR_SHIFT, *nr_zones,
> +				  copy_zone_info_cb, zones);
> +	if (ret < 0) {
> +		btrfs_err_in_rcu(device->fs_info,
> +				 "get zone at %llu on %s failed %d", pos,

		"zoned: failed to read zone %llu on %s (devid %llu)"

> +				 rcu_str_deref(device->name), ret);
> +		return ret;
> +	}
> +	*nr_zones = ret;
> +	if (!ret)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +int btrfs_get_dev_zone_info(struct btrfs_device *device)
> +{
> +	struct btrfs_zoned_device_info *zone_info = NULL;
> +	struct block_device *bdev = device->bdev;
> +	sector_t nr_sectors = bdev->bd_part->nr_sects;
> +	sector_t sector = 0;
> +	struct blk_zone *zones = NULL;
> +	unsigned int i, nreported = 0, nr_zones;
> +	unsigned int zone_sectors;
> +	int ret;
> +	char devstr[sizeof(device->fs_info->sb->s_id) +
> +		    sizeof(" (device )") - 1];

Can you avoid the local variable and print the contents conditionally?

> +	kfree(zones);
> +
> +	device->zone_info = zone_info;
> +
> +	devstr[0] = 0;
> +	if (device->fs_info)
> +		snprintf(devstr, sizeof(devstr), " (device %s)",
> +			 device->fs_info->sb->s_id);
> +
> +	rcu_read_lock();
> +	pr_info(
> +"BTRFS info%s: host-%s zoned block device %s, %u zones of %llu sectors",

I think zone size in bytes is more natural, is there a reason to print
that in sectors?

> +		devstr,
> +		bdev_zoned_model(bdev) == BLK_ZONED_HM ? "managed" : "aware",
> +		rcu_str_deref(device->name), zone_info->nr_zones,
> +		zone_info->zone_size >> SECTOR_SHIFT);
> +	rcu_read_unlock();
> +
> +	return 0;
> +
> +out:
> +	kfree(zones);
> +	bitmap_free(zone_info->empty_zones);
> +	bitmap_free(zone_info->seq_zones);
> +	kfree(zone_info);
> +
> +	return ret;
> +}

> --- /dev/null
> +++ b/fs/btrfs/zoned.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef BTRFS_ZONED_H
> +#define BTRFS_ZONED_H

This should at least include types.h as it's using u64
> +
> +struct btrfs_zoned_device_info {
> +	/*
> +	 * Number of zones, zone size and types of zones if bdev is a
> +	 * zoned block device.
> +	 */
> +	u64 zone_size;
> +	u8  zone_size_shift;
> +	u32 nr_zones;
> +	unsigned long *seq_zones;
> +	unsigned long *empty_zones;
> +};
> +
> +#ifdef CONFIG_BLK_DEV_ZONED
> +int btrfs_get_dev_zone(struct btrfs_device *device, u64 pos,
> +		       struct blk_zone *zone);
> +int btrfs_get_dev_zone_info(struct btrfs_device *device);
> +void btrfs_destroy_dev_zone_info(struct btrfs_device *device);
> +#else /* CONFIG_BLK_DEV_ZONED */
> +static inline int btrfs_get_dev_zone(struct btrfs_device *device, u64 pos,
> +				     struct blk_zone *zone)
> +{
> +	return 0;
> +}

newline

> +static inline int btrfs_get_dev_zone_info(struct btrfs_device *device)
> +{
> +	return 0;
> +}

newline

> +static inline void btrfs_destroy_dev_zone_info(struct btrfs_device *device) { }
> +#endif