[PATCH v4 0/3] Fix DM zone resource limits stacking

[PATCH v4 0/3] Fix DM zone resource limits stacking

[Damien Le Moal]

This is the updated patch 4/4 of the series "Zone write plugging and DM
zone fixes". This patch fixes DM zone resource limits stacking (max open
zones and max active zones limits). Patch 1 is new and is added to help
catch problems and eventual regressions of the handling of these limits.

Changes from v3:
 - Modify patch 1 to always check the zone resource limits values in
   disk_update_zone_resources(), including for DM devices that do not
   use zone write plugging. Simplify patch 2 accordingly by removing the
   same check and adjustment of the zone resource limits.
 - Added patch 3

Changes from v2:
 - Modify patch 1 to return an error for the case where the max open
   zones limit is greater than the max active zones limit.
 - Modify patch 2 to avoid duplicated actions on the limits and to
   remove warnings for unusual zone limits.

Changes from v1:
 - Added patch 1
 - Modified patch 2 to not cap the limits for a target with the number
   of sequential zones mapped but rather to use the device limits as is
   when more zones than the limits are mapped and 0 otherwise (no
   limits).

Damien Le Moal (3):
  block: Improve checks on zone resource limits
  dm: Improve zone resource limits handling
  dm: Remove unused macro DM_ZONE_INVALID_WP_OFST

 block/blk-settings.c |   8 ++
 block/blk-zoned.c    |  20 ++++-
 drivers/md/dm-zone.c | 206 +++++++++++++++++++++++++++++++++++--------
 3 files changed, 191 insertions(+), 43 deletions(-)

-- 
2.45.1

[PATCH v4 1/3] block: Improve checks on zone resource limits

[Damien Le Moal]

Make sure that the zone resource limits of a zoned block device are
correct by checking that:
(a) If the device has a max active zones limit, make sure that the max
    open zones limit is lower than the max active zones limit.
(b) If the device has zone resource limits, check that the limits
    values are lower than the number of sequential zones of the device.
    If it is not, assume that the zoned device has no limits by setting
    the limits to 0.

For (a), a check is added to blk_validate_zoned_limits() and an error
returned if the max open zones limit exceeds the value of the max active
zone limit (if there is one).

For (b), given that we need the number of sequential zones of the zoned
device, this check is added to disk_update_zone_resources(). This is
safe to do as that function is executed with the disk queue frozen and
the check executed after queue_limits_start_update() which takes the
queue limits lock. Of note is that the early return in this function
for zoned devices that do not use zone write plugging (e.g. DM devices
using native zone append) is moved to after the new check and adjustment
of the zone resource limits so that the check applies to any zoned
device.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-settings.c |  8 ++++++++
 block/blk-zoned.c    | 20 ++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index effeb9a639bb..474c709ea85b 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -80,6 +80,14 @@ static int blk_validate_zoned_limits(struct queue_limits *lim)
 	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_BLK_DEV_ZONED)))
 		return -EINVAL;
 
+	/*
+	 * Given that active zones include open zones, the maximum number of
+	 * open zones cannot be larger than the maximum numbber of active zones.
+	 */
+	if (lim->max_active_zones &&
+	    lim->max_open_zones > lim->max_active_zones)
+		return -EINVAL;
+
 	if (lim->zone_write_granularity < lim->logical_block_size)
 		lim->zone_write_granularity = lim->logical_block_size;
 
diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 52abebf56027..8f89705f5e1c 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -1647,8 +1647,22 @@ static int disk_update_zone_resources(struct gendisk *disk,
 		return -ENODEV;
 	}
 
+	lim = queue_limits_start_update(q);
+
+	/*
+	 * Some devices can advertize zone resource limits that are larger than
+	 * the number of sequential zones of the zoned block device, e.g. a
+	 * small ZNS namespace. For such case, assume that the zoned device has
+	 * no zone resource limits.
+	 */
+	nr_seq_zones = disk->nr_zones - nr_conv_zones;
+	if (lim.max_open_zones >= nr_seq_zones)
+		lim.max_open_zones = 0;
+	if (lim.max_active_zones >= nr_seq_zones)
+		lim.max_active_zones = 0;
+
 	if (!disk->zone_wplugs_pool)
-		return 0;
+		goto commit;
 
 	/*
 	 * If the device has no limit on the maximum number of open and active
@@ -1657,9 +1671,6 @@ static int disk_update_zone_resources(struct gendisk *disk,
 	 * dynamic zone write plug allocation when simultaneously writing to
 	 * more zones than the size of the mempool.
 	 */
