convert newly added dm-zone code to the atomic queue commit API

convert newly added dm-zone code to the atomic queue commit API

[Christoph Hellwig]

Hi all,

the new dm-zone code added by Damien in 6.10-rc directly modifies the
queue limits instead of using the commit-style API that dm has used
forever and that the block layer adopted now, and thus can only run
after all the other changes have been commited.  This is quite a land
mine and can be easily fixed.

Note that if this doesn't go into 6.10-rc we'll need a way to get this
in before more block work in this area for 6.11, i.e. probably through
the block tree.

Diffstat:
 dm-table.c |   19 +++++++---------
 dm-zone.c  |   72 +++++++++++++++++++++++++++----------------------------------
 dm.h       |    3 +-
 3 files changed, 44 insertions(+), 50 deletions(-)

[PATCH 1/3] dm: move setting zoned_enabled to dm_table_set_restrictions

[Christoph Hellwig]

Keep it together with the rest of the zoned code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-table.c | 3 ---
 drivers/md/dm-zone.c  | 8 +++++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index cc66a27c363a65..e291b78b307b13 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -2040,9 +2040,6 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 		r = dm_set_zones_restrictions(t, q);
 		if (r)
 			return r;
-		if (blk_queue_is_zoned(q) &&
-		    !static_key_enabled(&zoned_enabled.key))
-			static_branch_enable(&zoned_enabled);
 	}
 
 	dm_update_crypto_profile(q, t);
diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c
index 8e6bcb0d786a1a..eb1165324ab047 100644
--- a/drivers/md/dm-zone.c
+++ b/drivers/md/dm-zone.c
@@ -287,7 +287,13 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q)
 		       queue_emulates_zone_append(q) ? "emulated" : "native");
 	}
 
-	return dm_revalidate_zones(md, t);
+	ret = dm_revalidate_zones(md, t);
+	if (ret < 0)
+		return 0;
+
+	if (!static_key_enabled(&zoned_enabled.key))
+		static_branch_enable(&zoned_enabled);
+	return 0;
 }
 
 /*
-- 
2.43.0

Re: [PATCH 1/3] dm: move setting zoned_enabled to dm_table_set_restrictions

[Mike Snitzer]

On Fri, May 24, 2024 at 04:29:09PM +0200, Christoph Hellwig wrote:
> Keep it together with the rest of the zoned code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/md/dm-table.c | 3 ---
>  drivers/md/dm-zone.c  | 8 +++++++-
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index cc66a27c363a65..e291b78b307b13 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -2040,9 +2040,6 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  		r = dm_set_zones_restrictions(t, q);
>  		if (r)
>  			return r;
> -		if (blk_queue_is_zoned(q) &&
> -		    !static_key_enabled(&zoned_enabled.key))
> -			static_branch_enable(&zoned_enabled);
>  	}
>  
>  	dm_update_crypto_profile(q, t);
> diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c
> index 8e6bcb0d786a1a..eb1165324ab047 100644
> --- a/drivers/md/dm-zone.c
> +++ b/drivers/md/dm-zone.c
> @@ -287,7 +287,13 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q)
>  		       queue_emulates_zone_append(q) ? "emulated" : "native");
>  	}
>  
> -	return dm_revalidate_zones(md, t);
> +	ret = dm_revalidate_zones(md, t);
> +	if (ret < 0)
> +		return 0;

Not following why you're return 0 if r < 0.

blk_revalidate_disk_zones() has quite a few negative checks that
return regular error codes, so why drop those validation errors?

> +
> +	if (!static_key_enabled(&zoned_enabled.key))
> +		static_branch_enable(&zoned_enabled);
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.43.0
> 

Re: [PATCH 1/3] dm: move setting zoned_enabled to dm_table_set_restrictions

[Christoph Hellwig]

On Fri, May 24, 2024 at 11:07:59AM -0400, Mike Snitzer wrote:
> > +	ret = dm_revalidate_zones(md, t);
> > +	if (ret < 0)
> > +		return 0;
> 
> Not following why you're return 0 if r < 0.
> 
> blk_revalidate_disk_zones() has quite a few negative checks that
> return regular error codes, so why drop those validation errors?

Because I messed this up..

[PATCH 2/3] dm: remove dm_check_zoned

[Christoph Hellwig]

Fold it into the only caller in preparation to changes in the
queue limits setup.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-zone.c | 59 +++++++++++++++++---------------------------
 1 file changed, 23 insertions(+), 36 deletions(-)

diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c
index eb1165324ab047..13b4857d98fe5b 100644
--- a/drivers/md/dm-zone.c
+++ b/drivers/md/dm-zone.c
@@ -160,37 +160,6 @@ static int dm_check_zoned_cb(struct blk_zone *zone, unsigned int idx,
 	return 0;
 }
 
-static int dm_check_zoned(struct mapped_device *md, struct dm_table *t)
-{
-	struct gendisk *disk = md->disk;
-	unsigned int nr_conv_zones = 0;
-	int ret;
-
-	/* Count conventional zones */
-	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);
-		return ret;
-	}
-
-	/*
-	 * If we only have conventional zones, expose the mapped device as
-	 * a regular device.
-	 */
-	if (nr_conv_zones >= ret) {
-		disk->queue->limits.max_open_zones = 0;
-		disk->queue->limits.max_active_zones = 0;
-		disk->queue->limits.zoned = false;
-		clear_bit(DMF_EMULATE_ZONE_APPEND, &md->flags);
-		disk->nr_zones = 0;
-	}
-
-	return 0;
-}
-
 /*
  * Revalidate the zones of a mapped device to initialize resource necessary
  * for zone append emulation. Note that we cannot simply use the block layer
@@ -254,6 +223,8 @@ static bool dm_table_supports_zone_append(struct dm_table *t)
 int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q)
 {
 	struct mapped_device *md = t->md;
+	struct gendisk *disk = md->disk;
+	unsigned int nr_conv_zones = 0;
 	int ret;
 
 	/*
@@ -272,14 +243,30 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q)
 		return 0;
 
 	/*
-	 * Check that the mapped device will indeed be zoned, that is, that it
-	 * has sequential write required zones.
+	 * Count conventional zones to check that the mapped device will indeed 
+	 * have sequential write required zones.
 	 */
