[PATCH 0/2] scanning fixes

[PATCH 0/2] scanning fixes

[Hannes Reinecke]

Hi all,

here are two fixes for the scanning logic which resolve two long-standing
issues:

1) We need to send a 'TEST UNIT READY' to the LUN before scanning.
The INQUIRY command does _not_ clear any unit attentions,
so if there are any outstanding unit attention conditions they'll
be attached to the first command after INQUIRY.
Which typically wouldn't be too bad, as most of the commands
are now equipped with at least rudimentary error checking.
However, the problem arises when we're sending a REPORT LUN command
to that LUN. As per spec the REPORT LUN command will _always_
return something, but the list might not be fully populated if
the firmware is still starting up (see SPC for details here).
This will cause us to miss some devices during startup.
Added to that we're not handling the unit attention
'REPORT LUN DATA HAS CHANGED', so the kernel will never be
able to rescan all disks.
To fix this we should be issuing a TEST UNIT READY after
INQUIRY, and wait until any pending unit attention goes away.

2) Power-on/Reset handling
While we're not sending out uevents for various unit attention
conditions, we fail to observe the status precedence as per SAM.
This might cause any 29/XX sense code to effectively overwrite
any preceding unit attentions, causing us to miss those events.
Hence as a minimal fix we need to report the power-on reset
event via uevents, too.

Hannes Reinecke (2):
  scsi_scan: Send TEST UNIT READY to the LUN before scanning
  scsi: Handle power-on reset unit attention

 drivers/scsi/scsi_error.c  |  6 ++++
 drivers/scsi/scsi_lib.c    |  4 +++
 drivers/scsi/scsi_scan.c   | 86 +++++++++++++++++++++++++++++++++++++++-------
 include/scsi/scsi_device.h |  3 +-
 4 files changed, 85 insertions(+), 14 deletions(-)

-- 
1.7.12.4

[PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[Hannes Reinecke]

REPORT_LUN_SCAN does not report any outstanding unit attention
condition as per SAM. However, the target might not be fully
initialized at that time, so we might end up getting a
default entry (or even a partially filled one).
But as we're not able to process the REPORT LUN DATA HAS CHANGED
unit attention correctly we'll be missing out some LUNs during
startup.
So it's better to send a TEST UNIT READY for modern implementations
and wait until the unit attention condition goes away.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_scan.c | 86 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 73 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e02b3aa..a8e59c3 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -123,6 +123,13 @@ MODULE_PARM_DESC(inq_timeout,
 		 "Timeout (in seconds) waiting for devices to answer INQUIRY."
 		 " Default is 20. Some devices may need more; most need less.");
 
+static unsigned int scsi_scan_timeout = SCSI_TIMEOUT/HZ + 58;
+
+module_param_named(scan_timeout, scsi_scan_timeout, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(scan_timeout,
+		 "Timeout (in seconds) waiting for devices to become ready"
+		 " after INQUIRY. Default is 60.");
+
 /* This lock protects only this list */
 static DEFINE_SPINLOCK(async_scan_lock);
 static LIST_HEAD(scanning_hosts);
@@ -712,19 +719,6 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	}
 
 	/*
-	 * Related to the above issue:
-	 *
-	 * XXX Devices (disk or all?) should be sent a TEST UNIT READY,
-	 * and if not ready, sent a START_STOP to start (maybe spin up) and
-	 * then send the INQUIRY again, since the INQUIRY can change after
-	 * a device is initialized.
-	 *
-	 * Ideally, start a device if explicitly asked to do so.  This
-	 * assumes that a device is spun up on power on, spun down on
-	 * request, and then spun up on request.
-	 */
-
-	/*
 	 * The scanning code needs to know the scsi_level, even if no
 	 * device is attached at LUN 0 (SCSI_SCAN_TARGET_PRESENT) so
 	 * non-zero LUNs can be scanned.
@@ -739,6 +733,65 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 }
 
 /**
+ * scsi_test_lun - waiting for a LUN to become ready
+ * @sdev:	scsi_device to test
+ *
+ * Description:
+ *     Wait for the lun associated with @sdev to become ready
+ *
+ *     Send a TEST UNIT READY to detect any unit attention conditions.
+ *     Retry TEST UNIT READY for up to @scsi_scan_timeout if the
+ *     returned sense key is 02/04/01 (Not ready, Logical Unit is
+ *     in process of becoming ready)
+ **/
+static int
+scsi_test_lun(struct scsi_device *sdev)
+{
+	struct scsi_sense_hdr sshdr;
+	int res = SCSI_SCAN_TARGET_PRESENT;
+	int tur_result;
+	unsigned long tur_timeout = jiffies + scsi_scan_timeout * HZ;
+
+	/* Skip for older devices */
+	if (sdev->scsi_level <= SCSI_3)
+		return SCSI_SCAN_LUN_PRESENT;
+
+	/*
+	 * Wait for the device to become ready.
+	 *
+	 * Some targets take some time before the firmware is
+	 * fully initialized, during which time they might not
+	 * be able to fill out any REPORT_LUN command correctly.
+	 * And as we're not capable of handling the
+	 * INQUIRY DATA CHANGED unit attention correctly we'd
+	 * rather wait here.
+	 */
+	do {
+		tur_result = scsi_test_unit_ready(sdev, SCSI_TIMEOUT,
+							  3, &sshdr);
+		if (!tur_result) {
+			res = SCSI_SCAN_LUN_PRESENT;
+			break;
+		}
+		if ((driver_byte(tur_result) & DRIVER_SENSE) &&
+		    scsi_sense_valid(&sshdr)) {
+			SCSI_LOG_SCAN_BUS(3, sdev_printk(KERN_INFO, sdev,
+				"scsi_scan: tur returned %02x/%02x/%02x\n",
+				sshdr.sense_key, sshdr.asc, sshdr.ascq));
+			if (sshdr.sense_key == NOT_READY &&
+			    sshdr.asc == 0x04 && sshdr.ascq == 0x01) {
+				/* Logical Unit is in process
+				 * of becoming ready */
+				msleep(100);
+				continue;
+			}
+		}
+		res = SCSI_SCAN_LUN_PRESENT;
+	} while (time_before_eq(jiffies, tur_timeout));
+	return res;
+}
+
+/**
  * scsi_add_lun - allocate and fully initialze a scsi_device
  * @sdev:	holds information to be stored in the new scsi_device
  * @inq_result:	holds the result of a previous INQUIRY to the LUN
@@ -1142,6 +1195,13 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
 		goto out_free_result;
 	}
 
+	res = scsi_test_lun(sdev);
+	if (res == SCSI_SCAN_TARGET_PRESENT) {
+		SCSI_LOG_SCAN_BUS(1, sdev_printk(KERN_INFO, sdev,
+			"scsi scan: device not ready\n"));
+		goto out_free_result;
+	}
+
 	res = scsi_add_lun(sdev, result, &bflags, shost->async_scan);
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (bflags & BLIST_KEY) {
-- 
1.7.12.4

[PATCH 2/2] scsi: Handle power-on reset unit attention

[Hannes Reinecke]

As per SAM there is a status precedence, with any sense code 29/XX
taking second place just after an ACA ACTIVE status.
Additionally, each target might prefer to not queue any unit
attention conditions but just report one.
Due to the above this will be that one with the highest precedence.
This results in the sense code 29/XX effectively overwriting any
other unit attention.
Hence we should report the power-on reset to userland so that
it can take appropriate action.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_error.c  | 6 ++++++
 drivers/scsi/scsi_lib.c    | 4 ++++
 include/scsi/scsi_device.h | 3 ++-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 47a1ffc..65ed333 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -420,6 +420,12 @@ static void scsi_report_sense(struct scsi_device *sdev,
 				    "threshold.\n");
 		}
 
+		if (sshdr->asc == 0x29) {
+			evt_type = SDEV_EVT_POWER_ON_RESET_OCCURRED;
+			sdev_printk(KERN_WARNING, sdev,
+				    "Power-on or device reset occurred\n");
+		}
+
 		if (sshdr->asc == 0x2a && sshdr->ascq == 0x01) {
 			evt_type = SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED;
 			sdev_printk(KERN_WARNING, sdev,
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9f841df..ee158c1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2183,6 +2183,9 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 	case SDEV_EVT_LUN_CHANGE_REPORTED:
 		envp[idx++] = "SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED";
 		break;
+	case SDEV_EVT_POWER_ON_RESET_OCCURRED:
+		envp[idx++] = "SDEV_UA=POWER_ON_RESET_OCCURRED";
+		break;
 	default:
 		/* do nothing */
 		break;
@@ -2286,6 +2289,7 @@ struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
 	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
 	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
 	case SDEV_EVT_LUN_CHANGE_REPORTED:
+	case SDEV_EVT_POWER_ON_RESET_OCCURRED:
 	default:
 		/* do nothing */
 		break;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5853c91..7b9a886 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -57,9 +57,10 @@ enum scsi_device_event {
 	SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED,	/* 38 07  UA reported */
 	SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED,	/* 2A 01  UA reported */
 	SDEV_EVT_LUN_CHANGE_REPORTED,			/* 3F 0E  UA reported */
+	SDEV_EVT_POWER_ON_RESET_OCCURRED,		/* 29 00  UA reported */
 
 	SDEV_EVT_FIRST		= SDEV_EVT_MEDIA_CHANGE,
-	SDEV_EVT_LAST		= SDEV_EVT_LUN_CHANGE_REPORTED,
+	SDEV_EVT_LAST		= SDEV_EVT_POWER_ON_RESET_OCCURRED,
 
 	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
 };
-- 
1.7.12.4

Re: [PATCH 2/2] scsi: Handle power-on reset unit attention

[Christoph Hellwig]

On Thu, Jun 05, 2014 at 09:26:43AM +0200, Hannes Reinecke wrote:
> As per SAM there is a status precedence, with any sense code 29/XX
> taking second place just after an ACA ACTIVE status.
> Additionally, each target might prefer to not queue any unit
> attention conditions but just report one.
> Due to the above this will be that one with the highest precedence.
> This results in the sense code 29/XX effectively overwriting any
> other unit attention.
> Hence we should report the power-on reset to userland so that
> it can take appropriate action.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[Christoph Hellwig]

On Thu, Jun 05, 2014 at 09:26:42AM +0200, Hannes Reinecke wrote:
> REPORT_LUN_SCAN does not report any outstanding unit attention
> condition as per SAM. However, the target might not be fully
> initialized at that time, so we might end up getting a
> default entry (or even a partially filled one).
> But as we're not able to process the REPORT LUN DATA HAS CHANGED
> unit attention correctly we'll be missing out some LUNs during
> startup.
> So it's better to send a TEST UNIT READY for modern implementations
> and wait until the unit attention condition goes away.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_scan.c | 86 ++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 73 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index e02b3aa..a8e59c3 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -123,6 +123,13 @@ MODULE_PARM_DESC(inq_timeout,
>  		 "Timeout (in seconds) waiting for devices to answer INQUIRY."
>  		 " Default is 20. Some devices may need more; most need less.");
>  
> +static unsigned int scsi_scan_timeout = SCSI_TIMEOUT/HZ + 58;
> +
> +module_param_named(scan_timeout, scsi_scan_timeout, uint, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(scan_timeout,
> +		 "Timeout (in seconds) waiting for devices to become ready"
> +		 " after INQUIRY. Default is 60.");

Should this be called tur_timeout, similar to the inq_timeout parameter?

Otherwise looks good to me,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] scsi: Handle power-on reset unit attention

[Ewan Milne]

On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote:
> As per SAM there is a status precedence, with any sense code 29/XX
> taking second place just after an ACA ACTIVE status.
> Additionally, each target might prefer to not queue any unit
> attention conditions but just report one.
> Due to the above this will be that one with the highest precedence.
> This results in the sense code 29/XX effectively overwriting any
> other unit attention.
> Hence we should report the power-on reset to userland so that
> it can take appropriate action.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_error.c  | 6 ++++++
>  drivers/scsi/scsi_lib.c    | 4 ++++
>  include/scsi/scsi_device.h | 3 ++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 47a1ffc..65ed333 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -420,6 +420,12 @@ static void scsi_report_sense(struct scsi_device *sdev,
>  				    "threshold.\n");
>  		}
>  
> +		if (sshdr->asc == 0x29) {
> +			evt_type = SDEV_EVT_POWER_ON_RESET_OCCURRED;
> +			sdev_printk(KERN_WARNING, sdev,
> +				    "Power-on or device reset occurred\n");
> +		}
> +
>  		if (sshdr->asc == 0x2a && sshdr->ascq == 0x01) {
>  			evt_type = SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED;
>  			sdev_printk(KERN_WARNING, sdev,
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 9f841df..ee158c1 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2183,6 +2183,9 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
>  	case SDEV_EVT_LUN_CHANGE_REPORTED:
>  		envp[idx++] = "SDEV_UA=REPORTED_LUNS_DATA_HAS_CHANGED";
>  		break;
> +	case SDEV_EVT_POWER_ON_RESET_OCCURRED:
> +		envp[idx++] = "SDEV_UA=POWER_ON_RESET_OCCURRED";
> +		break;
>  	default:
>  		/* do nothing */
>  		break;
> @@ -2286,6 +2289,7 @@ struct scsi_event *sdev_evt_alloc(enum scsi_device_event evt_type,
>  	case SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED:
>  	case SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED:
>  	case SDEV_EVT_LUN_CHANGE_REPORTED:
> +	case SDEV_EVT_POWER_ON_RESET_OCCURRED:
>  	default:
>  		/* do nothing */
>  		break;
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 5853c91..7b9a886 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -57,9 +57,10 @@ enum scsi_device_event {
>  	SDEV_EVT_SOFT_THRESHOLD_REACHED_REPORTED,	/* 38 07  UA reported */
>  	SDEV_EVT_MODE_PARAMETER_CHANGE_REPORTED,	/* 2A 01  UA reported */
>  	SDEV_EVT_LUN_CHANGE_REPORTED,			/* 3F 0E  UA reported */
> +	SDEV_EVT_POWER_ON_RESET_OCCURRED,		/* 29 00  UA reported */
>  
>  	SDEV_EVT_FIRST		= SDEV_EVT_MEDIA_CHANGE,
> -	SDEV_EVT_LAST		= SDEV_EVT_LUN_CHANGE_REPORTED,
> +	SDEV_EVT_LAST		= SDEV_EVT_POWER_ON_RESET_OCCURRED,
>  
>  	SDEV_EVT_MAXBITS	= SDEV_EVT_LAST + 1
>  };

This looks fine to me.  We might want to figure out a way
to report the ASCQ in an environment variable on this uevent,
since there are multiple sub-cases:

29 00 D POWER ON, RESET, OR BUS DEVICE RESET OCCURRED
29 01 D POWER ON OCCURRED
29 02 D SCSI BUS RESET OCCURRED
29 03 D BUS DEVICE RESET FUNCTION OCCURRED
29 04 D DEVICE INTERNAL RESET
29 05 D TRANSCEIVER MODE CHANGED TO SINGLE-ENDED
29 06 D TRANSCEIVER MODE CHANGED TO LVD
29 07 D I_T NEXUS LOSS OCCURRED

...but we could add that in a subsequent patch.

A related problem is that when this UA is received, the
device may have changed some of its attributes, so some
of the information that the mid-layer caches may be stale.
The udev rule handling this event should probably rescan
the device.

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[James Bottomley]

On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote:
> REPORT_LUN_SCAN does not report any outstanding unit attention
> condition as per SAM. However, the target might not be fully
> initialized at that time, so we might end up getting a
> default entry (or even a partially filled one).
> But as we're not able to process the REPORT LUN DATA HAS CHANGED
> unit attention correctly we'll be missing out some LUNs during
> startup.
> So it's better to send a TEST UNIT READY for modern implementations
> and wait until the unit attention condition goes away.

Are you sure this is a good idea: we just spent ages tuning SCSI init so
we don't slow systems down.  This patch, in the event the array is
having a power on problem, takes us right back to waiting for init
again ... basically the busy wait in scsi_test_lun.

Since the array should send us a UA anyway when it's got itself sorted
out, what's wrong with just processing the report luns data has changed
condition?

James

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[Hannes Reinecke]

On 06/11/2014 04:24 PM, James Bottomley wrote:
> On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote:
>> REPORT_LUN_SCAN does not report any outstanding unit attention
>> condition as per SAM. However, the target might not be fully
>> initialized at that time, so we might end up getting a
>> default entry (or even a partially filled one).
>> But as we're not able to process the REPORT LUN DATA HAS CHANGED
>> unit attention correctly we'll be missing out some LUNs during
>> startup.
>> So it's better to send a TEST UNIT READY for modern implementations
>> and wait until the unit attention condition goes away.
>
> Are you sure this is a good idea: we just spent ages tuning SCSI init so
> we don't slow systems down.  This patch, in the event the array is
> having a power on problem, takes us right back to waiting for init
> again ... basically the busy wait in scsi_test_lun.
>
> Since the array should send us a UA anyway when it's got itself sorted
> out, what's wrong with just processing the report luns data has changed
> condition?
>
Because we can't.

_If_ we were attempting this we'd run into several issues:
a) Boot will fail, as REPORT LUNs will return 0 LUNs (or just LUN 0).
    So the scanning code will assume everything's fine. Booting will
    continue, only to figure out that no LUNs are present.
    As there is _no_ indication that REPORT LUNs should indeed have
    returned an error (only it can't due to SAM) we wouldn't even
    now that there _is_ an issue.
    (In fact, that's what triggered the patchset in the first place.)
b) Even _if_ we're able so somehow recover from that we will have
    to rescan the host and any attached devices.
    The only way to do this currently is to _remove_ all devices
    from that host and then do a full rescan.
    Trying this with any devices which are already part of some
    complex setup will become ... interesting.

So the easy way out here is indeed just to send a TEST UNIT READY.
And as we're checking for a reasonably SCSI compliance we should
be catching most of the oddballs.

Cheers,

Hannes

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[James Bottomley]

On Wed, 2014-06-11 at 16:33 +0200, Hannes Reinecke wrote:
> On 06/11/2014 04:24 PM, James Bottomley wrote:
> > On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote:
> >> REPORT_LUN_SCAN does not report any outstanding unit attention
> >> condition as per SAM. However, the target might not be fully
> >> initialized at that time, so we might end up getting a
> >> default entry (or even a partially filled one).
> >> But as we're not able to process the REPORT LUN DATA HAS CHANGED
> >> unit attention correctly we'll be missing out some LUNs during
> >> startup.
> >> So it's better to send a TEST UNIT READY for modern implementations
> >> and wait until the unit attention condition goes away.
> >
> > Are you sure this is a good idea: we just spent ages tuning SCSI init so
> > we don't slow systems down.  This patch, in the event the array is
> > having a power on problem, takes us right back to waiting for init
> > again ... basically the busy wait in scsi_test_lun.
> >
> > Since the array should send us a UA anyway when it's got itself sorted
> > out, what's wrong with just processing the report luns data has changed
> > condition?
> >
> Because we can't.
> 
> _If_ we were attempting this we'd run into several issues:
> a) Boot will fail, as REPORT LUNs will return 0 LUNs (or just LUN 0).
>     So the scanning code will assume everything's fine. Booting will
>     continue, only to figure out that no LUNs are present.
>     As there is _no_ indication that REPORT LUNs should indeed have
>     returned an error (only it can't due to SAM) we wouldn't even
>     now that there _is_ an issue.
>     (In fact, that's what triggered the patchset in the first place.)
> b) Even _if_ we're able so somehow recover from that we will have
>     to rescan the host and any attached devices.
>     The only way to do this currently is to _remove_ all devices
>     from that host and then do a full rescan.
>     Trying this with any devices which are already part of some
>     complex setup will become ... interesting.

OK, go back to first principles and tell us what the actual problem is,
with traces and details.  Is this some weird SCSI-3 device with a single
LUN that's screwing up report luns ... in which case we can just
blacklist it.  Or is it boot from an array?

> So the easy way out here is indeed just to send a TEST UNIT READY.
> And as we're checking for a reasonably SCSI compliance we should
> be catching most of the oddballs.

I don't object hugely to TUR ... except it binds us to spin up because
most devices will respond not ready.  I do object to busy waiting in the
init thread until we get the right answer.

James

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[Jeremy Linton]

On 6/11/2014 9:33 AM, Hannes Reinecke wrote:
> _If_ we were attempting this we'd run into several issues: a) Boot will
> fail, as REPORT LUNs will return 0 LUNs (or just LUN 0). So the scanning
> code will assume everything's fine. Booting will continue, only to figure
> out that no LUNs are present. As there is _no_ indication that REPORT LUNs
> should indeed have returned an error (only it can't due to SAM) we wouldn't
> even now that there _is_ an issue. (In fact, that's what triggered the
> patchset in the first place.)

	Isn't this what the root mount delay is for? I've had to use that on
assorted embedded devices in the past, and it doesn't seem ideal but it solves
the larger problem (because maybe there isn't even anything to send the TUR to).

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[Hannes Reinecke]

On 06/11/2014 04:46 PM, James Bottomley wrote:
> On Wed, 2014-06-11 at 16:33 +0200, Hannes Reinecke wrote:
>> On 06/11/2014 04:24 PM, James Bottomley wrote:
>>> On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote:
>>>> REPORT_LUN_SCAN does not report any outstanding unit attention
>>>> condition as per SAM. However, the target might not be fully
>>>> initialized at that time, so we might end up getting a
>>>> default entry (or even a partially filled one).
>>>> But as we're not able to process the REPORT LUN DATA HAS CHANGED
>>>> unit attention correctly we'll be missing out some LUNs during
>>>> startup.
>>>> So it's better to send a TEST UNIT READY for modern implementations
>>>> and wait until the unit attention condition goes away.
>>>
>>> Are you sure this is a good idea: we just spent ages tuning SCSI init so
>>> we don't slow systems down.  This patch, in the event the array is
>>> having a power on problem, takes us right back to waiting for init
>>> again ... basically the busy wait in scsi_test_lun.
>>>
>>> Since the array should send us a UA anyway when it's got itself sorted
>>> out, what's wrong with just processing the report luns data has changed
>>> condition?
>>>
>> Because we can't.
>>
>> _If_ we were attempting this we'd run into several issues:
>> a) Boot will fail, as REPORT LUNs will return 0 LUNs (or just LUN 0).
>>      So the scanning code will assume everything's fine. Booting will
>>      continue, only to figure out that no LUNs are present.
>>      As there is _no_ indication that REPORT LUNs should indeed have
>>      returned an error (only it can't due to SAM) we wouldn't even
>>      now that there _is_ an issue.
>>      (In fact, that's what triggered the patchset in the first place.)
>> b) Even _if_ we're able so somehow recover from that we will have
>>      to rescan the host and any attached devices.
>>      The only way to do this currently is to _remove_ all devices
>>      from that host and then do a full rescan.
>>      Trying this with any devices which are already part of some
>>      complex setup will become ... interesting.
>
> OK, go back to first principles and tell us what the actual problem is,
> with traces and details.  Is this some weird SCSI-3 device with a single
> LUN that's screwing up report luns ... in which case we can just
> blacklist it.  Or is it boot from an array?
>
The problem is as follows:

 > Right after the "inquiry" the scsi subsystem sends a "report luns"
 > to the RAID array.
 > The RAID answers the "report luns" with only the 8 byte header
 > and an empty (i.e. not existing) LUN list after this header
 > because the LUNs still execute their initialization phase and
 > did not reach their ready state yet.
 > The RAID manufacturer describes this behaviour as an indication
 > for: "there are no LUNs available".
 >
 > Then immediately follows a "test unit ready" command from the
 > scsi subsystem to LUN 0  which is answered by the RAID firmware
 > with a "check condition"  "not ready, initialisation in progress".
 >
As per SPC 'REPORT LUN' cannot return any check condition.
So we cannot distinguish by evaluating the 'REPORT LUN' response
whether it refers to a valid response or not.

Hence my approach to send a TEST UNIT READY prior to REPORT LUN,
as this would return any outstanding unit attention codes and
we can wait until the initialisation is finished.
Plus we're sending a TEST UNIT READY anyway when we're scanning
the LUN from sd.c:spin_up_disk(), so in effect we're just
moving the call.

>> So the easy way out here is indeed just to send a TEST UNIT READY.
>> And as we're checking for a reasonably SCSI compliance we should
>> be catching most of the oddballs.
>
> I don't object hugely to TUR ... except it binds us to spin up because
> most devices will respond not ready.  I do object to busy waiting in the
> init thread until we get the right answer.
>
The problem is indeed in SPC:

The REPORT LUNS parameter data should be returned even though the 
device server is not ready for other commands. The report of the 
logical unit inventory should be available without incurring any 
media access delays. If the device server is not ready with the 
logical unit inventory or if the inventory list is null for the
requesting I_T nexus and the SELECT REPORT field set to 02h, then 
the device server shall provide a default logical unit inventory 
that contains at least LUN 0 or the REPORT LUNS well known logical 
unit (see 8.2). A non-empty peripheral device logical unit inventory 
that does not contain either LUN 0 or the REPORT LUNS
well known logical unit is valid.

So the above array is perfectly within spec.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[James Bottomley]

On Wed, 2014-06-11 at 17:13 +0200, Hannes Reinecke wrote:
> On 06/11/2014 04:46 PM, James Bottomley wrote:
> > On Wed, 2014-06-11 at 16:33 +0200, Hannes Reinecke wrote:
> >> On 06/11/2014 04:24 PM, James Bottomley wrote:
> >>> On Thu, 2014-06-05 at 09:26 +0200, Hannes Reinecke wrote:
> >>>> REPORT_LUN_SCAN does not report any outstanding unit attention
> >>>> condition as per SAM. However, the target might not be fully
> >>>> initialized at that time, so we might end up getting a
> >>>> default entry (or even a partially filled one).
> >>>> But as we're not able to process the REPORT LUN DATA HAS CHANGED
> >>>> unit attention correctly we'll be missing out some LUNs during
> >>>> startup.
> >>>> So it's better to send a TEST UNIT READY for modern implementations
> >>>> and wait until the unit attention condition goes away.
> >>>
> >>> Are you sure this is a good idea: we just spent ages tuning SCSI init so
> >>> we don't slow systems down.  This patch, in the event the array is
> >>> having a power on problem, takes us right back to waiting for init
> >>> again ... basically the busy wait in scsi_test_lun.
> >>>
> >>> Since the array should send us a UA anyway when it's got itself sorted
> >>> out, what's wrong with just processing the report luns data has changed
> >>> condition?
> >>>
> >> Because we can't.
> >>
> >> _If_ we were attempting this we'd run into several issues:
> >> a) Boot will fail, as REPORT LUNs will return 0 LUNs (or just LUN 0).
> >>      So the scanning code will assume everything's fine. Booting will
> >>      continue, only to figure out that no LUNs are present.
> >>      As there is _no_ indication that REPORT LUNs should indeed have
> >>      returned an error (only it can't due to SAM) we wouldn't even
> >>      now that there _is_ an issue.
> >>      (In fact, that's what triggered the patchset in the first place.)
> >> b) Even _if_ we're able so somehow recover from that we will have
> >>      to rescan the host and any attached devices.
> >>      The only way to do this currently is to _remove_ all devices
> >>      from that host and then do a full rescan.
> >>      Trying this with any devices which are already part of some
> >>      complex setup will become ... interesting.
> >
> > OK, go back to first principles and tell us what the actual problem is,
> > with traces and details.  Is this some weird SCSI-3 device with a single
> > LUN that's screwing up report luns ... in which case we can just
> > blacklist it.  Or is it boot from an array?
> >
> The problem is as follows:
> 
>  > Right after the "inquiry" the scsi subsystem sends a "report luns"
>  > to the RAID array.
>  > The RAID answers the "report luns" with only the 8 byte header
>  > and an empty (i.e. not existing) LUN list after this header
>  > because the LUNs still execute their initialization phase and
>  > did not reach their ready state yet.
>  > The RAID manufacturer describes this behaviour as an indication
>  > for: "there are no LUNs available".
>  >
>  > Then immediately follows a "test unit ready" command from the
>  > scsi subsystem to LUN 0  which is answered by the RAID firmware
>  > with a "check condition"  "not ready, initialisation in progress".
>  >
> As per SPC 'REPORT LUN' cannot return any check condition.
> So we cannot distinguish by evaluating the 'REPORT LUN' response
> whether it refers to a valid response or not.
> 
> Hence my approach to send a TEST UNIT READY prior to REPORT LUN,
> as this would return any outstanding unit attention codes and
> we can wait until the initialisation is finished.
> Plus we're sending a TEST UNIT READY anyway when we're scanning
> the LUN from sd.c:spin_up_disk(), so in effect we're just
> moving the call.
> 
> >> So the easy way out here is indeed just to send a TEST UNIT READY.
> >> And as we're checking for a reasonably SCSI compliance we should
> >> be catching most of the oddballs.
> >
> > I don't object hugely to TUR ... except it binds us to spin up because
> > most devices will respond not ready.  I do object to busy waiting in the
> > init thread until we get the right answer.
> >
> The problem is indeed in SPC:
> 
> The REPORT LUNS parameter data should be returned even though the 
> device server is not ready for other commands. The report of the 
> logical unit inventory should be available without incurring any 
> media access delays. If the device server is not ready with the 
> logical unit inventory or if the inventory list is null for the
> requesting I_T nexus and the SELECT REPORT field set to 02h, then 
> the device server shall provide a default logical unit inventory 
> that contains at least LUN 0 or the REPORT LUNS well known logical 
> unit (see 8.2). A non-empty peripheral device logical unit inventory 
> that does not contain either LUN 0 or the REPORT LUNS
> well known logical unit is valid.
> 
> So the above array is perfectly within spec.

What array is this?  Presumably its a server box with an internal array?

I'm not really bothered about what the spec allows; most things boot
from physical (or virtual) discs.  In the 99.99% case report luns just
works as we expect.  We can't damage boot for the 0.01% case.

If it's just a single array, we can use the INQUIRY tag to add a
blacklist that either waits for it to be ready or does an automatic boot
timeout.  If we do this generally, the specific problem you're going to
cause is SAN connected arrays.  Chances are at some point one of them
will go into this condition and every booting system on the SAN will
then do an unexpected and unnecessary wait.

James

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[Christoph Hellwig]

Looks like you put a version into the SuSE tree with a blacklist entry
just for the Fujitsu array that requires the TEST UNIT READY.  Can you
send that version upstream as well?

Re: [PATCH 1/2] scsi_scan: Send TEST UNIT READY to the LUN before scanning

[Hannes Reinecke]

On 09/07/2014 06:24 PM, Christoph Hellwig wrote:
> Looks like you put a version into the SuSE tree with a blacklist entry
> just for the Fujitsu array that requires the TEST UNIT READY.  Can you
> send that version upstream as well?
>
I've just had reports that this is required, as it interferes with 
NetApp arrays.
So I'll be reposting a combined series with that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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