-	lim = queue_limits_start_update(q);
-
-	nr_seq_zones = disk->nr_zones - nr_conv_zones;
 	pool_size = max(lim.max_open_zones, lim.max_active_zones);
 	if (!pool_size)
 		pool_size = min(BLK_ZONE_WPLUG_DEFAULT_POOL_SIZE, nr_seq_zones);
@@ -1673,6 +1684,7 @@ static int disk_update_zone_resources(struct gendisk *disk,
 			lim.max_open_zones = 0;
 	}
 
+commit:
 	return queue_limits_commit_update(q, &lim);
 }
 
-- 
2.45.1

[PATCH v4 2/3] dm: Improve zone resource limits handling

[Damien Le Moal]

The generic stacking of limits implemented in the block layer cannot
correctly handle stacking of zone resource limits (max open zones and
max active zones) because these limits are for an entire device but the
stacking may be for a portion of that device (e.g. a dm-linear target
that does not cover an entire block device). As a result, when DM
devices are created on top of zoned block devices, the DM device never
has any zone resource limits advertized, which is only correct if all
underlying target devices also have no zone resource limits.
If at least one target device has resource limits, the user may see
either performance issues (if the max open zone limit of the device is
exceeded) or write I/O errors if the max active zone limit of one of
the underlying target devices is exceeded.

While it is very difficult to correctly and reliably stack zone resource
limits in general, cases where targets are not sharing zone resources of
the same device can be dealt with relatively easily. Such situation
happens when a target maps all sequential zones of a zoned block device:
for such mapping, other targets mapping other parts of the same zoned
block device can only contain conventional zones and thus will not
require any zone resource to correctly handle write operations.

For a mapped device constructed with such targets, which includes mapped
devices constructed with targets mapping entire zoned block devices, the
zone resource limits can be reliably determined using the non-zero
minimum of the zone resource limits of all targets.

For mapped devices that include targets partially mapping the set of
sequential write required zones of zoned block devices, instead of
advertizing no zone resource limits, it is also better to set the mapped
device limits to the non-zero minimum of the limits of all targets. In
this case the limits for a target depend on the number of sequential
zones being mapped: if this number of zone is larger than the limits,
then the limits of the device apply and can be used. If on the other
hand the target maps a number of zones smaller than the limits, then no
limits is needed and we can assume that the target has no limits (limits
set to 0).

This commit improves zone resource limits handling as described above
using the function dm_set_zone_resource_limits(). This function is
executed from dm_set_zones_restrictions() and iterates the targets of a
mapped device to evaluate the max open and max active zone limits. This
relies on an internal "stacking" of the limits of the target devices
combined with a direct counting of the number of sequential zones
mapped by the targets.
1) For a target mapping an entire zoned block device, the limits for the
   target are set to the limits of the device.
2) For a target partially mapping a zoned block device, the number of
   mapped sequential zones is used to determine the limits: if the
   target maps more sequential write required zones than the device
   limits, then the limits of the device are used as-is. If the number
   of mapped sequential zones is lower than the limits, then we assume
   that the target has no limits (limits set to 0).
As this evaluation is done for each target, the zone resource limits
for the mapped device are evaluated as the non-zero minimum of the
limits of all the targets.

For configurations resulting in unreliable limits, i.e. a table
containing a target partially mapping a zoned device, a warning message
is issued.

The counting of mapped sequential zones for the target is done using the
new function dm_device_count_zones() which performs a report zones on
the entire block device with the callback dm_device_count_zones_cb().
This count of mapped sequential zones is used to determine if the mapped
device contains only conventional zones. This allows simplifying
dm_set_zones_restrictions() to not do a report zones. For mapped devices
mapping only conventional zones, dm_set_zone_resource_limits() changes
the mapped device to a regular device.

To further cleanup dm_set_zones_restrictions(), the message about the
type of zone append (native or emulated) is moved inside
dm_revalidate_zones().

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/md/dm-zone.c | 204 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 167 insertions(+), 37 deletions(-)

diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c
index 5d66d916730e..1199804b05c5 100644
--- a/drivers/md/dm-zone.c
+++ b/drivers/md/dm-zone.c
@@ -145,17 +145,164 @@ bool dm_is_zone_write(struct mapped_device *md, struct bio *bio)
 	}
 }
 
+struct dm_device_zone_count {
+	sector_t start;
+	sector_t len;
+	unsigned int total_nr_seq_zones;
+	unsigned int target_nr_seq_zones;
+};
+
 /*
- * Count conventional zones of a mapped zoned device. If the device
- * only has conventional zones, do not expose it as zoned.
+ * Count the total number of and the number of mapped sequential zones of a
+ * target zoned device.
  */