-	ret = dm_check_zoned(md, t);
-	if (ret)
+	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);
 		return ret;
-	if (!blk_queue_is_zoned(q))
+	}
+
+	/*
+	 * If we only have conventional zones, expose the mapped device as
+	 * a regular device.
+	 */
+	if (nr_conv_zones >= ret) {
+		disk->queue->limits.max_open_zones = 0;
+		disk->queue->limits.max_active_zones = 0;
+		disk->queue->limits.zoned = false;
+		clear_bit(DMF_EMULATE_ZONE_APPEND, &md->flags);
+		disk->nr_zones = 0;
 		return 0;
+	}
 
 	if (!md->disk->nr_zones) {
 		DMINFO("%s using %s zone append",
-- 
2.43.0

Re: [PATCH 2/3] dm: remove dm_check_zoned

[Mike Snitzer]

On Fri, May 24, 2024 at 04:29:10PM +0200, Christoph Hellwig wrote:
> Fold it into the only caller in preparation to changes in the
> queue limits setup.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Mike Snitzer <snitzer@kernel.org>

[PATCH 3/3] dm: make dm_set_zones_restrictions work on the queue limits

[Christoph Hellwig]

Don't stuff the values directly into the queue without any
synchronization, but instead delay applying the queue limits in
the caller and let dm_set_zones_restrictions work on the limit
structure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/dm-table.c | 16 +++++++++-------
 drivers/md/dm-zone.c  | 11 ++++++-----
 drivers/md/dm.h       |  3 ++-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index e291b78b307b13..6e4ee829005e92 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1981,10 +1981,6 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	if (!dm_table_supports_secure_erase(t))
 		limits->max_secure_erase_sectors = 0;
 
-	r = queue_limits_set(q, limits);
-	if (r)
-		return r;
-
 	if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
 		wc = true;
 		if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))
@@ -2036,12 +2032,18 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	 * For a zoned target, setup the zones related queue attributes
 	 * and resources necessary for zone append emulation if necessary.
 	 */
