[PATCH 0/4] scsi: Bind sd to LUs in the ALUA unavailable state

[PATCH 0/4] scsi: Bind sd to LUs in the ALUA unavailable state

[Brian Bunker]

A LU that is in the ALUA unavailable state on a target port reports
peripheral qualifier 001b in its INQUIRY data. scsi_bus_match() only
matches devices with peripheral qualifier 000b, so if a host scans such
a path, no upper-level driver is ever bound to it. When the target port
later becomes available, nothing binds sd and the path stays unusable
until the device is deleted and rescanned. A host that was already
running when the path became unavailable keeps sd bound, so the result
depends on when the host scanned.

This series fixes that at scan time. The SCSI core reads the ALUA access
state of a device with REPORT TARGET PORT GROUPS before the device is
registered, and matches sd to PQ 001b devices whose access state is
unavailable. This is the same situation as a path in the standby state,
to which sd already binds. The path then stays in the multipath map as a
failed path, and the existing UNIT ATTENTION handling brings it back
when the target port becomes available.

PQ 001b cannot be matched on its own: an unmapped LUN reports it as
well, and some targets report every possible LUN in REPORT LUNS. Binding
sd to all of those would create disks with no backing LU. An unmapped
LUN fails REPORT TARGET PORT GROUPS, so the access state is what tells
the two cases apart.

  1/4 moves submit_rtpg() out of scsi_dh_alua into the SCSI core as
      scsi_report_tpgs(), so the core can read the ALUA state of a
      device itself. No functional change.
  2/4 sends REPORT TARGET PORT GROUPS from scsi_add_lun() for a device
      that reports TPGS support and sets sdev->access_state from the
      descriptor of the target port group of this path.
  3/4 starts a new port group in scsi_dh_alua in the access state the
      core read, rather than active/optimized, so that no I/O is let
      through to an unavailable path before the handler's first RTPG
      completes.
  4/4 matches PQ 001b devices whose access state is unavailable.

This is an alternative to the series that refreshes the INQUIRY data
and reprobes the device on rescan ("scsi: Refresh INQUIRY data and
reprobe on rescan", v8 last posted). That series needs a way to update
the INQUIRY data of a live device and to reprobe it. This one does not
change the INQUIRY data after the device is discovered, and has no
reprobe, so none of the locking and ordering issues that came with
refreshing it.

The INQUIRY data in sysfs is unchanged by this series. It is read and
created once, when the device is discovered, and keeps the peripheral
qualifier reported at that time like the rest of the INQUIRY data. At
any given time that cached peripheral qualifier can be out of sync with
the device, for example after the target port moves between available
and unavailable. An INQUIRY sent to the device returns the current
value, and the access_state attribute shows the current ALUA state.

Brian Bunker (4):
  scsi: core: Move REPORT TARGET PORT GROUPS into the SCSI core
  scsi: core: Read the ALUA access state when a device is scanned
  scsi: scsi_dh_alua: Start a port group in the scanned access state
  scsi: core: Match ULDs to logical units in the ALUA unavailable state

 drivers/scsi/device_handler/scsi_dh_alua.c | 72 ++++++-----------
 drivers/scsi/scsi_lib.c                    | 52 ++++++++++++
 drivers/scsi/scsi_scan.c                   | 94 ++++++++++++++++++++++
 drivers/scsi/scsi_sysfs.c                  | 14 +++-
 include/scsi/scsi_device.h                 |  3 +
 include/scsi/scsi_proto.h                  |  3 +
 6 files changed, 188 insertions(+), 50 deletions(-)

-- 
2.56.0

[PATCH 1/4] scsi: core: Move REPORT TARGET PORT GROUPS into the SCSI core

[Brian Bunker]

ALUA is a standard SCSI feature, independent of multipathing, but only
the ALUA device handler can currently send REPORT TARGET PORT GROUPS.
Move submit_rtpg() into the SCSI core as scsi_report_tpgs(), together
with the resubmission with a larger buffer that alua_rtpg() did when
the parameter data did not fit, so that the core can read the ALUA
state of a device itself. The parameter data format definitions move to
scsi_proto.h with it.

scsi_dh_alua calls scsi_report_tpgs() with the same timeout, retries
and parameter data format as before.

No functional change.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 63 ++++++----------------
 drivers/scsi/scsi_lib.c                    | 52 ++++++++++++++++++
 include/scsi/scsi_device.h                 |  3 ++
 include/scsi/scsi_proto.h                  |  3 ++
 4 files changed, 73 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 87ac0a6d63e5..06eb35cc9d7c 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -29,9 +29,6 @@
 #define TPGS_SUPPORT_TRANSITION		0x80
 #define TPGS_SUPPORT_ALL		0xdf
 
-#define RTPG_FMT_MASK			0x70
-#define RTPG_FMT_EXT_HDR		0x10
-
 #define TPGS_MODE_UNINITIALIZED		 -1
 #define TPGS_MODE_NONE			0x0
 #define TPGS_MODE_IMPLICIT		0x1
@@ -120,34 +117,6 @@ static void release_port_group(struct kref *kref)
 	kfree_rcu(pg, rcu);
 }
 
