[PATCH 0/7] ALUA device handler update

[PATCH 0/7] ALUA device handler update

[Hannes Reinecke]

This is an update to the ALUA device handler. The most important
bit is the implementation of a 'match' callback, which allows for
the implementation of a device_handler specific routine instead of
using a string comparison on vendor and model.

Rationale is that most modern arrays can operate in both modes,
ALUA and the 'legacy' failover mode. However, the vendor and
model will be identical to the legacy mode, so ALUA will never
be enabled per default on these machines.
With these patches the ALUA handler will be enabled whenever the
TPGS state is set in the inquiry data.

In contrast to what we've discussed at LinuxTag I have not tried
to attach the alua device handler directly from scsi_scan.
Reason is that I need to issue SCSI commands during activation,
which means I would have to attach it from near the end of
scsi_add_lun(), at which point the device_handler would be attached
via the current method anyway. So I fail to see the gain here.

Other fixes are:
- Evaluate the TPGS setting from the inquiry data instead of
  issuing our own inquiry
- Fixup kernel-doc comments
- Always update TPGS state on activate
- Errorneous TPG ID check
- Decrease retry interval when array is in state transitioning

Hannes Reinecke (7):
  scsi_dh_alua: Evaluate TPGS setting from inquiry data
  scsi_dh: Fixup kernel-doc comments
  scsi_dh: Implement match callback function
  scsi_dh_alua: always update TPGS status on activate
  scsi_dh: Check for sdev state in store_dh_state()
  Erroneous TPG ID check in SCSI ALUA Handler
  scsi_dh_alua: Decrease retry interval

 drivers/scsi/device_handler/scsi_dh.c      |   61 ++++++++++++++++---
 drivers/scsi/device_handler/scsi_dh_alua.c |   92 +++++----------------------
 include/scsi/scsi_device.h                 |    6 ++
 3 files changed, 75 insertions(+), 84 deletions(-)

[PATCH 1/7] scsi_dh_alua: Evaluate TPGS setting from inquiry data

[Hannes Reinecke]

Instead of issuing a standard inquiry from within the
alua device handler we can evaluate the TPGS setting from
the existing inquiry data of the sdev and save us the I/O.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c |   55 +++-------------------------
 include/scsi/scsi_device.h                 |    5 +++
 2 files changed, 11 insertions(+), 49 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 6fec9fe..4326778 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -128,43 +128,6 @@ static struct request *get_alua_req(struct scsi_device *sdev,
 }
 
 /*
- * submit_std_inquiry - Issue a standard INQUIRY command
- * @sdev: sdev the command should be send to
- */
-static int submit_std_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
-{
-	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
-
-	rq = get_alua_req(sdev, h->inq, ALUA_INQUIRY_SIZE, READ);
-	if (!rq)
-		goto done;
-
-	/* Prepare the command. */
-	rq->cmd[0] = INQUIRY;
-	rq->cmd[1] = 0;
-	rq->cmd[2] = 0;
-	rq->cmd[4] = ALUA_INQUIRY_SIZE;
-	rq->cmd_len = COMMAND_SIZE(INQUIRY);
-
-	rq->sense = h->sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
-
-	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: std inquiry failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
-		h->senselen = rq->sense_len;
-		err = SCSI_DH_IO;
-	}
-	blk_put_request(rq);
-done:
-	return err;
-}
-
-/*
  * submit_vpd_inquiry - Issue an INQUIRY VPD page 0x83 command
  * @sdev: sdev the command should be sent to
  */
@@ -338,23 +301,17 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 }
 
 /*
- * alua_std_inquiry - Evaluate standard INQUIRY command
+ * alua_check_tpgs - Evaluate TPGS setting
  * @sdev: device to be checked
  *
- * Just extract the TPGS setting to find out if ALUA
+ * Examine the TPGS setting of the sdev to find out if ALUA
  * is supported.
  */