-static int dm_check_zoned_cb(struct blk_zone *zone, unsigned int idx,
-			     void *data)
+static int dm_device_count_zones_cb(struct blk_zone *zone,
+				    unsigned int idx, void *data)
+{
+	struct dm_device_zone_count *zc = data;
+
+	if (zone->type != BLK_ZONE_TYPE_CONVENTIONAL) {
+		zc->total_nr_seq_zones++;
+		if (zone->start >= zc->start &&
+		    zone->start < zc->start + zc->len)
+			zc->target_nr_seq_zones++;
+	}
+
+	return 0;
+}
+
+static int dm_device_count_zones(struct dm_dev *dev,
+				 struct dm_device_zone_count *zc)
+{
+	int ret;
+
+	ret = blkdev_report_zones(dev->bdev, 0, UINT_MAX,
+				  dm_device_count_zones_cb, zc);
+	if (ret < 0)
+		return ret;
+	if (!ret)
+		return -EIO;
+	return 0;
+}
+
+struct dm_zone_resource_limits {
+	unsigned int mapped_nr_seq_zones;
+	struct queue_limits *lim;
+	bool reliable_limits;
+};
+
+static int device_get_zone_resource_limits(struct dm_target *ti,
+					   struct dm_dev *dev, sector_t start,
+					   sector_t len, void *data)
+{
+	struct dm_zone_resource_limits *zlim = data;
+	struct gendisk *disk = dev->bdev->bd_disk;
+	unsigned int max_open_zones, max_active_zones;
+	int ret;
+	struct dm_device_zone_count zc = {
+		.start = start,
+		.len = len,
+	};
+
+	/*
+	 * If the target is not the whole device, the device zone resources may
+	 * be shared between different targets. Check this by counting the
+	 * number of mapped sequential zones: if this number is smaller than the
+	 * total number of sequential zones of the target device, then resource
+	 * sharing may happen and the zone limits will not be reliable.
+	 */
+	ret = dm_device_count_zones(dev, &zc);
+	if (ret) {
+		DMERR("Count device %s zones failed",
+		      disk->disk_name);
+		return ret;
+	}
+
+	zlim->mapped_nr_seq_zones += zc.target_nr_seq_zones;
+
+	/*
+	 * If the target does not map any sequential zones, then we do not need
+	 * any zone resource limits.
+	 */
+	if (!zc.target_nr_seq_zones)
+		return 0;
+
+	/*
+	 * If the target does not map all sequential zones, the limits
+	 * will not be reliable.
+	 */
+	if (zc.target_nr_seq_zones < zc.total_nr_seq_zones)
+		zlim->reliable_limits = false;
+
+	/*
+	 * If the target maps less sequential zones than the limit values, then
+	 * we do not have limits for this target.
+	 */
+	max_active_zones = disk->queue->limits.max_active_zones;
+	if (max_active_zones >= zc.target_nr_seq_zones)
+		max_active_zones = 0;
+	zlim->lim->max_active_zones =
+		min_not_zero(max_active_zones, zlim->lim->max_active_zones);
+
+	max_open_zones = disk->queue->limits.max_open_zones;
+	if (max_open_zones >= zc.target_nr_seq_zones)
+		max_open_zones = 0;
+	zlim->lim->max_open_zones =
+		min_not_zero(max_open_zones, zlim->lim->max_open_zones);
+
+	return 0;
+}
+
+static int dm_set_zone_resource_limits(struct mapped_device *md,
+				struct dm_table *t, struct queue_limits *lim)
 {
-	unsigned int *nr_conv_zones = data;
+	struct gendisk *disk = md->disk;
+	struct dm_zone_resource_limits zlim = {
+		.reliable_limits = true,
+		.lim = lim,
+	};
+
+	/* Get the zone resource limits from the targets. */
+	for (unsigned int i = 0; i < t->num_targets; i++) {
+		struct dm_target *ti = dm_table_get_target(t, i);
 
-	if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL)
-		(*nr_conv_zones)++;
+		if (!ti->type->iterate_devices ||
+		    ti->type->iterate_devices(ti,
+				device_get_zone_resource_limits, &zlim)) {
+			DMERR("Could not determine %s zone resource limits",
+			      md->disk->disk_name);
+			return -ENODEV;
+		}
+	}
+
+	/*
+	 * If we only have conventional zones mapped, expose the mapped device
+	 + as a regular device.
+	 */
+	if (!zlim.mapped_nr_seq_zones) {
+		lim->max_open_zones = 0;
+		lim->max_active_zones = 0;
+		lim->max_zone_append_sectors = 0;
+		lim->zone_write_granularity = 0;
+		lim->zoned = false;
+		clear_bit(DMF_EMULATE_ZONE_APPEND, &md->flags);
+		md->nr_zones = 0;
+		disk->nr_zones = 0;
+		return 0;
+	}
+
+	/*
+	 * Warn if the mapped device is partially using zone resources of the
+	 * target devices as that leads to unreliable limits, i.e. if another
+	 * mapped device uses the same underlying devices, we cannot enforce
+	 * zone limits to guarantee that writing will not lead to errors.
+	 * Note that we really should return an error for such case but there is
+	 * no easy way to find out if another mapped device uses the same
+	 * underlying zoned devices.
+	 */
+	if (!zlim.reliable_limits)
+		DMWARN("%s zone resource limits may be unreliable",
+		       disk->disk_name);
 
 	return 0;
 }