-/*
- * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
- * @sdev: sdev the command should be sent to
- */
-static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
-		       int bufflen, struct scsi_sense_hdr *sshdr, int flags)
-{
-	u8 cdb[MAX_COMMAND_SIZE];
-	blk_opf_t opf = REQ_OP_DRV_IN | REQ_FAILFAST_DEV |
-				REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER;
-	const struct scsi_exec_args exec_args = {
-		.sshdr = sshdr,
-	};
-
-	/* Prepare the command. */
-	memset(cdb, 0x0, MAX_COMMAND_SIZE);
-	cdb[0] = MAINTENANCE_IN;
-	if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP))
-		cdb[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
-	else
-		cdb[1] = MI_REPORT_TARGET_PGS;
-	put_unaligned_be32(bufflen, &cdb[6]);
-
-	return scsi_execute_cmd(sdev, cdb, opf, buff, bufflen,
-				ALUA_FAILOVER_TIMEOUT * HZ,
-				ALUA_FAILOVER_RETRIES, &exec_args);
-}
-
 /*
  * submit_stpg - Issue a SET TARGET PORT GROUP command
  *
@@ -518,6 +487,9 @@ static int alua_tur(struct scsi_device *sdev)
 static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	struct scsi_sense_hdr sense_hdr;
+	const struct scsi_exec_args exec_args = {
+		.sshdr = &sense_hdr,
+	};
 	struct alua_port_group *tmp_pg;
 	int len, k, off, bufflen = ALUA_RTPG_SIZE;
 	int group_id_old, state_old, pref_old, valid_states_old;
@@ -549,7 +521,17 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 
  retry:
 	err = 0;
-	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
+	retval = scsi_report_tpgs(sdev, &buff, &bufflen,
+				  !(pg->flags & ALUA_RTPG_EXT_HDR_UNSUPP),
+				  ALUA_FAILOVER_TIMEOUT * HZ,
+				  ALUA_FAILOVER_RETRIES, &exec_args);
+	if (!buff) {
+		sdev_printk(KERN_WARNING, sdev,
+			    "%s: kmalloc buffer failed\n", __func__);
+		/* Temporary failure, bypass */
+		pg->expiry = 0;
+		return SCSI_DH_DEV_TEMP_BUSY;
+	}
 
 	if (retval) {
 		/*
@@ -581,7 +563,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		}
 
 		/*
-		 * submit_rtpg() has failed on existing arrays
+		 * scsi_report_tpgs() has failed on existing arrays
 		 * when requesting extended header info, and
 		 * the array doesn't support extended headers,
 		 * even though it shouldn't according to T10.
@@ -629,21 +611,6 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 
 	len = get_unaligned_be32(&buff[0]) + 4;
 
-	if (len > bufflen) {
-		/* Resubmit with the correct length */
-		kfree(buff);
-		bufflen = len;
-		buff = kmalloc(bufflen, GFP_KERNEL);
-		if (!buff) {
-			sdev_printk(KERN_WARNING, sdev,
-				    "%s: kmalloc buffer failed\n",__func__);
-			/* Temporary failure, bypass */
-			pg->expiry = 0;
-			return SCSI_DH_DEV_TEMP_BUSY;
-		}
-		goto retry;
-	}
-
 	orig_transition_tmo = pg->transition_tmo;
 	if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR && buff[5] != 0)
 		pg->transition_tmo = buff[5];
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f695a6605815..ebbfe3238d97 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3581,6 +3581,58 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id)
 }
 EXPORT_SYMBOL(scsi_vpd_tpg_id);
 
