[PATCH 0/2] scsi: sd: use READ/WRITE/SYNC (16) commands per ZBC

[PATCH 0/2] scsi: sd: use READ/WRITE/SYNC (16) commands per ZBC

[Shin'ichiro Kawasaki]

During work for a recent fix in libata-scsi [1], we checked which commands ZBC,
or Zoned Block Commands specification, mandates for zoned block devices. We
found two points to improve and this series has two patches to address them.

The first patch improves READ/WRITE (16) command enforcement. ZBC does not
mandate these commands to host-aware zoned block devices but current code in
sd_zbc_read_zones() assumes it. The second patch improves SYNCHRONIZE CACHE
(16) command enforcement. ZBC mandates it for host-managed zoned block
devices, and it should call it in place of SYNCHRONIZE CACHE (10).

Of note is that the second patch depends on the libata-scsi fix [1], and should
be merged to upstream after it.

[1] https://lore.kernel.org/linux-ide/20221107040229.1548793-1-shinichiro.kawasaki@wdc.com/

Shin'ichiro Kawasaki (2):
  scsi: sd_zbc: do not enforce READ/WRITE (16) on host-aware devices
  scsi: sd: enforce SYNCHRONIZE CACHE (16) on host-managed devices

 drivers/scsi/sd.c          | 16 ++++++++++++----
 drivers/scsi/sd_zbc.c      |  9 ++++++---
 include/scsi/scsi_device.h |  1 +
 3 files changed, 19 insertions(+), 7 deletions(-)

-- 
2.37.1

[PATCH 1/2] scsi: sd_zbc: do not enforce READ/WRITE (16) on host-aware devices

[Shin'ichiro Kawasaki]

ZBC Zoned Block Commands specification mandates READ (16) and WRITE (16)
commands only for host-managed zoned block devices. It does not mandate
the commands for host-aware zoned block devices. However, current
sd_zbc_read_zones() code assumes the commands were mandated for host-
aware devices also and enforces the commands. If the host-aware drives
do not support the commands, they may fail.

To conform to the ZBC specification and to avoid the failure, check
device type and modify use_16_for_rw and use_10_for_rw flags only for
host-managed zoned block devices, so that the READ (16) and WRITE (16)
commands are not enforced on host-aware zoned block devices.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/scsi/sd_zbc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index bd15624c6322..4717a55dbf35 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -921,9 +921,11 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 		return 0;
 	}
 
-	/* READ16/WRITE16 is mandatory for ZBC disks */
-	sdkp->device->use_16_for_rw = 1;
-	sdkp->device->use_10_for_rw = 0;
+	/* READ16/WRITE16 is mandatory for host-managed devices */
+	if (sdkp->device->type == TYPE_ZBC) {
+		sdkp->device->use_16_for_rw = 1;
+		sdkp->device->use_10_for_rw = 0;
+	}
 
 	if (!blk_queue_is_zoned(q)) {
 		/*
-- 
2.37.1

Re: [PATCH 1/2] scsi: sd_zbc: do not enforce READ/WRITE (16) on host-aware devices

[Damien Le Moal]

On 11/9/22 11:59, Shin'ichiro Kawasaki wrote:
> ZBC Zoned Block Commands specification mandates READ (16) and WRITE (16)
> commands only for host-managed zoned block devices. It does not mandate
> the commands for host-aware zoned block devices. However, current
> sd_zbc_read_zones() code assumes the commands were mandated for host-
> aware devices also and enforces the commands. If the host-aware drives
> do not support the commands, they may fail.
> 
> To conform to the ZBC specification and to avoid the failure, check
> device type and modify use_16_for_rw and use_10_for_rw flags only for
> host-managed zoned block devices, so that the READ (16) and WRITE (16)
> commands are not enforced on host-aware zoned block devices.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  drivers/scsi/sd_zbc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index bd15624c6322..4717a55dbf35 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -921,9 +921,11 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
>  		return 0;
>  	}
>  
> -	/* READ16/WRITE16 is mandatory for ZBC disks */
> -	sdkp->device->use_16_for_rw = 1;
> -	sdkp->device->use_10_for_rw = 0;
> +	/* READ16/WRITE16 is mandatory for host-managed devices */
> +	if (sdkp->device->type == TYPE_ZBC) {
> +		sdkp->device->use_16_for_rw = 1;
> +		sdkp->device->use_10_for_rw = 0;
> +	}
>  
>  	if (!blk_queue_is_zoned(q)) {
>  		/*

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/2] scsi: sd: enforce SYNCHRONIZE CACHE (16) on host-managed devices

[Shin'ichiro Kawasaki]

ZBC Zoned Block Commands specification mandates SYNCHRONIZE CACHE (16)
for host-managed zoned block devices, but does not mandate SYNCHRONIZE
CACHE (10). Call SYNCHRONIZE CACHE (16) in place of SYNCHRONIZE CACHE
(10) to ensure that the command is always supported. For this purpose,
add use_16_for_sync flag to struct scsi_device in same manner as
use_16_for_rw flag.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/scsi/sd.c          | 16 ++++++++++++----
 drivers/scsi/sd_zbc.c      |  3 ++-
 include/scsi/scsi_device.h |  1 +
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index eb76ba055021..faa2b55d1a21 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1026,8 +1026,13 @@ static blk_status_t sd_setup_flush_cmnd(struct scsi_cmnd *cmd)
 	/* flush requests don't perform I/O, zero the S/G table */
 	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 
-	cmd->cmnd[0] = SYNCHRONIZE_CACHE;
-	cmd->cmd_len = 10;
+	if (cmd->device->use_16_for_sync) {
+		cmd->cmnd[0] = SYNCHRONIZE_CACHE_16;
+		cmd->cmd_len = 16;
+	} else {
+		cmd->cmnd[0] = SYNCHRONIZE_CACHE;
+		cmd->cmd_len = 10;
+	}
 	cmd->transfersize = 0;
 	cmd->allowed = sdkp->max_retries;
 
@@ -1587,9 +1592,12 @@ static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr *sshdr)
 		sshdr = &my_sshdr;
 
 	for (retries = 3; retries > 0; --retries) {
-		unsigned char cmd[10] = { 0 };
+		unsigned char cmd[16] = { 0 };
 
-		cmd[0] = SYNCHRONIZE_CACHE;
+		if (sdp->use_16_for_sync)
+			cmd[0] = SYNCHRONIZE_CACHE_16;
+		else
+			cmd[0] = SYNCHRONIZE_CACHE;
 		/*
 		 * Leave the rest of the command zero to indicate
 		 * flush everything.
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 4717a55dbf35..a998f9c091dd 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -921,10 +921,11 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 		return 0;
 	}
 
-	/* READ16/WRITE16 is mandatory for host-managed devices */
+	/* READ16/WRITE16/SYNC16 is mandatory for host-managed devices */
 	if (sdkp->device->type == TYPE_ZBC) {
 		sdkp->device->use_16_for_rw = 1;
 		sdkp->device->use_10_for_rw = 0;
+		sdkp->device->use_16_for_sync = 1;
 	}
 
 	if (!blk_queue_is_zoned(q)) {
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c36656d8ac6c..afd2986007a4 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -184,6 +184,7 @@ struct scsi_device {
 	unsigned no_report_opcodes:1;	/* no REPORT SUPPORTED OPERATION CODES */
 	unsigned no_write_same:1;	/* no WRITE SAME command */
 	unsigned use_16_for_rw:1; /* Use read/write(16) over read/write(10) */
+	unsigned use_16_for_sync:1;	/* Use sync (16) over sync (10) */
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
 	unsigned skip_ms_page_3f:1;	/* do not use MODE SENSE page 0x3f */
 	unsigned skip_vpd_pages:1;	/* do not read VPD pages */
-- 
2.37.1

Re: [PATCH 2/2] scsi: sd: enforce SYNCHRONIZE CACHE (16) on host-managed devices

[Damien Le Moal]

On 11/9/22 11:59, Shin'ichiro Kawasaki wrote:
> ZBC Zoned Block Commands specification mandates SYNCHRONIZE CACHE (16)
> for host-managed zoned block devices, but does not mandate SYNCHRONIZE
> CACHE (10). Call SYNCHRONIZE CACHE (16) in place of SYNCHRONIZE CACHE
> (10) to ensure that the command is always supported. For this purpose,
> add use_16_for_sync flag to struct scsi_device in same manner as
> use_16_for_rw flag.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  drivers/scsi/sd.c          | 16 ++++++++++++----
>  drivers/scsi/sd_zbc.c      |  3 ++-
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index eb76ba055021..faa2b55d1a21 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1026,8 +1026,13 @@ static blk_status_t sd_setup_flush_cmnd(struct scsi_cmnd *cmd)
>  	/* flush requests don't perform I/O, zero the S/G table */
>  	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>  
> -	cmd->cmnd[0] = SYNCHRONIZE_CACHE;
> -	cmd->cmd_len = 10;
> +	if (cmd->device->use_16_for_sync) {
> +		cmd->cmnd[0] = SYNCHRONIZE_CACHE_16;
> +		cmd->cmd_len = 16;
> +	} else {
> +		cmd->cmnd[0] = SYNCHRONIZE_CACHE;
> +		cmd->cmd_len = 10;
> +	}
>  	cmd->transfersize = 0;
>  	cmd->allowed = sdkp->max_retries;
>  
> @@ -1587,9 +1592,12 @@ static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr *sshdr)
>  		sshdr = &my_sshdr;
>  
>  	for (retries = 3; retries > 0; --retries) {
> -		unsigned char cmd[10] = { 0 };
> +		unsigned char cmd[16] = { 0 };
>  
> -		cmd[0] = SYNCHRONIZE_CACHE;
> +		if (sdp->use_16_for_sync)
> +			cmd[0] = SYNCHRONIZE_CACHE_16;
> +		else
> +			cmd[0] = SYNCHRONIZE_CACHE;
>  		/*
>  		 * Leave the rest of the command zero to indicate
>  		 * flush everything.
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 4717a55dbf35..a998f9c091dd 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -921,10 +921,11 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
>  		return 0;
>  	}
>  
> -	/* READ16/WRITE16 is mandatory for host-managed devices */
> +	/* READ16/WRITE16/SYNC16 is mandatory for host-managed devices */
>  	if (sdkp->device->type == TYPE_ZBC) {
>  		sdkp->device->use_16_for_rw = 1;
>  		sdkp->device->use_10_for_rw = 0;
> +		sdkp->device->use_16_for_sync = 1;
>  	}
>  
>  	if (!blk_queue_is_zoned(q)) {
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index c36656d8ac6c..afd2986007a4 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -184,6 +184,7 @@ struct scsi_device {
>  	unsigned no_report_opcodes:1;	/* no REPORT SUPPORTED OPERATION CODES */
>  	unsigned no_write_same:1;	/* no WRITE SAME command */
>  	unsigned use_16_for_rw:1; /* Use read/write(16) over read/write(10) */
> +	unsigned use_16_for_sync:1;	/* Use sync (16) over sync (10) */
>  	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
>  	unsigned skip_ms_page_3f:1;	/* do not use MODE SENSE page 0x3f */
>  	unsigned skip_vpd_pages:1;	/* do not read VPD pages */

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/2] scsi: sd: use READ/WRITE/SYNC (16) commands per ZBC

[Christoph Hellwig]

What is the point in relaxing this?  Every modern device better support
16 byte commands even if they aren't strictly required for host aware
devices.

Re: [PATCH 0/2] scsi: sd: use READ/WRITE/SYNC (16) commands per ZBC

[Shinichiro Kawasaki]

On Nov 09, 2022 / 04:36, Christoph Hellwig wrote:
> What is the point in relaxing this?  Every modern device better support
> 16 byte commands even if they aren't strictly required for host aware
> devices.

My point was to make the check strictly follow the ZBC spec. But now I see that
it's the better to keep enforcing 16 byte commands to host-aware devices. I will
drop the first patch and revise the second patch to enforce SYNC 16 on both
host-aware and host-managed devices.

-- 
Shin'ichiro Kawasaki

Re: [PATCH 0/2] scsi: sd: use READ/WRITE/SYNC (16) commands per ZBC

[hch]

On Thu, Nov 10, 2022 at 02:20:09AM +0000, Shinichiro Kawasaki wrote:
> My point was to make the check strictly follow the ZBC spec. But now I see that
> it's the better to keep enforcing 16 byte commands to host-aware devices. I will
> drop the first patch and revise the second patch to enforce SYNC 16 on both
> host-aware and host-managed devices.

We don't "enforce" anything.  We just don't send the legacy commands for
devices that are guaranteed to be modern.  What is the advantage of
ever sending 10 bytes commands (inluding SYNCHRONIZE CACHE) to a modern
device?

Re: [PATCH 0/2] scsi: sd: use READ/WRITE/SYNC (16) commands per ZBC

[Damien Le Moal]

On 11/10/22 17:19, hch@infradead.org wrote:
> On Thu, Nov 10, 2022 at 02:20:09AM +0000, Shinichiro Kawasaki wrote:
>> My point was to make the check strictly follow the ZBC spec. But now I see that
>> it's the better to keep enforcing 16 byte commands to host-aware devices. I will
>> drop the first patch and revise the second patch to enforce SYNC 16 on both
>> host-aware and host-managed devices.
> 
> We don't "enforce" anything.  We just don't send the legacy commands for
> devices that are guaranteed to be modern.  What is the advantage of
> ever sending 10 bytes commands (inluding SYNCHRONIZE CACHE) to a modern
> device?

The ZBC specs define SYNC 16 as optional while SYNC 10 is mandatory. So
the device may not support SYNC 16 and we would get an invalid opcode
error. For SYNC, no advantages between SYNC 10 and SYNC 16. Not even
sure why they both exist. The point here is making sure we use the one
that the drive MUST support. That is, SYNC 16 for host managed and SYNC
10 for host aware (but these likely all also support SYNC 16, but we
cannot be sure of it).

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/2] scsi: sd: use READ/WRITE/SYNC (16) commands per ZBC

[Shinichiro Kawasaki]

On Nov 10, 2022 / 17:31, Damien Le Moal wrote:
> On 11/10/22 17:19, hch@infradead.org wrote:
> > On Thu, Nov 10, 2022 at 02:20:09AM +0000, Shinichiro Kawasaki wrote:
> >> My point was to make the check strictly follow the ZBC spec. But now I see that
> >> it's the better to keep enforcing 16 byte commands to host-aware devices. I will
> >> drop the first patch and revise the second patch to enforce SYNC 16 on both
> >> host-aware and host-managed devices.
> > 
> > We don't "enforce" anything.  We just don't send the legacy commands for
> > devices that are guaranteed to be modern.

I see, the word "enforce" was not a good choice. Will rephrase it in v3.

> > What is the advantage of
> > ever sending 10 bytes commands (inluding SYNCHRONIZE CACHE) to a modern
> > device?
> 
> The ZBC specs define SYNC 16 as optional while SYNC 10 is mandatory. So
> the device may not support SYNC 16 and we would get an invalid opcode
> error. For SYNC, no advantages between SYNC 10 and SYNC 16. Not even
> sure why they both exist. The point here is making sure we use the one
> that the drive MUST support. That is, SYNC 16 for host managed and SYNC
> 10 for host aware (but these likely all also support SYNC 16, but we
> cannot be sure of it).

Damien, thanks for the explanation. I think Christoph, Damien and I are on the
same page: we should call 16 byte WRITE/READ/SYNC commands to modern devices.
And both host-managed and host-aware ZBC devices are the modern devices.

Will prepare v3 based on this understanding.

-- 
Shin'ichiro Kawasaki