@@ -172,8 +319,13 @@ static int dm_revalidate_zones(struct mapped_device *md, struct dm_table *t)
 	int ret;
 
 	/* Revalidate only if something changed. */
-	if (!disk->nr_zones || disk->nr_zones != md->nr_zones)
+	if (!disk->nr_zones || disk->nr_zones != md->nr_zones) {
+		DMINFO("%s using %s zone append",
+		       md->disk->disk_name,
+		       queue_emulates_zone_append(disk->queue) ?
+		       "emulated" : "native");
 		md->nr_zones = 0;
+	}
 
 	if (md->nr_zones)
 		return 0;
@@ -224,8 +376,6 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q,
 		struct queue_limits *lim)
 {
 	struct mapped_device *md = t->md;
-	struct gendisk *disk = md->disk;
-	unsigned int nr_conv_zones = 0;
 	int ret;
 
 	/*
@@ -244,36 +394,16 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q,
 		return 0;
 
 	/*
-	 * Count conventional zones to check that the mapped device will indeed 
-	 * have sequential write required zones.
+	 * Determine the max open and max active zone limits for the mapped
+	 * device. For a mapped device containing only conventional zones, the
+	 * mapped device is changed to be a regular block device, so exit early
+	 * for such case.
 	 */
-	md->zone_revalidate_map = t;
-	ret = dm_blk_report_zones(disk, 0, UINT_MAX,
-				  dm_check_zoned_cb, &nr_conv_zones);
-	md->zone_revalidate_map = NULL;
-	if (ret < 0) {
-		DMERR("Check zoned failed %d", ret);
+	ret = dm_set_zone_resource_limits(md, t, lim);
+	if (ret)
 		return ret;
-	}
-
-	/*
-	 * If we only have conventional zones, expose the mapped device as
-	 * a regular device.
-	 */
-	if (nr_conv_zones >= ret) {
-		lim->max_open_zones = 0;
-		lim->max_active_zones = 0;
-		lim->zoned = false;
-		clear_bit(DMF_EMULATE_ZONE_APPEND, &md->flags);
-		disk->nr_zones = 0;
+	if (!lim->zoned)
 		return 0;
-	}
-
-	if (!md->disk->nr_zones) {
-		DMINFO("%s using %s zone append",
-		       md->disk->disk_name,
-		       queue_emulates_zone_append(q) ? "emulated" : "native");
-	}
 
 	ret = dm_revalidate_zones(md, t);
 	if (ret < 0)
-- 
2.45.1

[PATCH v4 3/3] dm: Remove unused macro DM_ZONE_INVALID_WP_OFST

[Damien Le Moal]

With the switch to using the zone append emulation of the block layer
zone write plugging, the macro DM_ZONE_INVALID_WP_OFST is no longer used
in dm-zone.c. Remove its definition.

Fixes: f211268ed1f9 ("dm: Use the block layer zone append emulation")
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/md/dm-zone.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c
index 1199804b05c5..164dc840d1f2 100644
--- a/drivers/md/dm-zone.c
+++ b/drivers/md/dm-zone.c
@@ -13,8 +13,6 @@
 
 #define DM_MSG_PREFIX "zone"
 
-#define DM_ZONE_INVALID_WP_OFST		UINT_MAX
-
 /*
  * For internal zone reports bypassing the top BIO submission path.
  */
-- 
2.45.1

Re: [PATCH v4 1/3] block: Improve checks on zone resource limits

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 2/3] dm: Improve zone resource limits handling

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 3/3] dm: Remove unused macro DM_ZONE_INVALID_WP_OFST

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 0/3] Fix DM zone resource limits stacking

[Johannes Thumshirn]

For the series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v4 1/3] block: Improve checks on zone resource limits

[Niklas Cassel]