+/**
+ * scsi_report_tpgs - Issue a REPORT TARGET PORT GROUPS command
+ * @sdev: SCSI device to send the command to
+ * @buff: buffer for the parameter data, replaced if it is too small
+ * @bufflen: length of @buff
+ * @ext_hdr: request the extended header parameter data format
+ * @timeout: command timeout in jiffies
+ * @retries: number of times to retry the command in the midlayer
+ * @args: optional arguments for scsi_execute_cmd()
+ *
+ * Resend the command with a larger buffer if the parameter data does not
+ * fit.
+ *
+ * Return: the result of the last command, or -ENOMEM with *@buff set to
+ * NULL if a larger buffer could not be allocated.
+ */
+int scsi_report_tpgs(struct scsi_device *sdev, unsigned char **buff,
+		     int *bufflen, bool ext_hdr, int timeout, int retries,
+		     const struct scsi_exec_args *args)
+{
+	blk_opf_t opf = REQ_OP_DRV_IN | REQ_FAILFAST_DEV |
+			REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER;
+	u8 cdb[MAX_COMMAND_SIZE] = { };
+	int ret, len;
+
+	cdb[0] = MAINTENANCE_IN;
+	cdb[1] = MI_REPORT_TARGET_PGS;
+	if (ext_hdr)
+		cdb[1] |= MI_EXT_HDR_PARAM_FMT;
+
+ retry:
+	put_unaligned_be32(*bufflen, &cdb[6]);
+	ret = scsi_execute_cmd(sdev, cdb, opf, *buff, *bufflen, timeout,
+			       retries, args);
+	if (ret)
+		return ret;
+
+	len = get_unaligned_be32(&(*buff)[0]) + 4;
+	if (len > *bufflen) {
+		/* Resubmit with the correct length */
+		kfree(*buff);
+		*bufflen = len;
+		*buff = kmalloc(len, GFP_KERNEL);
+		if (!*buff)
+			return -ENOMEM;
+		goto retry;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scsi_report_tpgs);
+
 /**
  * scsi_set_sense - build sense data for a command
  * @scmd:	scsi command for which the sense should be formatted
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index e6fd6bc30644..691275db0677 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -583,6 +583,9 @@ extern void sdev_enable_disk_events(struct scsi_device *sdev);
 extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
 extern int scsi_vpd_lun_serial(struct scsi_device *, char *, size_t);
 extern int scsi_vpd_tpg_id(struct scsi_device *, int *);
+int scsi_report_tpgs(struct scsi_device *sdev, unsigned char **buff,
+		     int *bufflen, bool ext_hdr, int timeout, int retries,
+		     const struct scsi_exec_args *args);
 
 #ifdef CONFIG_PM
 extern int scsi_autopm_get_device(struct scsi_device *);
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index 9c7d31aa6bae..6826f2d74407 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -145,6 +145,9 @@
 #define MI_MANAGEMENT_PROTOCOL_IN 0x10
 /* value for MI_REPORT_TARGET_PGS ext header */
 #define MI_EXT_HDR_PARAM_FMT  0x20
+/* format field in byte 4 of the REPORT TARGET PORT GROUPS parameter data */
+#define RTPG_FMT_MASK         0x70
+#define RTPG_FMT_EXT_HDR      0x10
 /* values for maintenance out */
 #define MO_SET_IDENTIFYING_INFORMATION 0x06
 #define MO_SET_TARGET_PGS     0x0a
-- 
2.56.0

[PATCH 2/4] scsi: core: Read the ALUA access state when a device is scanned

[Brian Bunker]

sdev->access_state is only set by a device handler, and the ALUA
device handler reads it with an RTPG it queues when it attaches, which
normally completes after the device has been registered and matched to
a driver. The SCSI core therefore has no ALUA state for a device while
it decides whether to bind an upper-level driver to it.

For a device that reports TPGS support, send REPORT TARGET PORT GROUPS
from scsi_add_lun(), after the VPD pages have been read, and set
sdev->access_state from the descriptor of the target port group of
this path. UNIT ATTENTIONs, which a target typically reports first
after a login or an ALUA state change, are retried, and the command is
resent without the extended header if the target rejects it. If the
command fails, e.g. with LOGICAL UNIT NOT SUPPORTED because no LU is
mapped at the LUN, sdev->access_state is left as it is.

The command uses the same timeout as the INQUIRY of the scan. The ALUA
device handler keeps updating sdev->access_state afterwards as before.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_scan.c | 94 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 8aae12fd38f3..a21dee1d9e9e 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -861,6 +861,93 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	return 0;
 }
 
