diff mbox series

[1/2] scsi: add BLIST_RETRY_SCAN to ignore errors during scanning

Message ID 20220615164149.3092-2-mwilck@suse.com
State New
Headers show
Series Fixes for device probing on flaky connections | expand

Commit Message

Martin Wilck June 15, 2022, 4:41 p.m. UTC 
From: Hannes Reinecke <hare@suse.de>

On flaky connections the INQUIRY command might run into a timeout,
but as we already have performed a REPORT LUNS command we know that
a LUN should be present. So ignore errors for the INQUIRY commands,
and retry until we exhaust the number of retries.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/scsi_scan.c    | 12 +++++++++---
 include/scsi/scsi_devinfo.h |  4 +++-
 2 files changed, 12 insertions(+), 4 deletions(-)

Comments

Martin K. Petersen June 22, 2022, 2:02 a.m. UTC | #1 
Martin,

> @@ -1531,9 +1536,10 @@ static int scsi_report_lun_scan(struct scsi_target *starget, blist_flags_t bflag
>  				    " allowed by the host adapter\n", lun);
>  		} else {
>  			int res;
> +			blist_flags_t bflags = BLIST_RETRY_SCAN;

I'm not a big fan of using the bflag as carrier of "I was reported and
therefore must exist".

Also: Why isn't patch #2 sufficient?
Martin Wilck June 22, 2022, 8:16 a.m. UTC | #2 
On Tue, 2022-06-21 at 22:02 -0400, Martin K. Petersen wrote:
> 
> Martin,
> 
> > @@ -1531,9 +1536,10 @@ static int scsi_report_lun_scan(struct
> > scsi_target *starget, blist_flags_t bflag
> >                                     " allowed by the host
> > adapter\n", lun);
> >                 } else {
> >                         int res;
> > +                       blist_flags_t bflags = BLIST_RETRY_SCAN;
> 
> I'm not a big fan of using the bflag as carrier of "I was reported
> and
> therefore must exist".
> 
> Also: Why isn't patch #2 sufficient?

I think it is. I can resubmit just #2 if you prefer and Hannes agrees.

Regards,
Martin
Hannes Reinecke June 22, 2022, 10:49 a.m. UTC | #3 
On 6/22/22 10:16, Martin Wilck wrote:
> On Tue, 2022-06-21 at 22:02 -0400, Martin K. Petersen wrote:
>>
>> Martin,
>>
>>> @@ -1531,9 +1536,10 @@ static int scsi_report_lun_scan(struct
>>> scsi_target *starget, blist_flags_t bflag
>>>                                      " allowed by the host
>>> adapter\n", lun);
>>>                  } else {
>>>                          int res;
>>> +                       blist_flags_t bflags = BLIST_RETRY_SCAN;
>>
>> I'm not a big fan of using the bflag as carrier of "I was reported
>> and
>> therefore must exist".
>>
>> Also: Why isn't patch #2 sufficient?
> 
> I think it is. I can resubmit just #2 if you prefer and Hannes agrees.
> 
I'm fine with just adding #2; #1 is really just there to provide the 
original behaviour. Device probing is one of the most arcane areas
in the SCSI stack due to all the various quirks etc and I didn't want
to change anything here.

But if it's okay, it's okay :-)

Cheers,

Hannes
Martin Wilck June 22, 2022, 12:05 p.m. UTC | #4 
On Wed, 2022-06-22 at 12:49 +0200, Hannes Reinecke wrote:
> On 6/22/22 10:16, Martin Wilck wrote:
> > On Tue, 2022-06-21 at 22:02 -0400, Martin K. Petersen wrote:
> > > 
> > > Martin,
> > > 
> > > > @@ -1531,9 +1536,10 @@ static int scsi_report_lun_scan(struct
> > > > scsi_target *starget, blist_flags_t bflag
> > > >                                      " allowed by the host
> > > > adapter\n", lun);
> > > >                  } else {
> > > >                          int res;
> > > > +                       blist_flags_t bflags =
> > > > BLIST_RETRY_SCAN;
> > > 
> > > I'm not a big fan of using the bflag as carrier of "I was
> > > reported
> > > and
> > > therefore must exist".
> > > 
> > > Also: Why isn't patch #2 sufficient?
> > 
> > I think it is. I can resubmit just #2 if you prefer and Hannes
> > agrees.
> > 
> I'm fine with just adding #2; #1 is really just there to provide the 
> original behaviour. Device probing is one of the most arcane areas
> in the SCSI stack due to all the various quirks etc and I didn't want
> to change anything here.
> 
> But if it's okay, it's okay :-)

Alright. To be certain, I'll ask our partner for another test.

Martin
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 91ac901a6682..b9b851ce1b72 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -649,8 +649,6 @@  static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	int pass, count, result;
 	struct scsi_sense_hdr sshdr;
 
-	*bflags = 0;
-
 	/* Perform up to 3 passes.  The first pass uses a conservative
 	 * transfer length of 36 unless sdev->inquiry_len specifies a
 	 * different value. */
@@ -697,6 +695,11 @@  static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 				    (sshdr.ascq == 0))
 					continue;
 			}
+			if (*bflags & BLIST_RETRY_SCAN) {
+				SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+						"scsi scan: retry inquiry after REPORT LUNs\n"));
+				continue;
+			}
 		} else if (result == 0) {
 			/*
 			 * if nothing was transferred, we try
@@ -709,6 +712,8 @@  static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 		break;
 	}
 
+	*bflags = 0;
+
 	if (result == 0) {
 		scsi_sanitize_inquiry_string(&inq_result[8], 8);
 		scsi_sanitize_inquiry_string(&inq_result[16], 16);
@@ -1531,9 +1536,10 @@  static int scsi_report_lun_scan(struct scsi_target *starget, blist_flags_t bflag
 				    " allowed by the host adapter\n", lun);
 		} else {
 			int res;
+			blist_flags_t bflags = BLIST_RETRY_SCAN;
 
 			res = scsi_probe_and_add_lun(starget,
-				lun, NULL, NULL, rescan, NULL);
+				lun, &bflags, NULL, rescan, NULL);
 			if (res == SCSI_SCAN_NO_RESPONSE) {
 				/*
 				 * Got some results, but now none, abort.
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 5d14adae21c7..e74a228d73d1 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -68,8 +68,10 @@ 
 #define BLIST_RETRY_ITF		((__force blist_flags_t)(1ULL << 32))
 /* Always retry ABORTED_COMMAND with ASC 0xc1 */
 #define BLIST_RETRY_ASC_C1	((__force blist_flags_t)(1ULL << 33))
+/* Retry errors during scanning */
+#define BLIST_RETRY_SCAN	((__force blist_flags_t)(1ULL << 34))
 
-#define __BLIST_LAST_USED BLIST_RETRY_ASC_C1
+#define __BLIST_LAST_USED BLIST_RETRY_SCAN
 
 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \