On 03.03.23 07:44, Damien Le Moal wrote:
> 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@xxxxxxx>
>> ---
>> 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:
>
Agreed, I like that :)
> 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;
> +}