-static int alua_std_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
+static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int err;
-
-	err = submit_std_inquiry(sdev, h);
-
-	if (err != SCSI_DH_OK)
-		return err;
+	int err = SCSI_DH_OK;
 
-	/* Check TPGS setting */
-	h->tpgs = (h->inq[5] >> 4) & 0x3;
+	h->tpgs = scsi_device_tpgs(sdev);
 	switch (h->tpgs) {
 	case TPGS_MODE_EXPLICIT|TPGS_MODE_IMPLICIT:
 		sdev_printk(KERN_INFO, sdev,
@@ -642,7 +599,7 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int err;
 
-	err = alua_std_inquiry(sdev, h);
+	err = alua_check_tpgs(sdev, h);
 	if (err != SCSI_DH_OK)
 		goto out;
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 6712446..4733635 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -472,6 +472,11 @@ static inline int scsi_device_protection(struct scsi_device *sdev)
 	return sdev->scsi_level > SCSI_2 && sdev->inquiry[5] & (1<<0);
 }
 
+static inline int scsi_device_tpgs(struct scsi_device *sdev)
+{
+	return sdev->inquiry ? (sdev->inquiry[5] >> 4) & 0x3 : 0;
+}
+
 #define MODULE_ALIAS_SCSI_DEVICE(type) \
 	MODULE_ALIAS("scsi:t-" __stringify(type) "*")
 #define SCSI_DEVICE_MODALIAS_FMT "scsi:t-0x%02x"
-- 
1.6.0.2

[PATCH 2/7] scsi_dh: Fixup kernel-doc comments

[Hannes Reinecke]

Fixup some kernel-doc comments to reference to the
correct function name.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index bc96ca6..a801c59 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -468,7 +468,7 @@ int scsi_dh_handler_exist(const char *name)
 EXPORT_SYMBOL_GPL(scsi_dh_handler_exist);
 
 /*
- * scsi_dh_handler_attach - Attach device handler
+ * scsi_dh_attach - Attach device handler
  * @sdev - sdev the handler should be attached to
  * @name - name of the handler to attach
  */
@@ -498,7 +498,7 @@ int scsi_dh_attach(struct request_queue *q, const char *name)
 EXPORT_SYMBOL_GPL(scsi_dh_attach);
 
 /*
- * scsi_dh_handler_detach - Detach device handler
+ * scsi_dh_detach - Detach device handler
  * @sdev - sdev the handler should be detached from
  *
  * This function will detach the device handler only
-- 
1.6.0.2

[PATCH 3/7] scsi_dh: Implement match callback function

[Hannes Reinecke]

Some device handler types are not tied to the vendor/model
but rather to a specific capability. Eg ALUA is supported
if the 'TPGS' setting in the standard inquiry is set.
This patch implements a 'match' callback for device handler
which supersedes the original vendor/model lookup and
implements the callback for the ALUA handler.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh.c      |   53 ++++++++++++++++++++++++----
 drivers/scsi/device_handler/scsi_dh_alua.c |   23 +++---------
 include/scsi/scsi_device.h                 |    1 +
 3 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index a801c59..458521d 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -60,6 +60,46 @@ static struct scsi_device_handler *get_device_handler_by_idx(int idx)
 }
 
 /*
+ * device_handler_match_function - Match a device handler to a device
+ * @sdev - SCSI device to be tested
+ *
+ * Tests @sdev against the match function of all registered device_handler.
+ * Returns the found device handler or NULL if not found.
+ */
+static struct scsi_device_handler *
+device_handler_match_function(struct scsi_device *sdev)
+{
+	struct scsi_device_handler *tmp_dh, *found_dh = NULL;
+
+	spin_lock(&list_lock);
+	list_for_each_entry(tmp_dh, &scsi_dh_list, list) {
+		if (tmp_dh->match && tmp_dh->match(sdev)) {
+			found_dh = tmp_dh;
+			break;
+		}
+	}
+	spin_unlock(&list_lock);
+	return found_dh;
+}
+
+/*
+ * device_handler_match_devlist - Match a device handler to a device
+ * @sdev - SCSI device to be tested
+ *
+ * Tests @sdev against all device_handler registered in the devlist.
+ * Returns the found device handler or NULL if not found.
+ */
+static struct scsi_device_handler *
+device_handler_match_devlist(struct scsi_device *sdev)
+{
+	int idx;
+
+	idx = scsi_get_device_flags_keyed(sdev, sdev->vendor, sdev->model,
+					  SCSI_DEVINFO_DH);
+	return get_device_handler_by_idx(idx);
+}
+
+/*
  * device_handler_match - Attach a device handler to a device
  * @scsi_dh - The device handler to match against or NULL
  * @sdev - SCSI device to be tested against @scsi_dh
@@ -72,12 +112,11 @@ static struct scsi_device_handler *
 device_handler_match(struct scsi_device_handler *scsi_dh,
 		     struct scsi_device *sdev)
 {
-	struct scsi_device_handler *found_dh = NULL;
-	int idx;
+	struct scsi_device_handler *found_dh;
 
-	idx = scsi_get_device_flags_keyed(sdev, sdev->vendor, sdev->model,
-					  SCSI_DEVINFO_DH);
-	found_dh = get_device_handler_by_idx(idx);
+	found_dh = device_handler_match_function(sdev);
+	if (!found_dh)
+		found_dh = device_handler_match_devlist(sdev);
 
 	if (scsi_dh && found_dh != scsi_dh)
 		found_dh = NULL;
@@ -327,7 +366,7 @@ int scsi_register_device_handler(struct scsi_device_handler *scsi_dh)
 	list_add(&scsi_dh->list, &scsi_dh_list);
 	spin_unlock(&list_lock);
 
-	for (i = 0; scsi_dh->devlist[i].vendor; i++) {
+	for (i = 0; scsi_dh->devlist && scsi_dh->devlist[i].vendor; i++) {
 		scsi_dev_info_list_add_keyed(0,
 					scsi_dh->devlist[i].vendor,
 					scsi_dh->devlist[i].model,
@@ -360,7 +399,7 @@ int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh)
 	bus_for_each_dev(&scsi_bus_type, NULL, scsi_dh,
 			 scsi_dh_notifier_remove);
 
-	for (i = 0; scsi_dh->devlist[i].vendor; i++) {
+	for (i = 0; scsi_dh->devlist && scsi_dh->devlist[i].vendor; i++) {
 		scsi_dev_info_list_del_keyed(scsi_dh->devlist[i].vendor,
 					     scsi_dh->devlist[i].model,
 					     SCSI_DEVINFO_DH);
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 4326778..80c5cf3 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -677,23 +677,10 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 
 }
 
-static const struct scsi_dh_devlist alua_dev_list[] = {
-	{"HP", "MSA VOLUME" },
-	{"HP", "HSV101" },
-	{"HP", "HSV111" },
-	{"HP", "HSV200" },
-	{"HP", "HSV210" },
-	{"HP", "HSV300" },
-	{"IBM", "2107900" },
-	{"IBM", "2145" },
-	{"Pillar", "Axiom" },
-	{"Intel", "Multi-Flex"},
-	{"NETAPP", "LUN"},
-	{"NETAPP", "LUN C-Mode"},
-	{"AIX", "NVDISK"},
-	{"Promise", "VTrak"},
-	{NULL, NULL}
-};
+static bool alua_match(struct scsi_device *sdev)
+{
+	return (scsi_device_tpgs(sdev) != 0);
+}
 
 static int alua_bus_attach(struct scsi_device *sdev);
 static void alua_bus_detach(struct scsi_device *sdev);
@@ -701,12 +688,12 @@ static void alua_bus_detach(struct scsi_device *sdev);
 static struct scsi_device_handler alua_dh = {
 	.name = ALUA_DH_NAME,
 	.module = THIS_MODULE,
-	.devlist = alua_dev_list,
 	.attach = alua_bus_attach,
 	.detach = alua_bus_detach,
 	.prep_fn = alua_prep_fn,
 	.check_sense = alua_check_sense,
 	.activate = alua_activate,
+	.match = alua_match,
 };
 
 /*
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 4733635..afcb403 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -197,6 +197,7 @@ struct scsi_device_handler {
 	int (*activate)(struct scsi_device *, activate_complete, void *);
 	int (*prep_fn)(struct scsi_device *, struct request *);
 	int (*set_params)(struct scsi_device *, const char *);
+	bool (*match)(struct scsi_device *);
 };
 
 struct scsi_dh_data {
-- 
1.6.0.2

[PATCH 4/7] scsi_dh_alua: always update TPGS status on activate

[Hannes Reinecke]

When activating a patch we should always update the TPGS state
as it might have changed in between.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 80c5cf3..f44dbf6 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -631,11 +631,9 @@ static int alua_activate(struct scsi_device *sdev,
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int err = SCSI_DH_OK;
 
-	if (h->group_id != -1) {
-		err = alua_rtpg(sdev, h);
-		if (err != SCSI_DH_OK)
-			goto out;
-	}
+	err = alua_rtpg(sdev, h);
+	if (err != SCSI_DH_OK)
+		goto out;
 
 	if (h->tpgs & TPGS_MODE_EXPLICIT &&
 	    h->state != TPGS_STATE_OPTIMIZED &&
-- 
1.6.0.2

[PATCH 5/7] scsi_dh: Check for sdev state in store_dh_state()

[Hannes Reinecke]

Avoid attaching a hardware handler to a device which is
already scheduled for deletion.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 458521d..520bcf0 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -190,6 +190,10 @@ store_dh_state(struct device *dev, struct device_attribute *attr,
 	struct scsi_device_handler *scsi_dh;
 	int err = -EINVAL;
 
+	if (sdev->sdev_state == SDEV_CANCEL ||
+	    sdev->sdev_state == SDEV_DEL)
+		return -ENODEV;
+
 	if (!sdev->scsi_dh_data) {
 		/*
 		 * Attach to a device handler
-- 
1.6.0.2

[PATCH 6/7] Erroneous TPG ID check in SCSI ALUA Handler

[Hannes Reinecke]

For Target Portal Group IDs occupying the full 2 bytes in the
RTPG response, the following group_id check in the alua_rtpg
routine always fails in scsi_dh_alua.c:

if (h->group_id == (ucp[2] << 8) + ucp[3]) {

This causes the ALUA handler to wrongly identify the AAS of
a specified device as well as incorrectly interpreting the
supported AAS of the target as seen by the following entries
in the /var/log/messages:

"alua: port group 3ea state A supports tousna"
"alua: port group 3e9 state A supports tousna"

This is because 'ucp' is wrongly declared in alua_rtpg as
a character pointer instead of an unsigned character pointer.

Signed-off-by: Martin George <marting@netapp.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index f44dbf6..34c6611 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -504,7 +504,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;
-	char *ucp;
+	unsigned char *ucp;
 	unsigned err;
 	unsigned long expiry, interval = 10;
 
-- 
1.6.0.2

[PATCH 7/7] scsi_dh_alua: Decrease retry interval

[Hannes Reinecke]

The alua device handler starts the first retry after 10 seconds,
and increases it times 10 for each round.
This leads to an unnecessary delay. This patch modifies it to
start after one second, and increase by a factor of two.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 34c6611..9dce367 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -506,7 +506,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
 	unsigned err;
-	unsigned long expiry, interval = 10;
+	unsigned long expiry, interval = 1;
 
 	expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT);
  retry:
@@ -567,7 +567,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	case TPGS_STATE_TRANSITIONING:
 		if (time_before(jiffies, expiry)) {
 			/* State transition, retry */
-			interval *= 10;
+			interval *= 2;
 			msleep(interval);
 			goto retry;
 		}
-- 
1.6.0.2