+/**
+ * scsi_read_access_state - Read the ALUA state of a device
+ * @sdev: SCSI device that supports target port groups
+ *
+ * Send REPORT TARGET PORT GROUPS and set sdev->access_state to the
+ * asymmetric access state that the device reports for the target port
+ * group of this path, as identified in VPD page 0x83. If the command
+ * fails, e.g. with LOGICAL UNIT NOT SUPPORTED because no LU is mapped at
+ * this LUN for this I_T nexus, sdev->access_state is left unchanged. The
+ * ALUA device handler, if attached, keeps the state up to date later.
+ */
+static void scsi_read_access_state(struct scsi_device *sdev)
+{
+	/*
+	 * Each UNIT ATTENTION reports a different condition queued for this
+	 * I_T nexus, e.g. a power on or reset after login followed by ALUA
+	 * state and INQUIRY data changes, so retry until the target answers
+	 * the command itself.
+	 */
+	struct scsi_failure failure_defs[] = {
+		{
+			.sense_key = UNIT_ATTENTION,
+			.sense_code = SCMD_FAILURE_SENSE_CODE_ANY,
+			.allowed = 5,
+			.result = SAM_STAT_CHECK_CONDITION,
+		},
+		{}
+	};
+	struct scsi_failures failures = {
+		.failure_definitions = failure_defs,
+	};
+	struct scsi_sense_hdr sshdr;
+	const struct scsi_exec_args exec_args = {
+		.sshdr = &sshdr,
+		.failures = &failures,
+	};
+	/* scsi_report_tpgs() replaces the buffer if this is too small */
+	int bufflen = 128;
+	int group_id, len, k, off;
+	unsigned char *buff, *desc;
+	bool ext_hdr = true;
+	int ret;
+
+	group_id = scsi_vpd_tpg_id(sdev, NULL);
+	if (group_id < 0)
+		return;
+
+	buff = kzalloc(bufflen, GFP_KERNEL);
+	if (!buff)
+		return;
+
+ retry:
+	ret = scsi_report_tpgs(sdev, &buff, &bufflen, ext_hdr,
+			       HZ / 2 + HZ * scsi_inq_timeout, 3, &exec_args);
+	if (ret) {
+		/*
+		 * Some targets reject the extended header format with
+		 * ILLEGAL REQUEST (see alua_rtpg()), so retry without it,
+		 * unless the reason is that no LU is mapped at this LUN.
+		 */
+		if (ret > 0 && ext_hdr && scsi_sense_valid(&sshdr) &&
+		    sshdr.sense_key == ILLEGAL_REQUEST &&
+		    sshdr.sense_code != LU_NOT_SUPPORTED) {
+			ext_hdr = false;
+			goto retry;
+		}
+		goto out;
+	}
+
+	len = get_unaligned_be32(&buff[0]) + 4;
+	if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
+		k = 8;
+	else
+		k = 4;
+
+	for (desc = buff + k; k + 8 <= len; k += off, desc += off) {
+		off = 8 + (desc[7] * 4);
+		if (get_unaligned_be16(&desc[2]) == group_id) {
+			sdev->access_state = desc[0];
+			break;
+		}
+	}
+
+ out:
+	kfree(buff);
+}
+
 /**
  * scsi_add_lun - allocate and fully initialize a scsi_device
  * @sdev:	holds information to be stored in the new scsi_device
@@ -1126,6 +1213,13 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 
 	scsi_cdl_check(sdev);
 
+	/*
+	 * Read the ALUA state before the device is registered, so that
+	 * scsi_bus_match() can use it. This needs VPD page 0x83.
+	 */
+	if (scsi_device_tpgs(sdev))
+		scsi_read_access_state(sdev);
+
 	sdev->max_queue_depth = sdev->queue_depth;
 	WARN_ON_ONCE(sdev->max_queue_depth > sdev->budget_map.depth);
 
-- 
2.56.0

[PATCH 3/4] scsi: scsi_dh_alua: Start a port group in the scanned access state

[Brian Bunker]

A new port group starts out active/optimized until the first RTPG of
the handler completes, so alua_prep_fn() lets I/O through to a path
that is actually unavailable. For a device whose driver is bound
straight away, the first I/O, typically the partition table read, then
reaches the target and fails with LOGICAL UNIT NOT ACCESSIBLE, TARGET
PORT IN UNAVAILABLE STATE, logging an I/O error.

