| Message ID | 20230303014422.2466103-2-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: sd: Fix physical block size issues of host-managed zoned disks | expand |
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
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; +}
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; > +}
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.
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.
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.
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.
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
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;
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(+)