[PATCH 0/2] scsi: sd: Fix physical block size issues of host-managed zoned disks

[PATCH 0/2] scsi: sd: Fix physical block size issues of host-managed zoned disks

[Shin'ichiro Kawasaki]

In general, writes to SCSI disks are required to align to the logical block
size. On the other hand, ZBC and ZAC specifications require that the writes to
sequential write required zones to align to the physical block size. When
ZBC/ZAC host-managed zoned disks have the physical block size different from the
logical block size, writes aligned to the logical block size fail. The sysfs
attribute zone_write_granularity was introduced so that userland programs can
tell what is the alignment size for sequential write required zones. As for
ZBC/ZAC host-managed zoned disks, zone_write_granularity shows the physical
block size.

However, there are two issues related to this requirement of the physical block
size alignment. The first issue is unclear failure report. On the write fail,
the disks return the unaligned write command error, which may happen for other
causes other than the physical block size alignment. The second issue is wrong
value of zone_write_granularity sysfs attribute. In most cases, it shows
correct values. But during revalidate of the disks, it shows wrong values. The
two patches in this series address the two issues.

Shin'ichiro Kawasaki (2):
  scsi: sd: Check physical sector alignment of sequential zone writes
  scsi: sd: Fix wrong zone_write_granularity value at revalidate

 drivers/scsi/sd.c     | 17 ++++++++++++++++-
 drivers/scsi/sd_zbc.c |  8 --------
 2 files changed, 16 insertions(+), 9 deletions(-)

-- 
2.38.1

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

[Shin'ichiro Kawasaki]

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

[PATCH 2/2] scsi: sd: Fix wrong zone_write_granularity value at revalidate

[Shin'ichiro Kawasaki]

When sd driver revalidates host-managed SMR disks, it calls
disk_set_zoned() which changes the zone_write_granularity attribute
value to the logical block size regardless of the device type. After
that, the sd driver overwrites the value in sd_zbc_read_zone() with
the physical block size, since ZBC/ZAC requires it for the host-managed
disks. Between the calls to disk_set_zoned() and sd_zbc_read_zone(),
there exists a window that the attribute shows the logical block size as
the zone_write_granularity value, which is wrong for the host-managed
disks. The duration of the window is from 20ms to 200ms, depending on
report zone command execution time.

To avoid the wrong zone_write_granularity value between disk_set_zoned()
and sd_zbc_read_zone(), modify the value not in sd_zbc_read_zone() but
just after disk_set_zoned() call.

Fixes: a805a4fa4fa3 ("block: introduce zone_write_granularity limit")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/scsi/sd.c     | 7 ++++++-
 drivers/scsi/sd_zbc.c | 8 --------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6d115b2fa99a..d9be7b387e1b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2984,8 +2984,13 @@ static void sd_read_block_characteristics(struct scsi_disk *sdkp)
 	}
 
 	if (sdkp->device->type == TYPE_ZBC) {
-		/* Host-managed */
+		/*
+		 * Host-managed: Per ZBC and ZAC specifications, writes in
+		 * sequential write required zones of host-managed devices must
+		 * be aligned to the device physical block size.
+		 */
 		disk_set_zoned(sdkp->disk, BLK_ZONED_HM);
+		blk_queue_zone_write_granularity(q, sdkp->physical_block_size);
 	} else {
 		sdkp->zoned = zoned;
 		if (sdkp->zoned == 1) {
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 62abebbaf2e7..d33da6e1910f 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -963,14 +963,6 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 	disk_set_max_active_zones(disk, 0);
 	nr_zones = round_up(sdkp->capacity, zone_blocks) >> ilog2(zone_blocks);
 
-	/*
-	 * Per ZBC and ZAC specifications, writes in sequential write required
-	 * zones of host-managed devices must be aligned to the device physical
-	 * block size.
-	 */
-	if (blk_queue_zoned_model(q) == BLK_ZONED_HM)
-		blk_queue_zone_write_granularity(q, sdkp->physical_block_size);
-
 	sdkp->early_zone_info.nr_zones = nr_zones;
 	sdkp->early_zone_info.zone_blocks = zone_blocks;
 
-- 
2.38.1

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

[kernel test robot]

Hi Shin'ichiro,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jejb-scsi/for-next]
[also build test ERROR on mkp-scsi/for-next linus/master v6.2 next-20230303]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shin-ichiro-Kawasaki/scsi-sd-Check-physical-sector-alignment-of-sequential-zone-writes/20230303-094618
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
patch link:    https://lore.kernel.org/r/20230303014422.2466103-2-shinichiro.kawasaki%40wdc.com
patch subject: [PATCH 1/2] scsi: sd: Check physical sector alignment of sequential zone writes
config: arc-randconfig-r031-20230302 (https://download.01.org/0day-ci/archive/20230303/202303031358.dwPeGHeZ-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/60573809bdc58708e29f2f9ecbddb06aeb9e8716
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Shin-ichiro-Kawasaki/scsi-sd-Check-physical-sector-alignment-of-sequential-zone-writes/20230303-094618
        git checkout 60573809bdc58708e29f2f9ecbddb06aeb9e8716
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/scsi/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303031358.dwPeGHeZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/scsi/sd.c: In function 'sd_setup_read_write_cmnd':
>> drivers/scsi/sd.c:1151:47: error: implicit declaration of function 'blk_rq_zone_is_seq'; did you mean 'bio_zone_is_seq'? [-Werror=implicit-function-declaration]
    1151 |         if (sdkp->device->type == TYPE_ZBC && blk_rq_zone_is_seq(rq) &&
         |                                               ^~~~~~~~~~~~~~~~~~
         |                                               bio_zone_is_seq
   cc1: some warnings being treated as errors


vim +1151 drivers/scsi/sd.c

  1119	
  1120	static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd)
  1121	{
  1122		struct request *rq = scsi_cmd_to_rq(cmd);
  1123		struct scsi_device *sdp = cmd->device;
  1124		struct scsi_disk *sdkp = scsi_disk(rq->q->disk);
  1125		sector_t lba = sectors_to_logical(sdp, blk_rq_pos(rq));
  1126		sector_t threshold;
  1127		unsigned int nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq));
  1128		unsigned int pb_sectors = sdkp->physical_block_size >> SECTOR_SHIFT;
  1129		unsigned int mask = logical_to_sectors(sdp, 1) - 1;
  1130		bool write = rq_data_dir(rq) == WRITE;
  1131		unsigned char protect, fua;
  1132		blk_status_t ret;
  1133		unsigned int dif;
  1134		bool dix;
  1135	
  1136		ret = scsi_alloc_sgtables(cmd);
  1137		if (ret != BLK_STS_OK)
  1138			return ret;
  1139	
  1140		ret = BLK_STS_IOERR;
  1141		if (!scsi_device_online(sdp) || sdp->changed) {
  1142			scmd_printk(KERN_ERR, cmd, "device offline or changed\n");
  1143			goto fail;
  1144		}
  1145	
  1146		if (blk_rq_pos(rq) + blk_rq_sectors(rq) > get_capacity(rq->q->disk)) {
  1147			scmd_printk(KERN_ERR, cmd, "access beyond end of device\n");
  1148			goto fail;
  1149		}
  1150	
> 1151		if (sdkp->device->type == TYPE_ZBC && blk_rq_zone_is_seq(rq) &&
  1152		    (req_op(rq) == REQ_OP_WRITE || req_op(rq) == REQ_OP_ZONE_APPEND) &&
  1153		    (!IS_ALIGNED(blk_rq_pos(rq), pb_sectors) ||
  1154		     !IS_ALIGNED(blk_rq_sectors(rq), pb_sectors))) {
  1155			scmd_printk(KERN_ERR, cmd,
  1156				    "Sequential write request not aligned to the physical block size\n");
  1157			goto fail;
  1158		}
  1159	
  1160		if ((blk_rq_pos(rq) & mask) || (blk_rq_sectors(rq) & mask)) {
  1161			scmd_printk(KERN_ERR, cmd, "request not aligned to the logical block size\n");
  1162			goto fail;
  1163		}
  1164	
  1165		/*
  1166		 * Some SD card readers can't handle accesses which touch the
  1167		 * last one or two logical blocks. Split accesses as needed.
  1168		 */
  1169		threshold = sdkp->capacity - SD_LAST_BUGGY_SECTORS;
  1170	
  1171		if (unlikely(sdp->last_sector_bug && lba + nr_blocks > threshold)) {
  1172			if (lba < threshold) {
  1173				/* Access up to the threshold but not beyond */
  1174				nr_blocks = threshold - lba;
  1175			} else {
  1176				/* Access only a single logical block */
  1177				nr_blocks = 1;
  1178			}
  1179		}
  1180	
  1181		if (req_op(rq) == REQ_OP_ZONE_APPEND) {
  1182			ret = sd_zbc_prepare_zone_append(cmd, &lba, nr_blocks);
  1183			if (ret)
  1184				goto fail;
  1185		}
  1186	
  1187		fua = rq->cmd_flags & REQ_FUA ? 0x8 : 0;
  1188		dix = scsi_prot_sg_count(cmd);
  1189		dif = scsi_host_dif_capable(cmd->device->host, sdkp->protection_type);
  1190	
  1191		if (dif || dix)
  1192			protect = sd_setup_protect_cmnd(cmd, dix, dif);
  1193		else
  1194			protect = 0;
  1195	
  1196		if (protect && sdkp->protection_type == T10_PI_TYPE2_PROTECTION) {
  1197			ret = sd_setup_rw32_cmnd(cmd, write, lba, nr_blocks,
  1198						 protect | fua);
  1199		} else if (sdp->use_16_for_rw || (nr_blocks > 0xffff)) {
  1200			ret = sd_setup_rw16_cmnd(cmd, write, lba, nr_blocks,
  1201						 protect | fua);
  1202		} else if ((nr_blocks > 0xff) || (lba > 0x1fffff) ||
  1203			   sdp->use_10_for_rw || protect) {
  1204			ret = sd_setup_rw10_cmnd(cmd, write, lba, nr_blocks,
  1205						 protect | fua);
  1206		} else {
  1207			ret = sd_setup_rw6_cmnd(cmd, write, lba, nr_blocks,
  1208						protect | fua);
  1209		}
  1210	
  1211		if (unlikely(ret != BLK_STS_OK))
  1212			goto fail;
  1213	
  1214		/*
  1215		 * We shouldn't disconnect in the middle of a sector, so with a dumb
  1216		 * host adapter, it's safe to assume that we can at least transfer
  1217		 * this many bytes between each connect / disconnect.
  1218		 */
  1219		cmd->transfersize = sdp->sector_size;
  1220		cmd->underflow = nr_blocks << 9;
  1221		cmd->allowed = sdkp->max_retries;
  1222		cmd->sdb.length = nr_blocks * sdp->sector_size;
  1223	
  1224		SCSI_LOG_HLQUEUE(1,
  1225				 scmd_printk(KERN_INFO, cmd,
  1226					     "%s: block=%llu, count=%d\n", __func__,
  1227					     (unsigned long long)blk_rq_pos(rq),
  1228					     blk_rq_sectors(rq)));
  1229		SCSI_LOG_HLQUEUE(2,
  1230				 scmd_printk(KERN_INFO, cmd,
  1231					     "%s %d/%u 512 byte blocks.\n",
  1232					     write ? "writing" : "reading", nr_blocks,
  1233					     blk_rq_sectors(rq)));
  1234	
  1235		/*
  1236		 * This indicates that the command is ready from our end to be queued.
  1237		 */
  1238		return BLK_STS_OK;
  1239	fail:
  1240		scsi_free_sgtables(cmd);
  1241		return ret;
  1242	}
  1243	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Damien Le Moal]

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

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

[Johannes Thumshirn]

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

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

[Bart Van Assche]

On 3/2/23 17:44, Shin'ichiro Kawasaki wrote:
> +	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;
> +	}

I vote -1 for this patch because my opinion is that we should not 
duplicate checks that must be performed by the storage controller anyway 
inside the sd driver.

Thanks,

Bart.

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

[Damien Le Moal]

On 3/4/23 03:03, Bart Van Assche wrote:
> On 3/2/23 17:44, Shin'ichiro Kawasaki wrote:
>> +	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;
>> +	}
> 
> I vote -1 for this patch because my opinion is that we should not 
> duplicate checks that must be performed by the storage controller anyway 
> inside the sd driver.