-	if (blk_queue_is_zoned(q)) {
-		r = dm_set_zones_restrictions(t, q);
-		if (r)
+	if (limits->zoned) {
+		r = dm_set_zones_restrictions(t, q, limits);
+		if (r) {
+			queue_limits_cancel_update(q);
 			return r;
+		}
 	}
 
+	r = queue_limits_set(q, limits);
+	if (r)
+		return r;
+
 	dm_update_crypto_profile(q, t);
 
 	/*
diff --git a/drivers/md/dm-zone.c b/drivers/md/dm-zone.c
index 13b4857d98fe5b..719238aa3006ba 100644
--- a/drivers/md/dm-zone.c
+++ b/drivers/md/dm-zone.c
@@ -220,7 +220,8 @@ static bool dm_table_supports_zone_append(struct dm_table *t)
 	return true;
 }
 
-int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q)
+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;
@@ -236,7 +237,7 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q)
 		clear_bit(DMF_EMULATE_ZONE_APPEND, &md->flags);
 	} else {
 		set_bit(DMF_EMULATE_ZONE_APPEND, &md->flags);
-		blk_queue_max_zone_append_sectors(q, 0);
+		lim->max_zone_append_sectors = 0;
 	}
 
 	if (!get_capacity(md->disk))
@@ -260,9 +261,9 @@ int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q)
 	 * a regular device.
 	 */
 	if (nr_conv_zones >= ret) {
-		disk->queue->limits.max_open_zones = 0;
-		disk->queue->limits.max_active_zones = 0;
-		disk->queue->limits.zoned = false;
+		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;
 		return 0;
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index e0c57f19839b29..53ef8207fe2c15 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -101,7 +101,8 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t);
 /*
  * Zoned targets related functions.
  */
-int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q);
+int dm_set_zones_restrictions(struct dm_table *t, struct request_queue *q,
+		struct queue_limits *lim);
 void dm_zone_endio(struct dm_io *io, struct bio *clone);
 #ifdef CONFIG_BLK_DEV_ZONED
 int dm_blk_report_zones(struct gendisk *disk, sector_t sector,
-- 
2.43.0

Re: [PATCH 3/3] dm: make dm_set_zones_restrictions work on the queue limits

[Mike Snitzer]

On Fri, May 24, 2024 at 04:29:11PM +0200, Christoph Hellwig wrote:
> Don't stuff the values directly into the queue without any
> synchronization, but instead delay applying the queue limits in
> the caller and let dm_set_zones_restrictions work on the limit
> structure.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/md/dm-table.c | 16 +++++++++-------
>  drivers/md/dm-zone.c  | 11 ++++++-----
>  drivers/md/dm.h       |  3 ++-
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index e291b78b307b13..6e4ee829005e92 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -1981,10 +1981,6 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  	if (!dm_table_supports_secure_erase(t))
>  		limits->max_secure_erase_sectors = 0;
>  
> -	r = queue_limits_set(q, limits);
> -	if (r)
> -		return r;
> -
>  	if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_WC))) {
>  		wc = true;
>  		if (dm_table_supports_flush(t, (1UL << QUEUE_FLAG_FUA)))
> @@ -2036,12 +2032,18 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  	 * For a zoned target, setup the zones related queue attributes
>  	 * and resources necessary for zone append emulation if necessary.
>  	 */
> -	if (blk_queue_is_zoned(q)) {
> -		r = dm_set_zones_restrictions(t, q);
> -		if (r)
> +	if (limits->zoned) {
> +		r = dm_set_zones_restrictions(t, q, limits);
> +		if (r) {
> +			queue_limits_cancel_update(q);

Not aware of a preceding queue_limits_start_update().

Otherwise, rest of patch looks fine.

Re: [PATCH 3/3] dm: make dm_set_zones_restrictions work on the queue limits

[Christoph Hellwig]

On Fri, May 24, 2024 at 11:15:13AM -0400, Mike Snitzer wrote:
> > +	if (limits->zoned) {
> > +		r = dm_set_zones_restrictions(t, q, limits);
> > +		if (r) {
> > +			queue_limits_cancel_update(q);
> 
> Not aware of a preceding queue_limits_start_update().

True, that's actually hidden in queue_limits_set for the stacking
case.

Re: convert newly added dm-zone code to the atomic queue commit API

[Mike Snitzer]

On Fri, May 24, 2024 at 04:29:08PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> the new dm-zone code added by Damien in 6.10-rc directly modifies the
> queue limits instead of using the commit-style API that dm has used
> forever and that the block layer adopted now, and thus can only run
> after all the other changes have been commited.  This is quite a land
> mine and can be easily fixed.
> 
> Note that if this doesn't go into 6.10-rc we'll need a way to get this
> in before more block work in this area for 6.11, i.e. probably through
> the block tree.
> 
> Diffstat:
>  dm-table.c |   19 +++++++---------
>  dm-zone.c  |   72 +++++++++++++++++++++++++++----------------------------------
>  dm.h       |    3 +-
>  3 files changed, 44 insertions(+), 50 deletions(-)

Found a couple issues/questions in patches 1 and 3.

But once all looks good: I'm fine with these changes going through
Jens for 6.10-rc (especially in that all DM's zoned changes for 6.10
were merged through block anyway).

Mike