diff mbox series

[v2,12/28] block: Allow BIO-based drivers to use blk_revalidate_disk_zones()

Message ID 20240325044452.3125418-13-dlemoal@kernel.org
State New
Headers show
Series Zone write plugging | expand

Commit Message

Damien Le Moal March 25, 2024, 4:44 a.m. UTC 
In preparation for allowing BIO based device drivers to use zone write
plugging and its zone append emulation, allow these drivers to call
blk_revalidate_disk_zones() so that all zone resources necessary to zone
write plugging can be initialized.

To do so, remove the check in blk_revalidate_disk_zones() restricting
the use of this function to mq request-based drivers to allow also
BIO-based drivers to use it. This is safe to do as long as the
BIO-based block device queue is already setup and usable, as it should,
and can be safely frozen. The zone write plug hash table and
conventional zone bitmap are allocated and initialized only for mq
devices and for BIO-based devices that require zone append emulation.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-zoned.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig March 26, 2024, 7:08 a.m. UTC | #1 
> +	/*
> +	 * For devices using a BIO-based driver, we need zone resources only
> +	 * if zone append emulation is required.
> +	 */
> +	if (!queue_is_mq(disk->queue) &&
> +	    !queue_emulates_zone_append(disk->queue))
> +		return 0;
> +

This code and the comment is duplicated in two places.  It also fails
to capture why bio drivers don't need zoned resources - that is that
we can't enforce QD=1 per zone for them.  Maybe add a little helper
to encapsulate this check and expand the comment a little?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Damien Le Moal March 26, 2024, 8:12 a.m. UTC | #2 
On 3/26/24 16:08, Christoph Hellwig wrote:
>> +	/*
>> +	 * For devices using a BIO-based driver, we need zone resources only
>> +	 * if zone append emulation is required.
>> +	 */
>> +	if (!queue_is_mq(disk->queue) &&
>> +	    !queue_emulates_zone_append(disk->queue))
>> +		return 0;
>> +
> 
> This code and the comment is duplicated in two places.  It also fails
> to capture why bio drivers don't need zoned resources - that is that
> we can't enforce QD=1 per zone for them.  Maybe add a little helper
> to encapsulate this check and expand the comment a little?

That is not the reason. Because we can actually enforce QD=1 write per zone for
DM devices with zone write plugging. E.g. dm-crypt requires that.
The reason is more that most DM devices don't need to have that enforcement as
they are responsible for handling the sequential writing of zones themselves. So
no hash table and no conv bitmap needed for them, unless like dm-crypt, they
request zone append emulation, in which case, zone write plugging is turned on
for the DM device.

But I agree about the helper. Will make that change.

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
diff mbox series

Patch

diff --git a/block/blk-zoned.c b/block/blk-zoned.c
index 5b86f1aa80f0..8dc7cdc539c6 100644
--- a/block/blk-zoned.c
+++ b/block/blk-zoned.c
@@ -1481,6 +1481,14 @@  static int disk_revalidate_zone_resources(struct gendisk *disk,
 	unsigned int hash_size;
 	int ret;
 
+	/*
+	 * For devices using a BIO-based driver, we need zone resources only
+	 * if zone append emulation is required.
+	 */
+	if (!queue_is_mq(disk->queue) &&
+	    !queue_emulates_zone_append(disk->queue))
+		return 0;
+
 	/*
 	 * If the device has no limit on the maximum number of open and active
 	 * zones, set the max_open_zones queue limit to indicate the size of
@@ -1571,6 +1579,13 @@  static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 	/* Check zone type */
 	switch (zone->type) {
 	case BLK_ZONE_TYPE_CONVENTIONAL:
+		/*
+		 * For devices using a BIO-based driver, we need zone resources
+		 * only if zone append emulation is required.
+		 */
+		if (!queue_is_mq(disk->queue) &&
+		    !queue_emulates_zone_append(disk->queue))
+			break;
 		if (!args->conv_zones_bitmap) {
 			args->conv_zones_bitmap =
 				blk_alloc_zone_bitmap(q->node, args->nr_zones);
@@ -1602,10 +1617,11 @@  static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
 		/*
 		 * We need to track the write pointer of all zones that are not
 		 * empty nor full. So make sure we have a zone write plug for
-		 * such zone.
+		 * such zone if the device has a zone write plug hash table.
 		 */
 		wp_offset = blk_zone_wp_offset(zone);
-		if (wp_offset && wp_offset < zone_sectors) {
+		if (disk->zone_wplugs_hash &&
+		    wp_offset && wp_offset < zone_sectors) {
 			zwplug = disk_get_zone_wplug_locked(disk, zone->start,
 							    GFP_NOIO, &flags);
 			if (!zwplug)
@@ -1636,8 +1652,8 @@  static int blk_revalidate_zone_cb(struct blk_zone *zone, unsigned int idx,
  * be called within the disk ->revalidate method for blk-mq based drivers.
  * Before calling this function, the device driver must already have set the
  * device zone size (chunk_sector limit) and the max zone append limit.
- * For BIO based drivers, this function cannot be used. BIO based device drivers
- * only need to set disk->nr_zones so that the sysfs exposed value is correct.
+ * BIO based drivers can also use this function as long as the device queue
+ * can be safely frozen.
  * If the @update_driver_data callback function is not NULL, the callback is
  * executed with the device request queue frozen after all zones have been
  * checked.
@@ -1654,8 +1670,6 @@  int blk_revalidate_disk_zones(struct gendisk *disk,
 
 	if (WARN_ON_ONCE(!blk_queue_is_zoned(q)))
 		return -EIO;
-	if (WARN_ON_ONCE(!queue_is_mq(q)))
-		return -EIO;
 
 	if (!capacity)
 		return -ENODEV;