diff mbox series

[v2,1/2] scsi: Fix handling of host-aware ZBC disks

Message ID 20200914003448.471624-2-damien.lemoal@wdc.com
State Superseded
Headers show
Series Fix handling of host-aware ZBC disks | expand

Commit Message

Damien Le Moal Sept. 14, 2020, 12:34 a.m. UTC
When CONFIG_BLK_DEV_ZONED is disabled, allow using host-aware ZBC
disks as regular disks. In this case, ensure that command completion
is correctly executed by changing sd_zbc_complete() to return good_bytes
instead of 0, causing a hang during device probe (endless retries).

When CONFIG_BLK_DEV_ZONED is enabled and a host-aware disk is detected
to have partitions, it will be used as a regular disk. In this case,
make sure to not do anything in sd_zbc_revalidate_zones() as that
triggers warnings.

Reported-by: Borislav Petkov <bp@alien8.de>
Fixes: b72053072c0b ("block: allow partitions on host aware zone devices")
Cc: <stable@vger.kernel.org>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Tested-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 drivers/scsi/sd.c     | 28 ++++++++++++++++++++++------
 drivers/scsi/sd.h     |  2 +-
 drivers/scsi/sd_zbc.c |  6 +++++-
 3 files changed, 28 insertions(+), 8 deletions(-)

Comments

Christoph Hellwig Sept. 14, 2020, 7:20 a.m. UTC | #1
On Mon, Sep 14, 2020 at 09:34:47AM +0900, Damien Le Moal wrote:
> When CONFIG_BLK_DEV_ZONED is disabled, allow using host-aware ZBC
> disks as regular disks. In this case, ensure that command completion
> is correctly executed by changing sd_zbc_complete() to return good_bytes
> instead of 0, causing a hang during device probe (endless retries).
> 
> When CONFIG_BLK_DEV_ZONED is enabled and a host-aware disk is detected
> to have partitions, it will be used as a regular disk. In this case,
> make sure to not do anything in sd_zbc_revalidate_zones() as that
> triggers warnings.
> 
> Reported-by: Borislav Petkov <bp@alien8.de>
> Fixes: b72053072c0b ("block: allow partitions on host aware zone devices")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> Tested-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  drivers/scsi/sd.c     | 28 ++++++++++++++++++++++------
>  drivers/scsi/sd.h     |  2 +-
>  drivers/scsi/sd_zbc.c |  6 +++++-
>  3 files changed, 28 insertions(+), 8 deletions(-)
> 
>  	} else {
>  		sdkp->zoned = (buffer[8] >> 4) & 3;
>  		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
> +			/*
> +			 * Host-aware disk without partition: use the disk as
> +			 * such if support for zoned block devices is enabled.
> +			 * Otherwise, use it as a regular disk.
> +			 */
> +			if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
> +				q->limits.zoned = BLK_ZONED_HA;
> +			else
> +				q->limits.zoned = BLK_ZONED_NONE;
>  		} else {
>  			/*
>  			 * Treat drive-managed devices and host-aware devices
>  			 * with partitions as regular block devices.
>  			 */
>  			q->limits.zoned = BLK_ZONED_NONE;
>  		}

I think we need to lift much of this into a block layer helper,
as the logic is way subtile.  Something like this (written in the MUA
editor, not even compile tested).

static inline void blk_queue_set_zoned(struct gendisk *disk,
		enum blk_zoned_model model)
{
	switch (model) {
	case BLK_ZONED_HM:
		WARN_ON_ONCE(!IS_ENABLED(CONFIG_BLK_DEV_ZONED));
		break;
	/*
	 * Host aware drivers are neither fish nor fowl.  We can either
	 * treat them like a drive managed devices, in which case they
	 * aren't different from a normal block device, or we can try
	 * to take advantage of the Zone command set, but in that case
	 * we need to treat them like a host managed device.  We try
	 * the latter if there are not partitions and zoned block device
	 * set support is enabled, else we do nothing special as far as
	 * the block layer is concerned.
	 */
	case BLK_ZONED_HA:
		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) ||
		    disk_has_partitions(disk)) {
			model = BLK_ZONED_NONE;
		break;
	default:
		break;

	disk->queue->limits.zoned = model;
}

