Re: [PATCH] scsi: sd: add runtime pm to open / release

[Martin Kepplinger]

On 09.08.20 17:26, Alan Stern wrote:
> On Sun, Aug 09, 2020 at 11:20:22AM +0200, Martin Kepplinger wrote:
>> Hey Alan, I'm really glad for that, I suspected some of this but I have
>> little experience in scsi/block layers, so that is super helpful.
>>
>> I'd appreciate an opinion on the below workaround that *seems* to work
>> now (let's see, I had thought so before :)
>>
>> Whether or not this helps to find a real solution, let's see. But
>> integration of such a flag in the error handling paths is what's
>> interesting for now:
>>
>>
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -565,6 +565,17 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
>>  				return NEEDS_RETRY;
>>  			}
>>  		}
>> +		if (scmd->device->expecting_media_change) {
>> +			if (sshdr.asc == 0x28 && sshdr.ascq == 0x00) {
>> +				/* clear expecting_media_change in
>> +				 * scsi_noretry_cmd() because we need
>> +				 * to override possible "failfast" overrides
>> +				 * that block readahead can cause.
>> +				 */
>> +				return NEEDS_RETRY;
> 
> This is a somewhat fragile approach.  You don't know for certain that 
> scsi_noretry_cmd will be called.  Also, scsi_noretry_cmd can be called 
> from other places.
> 
> It would be better to clear the expecting_media_change flag just before 
> returning from scsi_decide_disposition.  That way its use is localized 
> to one routine, not spread out between two.
> 
> Alan Stern
> 

Hi Alan,

maybe you're right. I initially just thought that I'd allow for specific
error codes in scsi_noretry_cmd() to return non-NULL (BUS_BUSY, PARITY,
ERROR) despite having the flag set.

The below version works equally fine for me but I'm not sure if it's
actually more safe.

James, when exposing a new writable sysfs option like
"suspend_no_media_change"(?) that drivers can check before setting the
new "expecting_media_change" flag (during resume), would this addition
make sense to you?

thanks,

                      martin



--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -565,6 +565,18 @@ int scsi_check_sense(struct scsi_cmnd *scmd)
 				return NEEDS_RETRY;
 			}
 		}
+		if (scmd->device->expecting_media_change) {
+			if (sshdr.asc == 0x28 && sshdr.ascq == 0x00) {
+				/*
+				 * clear the expecting_media_change in
+				 * scsi_decide_disposition() because we
+				 * need to catch possible "fail fast" overrides
+				 * that block readahead can cause.
+				 */
+				return NEEDS_RETRY;
+			}
+		}
+
 		/*
 		 * we might also expect a cc/ua if another LUN on the target
 		 * reported a UA with an ASC/ASCQ of 3F 0E -
@@ -1944,9 +1956,19 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
 	 * the request was not marked fast fail.  Note that above,
 	 * even if the request is marked fast fail, we still requeue
 	 * for queue congestion conditions (QUEUE_FULL or BUSY) */
-	if ((++scmd->retries) <= scmd->allowed
-	    && !scsi_noretry_cmd(scmd)) {
-		return NEEDS_RETRY;
+	if ((++scmd->retries) <= scmd->allowed) {
+		/*
+		 * but scsi_noretry_cmd() cannot override the
+		 * expecting_media_change flag.
+		 */
+		if (!scsi_noretry_cmd(scmd) ||
+		    scmd->device->expecting_media_change) {
+			scmd->device->expecting_media_change = 0;
+			return NEEDS_RETRY;
+		} else {
+			/* marked fast fail and not expected. */
+			return SUCCESS;
+		}
 	} else {
 		/*
 		 * no more retries - report this one back to upper level.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d90fefffe31b..bb583e403b81 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3642,6 +3642,8 @@ static int sd_resume(struct device *dev)
 	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
 		return 0;

+	sdkp->device->expecting_media_change = 1;
+
 	if (!sdkp->device->manage_start_stop)
 		return 0;

diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index bc5909033d13..f5fc1af68e00 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -169,6 +169,7 @@ struct scsi_device {
 				 * this device */
 	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
 				     * because we did a bus reset. */
+	unsigned expecting_media_change:1;
 	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
 	unsigned set_dbd_for_ms:1; /* Set "DBD" field in mode sense */
--