[PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[Yihang Li]

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

Re: [PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[Damien Le Moal]

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

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[Bart Van Assche]

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.

Re: [PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[Yihang Li]

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.

Re: [PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[Bart Van Assche]

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.

Re: [PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[Damien Le Moal]

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.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[Bart Van Assche]

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.

Re: [PATCH v5] scsi: sd: Ignore command SYNC CACHE error if format in progress

[Yihang Li]

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.