| Message ID | 20240819090934.2130592-1-liyihang9@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4f9eedfa27ae5806ed10906bcceee7bae49c8941 |
| Headers | show |
| Series | [v5] scsi: sd: Ignore command SYNC CACHE error if format in progress | expand |
On 8/19/24 18:09, Yihang Li wrote: > If formatting a suspended disk (such as formatting with different DIF > type), the disk will be resuming first, and then the format command will > submit to the disk through SG_IO ioctl. > > When the disk is processing the format command, the system does not submit > other commands to the disk. Therefore, the system attempts to suspend the > disk again and sends the SYNC CACHE command. However, the SYNC CACHE > command will fail because the disk is in the formatting process, which > will cause the runtime_status of the disk to error and it is difficult > for user to recover it. Error info like: > > [ 669.925325] sd 6:0:6:0: [sdg] Synchronizing SCSI cache > [ 670.202371] sd 6:0:6:0: [sdg] Synchronize Cache(10) failed: Result: hostbyte=0x00 driverbyte=DRIVER_OK > [ 670.216300] sd 6:0:6:0: [sdg] Sense Key : 0x2 [current] > [ 670.221860] sd 6:0:6:0: [sdg] ASC=0x4 ASCQ=0x4 > > To solve the issue, ignore the error and return success/0 when formatting > in progress. > > Cc: stable@vger.kernel.org > Signed-off-by: Yihang Li <liyihang9@huawei.com> > Reviewed-by: Bart Van Assche <bvanassche@acm.org> The patch changed significantly, so I do not think you can retain Bart's review tag... In any case, this looks OK to me, so: Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > --- > Changes since v4: > - Rename the commit title. > - Ignore the SYNC command error during formatting as suggested by Damien. > > Changes since v3: > - Add Cc tag for kernel stable. > > Changes since v2: > - Add Reviewed-by for Bart. > > Changes since v1: > - Updated and added error information to the patch description. > > --- > drivers/scsi/sd.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index adeaa8ab9951..2d7240a24b52 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -1823,13 +1823,15 @@ static int sd_sync_cache(struct scsi_disk *sdkp) > (sshdr.asc == 0x74 && sshdr.ascq == 0x71)) /* drive is password locked */ > /* this is no error here */ > return 0; > + > /* > - * This drive doesn't support sync and there's not much > - * we can do because this is called during shutdown > - * or suspend so just return success so those operations > - * can proceed. > + * If a format is in progress or if the drive does not > + * support sync, there is not much we can do because > + * this is called during shutdown or suspend so just > + * return success so those operations can proceed. > */ > - if (sshdr.sense_key == ILLEGAL_REQUEST) > + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || > + sshdr.sense_key == ILLEGAL_REQUEST) > return 0; > } >
On 8/19/24 3:57 AM, Damien Le Moal wrote: > The patch changed significantly, so I do not think you can retain Bart's review > tag... Agreed, my Reviewed-by definitely should have been removed. Bart.
On 8/19/24 2:09 AM, Yihang Li wrote:
> + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) ||
Shouldn't symbolic names be introduced for these numeric constants?
Although there is more code in the SCSI core that compares ASC / ASCQ
values with numeric constants, I think we need symbolic names for these
constants to make code like the above easier to read. There is already
a header file for definitions that come directly from the SCSI standard
and that is used by both SCSI initiator and SCSI target code:
<scsi/scsi_proto.h>.
Thanks,
Bart.
On 8/20/24 01:59, Bart Van Assche wrote: > On 8/19/24 2:09 AM, Yihang Li wrote: >> + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || > > Shouldn't symbolic names be introduced for these numeric constants? > Although there is more code in the SCSI core that compares ASC / ASCQ > values with numeric constants, I think we need symbolic names for these > constants to make code like the above easier to read. There is already > a header file for definitions that come directly from the SCSI standard > and that is used by both SCSI initiator and SCSI target code: > <scsi/scsi_proto.h>. That would be *a lot* to define... So in keeping with the current practice of using numerical values + comment documenting what the values are, it is why I suggested the comment change above this also as the asc/ascq names for this are added. It would be odd to have macros for just this asc/ascq here with so many other places using numerical values... Note: I personally find https://www.t10.org/lists/1spc-lst.htm more than enough to work with the scsi code. If we have to define macros for all this, that would be a lot of lines... > > Thanks, > > Bart.
On 2024/8/20 0:59, Bart Van Assche wrote: > On 8/19/24 2:09 AM, Yihang Li wrote: >> + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || > > Shouldn't symbolic names be introduced for these numeric constants? > Although there is more code in the SCSI core that compares ASC / ASCQ > values with numeric constants, I think we need symbolic names for these > constants to make code like the above easier to read. There is already > a header file for definitions that come directly from the SCSI standard > and that is used by both SCSI initiator and SCSI target code: > <scsi/scsi_proto.h>. > My idea is to be consistent with the style of the code context. That's why I use numerical values. If we want to use symbolic names to replace all numeric constants, I think that would be another series of patches, and the changes would be more. Thanks, Yihang.
On 2024/8/20 0:54, Bart Van Assche wrote: > On 8/19/24 3:57 AM, Damien Le Moal wrote: >> The patch changed significantly, so I do not think you can retain Bart's review >> tag... > > Agreed, my Reviewed-by definitely should have been removed. > Sorry about that forgot remove your Reviewed-by tag.
On 8/19/24 4:19 PM, Damien Le Moal wrote: > On 8/20/24 01:59, Bart Van Assche wrote: >> On 8/19/24 2:09 AM, Yihang Li wrote: >>> + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || >> >> Shouldn't symbolic names be introduced for these numeric constants? >> Although there is more code in the SCSI core that compares ASC / ASCQ >> values with numeric constants, I think we need symbolic names for these >> constants to make code like the above easier to read. There is already >> a header file for definitions that come directly from the SCSI standard >> and that is used by both SCSI initiator and SCSI target code: >> <scsi/scsi_proto.h>. > > That would be *a lot* to define... I meant introducing symbolic names only for the numerical constants that occur in this patch. Anyway, I'm fine with a descriptive comment too. Bart.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index adeaa8ab9951..2d7240a24b52 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1823,13 +1823,15 @@ static int sd_sync_cache(struct scsi_disk *sdkp) (sshdr.asc == 0x74 && sshdr.ascq == 0x71)) /* drive is password locked */ /* this is no error here */ return 0; + /* - * This drive doesn't support sync and there's not much - * we can do because this is called during shutdown - * or suspend so just return success so those operations - * can proceed. + * If a format is in progress or if the drive does not + * support sync, there is not much we can do because + * this is called during shutdown or suspend so just + * return success so those operations can proceed. */ - if (sshdr.sense_key == ILLEGAL_REQUEST) + if ((sshdr.asc == 0x04 && sshdr.ascq == 0x04) || + sshdr.sense_key == ILLEGAL_REQUEST) return 0; }