On Wed, Jun 05, 2024 at 04:51:42PM +0900, Damien Le Moal wrote:
> Make sure that the zone resource limits of a zoned block device are
> correct by checking that:
> (a) If the device has a max active zones limit, make sure that the max
>     open zones limit is lower than the max active zones limit.
> (b) If the device has zone resource limits, check that the limits
>     values are lower than the number of sequential zones of the device.
>     If it is not, assume that the zoned device has no limits by setting
>     the limits to 0.
> 
> For (a), a check is added to blk_validate_zoned_limits() and an error
> returned if the max open zones limit exceeds the value of the max active
> zone limit (if there is one).
> 
> For (b), given that we need the number of sequential zones of the zoned
> device, this check is added to disk_update_zone_resources(). This is
> safe to do as that function is executed with the disk queue frozen and
> the check executed after queue_limits_start_update() which takes the
> queue limits lock. Of note is that the early return in this function
> for zoned devices that do not use zone write plugging (e.g. DM devices
> using native zone append) is moved to after the new check and adjustment
> of the zone resource limits so that the check applies to any zoned
> device.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  block/blk-settings.c |  8 ++++++++
>  block/blk-zoned.c    | 20 ++++++++++++++++----
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index effeb9a639bb..474c709ea85b 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -80,6 +80,14 @@ static int blk_validate_zoned_limits(struct queue_limits *lim)
>  	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_BLK_DEV_ZONED)))
>  		return -EINVAL;
>  
> +	/*
> +	 * Given that active zones include open zones, the maximum number of
> +	 * open zones cannot be larger than the maximum numbber of active zones.

s/numbber/number/


> +	 */
> +	if (lim->max_active_zones &&
> +	    lim->max_open_zones > lim->max_active_zones)
> +		return -EINVAL;
> +
>  	if (lim->zone_write_granularity < lim->logical_block_size)
>  		lim->zone_write_granularity = lim->logical_block_size;
>  
> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
> index 52abebf56027..8f89705f5e1c 100644
> --- a/block/blk-zoned.c
> +++ b/block/blk-zoned.c
> @@ -1647,8 +1647,22 @@ static int disk_update_zone_resources(struct gendisk *disk,
>  		return -ENODEV;
>  	}
>  
> +	lim = queue_limits_start_update(q);
> +
> +	/*
> +	 * Some devices can advertize zone resource limits that are larger than
> +	 * the number of sequential zones of the zoned block device, e.g. a
> +	 * small ZNS namespace. For such case, assume that the zoned device has
> +	 * no zone resource limits.
> +	 */
> +	nr_seq_zones = disk->nr_zones - nr_conv_zones;
> +	if (lim.max_open_zones >= nr_seq_zones)
> +		lim.max_open_zones = 0;
> +	if (lim.max_active_zones >= nr_seq_zones)
> +		lim.max_active_zones = 0;
> +

Is this really correct to transform to no limits?

The MAR and MOR limits are defined in the I/O Command Set Specific Identify
Namespace Data Structure for the Zoned Namespace Command Set.

However, the user has no ability to control these limits themselves
during a namespace management create ns, or for the format command
(and this still seems to be the case in the latest ZNS spec 1.1d).

Which means that the controller has no way of knowing the number of
resources to allocate to each namespace.

Some (all?) controllers will right now simply report the same MAR/MOR
for all namespaces.


So if I use the namespace management command to create two small
zoned namespaces, the number of sequential zones might be smaller
than the limits in both namespaces, but could together be exceeding
the limit.

How is ignoring the limit that we got from the device better than
actually exposing the limit which we got from the device?

Since AFAICT, this also means that we will expose 0 to sysfs
instead of the value that the device reported.



Perhaps we should only do this optimization if:
- the device is not ZNS, or
- the device is ZNS and does not support NS management, or
- the device is ZNS and supports NS management and implements TP4115
  (Zoned Namespace Resource Management supported bit is set, even if
   that TP does not seem to be part of a Ratified ZNS version yet...)


Kind regards,
Niklas

Re: [PATCH v4 2/3] dm: Improve zone resource limits handling

[Benjamin Marzinski]

