Re: [PATCH 0/4] dm: zoned block device fixes

[Mike Snitzer <snitzer@redhat.com>]

On Mon, May 29 2017 at  6:23P -0400,
Damien Le Moal <damien.lemoal@wdc.com> wrote:

> Mike,
> 
> The first 3 patches of this series are incremental fixes for the zoned block
> device support patches that you committed to the for-4.13/dm branch.
> 
> The first patch correct the zone alignement checks so that the check is
> performed for any device, regardless of the device LBA size (it is skipped for
> 512B LBA devices otherwise).

I folded this first patch into the original commit (baf844bf4ae3).

> The second patch is a fix for commit baf844bf4ae3 "dm table: add zoned block
> devices validation". In that commit, the stacked limits zoned model was not
> set to the zoned model of the table target devices, leading to the exposed
> device always being exposed as a regular block device. With this fix, dm-flaky
> and dm-linear work fine on top of host-managed zoned block devices.
> 
> The third patch fixes zoned model validation again to allow for target types
> emulating a different zoned model than the model of the table target devices,
> e.g. dm-zoned.

The 2nd and 3rd seem over-done to me.  After spending more time than
ideal, the following patch would seem to be the equivalent to what
you've done in patches 2 and 3 (sans the "cleanup" of passing limits to
validate_hardware_zoned_model):

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 6545150..a39bcd9 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1523,19 +1524,39 @@ int dm_calculate_queue_limits(struct dm_table *table,
 			       dm_device_name(table->md),
 			       (unsigned long long) ti->begin,
 			       (unsigned long long) ti->len);
+
+		/*
+		 * FIXME: this should likely be moved to blk_stack_limits(), would
+		 * also eliminate limits->zoned stacking hack in dm_set_device_limits()
+		 */
+		if (limits->zoned == BLK_ZONED_NONE && ti_limits.zoned != BLK_ZONED_NONE) {
+			/*
+			 * By default, the stacked limits zoned model is set to
+			 * BLK_ZONED_NONE in blk_set_stacking_limits(). Update
+			 * this model using the first target model reported
+			 * that is not BLK_ZONED_NONE. This will be either the
+			 * first target device zoned model or the model reported
+			 * by the target .io_hints.
+			 */
+			limits->zoned = ti_limits.zoned;
+		}
 	}
 
 	/*
 	 * Verify that the zoned model and zone sectors, as determined before
 	 * any .io_hints override, are the same across all devices in the table.
-	 * - but if limits->zoned is not BLK_ZONED_NONE validate match for it
-	 * - simillarly, check all devices conform to limits->chunk_sectors if
-	 *   .io_hints altered them
+	 * - this is especially relevant if .io_hints is emulating a disk-managed
+	 *   zoned model (aka BLK_ZONED_NONE) on host-managed zoned block devices.
+	 * BUT...
 	 */
-	if (limits->zoned != BLK_ZONED_NONE)
+	if (limits->zoned != BLK_ZONED_NONE) {
+		/*
+		 * ...IF the above limits stacking determined a zoned model
+		 * validate that all of the table's devices conform to it.
+		 */
 		zoned_model = limits->zoned;
-	if (limits->chunk_sectors != zone_sectors)
 		zone_sectors = limits->chunk_sectors;
+	}
 	if (validate_hardware_zoned_model(table, zoned_model, zone_sectors))
 		return -EINVAL;
 
Anyway, I've folded this into the original commit too.  If you could
verify it works with your scenarios I'd appreciate it.

FYI, any additional cosmetic cleanup can wait (I happen to think this
code is clearer than how you relied on the matches functions to
initialize a temporary value).

I also folded in an validate_hardware_zoned_model() optimization to
return early if zoned_model == BLK_ZONED_NONE, please see/test the
rolled-up end result here:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-4.13/dm&id=9a6b54360f147c2d25fba7debc31a3251b804cc2

Also, please note that I've forcibly rebased linux-dm.git's
'for-4.13/dm' and staged it in 'for-next'.

> The last patch is dm-zoned with various fixes (mainly crashes on setup error
> and handling of the metadata cache shrinker). For your review, please use this
> version.

Will do, thanks.

Mike