The SCSI core now reads sdev->access_state when the device is scanned,
before the handler attaches. Start a new port group in that state. If
the core did not read one, sdev->access_state is zero, which is
active/optimized, so nothing changes for those devices.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 06eb35cc9d7c..15471e7bf19a 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -208,7 +208,14 @@ static struct alua_port_group *alua_alloc_pg(struct scsi_device *sdev,
 	}
 	pg->group_id = group_id;
 	pg->tpgs = tpgs;
-	pg->state = SCSI_ACCESS_STATE_OPTIMAL;
+	/*
+	 * Start in the access state the SCSI core read when the device was
+	 * scanned. That is active/optimized if it did not read one, as it
+	 * was before. alua_prep_fn() then already fails I/O to a path that
+	 * is unavailable before the first RTPG of the handler completes.
+	 */
+	pg->state = FIELD_GET(SCSI_ACCESS_STATE_MASK, sdev->access_state);
+	pg->pref = FIELD_GET(SCSI_ACCESS_STATE_PREFERRED, sdev->access_state);
 	pg->valid_states = TPGS_SUPPORT_ALL;
 	if (optimize_stpg)
 		pg->flags |= ALUA_OPTIMIZE_STPG;
-- 
2.56.0

[PATCH 4/4] scsi: core: Match ULDs to logical units in the ALUA unavailable state

[Brian Bunker]

scsi_bus_match() only matches devices with peripheral qualifier 000b,
so a LU that is in the ALUA unavailable state on a target port when
the host scans it never gets an upper-level driver on that path. When
the target port becomes available again, nothing binds sd and the path
stays unusable until the device is deleted and rescanned. A host that
was already running when the path became unavailable keeps sd bound,
so the result depends on when the host scanned.

Peripheral qualifier 001b cannot be matched on its own: an unmapped
LUN reports it as well, and some targets report every possible LUN in
REPORT LUNS. Binding sd to all of those would create disks with no
backing LU.

The SCSI core now reads the access state of a device that supports
target port groups with REPORT TARGET PORT GROUPS before the device is
registered, and an unmapped LUN fails that command. Also match PQ 001b
devices whose access state is unavailable. sd then binds to the
unavailable path like it does to a standby path, the path stays in the
multipath map as a failed path, and the existing UNIT ATTENTION
handling brings it back when the target port becomes available.

This is the same situation as a path in the standby state, to which sd
already binds: SPC-4 requires READ CAPACITY in neither state. The
cached INQUIRY data in sysfs keeps the peripheral qualifier reported at
scan time, like the rest of the INQUIRY data; access_state shows the
current ALUA state.

Co-developed-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Krishna Kant <krishna.kant@purestorage.com>
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 drivers/scsi/scsi_sysfs.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index fe54792ed69e..8d90f0c715ce 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -539,7 +539,19 @@ static int scsi_bus_match(struct device *dev, const struct device_driver *gendrv
 	sdp = to_scsi_device(dev);
 	if (sdp->no_uld_attach)
 		return 0;
-	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
+	if (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)
+		return 1;
+	/*
+	 * Peripheral qualifier 001b is also what a LU reports while it is in
+	 * the ALUA unavailable state on this target port (SPC-4 5.15.2.4.5).
+	 * scsi_add_lun() reads that state with REPORT TARGET PORT GROUPS
+	 * before the device is registered. A LUN that is not mapped reports
+	 * 001b too, but fails REPORT TARGET PORT GROUPS and so is not
+	 * matched.
+	 */
+	return sdp->inq_periph_qual == SCSI_INQ_PQ_NOT_CON &&
+	       (sdp->access_state & SCSI_ACCESS_STATE_MASK) ==
+			SCSI_ACCESS_STATE_UNAVAILABLE;
 }
 
 static int scsi_bus_uevent(const struct device *dev, struct kobj_uevent_env *env)
-- 
2.56.0

Re: [PATCH 4/4] scsi: core: Match ULDs to logical units in the ALUA unavailable state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: scsi_bus_match() fails to match PQ 001b devices after ALUA transition
--

commit 8555f2a23fa08a25f1e358e107e5863a5e3dec5f
Author: Brian Bunker <brian@purestorage.com>

scsi: core: Match ULDs to logical units in the ALUA unavailable state