On Wed, Jun 05, 2024 at 04:51:43PM +0900, Damien Le Moal wrote:
> +static int dm_device_count_zones(struct dm_dev *dev,
> +				 struct dm_device_zone_count *zc)
> +{
> +	int ret;
> +
> +	ret = blkdev_report_zones(dev->bdev, 0, UINT_MAX,
> +				  dm_device_count_zones_cb, zc);

Other than the nitpick that BLK_ALL_ZONES looks better than UINT_MAX
here, looks good.

-Ben

Re: [PATCH v4 3/3] dm: Remove unused macro DM_ZONE_INVALID_WP_OFST

[Benjamin Marzinski]

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

Re: [PATCH v4 2/3] dm: Improve zone resource limits handling

[Damien Le Moal]

On 6/6/24 4:47 AM, Benjamin Marzinski wrote:
> On Wed, Jun 05, 2024 at 04:51:43PM +0900, Damien Le Moal wrote:
>> +static int dm_device_count_zones(struct dm_dev *dev,
>> +				 struct dm_device_zone_count *zc)
>> +{
>> +	int ret;
>> +
>> +	ret = blkdev_report_zones(dev->bdev, 0, UINT_MAX,
>> +				  dm_device_count_zones_cb, zc);
> 
> Other than the nitpick that BLK_ALL_ZONES looks better than UINT_MAX
> here, looks good.

Thanks, will make this change.

However, I realized that we have another serious issue with this, so more
changes are needed. The issue is that we have the call chain:

dm_table_set_restrictions(t, q, lim) -> dm_set_zones_restrictions(t, q, lim) ->
dm_set_zone_resource_limits(md, t, lim) which  is fine as all these functions
operate looking at the same limits. But then after calling
dm_set_zone_resource_limits() which may modify the max open/max active limits,
dm_set_zones_restrictions() calls dm_revalidate_zones(md, t), which then calls
blk_revalidate_disk_zones(). This last function looks at the max open/max
active limits of the disk queue limits to setup zone write plugging (if needed
in the case of DM). But the disk queue limits are *different* from the lim
pointer passed from dm_table_set_restrictions() as these have not been applied
yet. So we have blk_revalidate_disk_zones() looking at the "old", not yet
corrected zone resource limits.

I have 22 different test cases for testing this and none of them can detect a
problem with this. But it is still wrong and needs to be fixed.

Christoph,

Unless you have a better idea, I think we need to pass a queue_limits struct
pointer to blk_revalidate_disk_zones() -> disk_update_zone_resources(). This
pointer can be NULL, in which case disk_update_zone_resources() needs to do the
limit start_update+commit. But if it is not NULL, then
disk_update_zone_resources() must use the passed limits.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 1/3] block: Improve checks on zone resource limits

[Damien Le Moal]

On 6/6/24 2:25 AM, Niklas Cassel wrote:
> On Wed, Jun 05, 2024 at 04:51:42PM +0900, Damien Le Moal wrote:
>> Make sure that the zone resource limits of a zoned block device are
>> correct by checking that:
>> (a) If the device has a max active zones limit, make sure that the max
>>     open zones limit is lower than the max active zones limit.
>> (b) If the device has zone resource limits, check that the limits
>>     values are lower than the number of sequential zones of the device.
>>     If it is not, assume that the zoned device has no limits by setting
>>     the limits to 0.
>>
>> For (a), a check is added to blk_validate_zoned_limits() and an error
>> returned if the max open zones limit exceeds the value of the max active
>> zone limit (if there is one).
>>
>> For (b), given that we need the number of sequential zones of the zoned
>> device, this check is added to disk_update_zone_resources(). This is
>> safe to do as that function is executed with the disk queue frozen and
>> the check executed after queue_limits_start_update() which takes the
>> queue limits lock. Of note is that the early return in this function
>> for zoned devices that do not use zone write plugging (e.g. DM devices
>> using native zone append) is moved to after the new check and adjustment
>> of the zone resource limits so that the check applies to any zoned
>> device.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  block/blk-settings.c |  8 ++++++++
>>  block/blk-zoned.c    | 20 ++++++++++++++++----
>>  2 files changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>> index effeb9a639bb..474c709ea85b 100644
>> --- a/block/blk-settings.c
>> +++ b/block/blk-settings.c
>> @@ -80,6 +80,14 @@ static int blk_validate_zoned_limits(struct queue_limits *lim)
>>  	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_BLK_DEV_ZONED)))
>>  		return -EINVAL;
>>  
>> +	/*
>> +	 * Given that active zones include open zones, the maximum number of
>> +	 * open zones cannot be larger than the maximum numbber of active zones.
> 
> s/numbber/number/
> 
> 
>> +	 */
>> +	if (lim->max_active_zones &&
>> +	    lim->max_open_zones > lim->max_active_zones)
>> +		return -EINVAL;
>> +
>>  	if (lim->zone_write_granularity < lim->logical_block_size)
>>  		lim->zone_write_granularity = lim->logical_block_size;
>>  
>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>> index 52abebf56027..8f89705f5e1c 100644
>> --- a/block/blk-zoned.c
>> +++ b/block/blk-zoned.c
>> @@ -1647,8 +1647,22 @@ static int disk_update_zone_resources(struct gendisk *disk,
>>  		return -ENODEV;
>>  	}
>>  
>> +	lim = queue_limits_start_update(q);
>> +
>> +	/*
>> +	 * Some devices can advertize zone resource limits that are larger than
>> +	 * the number of sequential zones of the zoned block device, e.g. a
>> +	 * small ZNS namespace. For such case, assume that the zoned device has
>> +	 * no zone resource limits.
>> +	 */
>> +	nr_seq_zones = disk->nr_zones - nr_conv_zones;
>> +	if (lim.max_open_zones >= nr_seq_zones)
>> +		lim.max_open_zones = 0;
>> +	if (lim.max_active_zones >= nr_seq_zones)
>> +		lim.max_active_zones = 0;
>> +
> 
> Is this really correct to transform to no limits?
> 
> The MAR and MOR limits are defined in the I/O Command Set Specific Identify
> Namespace Data Structure for the Zoned Namespace Command Set.
> 
> However, the user has no ability to control these limits themselves
> during a namespace management create ns, or for the format command
> (and this still seems to be the case in the latest ZNS spec 1.1d).
> 
> Which means that the controller has no way of knowing the number of
> resources to allocate to each namespace.
> 
> Some (all?) controllers will right now simply report the same MAR/MOR
> for all namespaces.
> 
> 
> So if I use the namespace management command to create two small
> zoned namespaces, the number of sequential zones might be smaller
> than the limits in both namespaces, but could together be exceeding
> the limit.
> 
> How is ignoring the limit that we got from the device better than
> actually exposing the limit which we got from the device?