Then in sd do:

	if (sdkp->device->type == TYPE_ZBC) {
		blk_queue_set_zoned(sdkp->disk, BLK_ZONED_HM);
	} else {
		sdkp->zoned = (buffer[8] >> 4) & 3;
		if (sdkp->zoned == 1)
			blk_queue_set_zoned(sdkp->disk, BLK_ZONED_HA);
		else
			blk_queue_set_zoned(sdkp->disk, BLK_ZONED_NONE);
	}
Damien Le Moal Sept. 14, 2020, 9:03 a.m. UTC | #2
On 2020/09/14 16:20, Christoph Hellwig wrote:
> On Mon, Sep 14, 2020 at 09:34:47AM +0900, Damien Le Moal wrote:
>> When CONFIG_BLK_DEV_ZONED is disabled, allow using host-aware ZBC
>> disks as regular disks. In this case, ensure that command completion
>> is correctly executed by changing sd_zbc_complete() to return good_bytes
>> instead of 0, causing a hang during device probe (endless retries).
>>
>> When CONFIG_BLK_DEV_ZONED is enabled and a host-aware disk is detected
>> to have partitions, it will be used as a regular disk. In this case,
>> make sure to not do anything in sd_zbc_revalidate_zones() as that
>> triggers warnings.
>>
>> Reported-by: Borislav Petkov <bp@alien8.de>
>> Fixes: b72053072c0b ("block: allow partitions on host aware zone devices")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> Tested-by: Borislav Petkov <bp@suse.de>
>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> ---
>>  drivers/scsi/sd.c     | 28 ++++++++++++++++++++++------
>>  drivers/scsi/sd.h     |  2 +-
>>  drivers/scsi/sd_zbc.c |  6 +++++-
>>  3 files changed, 28 insertions(+), 8 deletions(-)
>>
>>  	} else {
>>  		sdkp->zoned = (buffer[8] >> 4) & 3;
>>  		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
>> +			/*
>> +			 * Host-aware disk without partition: use the disk as
>> +			 * such if support for zoned block devices is enabled.
>> +			 * Otherwise, use it as a regular disk.
>> +			 */
>> +			if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
>> +				q->limits.zoned = BLK_ZONED_HA;
>> +			else
>> +				q->limits.zoned = BLK_ZONED_NONE;
>>  		} else {
>>  			/*
>>  			 * Treat drive-managed devices and host-aware devices
>>  			 * with partitions as regular block devices.
>>  			 */
>>  			q->limits.zoned = BLK_ZONED_NONE;
>>  		}
> 
> I think we need to lift much of this into a block layer helper,
> as the logic is way subtile.  Something like this (written in the MUA
> editor, not even compile tested).
> 
> static inline void blk_queue_set_zoned(struct gendisk *disk,
> 		enum blk_zoned_model model)
> {
> 	switch (model) {
> 	case BLK_ZONED_HM:
> 		WARN_ON_ONCE(!IS_ENABLED(CONFIG_BLK_DEV_ZONED));
> 		break;
> 	/*
> 	 * Host aware drivers are neither fish nor fowl.  We can either
> 	 * treat them like a drive managed devices, in which case they
> 	 * aren't different from a normal block device, or we can try
> 	 * to take advantage of the Zone command set, but in that case
> 	 * we need to treat them like a host managed device.  We try
> 	 * the latter if there are not partitions and zoned block device
> 	 * set support is enabled, else we do nothing special as far as
> 	 * the block layer is concerned.
> 	 */
> 	case BLK_ZONED_HA:
> 		if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) ||
> 		    disk_has_partitions(disk)) {
> 			model = BLK_ZONED_NONE;
> 		break;
> 	default:
> 		break;
> 
> 	disk->queue->limits.zoned = model;
> }
> 
> Then in sd do:
> 
> 	if (sdkp->device->type == TYPE_ZBC) {
> 		blk_queue_set_zoned(sdkp->disk, BLK_ZONED_HM);
> 	} else {
> 		sdkp->zoned = (buffer[8] >> 4) & 3;
> 		if (sdkp->zoned == 1)
> 			blk_queue_set_zoned(sdkp->disk, BLK_ZONED_HA);
> 		else
> 			blk_queue_set_zoned(sdkp->disk, BLK_ZONED_NONE);
> 	}
> 

