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

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

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@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:
> 


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;
> +}