If the limits are larger than the number of zones in the namespace, there is no
limits at all. This is what this change does. No question that this is correct.
Even if we do not change the values, any application/fs looking at these limits
being larger than the number of zones will conclude the same.

The problem you are raising is the reliability of the limits themselves, and
for NVMe ZNS, given that MOR/MAR are not defined per namespace, we are in the
same situation as with DM devices sharing the same zoned block dev through
different targets: even if the user respects the limits, write errors may
happen due to the backing dev limits (or controller limits for ZNS) being
exceeded. Nothing much we can do to easily deal with this right now. We would
need to constantly track zone states and implement a software driven zone state
machine checking the limits all the time to actually provide guarantees.

> Since AFAICT, this also means that we will expose 0 to sysfs
> instead of the value that the device reported.

Yes. But the value reported by the device is for the whole controller. The
sysfs attributes are for the block device == namespace.

> Perhaps we should only do this optimization if:
> - the device is not ZNS, or
> - the device is ZNS and does not support NS management, or
> - the device is ZNS and supports NS management and implements TP4115
>   (Zoned Namespace Resource Management supported bit is set, even if
>    that TP does not seem to be part of a Ratified ZNS version yet...)

Right now, this all works the same way for DM and nvme zns, so I think this is
all good. If anything, we should probably add a warning in the nvme driver
about the potentially unreliable moz/moz limits if we see a ZNS device with
multiple zoned namespaces.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 1/3] block: Improve checks on zone resource limits

[Niklas Cassel]

On Thu, Jun 06, 2024 at 09:06:58AM +0900, Damien Le Moal wrote:
> On 6/6/24 2:25 AM, Niklas Cassel wrote:
> > On Wed, Jun 05, 2024 at 04:51:42PM +0900, Damien Le Moal wrote:
> 
> The problem you are raising is the reliability of the limits themselves, and
> for NVMe ZNS, given that MOR/MAR are not defined per namespace, we are in the
> same situation as with DM devices sharing the same zoned block dev through
> different targets: even if the user respects the limits, write errors may
> happen due to the backing dev limits (or controller limits for ZNS) being
> exceeded. Nothing much we can do to easily deal with this right now. We would
> need to constantly track zone states and implement a software driven zone state
> machine checking the limits all the time to actually provide guarantees.
> 
> > Since AFAICT, this also means that we will expose 0 to sysfs
> > instead of the value that the device reported.
> 
> Yes. But the value reported by the device is for the whole controller. The
> sysfs attributes are for the block device == namespace.

The limits are defined in the I/O Command Set Specific Identify Namespace
Data Structure for the Zoned Namespace Command Set, so they are per NS,
otherwise they would have been defined in the I/O Command Set Specific
Identify Controller Data Structure for the Zoned Namespace Command Set.


