| Message ID | 1476249921-17017-1-git-send-email-krisman@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, 2016-10-12 at 02:25 -0300, Gabriel Krisman Bertazi wrote: > Usually, re-sending the SCSI command is enough to recover from a Unit > Attention (UA). This adds a generic retry code to the SCSI command > path in case of an UA, before giving up and returning the error > condition to the caller. > > I added the UA verification into scsi_execute instead of > scsi_execute_req because there are at least a few callers that invoke > scsi_execute directly and would benefit from the internal UA retry. > Also, I didn't use scsi_normalize_sense to not duplicate > functionality with scsi_execute_req_flags. Instead, scsi_execute > uses a small helper function that verifies only the UA condition > directly from the raw sense buffer. If this design is not OK, I can > refactor to use scsi_normalize_sense. > > This prevents us from duplicating the retry code in at least a few > places. In particular, it fixes an issue found in some IBM > enclosures, in which the device may return an Unit Attention during > probe, breaking the bind with the ses module: > > scsi 1:0:7:0: Failed to get diagnostic page 0x8000002 > scsi 1:0:7:0: Failed to bind enclosure -19 > > Finally, should we have a NORETRY_UA flag to allow callers to disable > this mechanism? I think not: let's use retries for this. Allowing no retries would be the signal that you want the UA condition returned. > Link: https://patchwork.kernel.org/patch/9336763/ > Suggested-by: Brian King <brking@linux.vnet.ibm.com> > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com> > Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com> > --- > drivers/scsi/scsi_lib.c | 47 > +++++++++++++++++++++++++++++++++++++++++----- > include/scsi/scsi_common.h | 9 +++++++++ > 2 files changed, 51 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index c71344aebdbb..a4af411de2a4 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -47,6 +47,9 @@ struct kmem_cache *scsi_sdb_cache; > */ > #define SCSI_QUEUE_DELAY 3 > > +/* Maximum number of retries when a scsi command triggers an Unit > Attention. */ > +#define UNIT_ATTENTION_RETRIES 5 We would also tick down the passed in retries so you don't need this artificial setting. > static void > scsi_set_blocked(struct scsi_cmnd *cmd, int reason) > { > @@ -164,7 +167,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int > reason) > __scsi_queue_insert(cmd, reason, 1); > } > /** > - * scsi_execute - insert request and wait for the result > + * __scsi_execute - insert request and wait for the result > * @sdev: scsi device > * @cmd: scsi command > * @data_direction: data direction > @@ -179,10 +182,10 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, > int reason) > * returns the req->errors value which is the scsi_cmnd result > * field. > */ > -int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > - int data_direction, void *buffer, unsigned bufflen, > - unsigned char *sense, int timeout, int retries, u64 > flags, > - int *resid) > +int __scsi_execute(struct scsi_device *sdev, const unsigned char > *cmd, > + int data_direction, void *buffer, unsigned > bufflen, > + unsigned char *sense, int timeout, int retries, > u64 flags, > + int *resid) > { > struct request *req; > int write = (data_direction == DMA_TO_DEVICE); > @@ -227,6 +230,40 @@ int scsi_execute(struct scsi_device *sdev, const > unsigned char *cmd, > > return ret; > } > + > +int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > + int data_direction, void *buffer, unsigned bufflen, > + unsigned char *sense, int timeout, int retries, u64 > flags, > + int *resid) > +{ > + int result; > + int retry = UNIT_ATTENTION_RETRIES; > + bool priv_sense = false; > + > + if (!sense) { > + sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO); > + priv_sense = true; > + if (!sense) > + return DRIVER_ERROR << 24; > + } > + > + while (retry--) { > + result = __scsi_execute(sdev, cmd, data_direction, > buffer, > + bufflen, sense, timeout, > retries, > + flags, resid); OK, so really this isn't what you want, because blk_execute_req may have used several of your retries, so you now get a maximum possible set of retries at UNIT_ATTENTION_RETRIES*retries. You need to start from the returned req->retries, which probably means this loop needs to be inside __scsi_execute. James > + > + if (!scsi_sense_unit_attention(sense)) > + break; > + > + if (retry) > + memset(sense, 0, SCSI_SENSE_BUFFERSIZE); > + } > + > + if (priv_sense) > + kfree(sense); > + > + return result; > +} > EXPORT_SYMBOL(scsi_execute); > > int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned > char *cmd, > diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h > index 20bf7eaef05a..747b632d5b57 100644 > --- a/include/scsi/scsi_common.h > +++ b/include/scsi/scsi_common.h > @@ -58,6 +58,15 @@ static inline bool scsi_sense_valid(const struct > scsi_sense_hdr *sshdr) > return (sshdr->response_code & 0x70) == 0x70; > } > > +static inline bool scsi_sense_unit_attention(const char *sense) > +{ > + int resp = sense[0] & 0x7f; > + > + return ((resp & 0x70) && > + ((resp >= 0x72 && (sense[1] & 0xf) == > UNIT_ATTENTION) || > + (resp < 0x72 && (sense[2] & 0xf) == > UNIT_ATTENTION))); > +} > + > extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len, > struct scsi_sense_hdr *sshdr); > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index c71344aebdbb..a4af411de2a4 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -47,6 +47,9 @@ struct kmem_cache *scsi_sdb_cache; */ #define SCSI_QUEUE_DELAY 3 +/* Maximum number of retries when a scsi command triggers an Unit Attention. */ +#define UNIT_ATTENTION_RETRIES 5 + static void scsi_set_blocked(struct scsi_cmnd *cmd, int reason) { @@ -164,7 +167,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason) __scsi_queue_insert(cmd, reason, 1); } /** - * scsi_execute - insert request and wait for the result + * __scsi_execute - insert request and wait for the result * @sdev: scsi device * @cmd: scsi command * @data_direction: data direction @@ -179,10 +182,10 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason) * returns the req->errors value which is the scsi_cmnd result * field. */ -int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, - int data_direction, void *buffer, unsigned bufflen, - unsigned char *sense, int timeout, int retries, u64 flags, - int *resid) +int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, + int data_direction, void *buffer, unsigned bufflen, + unsigned char *sense, int timeout, int retries, u64 flags, + int *resid) { struct request *req; int write = (data_direction == DMA_TO_DEVICE); @@ -227,6 +230,40 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, return ret; } + +int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, + int data_direction, void *buffer, unsigned bufflen, + unsigned char *sense, int timeout, int retries, u64 flags, + int *resid) +{ + int result; + int retry = UNIT_ATTENTION_RETRIES; + bool priv_sense = false; + + if (!sense) { + sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO); + priv_sense = true; + if (!sense) + return DRIVER_ERROR << 24; + } + + while (retry--) { + result = __scsi_execute(sdev, cmd, data_direction, buffer, + bufflen, sense, timeout, retries, + flags, resid); + + if (!scsi_sense_unit_attention(sense)) + break; + + if (retry) + memset(sense, 0, SCSI_SENSE_BUFFERSIZE); + } + + if (priv_sense) + kfree(sense); + + return result; +} EXPORT_SYMBOL(scsi_execute); int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned char *cmd, diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h index 20bf7eaef05a..747b632d5b57 100644 --- a/include/scsi/scsi_common.h +++ b/include/scsi/scsi_common.h @@ -58,6 +58,15 @@ static inline bool scsi_sense_valid(const struct scsi_sense_hdr *sshdr) return (sshdr->response_code & 0x70) == 0x70; } +static inline bool scsi_sense_unit_attention(const char *sense) +{ + int resp = sense[0] & 0x7f; + + return ((resp & 0x70) && + ((resp >= 0x72 && (sense[1] & 0xf) == UNIT_ATTENTION) || + (resp < 0x72 && (sense[2] & 0xf) == UNIT_ATTENTION))); +} + extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len, struct scsi_sense_hdr *sshdr);
Usually, re-sending the SCSI command is enough to recover from a Unit Attention (UA). This adds a generic retry code to the SCSI command path in case of an UA, before giving up and returning the error condition to the caller. I added the UA verification into scsi_execute instead of scsi_execute_req because there are at least a few callers that invoke scsi_execute directly and would benefit from the internal UA retry. Also, I didn't use scsi_normalize_sense to not duplicate functionality with scsi_execute_req_flags. Instead, scsi_execute uses a small helper function that verifies only the UA condition directly from the raw sense buffer. If this design is not OK, I can refactor to use scsi_normalize_sense. This prevents us from duplicating the retry code in at least a few places. In particular, it fixes an issue found in some IBM enclosures, in which the device may return an Unit Attention during probe, breaking the bind with the ses module: scsi 1:0:7:0: Failed to get diagnostic page 0x8000002 scsi 1:0:7:0: Failed to bind enclosure -19 Finally, should we have a NORETRY_UA flag to allow callers to disable this mechanism? Link: https://patchwork.kernel.org/patch/9336763/ Suggested-by: Brian King <brking@linux.vnet.ibm.com> Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com> Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com> --- drivers/scsi/scsi_lib.c | 47 +++++++++++++++++++++++++++++++++++++++++----- include/scsi/scsi_common.h | 9 +++++++++ 2 files changed, 51 insertions(+), 5 deletions(-)