Re: [PATCH v2 10/11] block: Add support for the zone capacity concept

[Niklas Cassel <nks@flawful.org>]

On Tue, Apr 18, 2023 at 03:40:01PM -0700, Bart Van Assche wrote:
> Make the zone capacity available in struct queue_limits for those
> drivers that need it.
> 
> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  Documentation/ABI/stable/sysfs-block |  8 ++++++++
>  block/blk-settings.c                 |  1 +
>  block/blk-sysfs.c                    |  7 +++++++
>  block/blk-zoned.c                    | 15 +++++++++++++++
>  include/linux/blkdev.h               |  1 +
>  5 files changed, 32 insertions(+)
> 
> diff --git a/Documentation/ABI/stable/sysfs-block b/Documentation/ABI/stable/sysfs-block
> index c57e5b7cb532..4527d0514fdb 100644
> --- a/Documentation/ABI/stable/sysfs-block
> +++ b/Documentation/ABI/stable/sysfs-block
> @@ -671,6 +671,14 @@ Description:
>  		regular block devices.
>  
>  
> +What:		/sys/block/<disk>/queue/zone_capacity
> +Date:		March 2023
> +Contact:	linux-block@vger.kernel.org
> +Description:
> +		[RO] The number of 512-byte sectors in a zone that can be read
> +		or written. This number is less than or equal to the zone size.
> +
> +
>  What:		/sys/block/<disk>/queue/zone_write_granularity
>  Date:		January 2021
>  Contact:	linux-block@vger.kernel.org
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 896b4654ab00..96f5dc63a815 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -685,6 +685,7 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
>  						   b->max_secure_erase_sectors);
>  	t->zone_write_granularity = max(t->zone_write_granularity,
>  					b->zone_write_granularity);
> +	t->zone_capacity = max(t->zone_capacity, b->zone_capacity);
>  	t->zoned = max(t->zoned, b->zoned);
>  	return ret;
>  }

(snip)

> @@ -496,12 +498,23 @@ static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
>  				disk->disk_name);
>  			return -ENODEV;
>  		}
> +		if (zone->capacity != args->zone_capacity) {
> +			pr_warn("%s: Invalid zoned device with non constant zone capacity\n",
> +				disk->disk_name);
> +			return -ENODEV;

Hello Bart,

The NVMe Zoned Namespace Command Set Specification:
https://nvmexpress.org/wp-content/uploads/NVM-Express-Zoned-Namespace-Command-Set-Specification-1.1c-2022.10.03-Ratified.pdf

specifies the Zone Capacity (ZCAP) field in each Zone Descriptor.
The Descriptors are part of the Report Zones Data Structure.

This means that while the zone size is the same for all zones,
the zone capacity can be different for each zone.

While the single NVMe ZNS SSD that I've encountered so far did
coincidentally have the same zone capacity for all zones, this
is not required by the specification.

The NVMe driver does reject a ZNS device that has support for
Variable Zone Capacity (which is defined in the ZOC field):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/zns.c?h=v6.3-rc7#n95

Variable Zone Capacity simply means the the zone capacities
cannot change without a NVM format.

However, even when Variable Zone Capacity is not supported,
a NVMe ZNS device can still have different zone capacities,
and AFAICT, such devices are currently supported.

With your change above, we would start rejecting such devices.


Is this reduction of supported NVMe ZNS SSD devices really desired?

If it is, then I would at least expect to find a motivation of why we
are reducing the scope of the currently supported NVMe ZNS devices
to be found somewhere in the commit message.


Kind regards,
Niklas