> 
> > Perhaps we should only do this optimization if:
> > - the device is not ZNS, or
> > - the device is ZNS and does not support NS management, or
> > - the device is ZNS and supports NS management and implements TP4115
> >   (Zoned Namespace Resource Management supported bit is set, even if
> >    that TP does not seem to be part of a Ratified ZNS version yet...)
> 
> Right now, this all works the same way for DM and nvme zns, so I think this is
> all good. If anything, we should probably add a warning in the nvme driver
> about the potentially unreliable moz/moz limits if we see a ZNS device with
> multiple zoned namespaces.

Well, it is only a problem for ZNS devices with NS management.

If there are two ZNS namespaces on the device, and the device does not
support NS management, the device vendor would have been seriously silly
to not allocate and set the limits in the I/O Command Set Specific Identify
Namespace Data Structure for the Zoned Namespace Command Set correctly.

But yes, this concern cannot be solved in disk_update_zone_resources(),
which operates on per gendisk (and there is one gendisk per namespace),
so not much this function can do. If we were to do something, it would
have to be done in the nvme driver.


Perhaps if the device is ZNS, and does support NS management, but does
not have the Zoned Namespace Resource Management supported bit is set,
divide the MAR/MOR values reported by each namespace by the number of
ZNS namespaces?


Kind regards,
Niklas

Re: [PATCH v4 1/3] block: Improve checks on zone resource limits

[Damien Le Moal]

On 6/6/24 10:21 AM, Niklas Cassel wrote:
>> Right now, this all works the same way for DM and nvme zns, so I think this is
>> all good. If anything, we should probably add a warning in the nvme driver
>> about the potentially unreliable moz/moz limits if we see a ZNS device with
>> multiple zoned namespaces.
> 
> Well, it is only a problem for ZNS devices with NS management.
> 
> If there are two ZNS namespaces on the device, and the device does not
> support NS management, the device vendor would have been seriously silly
> to not allocate and set the limits in the I/O Command Set Specific Identify
> Namespace Data Structure for the Zoned Namespace Command Set correctly.
> 
> But yes, this concern cannot be solved in disk_update_zone_resources(),
> which operates on per gendisk (and there is one gendisk per namespace),
> so not much this function can do. If we were to do something, it would
> have to be done in the nvme driver.
> 
> 
> Perhaps if the device is ZNS, and does support NS management, but does
> not have the Zoned Namespace Resource Management supported bit is set,
> divide the MAR/MOR values reported by each namespace by the number of
> ZNS namespaces?

Maybe. But that would still not provide any guarantee: a buggy application not
respecting the limits would be able to steal resources from the other namespace.

In any case, I think this is a discussion to have on the nvme list.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 2/3] dm: Improve zone resource limits handling

[Christoph Hellwig]

On Thu, Jun 06, 2024 at 08:52:32AM +0900, Damien Le Moal wrote:
> Unless you have a better idea, I think we need to pass a queue_limits struct
> pointer to blk_revalidate_disk_zones() -> disk_update_zone_resources(). This
> pointer can be NULL, in which case disk_update_zone_resources() needs to do the
> limit start_update+commit. But if it is not NULL, then
> disk_update_zone_resources() must use the passed limits.

I think the right thing is to simply move the call to dm_revalidate_zones
out of dm_set_zones_restrictions and after the queue_limits_set call
in dm_table_set_restrictions.

Re: [PATCH v4 1/3] block: Improve checks on zone resource limits

[Christoph Hellwig]

On Thu, Jun 06, 2024 at 11:12:08AM +0900, Damien Le Moal wrote:
> Maybe. But that would still not provide any guarantee: a buggy application not
> respecting the limits would be able to steal resources from the other namespace.
> 
> In any case, I think this is a discussion to have on the nvme list.

Yes, the per-controller limits in NVMe are a big mistake that was
already mentioned during ZNS standardization.  We don't really have
any good way to deal with it except strongly recommending users to
not use multiple ZNS namespaes per controller.

Re: [PATCH v4 2/3] dm: Improve zone resource limits handling

[Damien Le Moal]

On 6/6/24 1:39 PM, Christoph Hellwig wrote:
> On Thu, Jun 06, 2024 at 08:52:32AM +0900, Damien Le Moal wrote:
>> Unless you have a better idea, I think we need to pass a queue_limits struct
>> pointer to blk_revalidate_disk_zones() -> disk_update_zone_resources(). This
>> pointer can be NULL, in which case disk_update_zone_resources() needs to do the
>> limit start_update+commit. But if it is not NULL, then
>> disk_update_zone_resources() must use the passed limits.
> 
> I think the right thing is to simply move the call to dm_revalidate_zones
> out of dm_set_zones_restrictions and after the queue_limits_set call
> in dm_table_set_restrictions.

Indeed, that is a simpler approach. Resending with that fixed.

-- 
Damien Le Moal
Western Digital Research