This commit updates scsi_bus_match() to also match SCSI devices that report
peripheral qualifier 001b (SCSI_INQ_PQ_NOT_CON) when their ALUA access state
is SCSI_ACCESS_STATE_UNAVAILABLE, allowing upper-level drivers like sd to bind
to logical units scanned while in the ALUA unavailable state.

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index fe54792ed69e4..8d90f0c715ce2 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -539,7 +539,19 @@ static int scsi_bus_match(struct device *dev, const struct device_driver *gendrv
>  	sdp = to_scsi_device(dev);
>  	if (sdp->no_uld_attach)
>  		return 0;
> -	return (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)? 1: 0;
> +	if (sdp->inq_periph_qual == SCSI_INQ_PQ_CON)
> +		return 1;
> +	/*
> +	 * Peripheral qualifier 001b is also what a LU reports while it is in
> +	 * the ALUA unavailable state on this target port (SPC-4 5.15.2.4.5).
> +	 * scsi_add_lun() reads that state with REPORT TARGET PORT GROUPS
> +	 * before the device is registered. A LUN that is not mapped reports
> +	 * 001b too, but fails REPORT TARGET PORT GROUPS and so is not
> +	 * matched.
> +	 */
> +	return sdp->inq_periph_qual == SCSI_INQ_PQ_NOT_CON &&
> +	       (sdp->access_state & SCSI_ACCESS_STATE_MASK) ==
> +			SCSI_ACCESS_STATE_UNAVAILABLE;

[Severity: High]
What happens in scsi_bus_match() if a logical unit is scanned while in the
ALUA unavailable state (setting sdp->inq_periph_qual to SCSI_INQ_PQ_NOT_CON),
and its ALUA access state subsequently transitions out of
SCSI_ACCESS_STATE_UNAVAILABLE?

In drivers/scsi/scsi_scan.c, scsi_add_lun() sets sdev->inq_periph_qual once
from the initial INQUIRY buffer and never updates it:

drivers/scsi/scsi_scan.c:scsi_add_lun() {
	...
	sdev->inq_periph_qual = (inq_result[0] >> 5) & 7;
	...
}

Meanwhile, sdev->access_state is updated dynamically when the target port
group transitions (for example, to SCSI_ACCESS_STATE_OPTIMAL,
SCSI_ACCESS_STATE_ACTIVE, SCSI_ACCESS_STATE_STANDBY, or
SCSI_ACCESS_STATE_TRANSITIONING):

drivers/scsi/device_handler/scsi_dh_alua.c:alua_rtpg() {
	...
	rcu_read_lock();
	list_for_each_entry_rcu(h, &tmp_pg->dh_list, node) {
		if (!h->sdev)
			continue;
		h->sdev->access_state = desc[0];
	}
	rcu_read_unlock();
	...
}

Because scsi_bus_match() requires sdp->access_state to still equal
SCSI_ACCESS_STATE_UNAVAILABLE when sdp->inq_periph_qual is
SCSI_INQ_PQ_NOT_CON, won't driver matching fail whenever sdev->access_state
changes to an available state before or between scsi_bus_match() calls?

Specifically, this appears to break ULD binding in three situations:

1. In scsi_probe_and_add_lun() -> scsi_add_lun(), if the target port
transitions out of the unavailable state after scsi_probe_lun() issues INQUIRY
but before scsi_read_access_state() issues REPORT TARGET PORT GROUPS,
sdev->inq_periph_qual is recorded as SCSI_INQ_PQ_NOT_CON while
sdev->access_state is set to the new state (such as
SCSI_ACCESS_STATE_OPTIMAL). When device_add() runs, scsi_bus_match() returns
0.

2. In scsi_sysfs_add_sdev(), scsi_dh_add_device() attaches scsi_dh_alua and
queues pg->rtpg_work on kaluad_wq before device_add() runs:

drivers/scsi/scsi_sysfs.c:scsi_sysfs_add_sdev() {
	...
	scsi_dh_add_device(sdev);

	error = device_add(&sdev->sdev_gendev);
	...
}

Since sd_template sets PROBE_PREFER_ASYNCHRONOUS, __device_attach() also
defers probing to __device_attach_async_helper() -> driver_match_device() ->
scsi_bus_match(). If alua_rtpg() updates h->sdev->access_state before
device_add() or before __device_attach_async_helper() runs, scsi_bus_match()
sees the new access_state and returns 0.

3. If an upper-level driver module (such as sd_mod or sg) is loaded via udev
after the port has transitioned to an available state, or if a driver is
unbound and rebound via sysfs (bind_store() -> driver_match_device() ->
scsi_bus_match()), scsi_bus_match() sees sdp->inq_periph_qual ==
SCSI_INQ_PQ_NOT_CON and sdp->access_state != SCSI_ACCESS_STATE_UNAVAILABLE and
refuses to bind to the now-available logical unit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028184216.25453-1-brian@purestorage.com?part=4