Sure, the drive will fail this request, so the end result is the same. But what
is the point of issuing such unaligned request that we know will fail ? The
error message also make it easier to debug as it clarifies that this is not a
write pointer violation. So while this change is not critical, it does have
merits in my opinion.

> 
> Thanks,
> 
> Bart.

-- 
Damien Le Moal
Western Digital Research

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

[Bart Van Assche]

On 3/3/23 19:03, Damien Le Moal wrote:
> On 3/4/23 03:03, Bart Van Assche wrote:
>> On 3/2/23 17:44, Shin'ichiro Kawasaki wrote:
>>> +	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;
>>> +	}
>>
>> I vote -1 for this patch because my opinion is that we should not
>> duplicate checks that must be performed by the storage controller anyway
>> inside the sd driver.
> 
> Sure, the drive will fail this request, so the end result is the same. But what
> is the point of issuing such unaligned request that we know will fail ? The
> error message also make it easier to debug as it clarifies that this is not a
> write pointer violation. So while this change is not critical, it does have
> merits in my opinion.

I think that there are other ways to debug software that triggers an 
unaligned write, e.g. ftrace.

Thanks,

Bart.

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

[Shinichiro Kawasaki]

On Mar 04, 2023 / 07:21, Bart Van Assche wrote:
> On 3/3/23 19:03, Damien Le Moal wrote:
> > On 3/4/23 03:03, Bart Van Assche wrote:
> > > On 3/2/23 17:44, Shin'ichiro Kawasaki wrote:
> > > > +	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;
> > > > +	}
> > > 
> > > I vote -1 for this patch because my opinion is that we should not
> > > duplicate checks that must be performed by the storage controller anyway
> > > inside the sd driver.
> > 
> > Sure, the drive will fail this request, so the end result is the same. But what
> > is the point of issuing such unaligned request that we know will fail ? The
> > error message also make it easier to debug as it clarifies that this is not a
> > write pointer violation. So while this change is not critical, it does have
> > merits in my opinion.
> 
> I think that there are other ways to debug software that triggers an
> unaligned write, e.g. ftrace.

I see, then let me drop this patch. I will repost the second patch only for
reviews.

-- 
Shin'ichiro Kawasaki

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

[Johannes Thumshirn]

On 04.03.23 16:21, Bart Van Assche wrote:
>> Sure, the drive will fail this request, so the end result is the same. But what
>> is the point of issuing such unaligned request that we know will fail ? The
>> error message also make it easier to debug as it clarifies that this is not a
>> write pointer violation. So while this change is not critical, it does have
>> merits in my opinion.
> 
> I think that there are other ways to debug software that triggers an 
> unaligned write, e.g. ftrace.

Yeah but in order to do that, you have to restart your workload (that could 
potentially run days to trigger), start ftrace, yada yada.

So either that or have a message in dmesg, that your log collector can 
aggregate and is easy to parse.

It's not so much about duplication, but about user friendliness.

Johannes