Re: [PATCH 1/2] scsi: sd: Check physical sector alignment of sequential zone writes

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 3/3/23 10:44, Shin'ichiro Kawasaki wrote:
> When host-managed SMR disks have different physical sector size and
> logical sector size, writes to conventional zones should be aligned to
> the logical sector size. On the other hand, ZBC/ZAC requires that writes
> to sequential write required zones shall be aligned to the physical
> sector size. Otherwise, the disks return the unaligned write command
> error. However, this error is common with other failure reasons. The
> error is also reported when the write start sector is not at the write
> pointer, or the write end sector is not in the same zone.
> 
> To clarify the write failure cause is the physical sector alignment,
> confirm before issuing write commands that the writes to sequential
> write required zones are aligned to the physical sector size. If not,
> print a relevant error message. This makes failure analysis easier, and
> also avoids error handling in low level drivers.
> 
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  drivers/scsi/sd.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 47dafe6b8a66..6d115b2fa99a 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1123,6 +1123,7 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
>  	sector_t lba = sectors_to_logical(sdp, blk_rq_pos(rq));
>  	sector_t threshold;
>  	unsigned int nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq));
> +	unsigned int pb_sectors = sdkp->physical_block_size >> SECTOR_SHIFT;
>  	unsigned int mask = logical_to_sectors(sdp, 1) - 1;
>  	bool write = rq_data_dir(rq) == WRITE;
>  	unsigned char protect, fua;
> @@ -1145,6 +1146,15 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
>  		goto fail;
>  	}
>  
> +	if (sdkp->device->type == TYPE_ZBC && blk_rq_zone_is_seq(rq) &&
> +	    (req_op(rq) == REQ_OP_WRITE || req_op(rq) == REQ_OP_ZONE_APPEND) &&
> +	    (!IS_ALIGNED(blk_rq_pos(rq), pb_sectors) ||
> +	     !IS_ALIGNED(blk_rq_sectors(rq), pb_sectors))) {
> +		scmd_printk(KERN_ERR, cmd,
> +			    "Sequential write request not aligned to the physical block size\n");
> +		goto fail;
> +	}

A little helper for this complicated check would be better, and that will avoid
the built bot warning you got when CONFIG_BLK_DEV_ZONED is not set.
Something like this:

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a38c71511bc9..71e4e51898d8 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1146,6 +1146,9 @@ static blk_status_t sd_setup_read_write_cmnd(struct
scsi_cmnd *cmd)
 		goto fail;
 	}

+	if (sdkp->device->type == TYPE_ZBC && !sd_zbc_check_write(cmd))
+		goto fail;
+
 	if ((blk_rq_pos(rq) & mask) || (blk_rq_sectors(rq) & mask)) {
 		scmd_printk(KERN_ERR, cmd, "request not aligned to the logical block size\n");
 		goto fail;
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 5eea762f84d1..f19711b92f25 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -254,6 +254,8 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t sector,
 blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd, sector_t *lba,
 				        unsigned int nr_blocks);

+bool sd_zbc_check_write(struct scsi_cmnd *cmd);
+
 #else /* CONFIG_BLK_DEV_ZONED */

 static inline void sd_zbc_free_zone_info(struct scsi_disk *sdkp) {}
@@ -290,6 +292,11 @@ static inline blk_status_t
sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd,

 #define sd_zbc_report_zones NULL

+static inline bool sd_zbc_check_write(struct scsi_cmnd *cmd)
+{
+	return true;
+}
+
 #endif /* CONFIG_BLK_DEV_ZONED */

 void sd_print_sense_hdr(struct scsi_disk *sdkp, struct scsi_sense_hdr *sshdr);
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 6b3a02d4406c..3025cb35f30c 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -983,3 +983,33 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8
buf[SD_BUF_SIZE])

 	return ret;
 }
+
+/**
+ * sd_zbc_check_write - Check if a write to a sequential zone is aligned to
+ *			the physical block size of the disk.
+ * @cmd: The command to check.
+ *
+ * Return false for write and zone append commands that are not aligned to
+ * the disk physical block size and true otherwise.
+ */
+bool sd_zbc_check_write(struct scsi_cmnd *cmd)
+{
+	struct request *rq = scsi_cmd_to_rq(cmd);
+	struct scsi_disk *sdkp = scsi_disk(rq->q->disk);
+	unsigned int pb_sectors = sdkp->physical_block_size >> SECTOR_SHIFT;
+
+	if (!blk_rq_zone_is_seq(rq))
+		return true;
+
+	if (req_op(rq) != REQ_OP_WRITE && req_op(rq) != REQ_OP_ZONE_APPEND)
+		return true;
+
+	if (!IS_ALIGNED(blk_rq_pos(rq), pb_sectors) ||
+	     !IS_ALIGNED(blk_rq_sectors(rq), pb_sectors)) {
+		scmd_printk(KERN_ERR, cmd,
+			"Write request not aligned to the physical block size\n");
+		return false;
+	}
+
+	return true;
+}
-- 
2.39.2




> +
>  	if ((blk_rq_pos(rq) & mask) || (blk_rq_sectors(rq) & mask)) {
>  		scmd_printk(KERN_ERR, cmd, "request not aligned to the logical block size\n");
>  		goto fail;

-- 
Damien Le Moal
Western Digital Research