Yes, that's nice. Will send something along these lines. But since this is a bug
fix for the current cycle & stable, we probably should keep the patch as is and
add the improvement on top for 5.10, no ?
Christoph Hellwig Sept. 14, 2020, 2:20 p.m. UTC | #3
On Mon, Sep 14, 2020 at 09:03:49AM +0000, Damien Le Moal wrote:
> Yes, that's nice. Will send something along these lines. But since this is a bug
> fix for the current cycle & stable, we probably should keep the patch as is and
> add the improvement on top for 5.10, no ?

I'd much rather also add the trivial helper for 5.9 - otherwise we'll
need to pull current mainline into the for-5.10 tree and create
all kinds of mess.  And just adding the helper and using it for sd
only will have no side effects elsewhere.
Damien Le Moal Sept. 14, 2020, 11:26 p.m. UTC | #4
On 2020/09/14 23:20, hch@infradead.org wrote:
> On Mon, Sep 14, 2020 at 09:03:49AM +0000, Damien Le Moal wrote:
>> Yes, that's nice. Will send something along these lines. But since this is a bug
>> fix for the current cycle & stable, we probably should keep the patch as is and
>> add the improvement on top for 5.10, no ?
> 
> I'd much rather also add the trivial helper for 5.9 - otherwise we'll
> need to pull current mainline into the for-5.10 tree and create
> all kinds of mess.  And just adding the helper and using it for sd
> only will have no side effects elsewhere.
> 

OK. Sending a v3 with this added.
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 95018e650f2d..e99f57bfeff4 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2968,22 +2968,38 @@  static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 	} else {
 		sdkp->zoned = (buffer[8] >> 4) & 3;
 		if (sdkp->zoned == 1 && !disk_has_partitions(sdkp->disk)) {
-			/* Host-aware */
-			q->limits.zoned = BLK_ZONED_HA;
+			/*
+			 * Host-aware disk without partition: use the disk as
+			 * such if support for zoned block devices is enabled.
+			 * Otherwise, use it as a regular disk.
+			 */
+			if (IS_ENABLED(CONFIG_BLK_DEV_ZONED))
+				q->limits.zoned = BLK_ZONED_HA;
+			else
+				q->limits.zoned = BLK_ZONED_NONE;
 		} else {
 			/*
 			 * Treat drive-managed devices and host-aware devices
 			 * with partitions as regular block devices.
 			 */
 			q->limits.zoned = BLK_ZONED_NONE;
-			if (sdkp->zoned == 2 && sdkp->first_scan)
-				sd_printk(KERN_NOTICE, sdkp,
-					  "Drive-managed SMR disk\n");
 		}
 	}
-	if (blk_queue_is_zoned(q) && sdkp->first_scan)
+
+	if (!sdkp->first_scan)
+		goto out;
+
+	if (blk_queue_is_zoned(q)) {
 		sd_printk(KERN_NOTICE, sdkp, "Host-%s zoned block device\n",
 		      q->limits.zoned == BLK_ZONED_HM ? "managed" : "aware");
+	} else {
+		if (sdkp->zoned == 1)
+			sd_printk(KERN_NOTICE, sdkp,
+				  "Host-aware SMR disk used as regular disk\n");
+		else if (sdkp->zoned == 2)
+			sd_printk(KERN_NOTICE, sdkp,
+				  "Drive-managed SMR disk\n");
+	}
 
  out:
 	kfree(buffer);
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 4933e7daf17d..7251434100e6 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -259,7 +259,7 @@  static inline blk_status_t sd_zbc_setup_zone_mgmt_cmnd(struct scsi_cmnd *cmd,
 static inline unsigned int sd_zbc_complete(struct scsi_cmnd *cmd,
 			unsigned int good_bytes, struct scsi_sense_hdr *sshdr)
 {
-	return 0;
+	return good_bytes;
 }
 
 static inline blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd,
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 0e94ff056bff..a739456dea02 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -667,7 +667,11 @@  int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
 	u32 max_append;
 	int ret = 0;
 
-	if (!sd_is_zoned(sdkp))
+	/*
+	 * There is nothing to do for regular disks, including host-aware disks
+	 * that have partitions.
+	 */
+	if (!blk_queue_is_zoned(q))
 		return 0;
 
 	/*