[PATCHv2 00/16] scsi_dh_alua updates

[PATCHv2 00/16] scsi_dh_alua updates

[Hannes Reinecke]

Hi James,

here's an update for the ALUA device handler I've been hoarding
for quite some time. The major bit here is the asynchronous
RTPG handling. With the original design we would treat every
LUN independently, despite the fact that several LUNs might
in fact belong to the same target port group. So any
change on one LUN will affect the others, too.
And we now can treat LUNs in 'transitioning' ALUA mode
correctly, as now we'll be blocking any I/O in the prep_fn()
until the controller is in a working state again.

This is the second version of the patchset, containing
suggested changes from Mike Christie to move
GFP_ATOMIC to GFP_KERNEL allocations.

Hannes Reinecke (16):
  scsi_dh_alua: Improve error handling
  scsi_dh_alua: use flag for RTPG extended header
  scsi_dh_alua: Pass buffer as function argument
  scsi_dh_alua: Make stpg synchronous
  scsi_dh_alua: put sense buffer on stack
  scsi_dh_alua: use local buffer for VPD inquiry
  scsi_dh_alua: Use separate alua_port_group structure
  scsi_dh_alua: parse target device id
  scsi_dh_alua: simplify sense code handling
  scsi_dh_alua: Do not attach to management devices
  scsi_dh_alua: multipath failover fails with error 15
  scsi_dh: return individual errors in scsi_dh_activate()
  scsi_dh_alua: Clarify logging message
  scsi_dh: invoke callback if ->activate is not present
  scsi_dh_alua: revert commit a8e5a2d593cbfccf530c3382c2c328d2edaa7b66
  scsi_dh_alua: Use workqueue for RTPG

 drivers/scsi/device_handler/scsi_dh.c      |   18 +-
 drivers/scsi/device_handler/scsi_dh_alua.c | 1045 ++++++++++++++++++++--------
 2 files changed, 750 insertions(+), 313 deletions(-)

-- 
1.7.12.4

[PATCH 01/16] scsi_dh_alua: Improve error handling

[Hannes Reinecke]

Improve error handling and use standard logging functions
instead of hand-crafted ones.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 5248c88..e4e5497 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dh.h>
 
@@ -144,11 +145,13 @@ static struct request *get_alua_req(struct scsi_device *sdev,
 static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err;
 
 	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
+	if (!rq) {
+		err = DRIVER_BUSY << 24;
 		goto done;
+	}
 
 	/* Prepare the command. */
 	rq->cmd[0] = INQUIRY;
@@ -162,12 +165,12 @@ static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 	rq->sense_len = h->senselen = 0;
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: evpd inquiry failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
+	if (err < 0) {
+		if (!rq->errors)
+			err = DID_ERROR << 16;
+		else
+			err = rq->errors;
 		h->senselen = rq->sense_len;
-		err = SCSI_DH_IO;
 	}
 	blk_put_request(rq);
 done:
@@ -182,11 +185,13 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 			    bool rtpg_ext_hdr_req)
 {
 	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err;
 
 	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
+	if (!rq) {
+		err = DRIVER_BUSY << 24;
 		goto done;
+	}
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
@@ -205,12 +210,12 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 	rq->sense_len = h->senselen = 0;
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
+	if (err < 0) {
+		if (!rq->errors)
+			err = DID_ERROR << 16;
+		else
+			err = rq->errors;
 		h->senselen = rq->sense_len;
-		err = SCSI_DH_IO;
 	}
 	blk_put_request(rq);
 done:
@@ -218,13 +223,11 @@ done:
 }
 
 /*
- * alua_stpg - Evaluate SET TARGET GROUP STATES
+ * stpg_endio - Evaluate SET TARGET GROUP STATES
  * @sdev: the device to be evaluated
  * @state: the new target group state
  *
- * Send a SET TARGET GROUP STATES command to the device.
- * We only have to test here if we should resubmit the command;
- * any other error is assumed as a failure.
+ * Evaluate a SET TARGET GROUP STATES command response.
  */
 static void stpg_endio(struct request *req, int error)
 {
@@ -239,9 +242,8 @@ static void stpg_endio(struct request *req, int error)
 	}
 
 	if (req->sense_len > 0) {
-		err = scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					   &sense_hdr);
-		if (!err) {
+		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
 			err = SCSI_DH_IO;
 			goto done;
 		}
@@ -250,10 +252,12 @@ static void stpg_endio(struct request *req, int error)
 			err = SCSI_DH_RETRY;
 			goto done;
 		}
-		sdev_printk(KERN_INFO, h->sdev,
-			    "%s: stpg sense code: %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed, ",
+			    ALUA_DH_NAME);
+		scsi_show_sense_hdr(&sense_hdr);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed, ",
+			    ALUA_DH_NAME);
+		scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
 		err = SCSI_DH_IO;
 	} else if (error)
 		err = SCSI_DH_IO;
@@ -362,15 +366,43 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
  */
 static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int len;
-	unsigned err;
+	int len, timeout = ALUA_FAILOVER_TIMEOUT;
+	struct scsi_sense_hdr sense_hdr;
+	unsigned retval;
 	unsigned char *d;
+	unsigned long expiry;
 
+	expiry = round_jiffies_up(jiffies + timeout);
  retry:
-	err = submit_vpd_inquiry(sdev, h);
-
-	if (err != SCSI_DH_OK)
-		return err;
+	retval = submit_vpd_inquiry(sdev, h);
+	if (retval) {
+		unsigned err;
+
+		if (h->senselen == 0 ||
+		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: evpd inquiry failed, ", ALUA_DH_NAME);
+			scsi_show_result(retval);
+			if (driver_byte(retval) == DRIVER_BUSY)
+				err = SCSI_DH_DEV_TEMP_BUSY;
+			else
+				err = SCSI_DH_IO;
+			return err;
+		}
+		err = alua_check_sense(sdev, &sense_hdr);
+		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
+			goto retry;
+		if (err != SUCCESS) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: evpd inquiry failed, ", ALUA_DH_NAME);
+			scsi_show_sense_hdr(&sense_hdr);
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: evpd inquiry failed, ", ALUA_DH_NAME);
+			scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
+			return SCSI_DH_IO;
+		}
+	}
 
 	/* Check if vpd page exceeds initial buffer */
 	len = (h->buff[2] << 8) + h->buff[3] + 4;
@@ -417,14 +449,13 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			    ALUA_DH_NAME);
 		h->state = TPGS_STATE_OPTIMIZED;
 		h->tpgs = TPGS_MODE_NONE;
-		err = SCSI_DH_DEV_UNSUPP;
-	} else {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: port group %02x rel port %02x\n",
-			    ALUA_DH_NAME, h->group_id, h->rel_port);
+		return SCSI_DH_DEV_UNSUPP;
 	}
+	sdev_printk(KERN_INFO, sdev,
+		    "%s: port group %02x rel port %02x\n",
+		    ALUA_DH_NAME, h->group_id, h->rel_port);
 
-	return err;
+	return SCSI_DH_OK;
 }
 
 static char print_alua_state(int state)
@@ -533,7 +564,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
-	unsigned err;
+	unsigned err, retval;
 	bool rtpg_ext_hdr_req = 1;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
@@ -545,13 +576,21 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	err = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
 
-	if (err == SCSI_DH_IO && h->senselen > 0) {
-		err = scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					   &sense_hdr);
-		if (!err)
-			return SCSI_DH_IO;
+	if (retval) {
+		if (h->senselen == 0 ||
+		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev, "%s: rtpg failed, ",
+				    ALUA_DH_NAME);
+			scsi_show_result(retval);
+			if (driver_byte(retval) == DRIVER_BUSY)
+				err = SCSI_DH_DEV_TEMP_BUSY;
+			else
+				err = SCSI_DH_IO;
+			return err;
+		}
 
 		/*
 		 * submit_rtpg() has failed on existing arrays
@@ -569,16 +608,23 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		}
 
 		err = alua_check_sense(sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
+		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
+			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry, ",
+				    ALUA_DH_NAME);
+			scsi_show_sense_hdr(&sense_hdr);
+			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry, ",
+				    ALUA_DH_NAME);
+			scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
 			goto retry;
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg sense code %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
-		err = SCSI_DH_IO;
+		}
+		sdev_printk(KERN_INFO, sdev, "%s: rtpg failed, ",
+			    ALUA_DH_NAME);
+		scsi_show_sense_hdr(&sense_hdr);
+		sdev_printk(KERN_INFO, sdev, "%s: rtpg failed, ",
+			    ALUA_DH_NAME);
+		scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
+		return SCSI_DH_IO;
 	}
-	if (err != SCSI_DH_OK)
-		return err;
 
 	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
 		(h->buff[2] << 8) + h->buff[3] + 4;
-- 
1.7.12.4

[PATCH 02/16] scsi_dh_alua: use flag for RTPG extended header

[Hannes Reinecke]

We should be using a flag when RTPG extended header is not
supported, that saves us sending RTPG twice for older arrays.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index e4e5497..ece2255 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -61,6 +61,7 @@
 
 /* flags passed from user level */
 #define ALUA_OPTIMIZE_STPG		1
+#define ALUA_RTPG_EXT_HDR_UNSUPP	2
 
 struct alua_dh_data {
 	int			group_id;
@@ -181,8 +182,7 @@ done:
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
  */
-static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
-			    bool rtpg_ext_hdr_req)
+static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	struct request *rq;
 	int err;
@@ -195,7 +195,7 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
-	if (rtpg_ext_hdr_req)
+	if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP))
 		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
 		rq->cmd[1] = MI_REPORT_TARGET_PGS;
@@ -565,7 +565,6 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
 	unsigned err, retval;
-	bool rtpg_ext_hdr_req = 1;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
@@ -576,7 +575,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+	retval = submit_rtpg(sdev, h);
 
 	if (retval) {
 		if (h->senselen == 0 ||
@@ -600,10 +599,10 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		 * The retry without rtpg_ext_hdr_req set
 		 * handles this.
 		 */
-		if (rtpg_ext_hdr_req == 1 &&
+		if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&
 		    sense_hdr.sense_key == ILLEGAL_REQUEST &&
 		    sense_hdr.asc == 0x24 && sense_hdr.ascq == 0) {
-			rtpg_ext_hdr_req = 0;
+			h->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
 			goto retry;
 		}
 
-- 
1.7.12.4

[PATCH 03/16] scsi_dh_alua: Pass buffer as function argument

[Hannes Reinecke]

Pass in the buffer as a function argument for submit_vpd() and
submit_rtpg().

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index ece2255..5358c2f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -143,12 +143,13 @@ static struct request *get_alua_req(struct scsi_device *sdev,
  * submit_vpd_inquiry - Issue an INQUIRY VPD page 0x83 command
  * @sdev: sdev the command should be sent to
  */
-static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
+static int submit_vpd_inquiry(struct scsi_device *sdev, unsigned char *buff,
+			      int bufflen, unsigned char *sense)
 {
 	struct request *rq;
 	int err;
 
-	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
+	rq = get_alua_req(sdev, buff, bufflen, READ);
 	if (!rq) {
 		err = DRIVER_BUSY << 24;
 		goto done;
@@ -158,12 +159,12 @@ static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 	rq->cmd[0] = INQUIRY;
 	rq->cmd[1] = 1;
 	rq->cmd[2] = 0x83;
-	rq->cmd[4] = h->bufflen;
+	rq->cmd[4] = bufflen;
 	rq->cmd_len = COMMAND_SIZE(INQUIRY);
 
-	rq->sense = h->sense;
+	rq->sense = sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
+	rq->sense_len = 0;
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
 	if (err < 0) {
@@ -171,7 +172,8 @@ static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			err = DID_ERROR << 16;
 		else
 			err = rq->errors;
-		h->senselen = rq->sense_len;
+		if (rq->sense_len)
+			err |= (DRIVER_SENSE << 24);
 	}
 	blk_put_request(rq);
 done:
@@ -182,12 +184,13 @@ done:
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
  */
-static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
+static unsigned submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
+			    int bufflen, unsigned char *sense, int flags)
 {
 	struct request *rq;
 	int err;
 
-	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
+	rq = get_alua_req(sdev, buff, bufflen, READ);
 	if (!rq) {
 		err = DRIVER_BUSY << 24;
 		goto done;
@@ -195,19 +198,19 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
-	if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP))
+	if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP))
 		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
 		rq->cmd[1] = MI_REPORT_TARGET_PGS;
-	rq->cmd[6] = (h->bufflen >> 24) & 0xff;
-	rq->cmd[7] = (h->bufflen >> 16) & 0xff;
-	rq->cmd[8] = (h->bufflen >>  8) & 0xff;
-	rq->cmd[9] = h->bufflen & 0xff;
+	rq->cmd[6] = (bufflen >> 24) & 0xff;
+	rq->cmd[7] = (bufflen >> 16) & 0xff;
+	rq->cmd[8] = (bufflen >>  8) & 0xff;
+	rq->cmd[9] = bufflen & 0xff;
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
 
-	rq->sense = h->sense;
+	rq->sense = sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
+	rq->sense_len = 0;
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
 	if (err < 0) {
@@ -215,7 +218,8 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 			err = DID_ERROR << 16;
 		else
 			err = rq->errors;
-		h->senselen = rq->sense_len;
+		if (rq->sense_len)
+			err |= (DRIVER_SENSE << 24);
 	}
 	blk_put_request(rq);
 done:
@@ -374,11 +378,11 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 
 	expiry = round_jiffies_up(jiffies + timeout);
  retry:
-	retval = submit_vpd_inquiry(sdev, h);
+	retval = submit_vpd_inquiry(sdev, h->buff, h->bufflen, h->sense);
 	if (retval) {
 		unsigned err;
 
-		if (h->senselen == 0 ||
+		if (!(driver_byte(retval) & DRIVER_SENSE) ||
 		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
@@ -575,10 +579,10 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h);
+	retval = submit_rtpg(sdev, h->buff, h->bufflen, h->sense, h->flags);
 
 	if (retval) {
-		if (h->senselen == 0 ||
+		if (!(driver_byte(retval) & DRIVER_SENSE) ||
 		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev, "%s: rtpg failed, ",
-- 
1.7.12.4

[PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

We should be issuing STPG synchronously as we need to
evaluate the return code on failure.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 5358c2f..ef92008 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -227,82 +227,27 @@ done:
 }
 
 /*
- * stpg_endio - Evaluate SET TARGET GROUP STATES
- * @sdev: the device to be evaluated
- * @state: the new target group state
- *
- * Evaluate a SET TARGET GROUP STATES command response.
- */
-static void stpg_endio(struct request *req, int error)
-{
-	struct alua_dh_data *h = req->end_io_data;
-	struct scsi_sense_hdr sense_hdr;
-	unsigned err = SCSI_DH_OK;
-
-	if (host_byte(req->errors) != DID_OK ||
-	    msg_byte(req->errors) != COMMAND_COMPLETE) {
-		err = SCSI_DH_IO;
-		goto done;
-	}
-
-	if (req->sense_len > 0) {
-		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					  &sense_hdr)) {
-			err = SCSI_DH_IO;
-			goto done;
-		}
-		err = alua_check_sense(h->sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE) {
-			err = SCSI_DH_RETRY;
-			goto done;
-		}
-		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed, ",
-			    ALUA_DH_NAME);
-		scsi_show_sense_hdr(&sense_hdr);
-		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed, ",
-			    ALUA_DH_NAME);
-		scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
-		err = SCSI_DH_IO;
-	} else if (error)
-		err = SCSI_DH_IO;
-
-	if (err == SCSI_DH_OK) {
-		h->state = TPGS_STATE_OPTIMIZED;
-		sdev_printk(KERN_INFO, h->sdev,
-			    "%s: port group %02x switched to state %c\n",
-			    ALUA_DH_NAME, h->group_id,
-			    print_alua_state(h->state));
-	}
-done:
-	req->end_io_data = NULL;
-	__blk_put_request(req->q, req);
-	if (h->callback_fn) {
-		h->callback_fn(h->callback_data, err);
-		h->callback_fn = h->callback_data = NULL;
-	}
-	return;
-}
-
-/*
  * submit_stpg - Issue a SET TARGET GROUP STATES command
  *
  * Currently we're only setting the current target port group state
  * to 'active/optimized' and let the array firmware figure out
  * the states of the remaining groups.
  */
-static unsigned submit_stpg(struct alua_dh_data *h)
+static unsigned submit_stpg(struct scsi_device *sdev, int group_id,
+			    unsigned char *sense)
 {
 	struct request *rq;
+	unsigned char stpg_data[8];
 	int stpg_len = 8;
-	struct scsi_device *sdev = h->sdev;
+	int err;
 
 	/* Prepare the data buffer */
-	memset(h->buff, 0, stpg_len);
-	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
-	h->buff[6] = (h->group_id >> 8) & 0xff;
-	h->buff[7] = h->group_id & 0xff;
+	memset(stpg_data, 0, stpg_len);
+	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
+	stpg_data[6] = (group_id >> 8) & 0xff;
+	stpg_data[7] = group_id & 0xff;
 
-	rq = get_alua_req(sdev, h->buff, stpg_len, WRITE);
+	rq = get_alua_req(sdev, stpg_data, stpg_len, WRITE);
 	if (!rq)
 		return SCSI_DH_RES_TEMP_UNAVAIL;
 
@@ -315,13 +260,22 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 	rq->cmd[9] = stpg_len & 0xff;
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_OUT);
 
-	rq->sense = h->sense;
+	rq->sense = sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
-	rq->end_io_data = h;
+	rq->sense_len = 0;
 
-	blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio);
-	return SCSI_DH_OK;
+	err = blk_execute_rq(rq->q, NULL, rq, 1);
+	if (err < 0) {
+		if (!rq->errors)
+			err = DID_ERROR << 16;
+		else
+			err = rq->errors;
+		if (rq->sense_len)
+			err |= (DRIVER_SENSE << 24);
+	}
+	blk_put_request(rq);
+
+	return err;
 }
 
 /*
@@ -715,6 +669,69 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 }
 
 /*
+ * alua_stpg - Issue a SET TARGET GROUP STATES command
+ *
+ * Issue a SET TARGET GROUP STATES command and evaluate the
+ * response. Returns SCSI_DH_RETRY per default to trigger
+ * a re-evaluation of the target group state.
+ */
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
+{
+	int retval, err = SCSI_DH_RETRY;
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	struct scsi_sense_hdr sense_hdr;
+
+	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
+		/* Only implicit ALUA supported, retry */
+		return SCSI_DH_RETRY;
+	}
+	switch (h->state) {
+	case TPGS_STATE_NONOPTIMIZED:
+		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
+		    (!h->pref) &&
+		    (h->tpgs & TPGS_MODE_IMPLICIT))
+			return SCSI_DH_OK;
+		break;
+	case TPGS_STATE_STANDBY:
+	case TPGS_STATE_UNAVAILABLE:
+		break;
+	case TPGS_STATE_OFFLINE:
+		return SCSI_DH_IO;
+		break;
+	case TPGS_STATE_TRANSITIONING:
+		return SCSI_DH_RETRY;
+		break;
+	default:
+		return SCSI_DH_NOSYS;
+		break;
+	}
+	/* Set state to transitioning */
+	h->state = TPGS_STATE_TRANSITIONING;
+	retval = submit_stpg(sdev, h->group_id, sense);
+
+	if (retval) {
+		if (!(driver_byte(retval) & DRIVER_SENSE) ||
+		    !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev, "%s: stpg failed, ",
+				    ALUA_DH_NAME);
+			scsi_show_result(retval);
+			/* Retry RTPG */
+			return err;
+		}
+		err = alua_check_sense(h->sdev, &sense_hdr);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed, ",
+			    ALUA_DH_NAME);
+		scsi_show_sense_hdr(&sense_hdr);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed, ",
+			    ALUA_DH_NAME);
+		scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
+		err = SCSI_DH_RETRY;
+	}
+	return err;
+}
+
+/*
  * alua_initialize - Initialize ALUA state
  * @sdev: the device to be initialized
  *
@@ -791,7 +808,6 @@ static int alua_activate(struct scsi_device *sdev,
 {
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int err = SCSI_DH_OK;
-	int stpg = 0;
 
 	err = alua_rtpg(sdev, h, 1);
 	if (err != SCSI_DH_OK)
@@ -800,39 +816,9 @@ static int alua_activate(struct scsi_device *sdev,
 	if (optimize_stpg)
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
-	if (h->tpgs & TPGS_MODE_EXPLICIT) {
-		switch (h->state) {
-		case TPGS_STATE_NONOPTIMIZED:
-			stpg = 1;
-			if ((h->flags & ALUA_OPTIMIZE_STPG) &&
-			    (!h->pref) &&
-			    (h->tpgs & TPGS_MODE_IMPLICIT))
-				stpg = 0;
-			break;
-		case TPGS_STATE_STANDBY:
-		case TPGS_STATE_UNAVAILABLE:
-			stpg = 1;
-			break;
-		case TPGS_STATE_OFFLINE:
-			err = SCSI_DH_IO;
-			break;
-		case TPGS_STATE_TRANSITIONING:
-			err = SCSI_DH_RETRY;
-			break;
-		default:
-			break;
-		}
-	}
-
-	if (stpg) {
-		h->callback_fn = fn;
-		h->callback_data = data;
-		err = submit_stpg(h);
-		if (err == SCSI_DH_OK)
-			return 0;
-		h->callback_fn = h->callback_data = NULL;
-	}
-
+	err = alua_stpg(sdev, h);
+	if (err == SCSI_DH_RETRY)
+		err = alua_rtpg(sdev, h);
 out:
 	if (fn)
 		fn(data, err);
-- 
1.7.12.4

[PATCH 05/16] scsi_dh_alua: put sense buffer on stack

[Hannes Reinecke]

There is no need to have the sense buffer as part of the per-device
structure, we can put the sense buffer on the stack.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index ef92008..adc77ef 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -74,8 +74,6 @@ struct alua_dh_data {
 	unsigned char		*buff;
 	int			bufflen;
 	unsigned char		transition_tmo;
-	unsigned char		sense[SCSI_SENSE_BUFFERSIZE];
-	int			senselen;
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;
 	void			*callback_data;
@@ -325,6 +323,7 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int len, timeout = ALUA_FAILOVER_TIMEOUT;
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
 	unsigned retval;
 	unsigned char *d;
@@ -332,12 +331,12 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 
 	expiry = round_jiffies_up(jiffies + timeout);
  retry:
-	retval = submit_vpd_inquiry(sdev, h->buff, h->bufflen, h->sense);
+	retval = submit_vpd_inquiry(sdev, h->buff, h->bufflen, sense);
 	if (retval) {
 		unsigned err;
 
 		if (!(driver_byte(retval) & DRIVER_SENSE) ||
-		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+		    !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: evpd inquiry failed, ", ALUA_DH_NAME);
@@ -519,6 +518,7 @@ static int alua_check_sense(struct scsi_device *sdev,
  */
 static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_for_transition)
 {
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
@@ -533,11 +533,11 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h->buff, h->bufflen, h->sense, h->flags);
+	retval = submit_rtpg(sdev, h->buff, h->bufflen, sense, h->flags);
 
 	if (retval) {
 		if (!(driver_byte(retval) & DRIVER_SENSE) ||
-		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+		    !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev, "%s: rtpg failed, ",
 				    ALUA_DH_NAME);
-- 
1.7.12.4

[PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry

[Hannes Reinecke]

VPD inquiry need to be done only once, so we can be using
a local buffer here.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index adc77ef..88f98e0 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -322,16 +322,27 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
  */
 static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 {
+	unsigned char *buff;
+	unsigned char bufflen = 36;
 	int len, timeout = ALUA_FAILOVER_TIMEOUT;
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
 	unsigned retval;
 	unsigned char *d;
 	unsigned long expiry;
+	int err;
 
 	expiry = round_jiffies_up(jiffies + timeout);
  retry:
-	retval = submit_vpd_inquiry(sdev, h->buff, h->bufflen, sense);
+	buff = kmalloc(bufflen, GFP_KERNEL);
+	if (!buff) {
+		sdev_printk(KERN_WARNING, sdev,
+			    "%s: kmalloc buffer failed\n",
+			    ALUA_DH_NAME);
+		/* Temporary failure, bypass */
+		return SCSI_DH_DEV_TEMP_BUSY;
+	}
+	retval = submit_vpd_inquiry(sdev, buff, bufflen, sense);
 	if (retval) {
 		unsigned err;
 
@@ -345,6 +356,7 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 				err = SCSI_DH_DEV_TEMP_BUSY;
 			else
 				err = SCSI_DH_IO;
+			kfree(buff);
 			return err;
 		}
 		err = alua_check_sense(sdev, &sense_hdr);
@@ -362,24 +374,19 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 	}
 
 	/* Check if vpd page exceeds initial buffer */
-	len = (h->buff[2] << 8) + h->buff[3] + 4;
-	if (len > h->bufflen) {
+	len = (buff[2] << 8) + buff[3] + 4;
+	if (len > bufflen) {
 		/* Resubmit with the correct length */
-		if (realloc_buffer(h, len)) {
-			sdev_printk(KERN_WARNING, sdev,
-				    "%s: kmalloc buffer failed\n",
-				    ALUA_DH_NAME);
-			/* Temporary failure, bypass */
-			return SCSI_DH_DEV_TEMP_BUSY;
-		}
+		kfree(buff);
+		bufflen = len;
 		goto retry;
 	}
 
 	/*
 	 * Now look for the correct descriptor.
 	 */
-	d = h->buff + 4;
-	while (d < h->buff + len) {
+	d = buff + 4;
+	while (d < buff + len) {
 		switch (d[1] & 0xf) {
 		case 0x4:
 			/* Relative target port */
@@ -406,13 +413,15 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			    ALUA_DH_NAME);
 		h->state = TPGS_STATE_OPTIMIZED;
 		h->tpgs = TPGS_MODE_NONE;
-		return SCSI_DH_DEV_UNSUPP;
+		err = SCSI_DH_DEV_UNSUPP;
+	} else {
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: port group %02x rel port %02x\n",
+			    ALUA_DH_NAME, h->group_id, h->rel_port);
+		err = SCSI_DH_OK;
 	}
-	sdev_printk(KERN_INFO, sdev,
-		    "%s: port group %02x rel port %02x\n",
-		    ALUA_DH_NAME, h->group_id, h->rel_port);
-
-	return SCSI_DH_OK;
+	kfree(buff);
+	return err;
 }
 
 static char print_alua_state(int state)
-- 
1.7.12.4

[PATCH 07/16] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke]

The port group needs to be a separate structure as several
LUNs might belong to the same group.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 88f98e0..0af6866 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -63,9 +63,13 @@
 #define ALUA_OPTIMIZE_STPG		1
 #define ALUA_RTPG_EXT_HDR_UNSUPP	2
 
-struct alua_dh_data {
+static LIST_HEAD(port_group_list);
+static DEFINE_SPINLOCK(port_group_lock);
+
+struct alua_port_group {
+	struct kref		kref;
+	struct list_head	node;
 	int			group_id;
-	int			rel_port;
 	int			tpgs;
 	int			state;
 	int			pref;
@@ -74,6 +78,13 @@ struct alua_dh_data {
 	unsigned char		*buff;
 	int			bufflen;
 	unsigned char		transition_tmo;
+};
+
+struct alua_dh_data {
+	struct alua_port_group	*pg;
+	int			rel_port;
+	int			tpgs;
+	unsigned		flags; /* used for optimizing STPG */
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;
 	void			*callback_data;
@@ -92,18 +103,18 @@ static inline struct alua_dh_data *get_alua_data(struct scsi_device *sdev)
 	return ((struct alua_dh_data *) scsi_dh_data->buf);
 }
 
-static int realloc_buffer(struct alua_dh_data *h, unsigned len)
+static int realloc_buffer(struct alua_port_group *pg, unsigned len)
 {
-	if (h->buff && h->buff != h->inq)
-		kfree(h->buff);
+	if (pg->buff && pg->buff != pg->inq)
+		kfree(pg->buff);
 
-	h->buff = kmalloc(len, GFP_NOIO);
-	if (!h->buff) {
-		h->buff = h->inq;
-		h->bufflen = ALUA_INQUIRY_SIZE;
+	pg->buff = kmalloc(len, GFP_NOIO);
+	if (!pg->buff) {
+		pg->buff = pg->inq;
+		pg->bufflen = ALUA_INQUIRY_SIZE;
 		return 1;
 	}
-	h->bufflen = len;
+	pg->bufflen = len;
 	return 0;
 }
 
@@ -137,6 +148,20 @@ static struct request *get_alua_req(struct scsi_device *sdev,
 	return rq;
 }
 
+static void release_port_group(struct kref *kref)
+{
+	struct alua_port_group *pg;
+
+	pg = container_of(kref, struct alua_port_group, kref);
+	printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
+	spin_lock(&port_group_lock);
+	list_del(&pg->node);
+	spin_unlock(&port_group_lock);
+	if (pg->buff && pg->inq != pg->buff)
+		kfree(pg->buff);
+	kfree(pg);
+}
+
 /*
  * submit_vpd_inquiry - Issue an INQUIRY VPD page 0x83 command
  * @sdev: sdev the command should be sent to
@@ -327,10 +352,11 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 	int len, timeout = ALUA_FAILOVER_TIMEOUT;
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
-	unsigned retval;
+	unsigned retval, err;
+	int group_id = -1;
 	unsigned char *d;
 	unsigned long expiry;
-	int err;
+	struct alua_port_group *pg = NULL;
 
 	expiry = round_jiffies_up(jiffies + timeout);
  retry:
@@ -344,8 +370,6 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 	}
 	retval = submit_vpd_inquiry(sdev, buff, bufflen, sense);
 	if (retval) {
-		unsigned err;
-
 		if (!(driver_byte(retval) & DRIVER_SENSE) ||
 		    !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {
@@ -356,8 +380,7 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 				err = SCSI_DH_DEV_TEMP_BUSY;
 			else
 				err = SCSI_DH_IO;
-			kfree(buff);
-			return err;
+			goto out;
 		}
 		err = alua_check_sense(sdev, &sense_hdr);
 		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
@@ -369,7 +392,8 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: evpd inquiry failed, ", ALUA_DH_NAME);
 			scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
-			return SCSI_DH_IO;
+			err = SCSI_DH_IO;
+			goto out;
 		}
 	}
 
@@ -394,7 +418,7 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			break;
 		case 0x5:
 			/* Target port group */
-			h->group_id = (d[6] << 8) + d[7];
+			group_id = (d[6] << 8) + d[7];
 			break;
 		default:
 			break;
@@ -402,7 +426,7 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 		d += d[3] + 4;
 	}
 
-	if (h->group_id == -1) {
+	if (group_id == -1) {
 		/*
 		 * Internal error; TPGS supported but required
 		 * VPD identification descriptors not present.
@@ -411,15 +435,37 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: No target port descriptors found\n",
 			    ALUA_DH_NAME);
-		h->state = TPGS_STATE_OPTIMIZED;
 		h->tpgs = TPGS_MODE_NONE;
 		err = SCSI_DH_DEV_UNSUPP;
-	} else {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: port group %02x rel port %02x\n",
-			    ALUA_DH_NAME, h->group_id, h->rel_port);
-		err = SCSI_DH_OK;
+		goto out;
 	}
+
+	sdev_printk(KERN_INFO, sdev,
+		    "%s: port group %02x rel port %02x\n",
+		    ALUA_DH_NAME, group_id, h->rel_port);
+	spin_lock(&port_group_lock);
+	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
+	if (!pg) {
+		sdev_printk(KERN_WARNING, sdev,
+			    "%s: kzalloc port group failed\n",
+			    ALUA_DH_NAME);
+		/* Temporary failure, bypass */
+		spin_unlock(&port_group_lock);
+		err = SCSI_DH_DEV_TEMP_BUSY;
+		goto out;
+	}
+	pg->group_id = group_id;
+	pg->buff = pg->inq;
+	pg->bufflen = ALUA_INQUIRY_SIZE;
+	pg->tpgs = h->tpgs;
+	pg->state = TPGS_STATE_OPTIMIZED;
+	pg->flags = h->flags;
+	kref_init(&pg->kref);
+	list_add(&pg->node, &port_group_list);
+	h->pg = pg;
+	spin_unlock(&port_group_lock);
+	err = SCSI_DH_OK;
+out:
 	kfree(buff);
 	return err;
 }
@@ -525,7 +571,7 @@ static int alua_check_sense(struct scsi_device *sdev,
  * Returns SCSI_DH_DEV_OFFLINED if the path is
  * found to be unusable.
  */
-static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_for_transition)
+static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int wait_for_transition)
 {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
@@ -536,13 +582,13 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 
-	if (!h->transition_tmo)
+	if (!pg->transition_tmo)
 		expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT * HZ);
 	else
-		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
+		expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h->buff, h->bufflen, sense, h->flags);
+	retval = submit_rtpg(sdev, pg->buff, pg->bufflen, sense, pg->flags);
 
 	if (retval) {
 		if (!(driver_byte(retval) & DRIVER_SENSE) ||
@@ -566,10 +612,10 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		 * The retry without rtpg_ext_hdr_req set
 		 * handles this.
 		 */
-		if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&
+		if (!(pg->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&
 		    sense_hdr.sense_key == ILLEGAL_REQUEST &&
 		    sense_hdr.asc == 0x24 && sense_hdr.ascq == 0) {
-			h->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
+			pg->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
 			goto retry;
 		}
 
@@ -592,12 +638,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		return SCSI_DH_IO;
 	}
 
-	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
-		(h->buff[2] << 8) + h->buff[3] + 4;
+	len = (pg->buff[0] << 24) + (pg->buff[1] << 16) +
+		(pg->buff[2] << 8) + pg->buff[3] + 4;
 
-	if (len > h->bufflen) {
+	if (len > pg->bufflen) {
 		/* Resubmit with the correct length */
-		if (realloc_buffer(h, len)) {
+		if (realloc_buffer(pg, len)) {
 			sdev_printk(KERN_WARNING, sdev,
 				    "%s: kmalloc buffer failed\n",__func__);
 			/* Temporary failure, bypass */
@@ -606,31 +652,32 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		goto retry;
 	}
 
-	orig_transition_tmo = h->transition_tmo;
-	if ((h->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR && h->buff[5] != 0)
-		h->transition_tmo = h->buff[5];
+	orig_transition_tmo = pg->transition_tmo;
+	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR &&
+	    pg->buff[5] != 0)
+		pg->transition_tmo = pg->buff[5];
 	else
-		h->transition_tmo = ALUA_FAILOVER_TIMEOUT;
+		pg->transition_tmo = ALUA_FAILOVER_TIMEOUT;
 
-	if (wait_for_transition && (orig_transition_tmo != h->transition_tmo)) {
+	if (wait_for_transition && (orig_transition_tmo != pg->transition_tmo)) {
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: transition timeout set to %d seconds\n",
-			    ALUA_DH_NAME, h->transition_tmo);
-		expiry = jiffies + h->transition_tmo * HZ;
+			    ALUA_DH_NAME, pg->transition_tmo);
+		expiry = jiffies + pg->transition_tmo * HZ;
 	}
 
-	if ((h->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
+	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
 		tpg_desc_tbl_off = 8;
 	else
 		tpg_desc_tbl_off = 4;
 
-	for (k = tpg_desc_tbl_off, ucp = h->buff + tpg_desc_tbl_off;
+	for (k = tpg_desc_tbl_off, ucp = pg->buff + tpg_desc_tbl_off;
 	     k < len;
 	     k += off, ucp += off) {
 
-		if (h->group_id == (ucp[2] << 8) + ucp[3]) {
-			h->state = ucp[0] & 0x0f;
-			h->pref = ucp[0] >> 7;
+		if (pg->group_id == (ucp[2] << 8) + ucp[3]) {
+			pg->state = ucp[0] & 0x0f;
+			pg->pref = ucp[0] >> 7;
 			valid_states = ucp[1];
 		}
 		off = 8 + (ucp[7] * 4);
@@ -638,8 +685,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 
 	sdev_printk(KERN_INFO, sdev,
 		    "%s: port group %02x state %c %s supports %c%c%c%c%c%c%c\n",
-		    ALUA_DH_NAME, h->group_id, print_alua_state(h->state),
-		    h->pref ? "preferred" : "non-preferred",
+		    ALUA_DH_NAME, pg->group_id, print_alua_state(pg->state),
+		    pg->pref ? "preferred" : "non-preferred",
 		    valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
 		    valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
 		    valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
@@ -648,7 +695,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		    valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
 		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
 
-	switch (h->state) {
+	switch (pg->state) {
 	case TPGS_STATE_TRANSITIONING:
 		if (wait_for_transition) {
 			if (time_before(jiffies, expiry)) {
@@ -663,7 +710,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		}
 
 		/* Transitioning time exceeded, set port to standby */
-		h->state = TPGS_STATE_STANDBY;
+		pg->state = TPGS_STATE_STANDBY;
 		break;
 	case TPGS_STATE_OFFLINE:
 		/* Path unusable */
@@ -684,21 +731,21 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
  * response. Returns SCSI_DH_RETRY per default to trigger
  * a re-evaluation of the target group state.
  */
-static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	int retval, err = SCSI_DH_RETRY;
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
 
-	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
+	if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
 		/* Only implicit ALUA supported, retry */
 		return SCSI_DH_RETRY;
 	}
-	switch (h->state) {
+	switch (pg->state) {
 	case TPGS_STATE_NONOPTIMIZED:
-		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
-		    (!h->pref) &&
-		    (h->tpgs & TPGS_MODE_IMPLICIT))
+		if ((pg->flags & ALUA_OPTIMIZE_STPG) &&
+		    (!pg->pref) &&
+		    (pg->tpgs & TPGS_MODE_IMPLICIT))
 			return SCSI_DH_OK;
 		break;
 	case TPGS_STATE_STANDBY:
@@ -715,8 +762,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		break;
 	}
 	/* Set state to transitioning */
-	h->state = TPGS_STATE_TRANSITIONING;
-	retval = submit_stpg(sdev, h->group_id, sense);
+	pg->state = TPGS_STATE_TRANSITIONING;
+	retval = submit_stpg(sdev, pg->group_id, sense);
 
 	if (retval) {
 		if (!(driver_byte(retval) & DRIVER_SENSE) ||
@@ -728,11 +775,11 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 			/* Retry RTPG */
 			return err;
 		}
-		err = alua_check_sense(h->sdev, &sense_hdr);
-		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed, ",
+		err = alua_check_sense(sdev, &sense_hdr);
+		sdev_printk(KERN_INFO, sdev, "%s: stpg failed, ",
 			    ALUA_DH_NAME);
 		scsi_show_sense_hdr(&sense_hdr);
-		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed, ",
+		sdev_printk(KERN_INFO, sdev, "%s: stpg failed, ",
 			    ALUA_DH_NAME);
 		scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
 		err = SCSI_DH_RETRY;
@@ -756,16 +803,16 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 		goto out;
 
 	err = alua_vpd_inquiry(sdev, h);
-	if (err != SCSI_DH_OK)
-		goto out;
-
-	err = alua_rtpg(sdev, h, 0);
-	if (err != SCSI_DH_OK)
+	if (err != SCSI_DH_OK || !h->pg)
 		goto out;
 
+	kref_get(&h->pg->kref);
+	err = alua_rtpg(sdev, h->pg);
+	kref_put(&h->pg->kref, release_port_group);
 out:
 	return err;
 }
+
 /*
  * alua_set_params - set/unset the optimize flag
  * @sdev: device on the path to be activated
@@ -818,16 +865,22 @@ static int alua_activate(struct scsi_device *sdev,
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int err = SCSI_DH_OK;
 
-	err = alua_rtpg(sdev, h, 1);
-	if (err != SCSI_DH_OK)
+	if (!h->pg)
 		goto out;
 
 	if (optimize_stpg)
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
-	err = alua_stpg(sdev, h);
+	kref_get(&h->pg->kref);
+	err = alua_rtpg(sdev, h->pg);
+	if (err != SCSI_DH_OK) {
+		kref_put(&h->pg->kref, release_port_group);
+		goto out;
+	}
+	err = alua_stpg(sdev, h->pg);
 	if (err == SCSI_DH_RETRY)
-		err = alua_rtpg(sdev, h);
+		err = alua_rtpg(sdev, h->pg);
+	kref_put(&h->pg->kref, release_port_group);
 out:
 	if (fn)
 		fn(data, err);
@@ -843,13 +896,19 @@ out:
 static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 {
 	struct alua_dh_data *h = get_alua_data(sdev);
+	int state;
 	int ret = BLKPREP_OK;
 
-	if (h->state == TPGS_STATE_TRANSITIONING)
+	if (!h->pg)
+		return ret;
+	kref_get(&h->pg->kref);
+	state = h->pg->state;
+	kref_put(&h->pg->kref, release_port_group);
+	if (state == TPGS_STATE_TRANSITIONING)
 		ret = BLKPREP_DEFER;
-	else if (h->state != TPGS_STATE_OPTIMIZED &&
-		 h->state != TPGS_STATE_NONOPTIMIZED &&
-		 h->state != TPGS_STATE_LBA_DEPENDENT) {
+	else if (state != TPGS_STATE_OPTIMIZED &&
+		 state != TPGS_STATE_NONOPTIMIZED &&
+		 state != TPGS_STATE_LBA_DEPENDENT) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}
@@ -899,11 +958,8 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	scsi_dh_data->scsi_dh = &alua_dh;
 	h = (struct alua_dh_data *) scsi_dh_data->buf;
 	h->tpgs = TPGS_MODE_UNINITIALIZED;
-	h->state = TPGS_STATE_OPTIMIZED;
-	h->group_id = -1;
+	h->pg = NULL;
 	h->rel_port = -1;
-	h->buff = h->inq;
-	h->bufflen = ALUA_INQUIRY_SIZE;
 	h->sdev = sdev;
 
 	err = alua_initialize(sdev, h);
@@ -942,8 +998,10 @@ static void alua_bus_detach(struct scsi_device *sdev)
 	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
 	h = (struct alua_dh_data *) scsi_dh_data->buf;
-	if (h->buff && h->inq != h->buff)
-		kfree(h->buff);
+	if (h->pg) {
+		kref_put(&h->pg->kref, release_port_group);
+		h->pg = NULL;
+	}
 	kfree(scsi_dh_data);
 	module_put(THIS_MODULE);
 	sdev_printk(KERN_NOTICE, sdev, "%s: Detached\n", ALUA_DH_NAME);
-- 
1.7.12.4

[PATCH 08/16] scsi_dh_alua: parse target device id

[Hannes Reinecke]

VPD descriptor association 0x2 in VPD page 0x83 identification
descrioptors can be used to identify the array / target device.
Some tricks need to be taken for EMC and HP, which put the
array identification into the standard inquiry.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 0af6866..857a999 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -69,6 +69,9 @@ static DEFINE_SPINLOCK(port_group_lock);
 struct alua_port_group {
 	struct kref		kref;
 	struct list_head	node;
+	unsigned char		target_id[256];
+	unsigned char		target_id_str[256];
+	int			target_id_size;
 	int			group_id;
 	int			tpgs;
 	int			state;
@@ -351,12 +354,14 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 	unsigned char bufflen = 36;
 	int len, timeout = ALUA_FAILOVER_TIMEOUT;
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	char target_id_str[256], *target_id = NULL;
+	int target_id_size;
 	struct scsi_sense_hdr sense_hdr;
 	unsigned retval, err;
 	int group_id = -1;
 	unsigned char *d;
 	unsigned long expiry;
-	struct alua_port_group *pg = NULL;
+	struct alua_port_group *tmp_pg, *pg = NULL;
 
 	expiry = round_jiffies_up(jiffies + timeout);
  retry:
@@ -409,9 +414,54 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 	/*
 	 * Now look for the correct descriptor.
 	 */
+	memset(target_id_str, 0, 256);
+	target_id_size = 0;
 	d = buff + 4;
 	while (d < buff + len) {
 		switch (d[1] & 0xf) {
+		case 0x2:
+			/* EUI-64 */
+			if ((d[1] & 0x30) == 0x20) {
+				target_id_size = d[3];
+				target_id = d + 4;
+				switch (target_id_size) {
+				case 8:
+					sprintf(target_id_str,
+						"eui.%8phN", d + 4);
+					break;
+				case 12:
+					sprintf(target_id_str,
+						"eui.%12phN", d + 4);
+					break;
+				case 16:
+					sprintf(target_id_str,
+						"eui.%16phN", d + 4);
+					break;
+				default:
+					target_id_size = 0;
+					break;
+				}
+			}
+			break;
+		case 0x3:
+			/* NAA */
+			if ((d[1] & 0x30) == 0x20) {
+				target_id_size = d[3];
+				target_id = d + 4;
+				switch (target_id_size) {
+				case 8:
+					sprintf(target_id_str,
+						"naa.%8phN", d + 4);
+					break;
+				case 16:
+					sprintf(target_id_str,
+						"naa.%16phN", d + 4);
+					break;
+				default:
+					target_id_size = 0;
+					break;
+				}
+			}
 		case 0x4:
 			/* Relative target port */
 			h->rel_port = (d[6] << 8) + d[7];
@@ -420,6 +470,18 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			/* Target port group */
 			group_id = (d[6] << 8) + d[7];
 			break;
+		case 0x8:
+			/* SCSI name string */
+			if ((d[1] & 0x30) == 0x20) {
+				/* SCSI name */
+				target_id_size = d[3];
+				target_id = d + 4;
+				strncpy(target_id_str, d + 4, 256);
+				if (target_id_size > 255)
+					target_id_size = 255;
+				target_id_str[target_id_size] = '\0';
+			}
+			break;
 		default:
 			break;
 		}
@@ -439,11 +501,56 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 		err = SCSI_DH_DEV_UNSUPP;
 		goto out;
 	}
+	if (!target_id_size) {
+		/* Check for EMC Clariion extended inquiry */
+		if (!strncmp(sdev->vendor, "DGC     ", 8) &&
+		    sdev->inquiry_len > 160) {
+			target_id_size = sdev->inquiry[160];
+			target_id = sdev->inquiry + 161;
+			strcpy(target_id_str, "emc.");
+			memcpy(target_id_str + 4, target_id, target_id_size);
+		}
+		/* Check for HP EVA extended inquiry */
+		if (!strncmp(sdev->vendor, "HP      ", 8) &&
+		    !strncmp(sdev->model, "HSV", 3) &&
+		    sdev->inquiry_len > 170) {
+			target_id_size = 16;
+			target_id = sdev->inquiry + 154;
+			strcpy(target_id_str, "naa.");
+			memcpy(target_id_str + 4, target_id, target_id_size);
+		}
+	}
 
-	sdev_printk(KERN_INFO, sdev,
-		    "%s: port group %02x rel port %02x\n",
-		    ALUA_DH_NAME, group_id, h->rel_port);
 	spin_lock(&port_group_lock);
+	pg = NULL;
+	if (target_id_size) {
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: target %s port group %02x "
+			    "rel port %02x\n", ALUA_DH_NAME,
+			    target_id_str, group_id, h->rel_port);
+		list_for_each_entry(tmp_pg, &port_group_list, node) {
+			if (tmp_pg->group_id != group_id)
+				continue;
+			if (tmp_pg->target_id_size != target_id_size)
+				continue;
+			if (memcmp(tmp_pg->target_id, target_id,
+				   target_id_size))
+				continue;
+			pg = tmp_pg;
+			break;
+		}
+	} else {
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: port group %02x rel port %02x\n",
+			    ALUA_DH_NAME, group_id, h->rel_port);
+	}
+	if (pg) {
+		h->pg = pg;
+		kref_get(&pg->kref);
+		spin_unlock(&port_group_lock);
+		err = SCSI_DH_OK;
+		goto out;
+	}
 	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
 	if (!pg) {
 		sdev_printk(KERN_WARNING, sdev,
@@ -454,6 +561,14 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 		err = SCSI_DH_DEV_TEMP_BUSY;
 		goto out;
 	}
+	if (target_id_size) {
+		memcpy(pg->target_id, target_id, target_id_size);
+		strncpy(pg->target_id_str, target_id_str, 256);
+	} else {
+		memset(pg->target_id, 0, 256);
+		pg->target_id_str[0] = '\0';
+	}
+	pg->target_id_size = target_id_size;
 	pg->group_id = group_id;
 	pg->buff = pg->inq;
 	pg->bufflen = ALUA_INQUIRY_SIZE;
-- 
1.7.12.4

[PATCH 09/16] scsi_dh_alua: simplify sense code handling

[Hannes Reinecke]

Most sense code is already handled in the generic
code, so we shouldn't be adding special cases here.
However, when doing so we need to check for
unit attention whenever we're sending an internal
command.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 857a999..174ff45 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -388,6 +388,8 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			goto out;
 		}
 		err = alua_check_sense(sdev, &sense_hdr);
+		if (sense_hdr.sense_key == UNIT_ATTENTION)
+			err = ADD_TO_MLQUEUE;
 		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
 			goto retry;
 		if (err != SUCCESS) {
@@ -617,21 +619,6 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * LUN Not Accessible - ALUA state transition
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0b)
-			/*
-			 * LUN Not Accessible -- Target port in standby state
-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0c)
-			/*
-			 * LUN Not Accessible -- Target port in unavailable state
-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x12)
-			/*
-			 * LUN Not Ready -- Offline
-			 */
-			return SUCCESS;
 		break;
 	case UNIT_ATTENTION:
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
@@ -646,7 +633,7 @@ static int alua_check_sense(struct scsi_device *sdev,
 			return ADD_TO_MLQUEUE;
 		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x01)
 			/*
-			 * Mode Parameters Changed
+			 * Mode parameter changed
 			 */
 			return ADD_TO_MLQUEUE;
 		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06)
@@ -659,18 +646,6 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * Implicit ALUA state transition failed
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x03)
-			/*
-			 * Inquiry data has changed
-			 */
-			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x0e)
-			/*
-			 * REPORTED_LUNS_DATA_HAS_CHANGED is reported
-			 * when switching controllers on targets like
-			 * Intel Multi-Flex. We can just retry.
-			 */
-			return ADD_TO_MLQUEUE;
 		break;
 	}
 
@@ -735,6 +710,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		}
 
 		err = alua_check_sense(sdev, &sense_hdr);
+		if (sense_hdr.sense_key == UNIT_ATTENTION)
+			err = ADD_TO_MLQUEUE;
 		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry, ",
 				    ALUA_DH_NAME);
-- 
1.7.12.4

[PATCH 10/16] scsi_dh_alua: Do not attach to management devices

[Hannes Reinecke]

Management devices should be ignored when
detecting ALUA capabilites.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 174ff45..a1c69bb 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -315,6 +315,23 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int err = SCSI_DH_OK;
 
+	if (scsi_is_wlun(sdev->lun)) {
+		h->tpgs = TPGS_MODE_NONE;
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: disable for WLUN\n",
+			    ALUA_DH_NAME);
+		return SCSI_DH_DEV_UNSUPP;
+	}
+	if (sdev->type == TYPE_RAID ||
+	    sdev->type == TYPE_ENCLOSURE ||
+	    sdev->type == 0x1F) {
+		h->tpgs = TPGS_MODE_NONE;
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: disable for enclosure devices\n",
+			    ALUA_DH_NAME);
+		return SCSI_DH_DEV_UNSUPP;
+	}
+
 	h->tpgs = scsi_device_tpgs(sdev);
 	switch (h->tpgs) {
 	case TPGS_MODE_EXPLICIT|TPGS_MODE_IMPLICIT:
-- 
1.7.12.4

[PATCH 11/16] scsi_dh_alua: multipath failover fails with error 15

[Hannes Reinecke]

When a path is already optimized multipath failover will fail
with the message
Could not failover device X:Y: Handler scsi_dh_alua Error 15

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index a1c69bb..8ea35a9 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -851,6 +851,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		return SCSI_DH_RETRY;
 	}
 	switch (pg->state) {
+	case TPGS_STATE_OPTIMIZED:
+		return SCSI_DH_OK;
 	case TPGS_STATE_NONOPTIMIZED:
 		if ((pg->flags & ALUA_OPTIMIZE_STPG) &&
 		    (!pg->pref) &&
@@ -865,10 +867,11 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		break;
 	case TPGS_STATE_TRANSITIONING:
 		return SCSI_DH_RETRY;
-		break;
 	default:
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: stpg failed, unhandled TPGS state %d",
+			    ALUA_DH_NAME, pg->state);
 		return SCSI_DH_NOSYS;
-		break;
 	}
 	/* Set state to transitioning */
 	pg->state = TPGS_STATE_TRANSITIONING;
-- 
1.7.12.4

[PATCH 12/16] scsi_dh: return individual errors in scsi_dh_activate()

[Hannes Reinecke]

When calling scsi_dh_activate() we should be returning
individual errors and not lumping all into one.

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

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 33e422e..ae7f399 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -381,7 +381,7 @@ EXPORT_SYMBOL_GPL(scsi_unregister_device_handler);
  */
 int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 {
-	int err = 0;
+	int err = SCSI_DH_OK;
 	unsigned long flags;
 	struct scsi_device *sdev;
 	struct scsi_device_handler *scsi_dh = NULL;
@@ -400,15 +400,18 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 	if (sdev->scsi_dh_data)
 		scsi_dh = sdev->scsi_dh_data->scsi_dh;
 	dev = get_device(&sdev->sdev_gendev);
-	if (!scsi_dh || !dev ||
-	    sdev->sdev_state == SDEV_CANCEL ||
-	    sdev->sdev_state == SDEV_DEL)
+	if (!scsi_dh)
 		err = SCSI_DH_NOSYS;
-	if (sdev->sdev_state == SDEV_OFFLINE)
+	else if (!dev)
+		err = SCSI_DH_DEV_OFFLINED;
+	else if (sdev->sdev_state == SDEV_CANCEL ||
+		 sdev->sdev_state == SDEV_DEL)
+		err = SCSI_DH_NOTCONN;
+	else if (sdev->sdev_state == SDEV_OFFLINE)
 		err = SCSI_DH_DEV_OFFLINED;
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
-	if (err) {
+	if (err != SCSI_DH_OK) {
 		if (fn)
 			fn(data, err);
 		goto out;
@@ -417,7 +420,8 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 	if (scsi_dh->activate)
 		err = scsi_dh->activate(sdev, fn, data);
 out:
-	put_device(dev);
+	if (dev)
+		put_device(dev);
 	return err;
 }
 EXPORT_SYMBOL_GPL(scsi_dh_activate);
-- 
1.7.12.4

[PATCH 13/16] scsi_dh_alua: Clarify logging message

[Hannes Reinecke]

We should be diffentiating between an invalid TPGS setting
and unsupported.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8ea35a9..7f03417 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -347,12 +347,18 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 		sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n",
 			    ALUA_DH_NAME);
 		break;
-	default:
-		h->tpgs = TPGS_MODE_NONE;
+	case 0:
 		sdev_printk(KERN_INFO, sdev, "%s: not supported\n",
 			    ALUA_DH_NAME);
 		err = SCSI_DH_DEV_UNSUPP;
 		break;
+	default:
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: unsupported TPGS setting %d\n",
+			    ALUA_DH_NAME, h->tpgs);
+		h->tpgs = TPGS_MODE_NONE;
+		err = SCSI_DH_DEV_UNSUPP;
+		break;
 	}
 
 	return err;
-- 
1.7.12.4

[PATCH 14/16] scsi_dh: invoke callback if ->activate is not present

[Hannes Reinecke]

When ->activate isn't present we still need to invoke the
callbacks, otherwise the system might stall.

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

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index ae7f399..a90380f 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -411,7 +411,7 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 		err = SCSI_DH_DEV_OFFLINED;
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
-	if (err != SCSI_DH_OK) {
+	if (err != SCSI_DH_OK || !scsi_dh->activate) {
 		if (fn)
 			fn(data, err);
 		goto out;
-- 
1.7.12.4

[PATCH 15/16] scsi_dh_alua: revert commit a8e5a2d593cbfccf530c3382c2c328d2edaa7b66

[Hannes Reinecke]

Obsoleted by the next patch.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 7f03417..6591ac1 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -678,13 +678,12 @@ static int alua_check_sense(struct scsi_device *sdev,
 /*
  * alua_rtpg - Evaluate REPORT TARGET GROUP STATES
  * @sdev: the device to be evaluated.
- * @wait_for_transition: if nonzero, wait ALUA_FAILOVER_TIMEOUT seconds for device to exit transitioning state
  *
  * Evaluate the Target Port Group State.
  * Returns SCSI_DH_DEV_OFFLINED if the path is
  * found to be unusable.
  */
-static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int wait_for_transition)
+static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
@@ -774,7 +773,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 	else
 		pg->transition_tmo = ALUA_FAILOVER_TIMEOUT;
 
-	if (wait_for_transition && (orig_transition_tmo != pg->transition_tmo)) {
+	if (orig_transition_tmo != pg->transition_tmo) {
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: transition timeout set to %d seconds\n",
 			    ALUA_DH_NAME, pg->transition_tmo);
@@ -812,19 +811,14 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 
 	switch (pg->state) {
 	case TPGS_STATE_TRANSITIONING:
-		if (wait_for_transition) {
-			if (time_before(jiffies, expiry)) {
-				/* State transition, retry */
-				interval += 2000;
-				msleep(interval);
-				goto retry;
-			}
-			err = SCSI_DH_RETRY;
-		} else {
-			err = SCSI_DH_OK;
+		if (time_before(jiffies, expiry)) {
+			/* State transition, retry */
+			interval += 2000;
+			msleep(interval);
+			goto retry;
 		}
-
 		/* Transitioning time exceeded, set port to standby */
+		err = SCSI_DH_RETRY;
 		pg->state = TPGS_STATE_STANDBY;
 		break;
 	case TPGS_STATE_OFFLINE:
-- 
1.7.12.4

[PATCH 16/16] scsi_dh_alua: Use workqueue for RTPG

[Hannes Reinecke]

The current ALUA device_handler has two drawbacks:
- We're sending a 'SET TARGET PORT GROUP' command to every LUN,
  disregarding the fact that several LUNs might be in a port group
  and will be automatically switched whenever _any_ LUN within
  that port group receives the command.
- Whenever a LUN is in 'transitioning' mode we cannot block I/O
  to that LUN, instead the controller has to abort the command.
  This leads to increased traffic across the wire and heavy load
  on the controller during switchover.

With this patch the RTPG handling is moved to a workqueue, which
is being run once per port group. This reduces the number of
'REPORT TARGET PORT GROUP' and 'SET TARGET PORT GROUPS' which
will be send to the controller. It also allows us to block
I/O to any LUN / port group found to be in 'transitioning' ALUA
mode, as the workqueue item will be requeued until the controller
moves out of transitioning.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 6591ac1..99649b9 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -22,6 +22,8 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/rcupdate.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>
@@ -58,13 +60,20 @@
 #define ALUA_INQUIRY_SIZE		36
 #define ALUA_FAILOVER_TIMEOUT		60
 #define ALUA_FAILOVER_RETRIES		5
+#define ALUA_RTPG_DELAY_MSECS		5
 
 /* flags passed from user level */
-#define ALUA_OPTIMIZE_STPG		1
-#define ALUA_RTPG_EXT_HDR_UNSUPP	2
+#define ALUA_OPTIMIZE_STPG		0x01
+#define ALUA_RTPG_EXT_HDR_UNSUPP	0x02
+/* State machine flags */
+#define ALUA_PG_RUN_RTPG		0x10
+#define ALUA_PG_RUN_STPG		0x20
+#define ALUA_PG_STPG_DONE		0x40
+
 
 static LIST_HEAD(port_group_list);
 static DEFINE_SPINLOCK(port_group_lock);
+static struct workqueue_struct *kmpath_aluad;
 
 struct alua_port_group {
 	struct kref		kref;
@@ -81,14 +90,26 @@ struct alua_port_group {
 	unsigned char		*buff;
 	int			bufflen;
 	unsigned char		transition_tmo;
+	unsigned long		expiry;
+	unsigned long		interval;
+	struct delayed_work	rtpg_work;
+	spinlock_t		rtpg_lock;
+	struct list_head	rtpg_list;
+	struct scsi_device	*rtpg_sdev;
 };
 
 struct alua_dh_data {
 	struct alua_port_group	*pg;
+	spinlock_t		pg_lock;
 	int			rel_port;
 	int			tpgs;
+	int			error;
 	unsigned		flags; /* used for optimizing STPG */
-	struct scsi_device	*sdev;
+	struct completion       init_complete;
+};
+
+struct alua_queue_data {
+	struct list_head	entry;
 	activate_complete	callback_fn;
 	void			*callback_data;
 };
@@ -98,11 +119,13 @@ struct alua_dh_data {
 
 static char print_alua_state(int);
 static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);
+static void alua_rtpg_work(struct work_struct *work);
+static void alua_check(struct scsi_device *sdev);
 
 static inline struct alua_dh_data *get_alua_data(struct scsi_device *sdev)
 {
 	struct scsi_dh_data *scsi_dh_data = sdev->scsi_dh_data;
-	BUG_ON(scsi_dh_data == NULL);
+
 	return ((struct alua_dh_data *) scsi_dh_data->buf);
 }
 
@@ -570,9 +593,12 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			    ALUA_DH_NAME, group_id, h->rel_port);
 	}
 	if (pg) {
-		h->pg = pg;
 		kref_get(&pg->kref);
 		spin_unlock(&port_group_lock);
+		spin_lock(&h->pg_lock);
+		rcu_assign_pointer(h->pg, pg);
+		spin_unlock(&h->pg_lock);
+		synchronize_rcu();
 		err = SCSI_DH_OK;
 		goto out;
 	}
@@ -601,9 +627,17 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 	pg->state = TPGS_STATE_OPTIMIZED;
 	pg->flags = h->flags;
 	kref_init(&pg->kref);
+	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
+	INIT_LIST_HEAD(&pg->rtpg_list);
+	spin_lock_init(&pg->rtpg_lock);
 	list_add(&pg->node, &port_group_list);
-	h->pg = pg;
+	kref_get(&pg->kref);
 	spin_unlock(&port_group_lock);
+	spin_lock(&h->pg_lock);
+	rcu_assign_pointer(h->pg, pg);
+	spin_unlock(&h->pg_lock);
+	kref_put(&pg->kref, release_port_group);
+	synchronize_rcu();
 	err = SCSI_DH_OK;
 out:
 	kfree(buff);
@@ -637,11 +671,14 @@ static int alua_check_sense(struct scsi_device *sdev,
 {
 	switch (sense_hdr->sense_key) {
 	case NOT_READY:
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0a)
+		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0a) {
 			/*
 			 * LUN Not Accessible - ALUA state transition
+			 * Kickoff worker to update internal state.
 			 */
+			alua_check(sdev);
 			return ADD_TO_MLQUEUE;
+		}
 		break;
 	case UNIT_ATTENTION:
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
@@ -690,15 +727,15 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
 	unsigned err, retval;
-	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 
-	if (!pg->transition_tmo)
-		expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT * HZ);
-	else
-		expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
-
+	if (!pg->expiry) {
+		if (!pg->transition_tmo)
+			pg->expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT * HZ);
+		else
+			pg->expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
+	}
  retry:
 	retval = submit_rtpg(sdev, pg->buff, pg->bufflen, sense, pg->flags);
 
@@ -713,6 +750,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 				err = SCSI_DH_DEV_TEMP_BUSY;
 			else
 				err = SCSI_DH_IO;
+			pg->expiry = 0;
 			return err;
 		}
 
@@ -734,14 +772,15 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		err = alua_check_sense(sdev, &sense_hdr);
 		if (sense_hdr.sense_key == UNIT_ATTENTION)
 			err = ADD_TO_MLQUEUE;
-		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
+		if (err == ADD_TO_MLQUEUE &&
+		    pg->expiry != 0 && time_before(jiffies, pg->expiry)) {
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry, ",
 				    ALUA_DH_NAME);
 			scsi_show_sense_hdr(&sense_hdr);
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry, ",
 				    ALUA_DH_NAME);
 			scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
-			goto retry;
+			return SCSI_DH_RETRY;
 		}
 		sdev_printk(KERN_INFO, sdev, "%s: rtpg failed, ",
 			    ALUA_DH_NAME);
@@ -749,6 +788,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		sdev_printk(KERN_INFO, sdev, "%s: rtpg failed, ",
 			    ALUA_DH_NAME);
 		scsi_show_extd_sense(sense_hdr.asc, sense_hdr.ascq);
+		pg->expiry = 0;
 		return SCSI_DH_IO;
 	}
 
@@ -761,6 +801,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			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;
@@ -774,10 +815,10 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		pg->transition_tmo = ALUA_FAILOVER_TIMEOUT;
 
 	if (orig_transition_tmo != pg->transition_tmo) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: transition timeout set to %d seconds\n",
-			    ALUA_DH_NAME, pg->transition_tmo);
-		expiry = jiffies + pg->transition_tmo * HZ;
+		printk(KERN_INFO
+		       "%s: target %s transition timeout set to %d seconds\n",
+		       ALUA_DH_NAME, pg->target_id_str, pg->transition_tmo);
+		pg->expiry = jiffies + pg->transition_tmo * HZ;
 	}
 
 	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
@@ -797,37 +838,40 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		off = 8 + (ucp[7] * 4);
 	}
 
-	sdev_printk(KERN_INFO, sdev,
-		    "%s: port group %02x state %c %s supports %c%c%c%c%c%c%c\n",
-		    ALUA_DH_NAME, pg->group_id, print_alua_state(pg->state),
-		    pg->pref ? "preferred" : "non-preferred",
-		    valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
-		    valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
-		    valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
-		    valid_states&TPGS_SUPPORT_UNAVAILABLE?'U':'u',
-		    valid_states&TPGS_SUPPORT_STANDBY?'S':'s',
-		    valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
-		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
+	printk(KERN_INFO "%s: target %s port group %02x state %c %s "
+	       "supports %c%c%c%c%c%c%c\n", ALUA_DH_NAME, pg->target_id_str,
+	       pg->group_id, print_alua_state(pg->state),
+	       pg->pref ? "preferred" : "non-preferred",
+	       valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
+	       valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
+	       valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
+	       valid_states&TPGS_SUPPORT_UNAVAILABLE?'U':'u',
+	       valid_states&TPGS_SUPPORT_STANDBY?'S':'s',
+	       valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
+	       valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
 
 	switch (pg->state) {
 	case TPGS_STATE_TRANSITIONING:
-		if (time_before(jiffies, expiry)) {
+		if (time_before(jiffies, pg->expiry)) {
 			/* State transition, retry */
-			interval += 2000;
-			msleep(interval);
-			goto retry;
+			pg->interval += 2;
+			err = SCSI_DH_RETRY;
+		} else {
+			/* Transitioning time exceeded, set port to standby */
+			err = SCSI_DH_IO;
+			pg->state = TPGS_STATE_STANDBY;
+			pg->expiry = 0;
 		}
-		/* Transitioning time exceeded, set port to standby */
-		err = SCSI_DH_RETRY;
-		pg->state = TPGS_STATE_STANDBY;
 		break;
 	case TPGS_STATE_OFFLINE:
 		/* Path unusable */
 		err = SCSI_DH_DEV_OFFLINED;
+		pg->expiry = 0;
 		break;
 	default:
 		/* Useable path if active */
 		err = SCSI_DH_OK;
+		pg->expiry = 0;
 		break;
 	}
 	return err;
@@ -847,8 +891,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	struct scsi_sense_hdr sense_hdr;
 
 	if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
-		/* Only implicit ALUA supported, retry */
-		return SCSI_DH_RETRY;
+		/* Only implicit ALUA supported */
+		return SCSI_DH_OK;
 	}
 	switch (pg->state) {
 	case TPGS_STATE_OPTIMIZED:
@@ -873,8 +917,6 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			    ALUA_DH_NAME, pg->state);
 		return SCSI_DH_NOSYS;
 	}
-	/* Set state to transitioning */
-	pg->state = TPGS_STATE_TRANSITIONING;
 	retval = submit_stpg(sdev, pg->group_id, sense);
 
 	if (retval) {
@@ -899,6 +941,102 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	return err;
 }
 
+static void alua_rtpg_work(struct work_struct *work)
+{
+	struct alua_port_group *pg =
+		container_of(work, struct alua_port_group, rtpg_work.work);
+	struct scsi_device *sdev = pg->rtpg_sdev;
+	LIST_HEAD(qdata_list);
+	int err = SCSI_DH_OK;
+	struct alua_queue_data *qdata, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pg->rtpg_lock, flags);
+	if (pg->flags & ALUA_PG_RUN_RTPG) {
+		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+		err = alua_rtpg(sdev, pg);
+		if (err == SCSI_DH_RETRY) {
+			queue_delayed_work(kmpath_aluad, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+		spin_lock_irqsave(&pg->rtpg_lock, flags);
+		pg->flags &= ~ALUA_PG_RUN_RTPG;
+	}
+	if (pg->flags & ALUA_PG_RUN_STPG) {
+		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+		err = alua_stpg(sdev, pg);
+		spin_lock_irqsave(&pg->rtpg_lock, flags);
+		pg->flags &= ~ALUA_PG_RUN_STPG;
+		pg->flags |= ALUA_PG_STPG_DONE;
+		if (err == SCSI_DH_RETRY) {
+			pg->flags |= ALUA_PG_RUN_RTPG;
+			pg->interval = ALUA_RTPG_DELAY_MSECS * 1000;
+			spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+			queue_delayed_work(kmpath_aluad, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+	}
+
+	list_splice_init(&pg->rtpg_list, &qdata_list);
+	pg->rtpg_sdev = NULL;
+	spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+
+	list_for_each_entry_safe(qdata, tmp, &qdata_list, entry) {
+		list_del(&qdata->entry);
+		if (qdata->callback_fn)
+			qdata->callback_fn(qdata->callback_data, err);
+		kfree(qdata);
+	}
+	kref_put(&pg->kref, release_port_group);
+	scsi_device_put(sdev);
+}
+
+static void alua_rtpg_queue(struct alua_port_group *pg,
+			    struct scsi_device *sdev,
+			    struct alua_queue_data *qdata)
+{
+	int start_queue = 0;
+	unsigned long flags;
+
+	if (!pg)
+		return;
+
+	kref_get(&pg->kref);
+	spin_lock_irqsave(&pg->rtpg_lock, flags);
+	if (qdata)
+		list_add_tail(&qdata->entry, &pg->rtpg_list);
+	if (pg->rtpg_sdev == NULL) {
+		pg->interval = 0;
+		pg->flags &= ~ALUA_PG_STPG_DONE;
+		pg->flags |= ALUA_PG_RUN_RTPG;
+		if (qdata)
+			pg->flags |= ALUA_PG_RUN_STPG;
+
+		kref_get(&pg->kref);
+		pg->rtpg_sdev = sdev;
+		scsi_device_get(sdev);
+		start_queue = 1;
+	} else {
+		/*
+		 * RTPG update is already queued.
+		 * Check if STPG is scheduled or done,
+		 * and set the STPG flag if not.
+		 */
+		if (qdata &&
+		    !(pg->flags & ALUA_PG_RUN_STPG) &&
+		    !(pg->flags & ALUA_PG_STPG_DONE))
+			pg->flags |= ALUA_PG_RUN_STPG;
+	}
+	spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+
+	if (start_queue)
+		queue_delayed_work(kmpath_aluad, &pg->rtpg_work,
+				   msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS));
+	kref_put(&pg->kref, release_port_group);
+}
+
 /*
  * alua_initialize - Initialize ALUA state
  * @sdev: the device to be initialized
@@ -908,21 +1046,30 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
  */
 static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int err;
-
-	err = alua_check_tpgs(sdev, h);
-	if (err != SCSI_DH_OK)
-		goto out;
-
-	err = alua_vpd_inquiry(sdev, h);
-	if (err != SCSI_DH_OK || !h->pg)
-		goto out;
-
-	kref_get(&h->pg->kref);
-	err = alua_rtpg(sdev, h->pg);
-	kref_put(&h->pg->kref, release_port_group);
-out:
-	return err;
+	struct alua_port_group *pg = NULL;
+
+	h->error = alua_check_tpgs(sdev, h);
+	if (h->error == SCSI_DH_OK) {
+		h->error = alua_vpd_inquiry(sdev, h);
+		rcu_read_lock();
+		pg = rcu_dereference(h->pg);
+		if (!pg) {
+			rcu_read_unlock();
+			h->tpgs = TPGS_MODE_NONE;
+			if (h->error == SCSI_DH_OK)
+				h->error = SCSI_DH_IO;
+		} else {
+			kref_get(&pg->kref);
+			rcu_read_unlock();
+		}
+	}
+	complete(&h->init_complete);
+	if (pg) {
+		pg->expiry = 0;
+		alua_rtpg_queue(pg, sdev, NULL);
+	}
+	kref_put(&pg->kref, release_port_group);
+	return h->error;
 }
 
 /*
@@ -941,6 +1088,9 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	const char *p = params;
 	int result = SCSI_DH_OK;
 
+	if (!h)
+		return -ENXIO;
+
 	if ((sscanf(params, "%u", &argc) != 1) || (argc != 1))
 		return -EINVAL;
 
@@ -975,31 +1125,77 @@ static int alua_activate(struct scsi_device *sdev,
 			activate_complete fn, void *data)
 {
 	struct alua_dh_data *h = get_alua_data(sdev);
-	int err = SCSI_DH_OK;
+	struct alua_queue_data *qdata;
+	struct alua_port_group *pg;
 
-	if (!h->pg)
-		goto out;
+	if (!h) {
+		if (fn)
+			fn(data, SCSI_DH_NOSYS);
+		return 0;
+	}
 
 	if (optimize_stpg)
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
-	kref_get(&h->pg->kref);
-	err = alua_rtpg(sdev, h->pg);
-	if (err != SCSI_DH_OK) {
-		kref_put(&h->pg->kref, release_port_group);
-		goto out;
+	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
+	if (!qdata) {
+		if (fn)
+			fn(data, SCSI_DH_RETRY);
+		return 0;
 	}
-	err = alua_stpg(sdev, h->pg);
-	if (err == SCSI_DH_RETRY)
-		err = alua_rtpg(sdev, h->pg);
-	kref_put(&h->pg->kref, release_port_group);
-out:
-	if (fn)
-		fn(data, err);
+
+	qdata->callback_fn = fn;
+	qdata->callback_data = data;
+
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
+		wait_for_completion(&h->init_complete);
+		rcu_read_lock();
+		pg = rcu_dereference(h->pg);
+		if (!pg) {
+			rcu_read_unlock();
+			kfree(qdata);
+			if (fn)
+				fn(data, h->error);
+			return 0;
+		}
+	}
+	kref_get(&pg->kref);
+	rcu_read_unlock();
+
+	alua_rtpg_queue(pg, sdev, qdata);
+	kref_put(&pg->kref, release_port_group);
 	return 0;
 }
 
 /*
+ * alua_check - check path status
+ * @sdev: device on the path to be checked
+ *
+ * Check the device status
+ */
+static void alua_check(struct scsi_device *sdev)
+{
+	struct alua_dh_data *h = get_alua_data(sdev);
+	struct alua_port_group *pg;
+
+	if (!h)
+		return;
+
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (pg) {
+		kref_get(&pg->kref);
+		rcu_read_unlock();
+		alua_rtpg_queue(pg, sdev, NULL);
+		kref_put(&pg->kref, release_port_group);
+	} else
+		rcu_read_unlock();
+}
+
+/*
  * alua_prep_fn - request callback
  *
  * Fail I/O to all paths not in state
@@ -1008,14 +1204,22 @@ out:
 static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 {
 	struct alua_dh_data *h = get_alua_data(sdev);
-	int state;
+	struct alua_port_group *pg;
+	int state = TPGS_STATE_OPTIMIZED;
 	int ret = BLKPREP_OK;
 
-	if (!h->pg)
+	if (!h)
 		return ret;
-	kref_get(&h->pg->kref);
-	state = h->pg->state;
-	kref_put(&h->pg->kref, release_port_group);
+
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (pg) {
+		state = pg->state;
+		/* Defer I/O while rtpg_work is active */
+		if (pg->rtpg_sdev)
+			state = TPGS_STATE_TRANSITIONING;
+	}
+	rcu_read_unlock();
 	if (state == TPGS_STATE_TRANSITIONING)
 		ret = BLKPREP_DEFER;
 	else if (state != TPGS_STATE_OPTIMIZED &&
@@ -1069,11 +1273,12 @@ static int alua_bus_attach(struct scsi_device *sdev)
 
 	scsi_dh_data->scsi_dh = &alua_dh;
 	h = (struct alua_dh_data *) scsi_dh_data->buf;
+	spin_lock_init(&h->pg_lock);
 	h->tpgs = TPGS_MODE_UNINITIALIZED;
-	h->pg = NULL;
+	rcu_assign_pointer(h->pg, NULL);
 	h->rel_port = -1;
-	h->sdev = sdev;
-
+	h->error = SCSI_DH_OK;
+	init_completion(&h->init_complete);
 	err = alua_initialize(sdev, h);
 	if ((err != SCSI_DH_OK) && (err != SCSI_DH_DEV_OFFLINED))
 		goto failed;
@@ -1102,17 +1307,23 @@ static void alua_bus_detach(struct scsi_device *sdev)
 {
 	struct scsi_dh_data *scsi_dh_data;
 	struct alua_dh_data *h;
+	struct alua_port_group *pg = NULL;
 	unsigned long flags;
 
 	spin_lock_irqsave(sdev->request_queue->queue_lock, flags);
 	scsi_dh_data = sdev->scsi_dh_data;
 	sdev->scsi_dh_data = NULL;
-	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
-
 	h = (struct alua_dh_data *) scsi_dh_data->buf;
-	if (h->pg) {
-		kref_put(&h->pg->kref, release_port_group);
-		h->pg = NULL;
+	spin_lock(&h->pg_lock);
+	pg = h->pg;
+	rcu_assign_pointer(h->pg, NULL);
+	spin_unlock(&h->pg_lock);
+	spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
+	synchronize_rcu();
+	if (pg) {
+		if (pg->rtpg_sdev)
+			flush_workqueue(kmpath_aluad);
+		kref_put(&pg->kref, release_port_group);
 	}
 	kfree(scsi_dh_data);
 	module_put(THIS_MODULE);
@@ -1123,16 +1334,24 @@ static int __init alua_init(void)
 {
 	int r;
 
+	kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
+	if (!kmpath_aluad) {
+		printk(KERN_ERR "kmpath_aluad creation failed.\n");
+		return -EINVAL;
+	}
 	r = scsi_register_device_handler(&alua_dh);
-	if (r != 0)
+	if (r != 0) {
 		printk(KERN_ERR "%s: Failed to register scsi device handler",
 			ALUA_DH_NAME);
+		destroy_workqueue(kmpath_aluad);
+	}
 	return r;
 }
 
 static void __exit alua_exit(void)
 {
 	scsi_unregister_device_handler(&alua_dh);
+	destroy_workqueue(kmpath_aluad);
 }
 
 module_init(alua_init);
-- 
1.7.12.4

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Mike Christie]

On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
> We should be issuing STPG synchronously as we need to
> evaluate the return code on failure.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

I think we need to also make dm-mpath.c use a non-ordered workqueue for
kmpath_handlerd. With this patch and the current ordered workqueue in
dm-mpath I think we will only be able to do one STPG at a time. I think
if we use a normal old non-ordered workqueue then we would be limited to
have max_active STPGs executing.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Mike Christie]

On 02/06/2014 07:24 PM, Mike Christie wrote:
> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>> We should be issuing STPG synchronously as we need to
>> evaluate the return code on failure.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> I think we need to also make dm-mpath.c use a non-ordered workqueue for
> kmpath_handlerd. With this patch and the current ordered workqueue in
> dm-mpath I think we will only be able to do one STPG at a time. I think
> if we use a normal old non-ordered workqueue then we would be limited to
> have max_active STPGs executing.

I goofed and commented in the order I saw the patches :) I take this
comment back for this patch, because I see in 16/16 you added a new
workqueue to scsi_dh_alua to do rtpgs and stpgs.

For 16/16 though, do we want to make kmpath_aluad a non single threaded
workqueue? It looks like max_active for single threaded is only one work
at a time too.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

On 02/07/2014 02:54 AM, Mike Christie wrote:
> On 02/06/2014 07:24 PM, Mike Christie wrote:
>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>> We should be issuing STPG synchronously as we need to
>>> evaluate the return code on failure.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>
>> I think we need to also make dm-mpath.c use a non-ordered workqueue for
>> kmpath_handlerd. With this patch and the current ordered workqueue in
>> dm-mpath I think we will only be able to do one STPG at a time. I think
>> if we use a normal old non-ordered workqueue then we would be limited to
>> have max_active STPGs executing.
> 
> I goofed and commented in the order I saw the patches :) I take this
> comment back for this patch, because I see in 16/16 you added a new
> workqueue to scsi_dh_alua to do rtpgs and stpgs.
> 
> For 16/16 though, do we want to make kmpath_aluad a non single threaded
> workqueue? It looks like max_active for single threaded is only one work
> at a time too.
> 
Well, that was by intention.

The workqueue will be triggered very infrequently (basically for
every path switch).
For implicit ALUA we just need to issue a RTPG to get the new path
status; there we might be suffering from single threaded behaviour.
But we need to issue it only once and it should be processed
reasonably fast.
For explicit ALUA we'll have to send an STPG, which has potentially
system-wide implications. So sending several to (supposedly)
different targets might actually be contraproductive, as the first
might have already set the status for the second call.
Here we most definitely _want_ serialisation to avoid superfluous STPGs.

So for now I think we can stick with the single threaded workqueue,
and can revise this later if it's found to be a scalability issue.

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 04/16] scsi_dh_alua: Make stpg synchronous

[Mike Christie]

On 2/12/14 9:29 AM, Hannes Reinecke wrote:
> On 02/07/2014 02:54 AM, Mike Christie wrote:
>> On 02/06/2014 07:24 PM, Mike Christie wrote:
>>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>>> We should be issuing STPG synchronously as we need to
>>>> evaluate the return code on failure.
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>
>>> I think we need to also make dm-mpath.c use a non-ordered workqueue for
>>> kmpath_handlerd. With this patch and the current ordered workqueue in
>>> dm-mpath I think we will only be able to do one STPG at a time. I think
>>> if we use a normal old non-ordered workqueue then we would be limited to
>>> have max_active STPGs executing.
>>
>> I goofed and commented in the order I saw the patches :) I take this
>> comment back for this patch, because I see in 16/16 you added a new
>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>
>> For 16/16 though, do we want to make kmpath_aluad a non single threaded
>> workqueue? It looks like max_active for single threaded is only one work
>> at a time too.
>>
> Well, that was by intention.
>
> The workqueue will be triggered very infrequently (basically for
> every path switch).
> For implicit ALUA we just need to issue a RTPG to get the new path
> status; there we might be suffering from single threaded behaviour.
> But we need to issue it only once and it should be processed
> reasonably fast.
> For explicit ALUA we'll have to send an STPG, which has potentially
> system-wide implications. So sending several to (supposedly)
> different targets might actually be contraproductive, as the first
> might have already set the status for the second call.
> Here we most definitely _want_ serialisation to avoid superfluous STPGs.
>

What target is this?

For our target it adds a regression. It only affects devices on the same 
port group. We then have multiple groups. Before the patch, we could 
failover/failback multiple devices in parallel. To do 64 devices it took 
about 3 seconds. With your patch it takes around 3 minutes.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Mike Christie]

On 2/12/14 10:11 AM, Mike Christie wrote:
> On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>> On 02/07/2014 02:54 AM, Mike Christie wrote:
>>> On 02/06/2014 07:24 PM, Mike Christie wrote:
>>>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>>>> We should be issuing STPG synchronously as we need to
>>>>> evaluate the return code on failure.
>>>>>
>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>
>>>> I think we need to also make dm-mpath.c use a non-ordered workqueue for
>>>> kmpath_handlerd. With this patch and the current ordered workqueue in
>>>> dm-mpath I think we will only be able to do one STPG at a time. I think
>>>> if we use a normal old non-ordered workqueue then we would be
>>>> limited to
>>>> have max_active STPGs executing.
>>>
>>> I goofed and commented in the order I saw the patches :) I take this
>>> comment back for this patch, because I see in 16/16 you added a new
>>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>>
>>> For 16/16 though, do we want to make kmpath_aluad a non single threaded
>>> workqueue? It looks like max_active for single threaded is only one work
>>> at a time too.
>>>
>> Well, that was by intention.
>>
>> The workqueue will be triggered very infrequently (basically for
>> every path switch).
>> For implicit ALUA we just need to issue a RTPG to get the new path
>> status; there we might be suffering from single threaded behaviour.
>> But we need to issue it only once and it should be processed
>> reasonably fast.
>> For explicit ALUA we'll have to send an STPG, which has potentially
>> system-wide implications. So sending several to (supposedly)
>> different targets might actually be contraproductive, as the first
>> might have already set the status for the second call.
>> Here we most definitely _want_ serialisation to avoid superfluous STPGs.
>>
>
> What target is this?
>
> For our target it adds a regression. It only affects devices on the same
> port group. We then have multiple groups. Before the patch, we could
> failover/failback multiple devices in parallel. To do 64 devices it took
> about 3 seconds. With your patch it takes around 3 minutes.
>

Also, with your pg change patch, I think we can serialize based on group 
and it will do what you want and also allow us to do STPGs to different 
groups in parallel.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Mike Christie]

On 2/12/14 10:26 AM, Mike Christie wrote:
> On 2/12/14 10:11 AM, Mike Christie wrote:
>> On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>>> On 02/07/2014 02:54 AM, Mike Christie wrote:
>>>> On 02/06/2014 07:24 PM, Mike Christie wrote:
>>>>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>>>>> We should be issuing STPG synchronously as we need to
>>>>>> evaluate the return code on failure.
>>>>>>
>>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>>
>>>>> I think we need to also make dm-mpath.c use a non-ordered workqueue
>>>>> for
>>>>> kmpath_handlerd. With this patch and the current ordered workqueue in
>>>>> dm-mpath I think we will only be able to do one STPG at a time. I
>>>>> think
>>>>> if we use a normal old non-ordered workqueue then we would be
>>>>> limited to
>>>>> have max_active STPGs executing.
>>>>
>>>> I goofed and commented in the order I saw the patches :) I take this
>>>> comment back for this patch, because I see in 16/16 you added a new
>>>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>>>
>>>> For 16/16 though, do we want to make kmpath_aluad a non single threaded
>>>> workqueue? It looks like max_active for single threaded is only one
>>>> work
>>>> at a time too.
>>>>
>>> Well, that was by intention.
>>>
>>> The workqueue will be triggered very infrequently (basically for
>>> every path switch).
>>> For implicit ALUA we just need to issue a RTPG to get the new path
>>> status; there we might be suffering from single threaded behaviour.
>>> But we need to issue it only once and it should be processed
>>> reasonably fast.
>>> For explicit ALUA we'll have to send an STPG, which has potentially
>>> system-wide implications. So sending several to (supposedly)
>>> different targets might actually be contraproductive, as the first
>>> might have already set the status for the second call.
>>> Here we most definitely _want_ serialisation to avoid superfluous STPGs.
>>>
>>
>> What target is this?
>>
>> For our target it adds a regression. It only affects devices on the same
>> port group. We then have multiple groups. Before the patch, we could
>> failover/failback multiple devices in parallel. To do 64 devices it took
>> about 3 seconds. With your patch it takes around 3 minutes.
>>
>
> Also, with your pg change patch, I think we can serialize based on group
> and it will do what you want and also allow us to do STPGs to different
> groups in parallel.

I guess that would work for me, but I think if you had the same target 
port in multiple port groups then you could hit the issue you described, 
right?

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

On 02/12/2014 06:31 PM, Mike Christie wrote:
> On 2/12/14 10:26 AM, Mike Christie wrote:
>> On 2/12/14 10:11 AM, Mike Christie wrote:
>>> On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>>>> On 02/07/2014 02:54 AM, Mike Christie wrote:
>>>>> On 02/06/2014 07:24 PM, Mike Christie wrote:
>>>>>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>>>>>> We should be issuing STPG synchronously as we need to
>>>>>>> evaluate the return code on failure.
>>>>>>>
>>>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>>>
>>>>>> I think we need to also make dm-mpath.c use a non-ordered
>>>>>> workqueue
>>>>>> for
>>>>>> kmpath_handlerd. With this patch and the current ordered
>>>>>> workqueue in
>>>>>> dm-mpath I think we will only be able to do one STPG at a time. I
>>>>>> think
>>>>>> if we use a normal old non-ordered workqueue then we would be
>>>>>> limited to
>>>>>> have max_active STPGs executing.
>>>>>
>>>>> I goofed and commented in the order I saw the patches :) I take
>>>>> this
>>>>> comment back for this patch, because I see in 16/16 you added a
>>>>> new
>>>>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>>>>
>>>>> For 16/16 though, do we want to make kmpath_aluad a non single
>>>>> threaded
>>>>> workqueue? It looks like max_active for single threaded is only
>>>>> one
>>>>> work
>>>>> at a time too.
>>>>>
>>>> Well, that was by intention.
>>>>
>>>> The workqueue will be triggered very infrequently (basically for
>>>> every path switch).
>>>> For implicit ALUA we just need to issue a RTPG to get the new path
>>>> status; there we might be suffering from single threaded behaviour.
>>>> But we need to issue it only once and it should be processed
>>>> reasonably fast.
>>>> For explicit ALUA we'll have to send an STPG, which has potentially
>>>> system-wide implications. So sending several to (supposedly)
>>>> different targets might actually be contraproductive, as the first
>>>> might have already set the status for the second call.
>>>> Here we most definitely _want_ serialisation to avoid
>>>> superfluous STPGs.
>>>>
>>>
>>> What target is this?
>>>
>>> For our target it adds a regression. It only affects devices on
>>> the same
>>> port group. We then have multiple groups. Before the patch, we could
>>> failover/failback multiple devices in parallel. To do 64 devices
>>> it took
>>> about 3 seconds. With your patch it takes around 3 minutes.
>>>
>>
>> Also, with your pg change patch, I think we can serialize based on
>> group
>> and it will do what you want and also allow us to do STPGs to
>> different
>> groups in parallel.
> 
> I guess that would work for me, but I think if you had the same
> target port in multiple port groups then you could hit the issue you
> described, right?
> 
Yes, we might. But it's worth a shot anyway.

So to alleviate all this, this patch:

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
b/drivers/scsi/device_ha
ndler/scsi_dh_alua.c
index 569af9d..46cc1ef 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -1353,7 +1353,7 @@ static int __init alua_init(void)
 {
        int r;

-       kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
+       kmpath_aluad = create_workqueue("kmpath_aluad");
        if (!kmpath_aluad) {
                printk(KERN_ERR "kmpath_aluad creation failed.\n");
                return -EINVAL;

should be sufficient, right?

Could you test and see if it makes a difference?

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 06/16] scsi_dh_alua: use local buffer for VPD inquiry

[Maurizio Lombardi]

Hi,

On 01/31/2014 10:29 AM, Hannes Reinecke wrote:
>  static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
>  {
> +	unsigned char *buff;
> +	unsigned char bufflen = 36;
> 	int len, timeout = ALUA_FAILOVER_TIMEOUT;
[...]
>+	len = (buff[2] << 8) + buff[3] + 4;
>+	if (len > bufflen) {
[...]
>+		bufflen = len;

just a nit: is it safe to use char as the type of bufflen? Isn't better
to declare it as int just in case len is > than 255 ?

Regards,
Maurizio Lombardi

Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry

[Hannes Reinecke]

On 02/13/2014 10:51 AM, Maurizio Lombardi wrote:
> Hi,
> 
> On 01/31/2014 10:29 AM, Hannes Reinecke wrote:
>>  static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
>>  {
>> +	unsigned char *buff;
>> +	unsigned char bufflen = 36;
>> 	int len, timeout = ALUA_FAILOVER_TIMEOUT;
> [...]
>> +	len = (buff[2] << 8) + buff[3] + 4;
>> +	if (len > bufflen) {
> [...]
>> +		bufflen = len;
> 
> just a nit: is it safe to use char as the type of bufflen? Isn't better
> to declare it as int just in case len is > than 255 ?
> 
Yes, true.

However, this whole section needs to be reworked anyway, as there's
a fair chance we're getting the VPD page 0x83 for free.
(Cf my latest patchset 'Display EVPD pages in sysfs').

And jejb made some positive noises that, so I'll be reworking the
scsi_dh_alua patchset to take advantage of the new fields.

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 01/16] scsi_dh_alua: Improve error handling

[Bart Van Assche]

On 01/31/14 10:29, Hannes Reinecke wrote:
> - * alua_stpg - Evaluate SET TARGET GROUP STATES
> + * stpg_endio - Evaluate SET TARGET GROUP STATES

Great that you are fixing the function header of stpg_endio(). But
please consider to use here the official terminology from SPC ("SET
TARGET PORT GROUPS").

Bart.

Re: [PATCH 03/16] scsi_dh_alua: Pass buffer as function argument

[Bart Van Assche]

On 01/31/14 10:29, Hannes Reinecke wrote:
> -	rq->cmd[6] = (h->bufflen >> 24) & 0xff;
> -	rq->cmd[7] = (h->bufflen >> 16) & 0xff;
> -	rq->cmd[8] = (h->bufflen >>  8) & 0xff;
> -	rq->cmd[9] = h->bufflen & 0xff;
> +	rq->cmd[6] = (bufflen >> 24) & 0xff;
> +	rq->cmd[7] = (bufflen >> 16) & 0xff;
> +	rq->cmd[8] = (bufflen >>  8) & 0xff;
> +	rq->cmd[9] = bufflen & 0xff;

Since you are changing this code, please use put_unaligned_be32() for
storing bufflen in the CDB.

Bart.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Bart Van Assche]

On 01/31/14 10:29, Hannes Reinecke wrote:
> -	memset(h->buff, 0, stpg_len);
> -	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
> -	h->buff[6] = (h->group_id >> 8) & 0xff;
> -	h->buff[7] = h->group_id & 0xff;
> +	memset(stpg_data, 0, stpg_len);
> +	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
> +	stpg_data[6] = (group_id >> 8) & 0xff;
> +	stpg_data[7] = group_id & 0xff;

Any reason why put_unaligned_be16() is not used here ?

Bart.

Re: [PATCH 01/16] scsi_dh_alua: Improve error handling

[Hannes Reinecke]

On 02/14/2014 12:16 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> - * alua_stpg - Evaluate SET TARGET GROUP STATES
>> + * stpg_endio - Evaluate SET TARGET GROUP STATES
> 
> Great that you are fixing the function header of stpg_endio(). But
> please consider to use here the official terminology from SPC ("SET
> TARGET PORT GROUPS").
> 
Ok. Will be fixing it for the next round.

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 03/16] scsi_dh_alua: Pass buffer as function argument

[Hannes Reinecke]

On 02/14/2014 12:22 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> -	rq->cmd[6] = (h->bufflen >> 24) & 0xff;
>> -	rq->cmd[7] = (h->bufflen >> 16) & 0xff;
>> -	rq->cmd[8] = (h->bufflen >>  8) & 0xff;
>> -	rq->cmd[9] = h->bufflen & 0xff;
>> +	rq->cmd[6] = (bufflen >> 24) & 0xff;
>> +	rq->cmd[7] = (bufflen >> 16) & 0xff;
>> +	rq->cmd[8] = (bufflen >>  8) & 0xff;
>> +	rq->cmd[9] = bufflen & 0xff;
> 
> Since you are changing this code, please use put_unaligned_be32() for
> storing bufflen in the CDB.
> 
Ok. Will be fixing it up in the next round.

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 04/16] scsi_dh_alua: Make stpg synchronous

[Bart Van Assche]

On 02/13/14 10:31, Hannes Reinecke wrote:
> On 02/12/2014 06:31 PM, Mike Christie wrote:
>> On 2/12/14 10:26 AM, Mike Christie wrote:
>>> On 2/12/14 10:11 AM, Mike Christie wrote:
>>>> On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>>>>> On 02/07/2014 02:54 AM, Mike Christie wrote:
>>>>>> On 02/06/2014 07:24 PM, Mike Christie wrote:
>>>>>>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>>>>>>> We should be issuing STPG synchronously as we need to
>>>>>>>> evaluate the return code on failure.
>>>>>>>>
>>>>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>>>>
>>>>>>> I think we need to also make dm-mpath.c use a non-ordered
>>>>>>> workqueue
>>>>>>> for
>>>>>>> kmpath_handlerd. With this patch and the current ordered
>>>>>>> workqueue in
>>>>>>> dm-mpath I think we will only be able to do one STPG at a time. I
>>>>>>> think
>>>>>>> if we use a normal old non-ordered workqueue then we would be
>>>>>>> limited to
>>>>>>> have max_active STPGs executing.
>>>>>>
>>>>>> I goofed and commented in the order I saw the patches :) I take
>>>>>> this
>>>>>> comment back for this patch, because I see in 16/16 you added a
>>>>>> new
>>>>>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>>>>>
>>>>>> For 16/16 though, do we want to make kmpath_aluad a non single
>>>>>> threaded
>>>>>> workqueue? It looks like max_active for single threaded is only
>>>>>> one
>>>>>> work
>>>>>> at a time too.
>>>>>>
>>>>> Well, that was by intention.
>>>>>
>>>>> The workqueue will be triggered very infrequently (basically for
>>>>> every path switch).
>>>>> For implicit ALUA we just need to issue a RTPG to get the new path
>>>>> status; there we might be suffering from single threaded behaviour.
>>>>> But we need to issue it only once and it should be processed
>>>>> reasonably fast.
>>>>> For explicit ALUA we'll have to send an STPG, which has potentially
>>>>> system-wide implications. So sending several to (supposedly)
>>>>> different targets might actually be contraproductive, as the first
>>>>> might have already set the status for the second call.
>>>>> Here we most definitely _want_ serialisation to avoid
>>>>> superfluous STPGs.
>>>>>
>>>>
>>>> What target is this?
>>>>
>>>> For our target it adds a regression. It only affects devices on
>>>> the same
>>>> port group. We then have multiple groups. Before the patch, we could
>>>> failover/failback multiple devices in parallel. To do 64 devices
>>>> it took
>>>> about 3 seconds. With your patch it takes around 3 minutes.
>>>>
>>>
>>> Also, with your pg change patch, I think we can serialize based on
>>> group
>>> and it will do what you want and also allow us to do STPGs to
>>> different
>>> groups in parallel.
>>
>> I guess that would work for me, but I think if you had the same
>> target port in multiple port groups then you could hit the issue you
>> described, right?
>>
> Yes, we might. But it's worth a shot anyway.
> 
> So to alleviate all this, this patch:
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> b/drivers/scsi/device_ha
> ndler/scsi_dh_alua.c
> index 569af9d..46cc1ef 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -1353,7 +1353,7 @@ static int __init alua_init(void)
>  {
>         int r;
> 
> -       kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
> +       kmpath_aluad = create_workqueue("kmpath_aluad");
>         if (!kmpath_aluad) {
>                 printk(KERN_ERR "kmpath_aluad creation failed.\n");
>                 return -EINVAL;
> 
> should be sufficient, right?

I think this will only be sufficient if there are more CPU threads
available than the number of LUNs for which an STPG has to be issued.
My preference is also to keep the asynchronous invocation of the STPG
commands to avoid introducing a regression if the number of LUNs that
has to be failed over is large. Has it been considered to add the "if
(err == SCSI_DH_RETRY) err = alua_rtpg(sdev, h)" code in the
stpg_endio() handler, together with a counter mechanism that prevents
infinite retries ? And if a storage array reports that the target portal
group state is transitioning, shouldn't the retry be delayed instead of
submitting it immediately ?

Bart.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

On 02/14/2014 12:27 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> -	memset(h->buff, 0, stpg_len);
>> -	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
>> -	h->buff[6] = (h->group_id >> 8) & 0xff;
>> -	h->buff[7] = h->group_id & 0xff;
>> +	memset(stpg_data, 0, stpg_len);
>> +	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
>> +	stpg_data[6] = (group_id >> 8) & 0xff;
>> +	stpg_data[7] = group_id & 0xff;
> 
> Any reason why put_unaligned_be16() is not used here ?
> 
No specific one. Will be fixing it up in the next round.

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 06/16] scsi_dh_alua: use local buffer for VPD inquiry

[Bart Van Assche]

On 01/31/14 10:29, Hannes Reinecke wrote:
> -	len = (h->buff[2] << 8) + h->buff[3] + 4;
> -	if (len > h->bufflen) {
> +	len = (buff[2] << 8) + buff[3] + 4;
> +	if (len > bufflen) {

I think this code will become easier to read when using
get_unaligned_be16() for extracting the length from the VPD response.

Bart.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

On 02/14/2014 12:37 PM, Bart Van Assche wrote:
> On 02/13/14 10:31, Hannes Reinecke wrote:
>> On 02/12/2014 06:31 PM, Mike Christie wrote:
>>> On 2/12/14 10:26 AM, Mike Christie wrote:
>>>> On 2/12/14 10:11 AM, Mike Christie wrote:
>>>>> On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>>>>>> On 02/07/2014 02:54 AM, Mike Christie wrote:
>>>>>>> On 02/06/2014 07:24 PM, Mike Christie wrote:
>>>>>>>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>>>>>>>> We should be issuing STPG synchronously as we need to
>>>>>>>>> evaluate the return code on failure.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>>>>>
>>>>>>>> I think we need to also make dm-mpath.c use a non-ordered
>>>>>>>> workqueue
>>>>>>>> for
>>>>>>>> kmpath_handlerd. With this patch and the current ordered
>>>>>>>> workqueue in
>>>>>>>> dm-mpath I think we will only be able to do one STPG at a time. I
>>>>>>>> think
>>>>>>>> if we use a normal old non-ordered workqueue then we would be
>>>>>>>> limited to
>>>>>>>> have max_active STPGs executing.
>>>>>>>
>>>>>>> I goofed and commented in the order I saw the patches :) I take
>>>>>>> this
>>>>>>> comment back for this patch, because I see in 16/16 you added a
>>>>>>> new
>>>>>>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>>>>>>
>>>>>>> For 16/16 though, do we want to make kmpath_aluad a non single
>>>>>>> threaded
>>>>>>> workqueue? It looks like max_active for single threaded is only
>>>>>>> one
>>>>>>> work
>>>>>>> at a time too.
>>>>>>>
>>>>>> Well, that was by intention.
>>>>>>
>>>>>> The workqueue will be triggered very infrequently (basically for
>>>>>> every path switch).
>>>>>> For implicit ALUA we just need to issue a RTPG to get the new path
>>>>>> status; there we might be suffering from single threaded behaviour.
>>>>>> But we need to issue it only once and it should be processed
>>>>>> reasonably fast.
>>>>>> For explicit ALUA we'll have to send an STPG, which has potentially
>>>>>> system-wide implications. So sending several to (supposedly)
>>>>>> different targets might actually be contraproductive, as the first
>>>>>> might have already set the status for the second call.
>>>>>> Here we most definitely _want_ serialisation to avoid
>>>>>> superfluous STPGs.
>>>>>>
>>>>>
>>>>> What target is this?
>>>>>
>>>>> For our target it adds a regression. It only affects devices on
>>>>> the same
>>>>> port group. We then have multiple groups. Before the patch, we could
>>>>> failover/failback multiple devices in parallel. To do 64 devices
>>>>> it took
>>>>> about 3 seconds. With your patch it takes around 3 minutes.
>>>>>
>>>>
>>>> Also, with your pg change patch, I think we can serialize based on
>>>> group
>>>> and it will do what you want and also allow us to do STPGs to
>>>> different
>>>> groups in parallel.
>>>
>>> I guess that would work for me, but I think if you had the same
>>> target port in multiple port groups then you could hit the issue you
>>> described, right?
>>>
>> Yes, we might. But it's worth a shot anyway.
>>
>> So to alleviate all this, this patch:
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
>> b/drivers/scsi/device_ha
>> ndler/scsi_dh_alua.c
>> index 569af9d..46cc1ef 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -1353,7 +1353,7 @@ static int __init alua_init(void)
>>  {
>>         int r;
>>
>> -       kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
>> +       kmpath_aluad = create_workqueue("kmpath_aluad");
>>         if (!kmpath_aluad) {
>>                 printk(KERN_ERR "kmpath_aluad creation failed.\n");
>>                 return -EINVAL;
>>
>> should be sufficient, right?
> 
> I think this will only be sufficient if there are more CPU threads
> available than the number of LUNs for which an STPG has to be issued.
> My preference is also to keep the asynchronous invocation of the STPG
> commands to avoid introducing a regression if the number of LUNs that
> has to be failed over is large. Has it been considered to add the "if
> (err == SCSI_DH_RETRY) err = alua_rtpg(sdev, h)" code in the
> stpg_endio() handler, together with a counter mechanism that prevents
> infinite retries ? And if a storage array reports that the target portal
> group state is transitioning, shouldn't the retry be delayed instead of
> submitting it immediately ?
> 
Errm. I thought I was doing that ...

	if (pg->flags & ALUA_PG_RUN_RTPG) {
		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
		err = alua_rtpg(sdev, pg);
		if (err == SCSI_DH_RETRY) {
			queue_delayed_work(kmpath_aluad, &pg->rtpg_work,
					   pg->interval * HZ);
			return;
		}
		spin_lock_irqsave(&pg->rtpg_lock, flags);
		pg->flags &= ~ALUA_PG_RUN_RTPG;
	}

As for making stpg asynchronous again; I haven't thought about it as
of now, but it seems like a good idea.

I'll checking what would need to be done for 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

Re: [PATCH 07/16] scsi_dh_alua: Use separate alua_port_group structure

[Bart Van Assche]

On 01/31/14 10:29, Hannes Reinecke wrote:
> +static void release_port_group(struct kref *kref)
> +{
> +	struct alua_port_group *pg;
> +
> +	pg = container_of(kref, struct alua_port_group, kref);
> +	printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
> +	spin_lock(&port_group_lock);
> +	list_del(&pg->node);
> +	spin_unlock(&port_group_lock);
> +	if (pg->buff && pg->inq != pg->buff)
> +		kfree(pg->buff);
> +	kfree(pg);
> +}

Does that message really need level "WARNING" ?

> +	sdev_printk(KERN_INFO, sdev,
> +		    "%s: port group %02x rel port %02x\n",
> +		    ALUA_DH_NAME, group_id, h->rel_port);
> +	spin_lock(&port_group_lock);
> +	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
> +	if (!pg) {
> +		sdev_printk(KERN_WARNING, sdev,
> +			    "%s: kzalloc port group failed\n",
> +			    ALUA_DH_NAME);
> +		/* Temporary failure, bypass */
> +		spin_unlock(&port_group_lock);
> +		err = SCSI_DH_DEV_TEMP_BUSY;
> +		goto out;
> +	}
> +	pg->group_id = group_id;
> +	pg->buff = pg->inq;
> +	pg->bufflen = ALUA_INQUIRY_SIZE;
> +	pg->tpgs = h->tpgs;
> +	pg->state = TPGS_STATE_OPTIMIZED;
> +	pg->flags = h->flags;
> +	kref_init(&pg->kref);
> +	list_add(&pg->node, &port_group_list);
> +	h->pg = pg;
> +	spin_unlock(&port_group_lock);
> +	err = SCSI_DH_OK;

A GFP_KERNEL allocation with a spin lock held ?? Please move that
allocation outside the critical section and also the code for
initializing *pg. What's not clear to me is why no uniqueness check is
performed before invoking list_add() ? Does that mean that information
for the same port group ID can end up multiple times in the
port_group_list ? Such a uniqueness check can only be performed if a
storage array identification is available so patches 07 and 08 may have
to be swapped.

Bart.

Re: [PATCH 08/16] scsi_dh_alua: parse target device id

[Bart Van Assche]

On 01/31/14 10:29, Hannes Reinecke wrote:
> +	if (!target_id_size) {
> +		/* Check for EMC Clariion extended inquiry */
> +		if (!strncmp(sdev->vendor, "DGC     ", 8) &&
> +		    sdev->inquiry_len > 160) {
> +			target_id_size = sdev->inquiry[160];
> +			target_id = sdev->inquiry + 161;
> +			strcpy(target_id_str, "emc.");
> +			memcpy(target_id_str + 4, target_id, target_id_size);
> +		}
> +		/* Check for HP EVA extended inquiry */
> +		if (!strncmp(sdev->vendor, "HP      ", 8) &&
> +		    !strncmp(sdev->model, "HSV", 3) &&
> +		    sdev->inquiry_len > 170) {
> +			target_id_size = 16;
> +			target_id = sdev->inquiry + 154;
> +			strcpy(target_id_str, "naa.");
> +			memcpy(target_id_str + 4, target_id, target_id_size);
> +		}
> +	}

Being able to identify a storage array unambiguously is essential for
the new ALUA device handler algorithm introduced by this patch series.
What if a new storage array is introduced that is not covered by one of
the heuristics in this patch ? Has it been considered to let storage
array identification occur in user space instead of in the kernel ?

Thanks,

Bart.

Re: [PATCH 07/16] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke]

On 02/14/2014 12:56 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> +static void release_port_group(struct kref *kref)
>> +{
>> +	struct alua_port_group *pg;
>> +
>> +	pg = container_of(kref, struct alua_port_group, kref);
>> +	printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
>> +	spin_lock(&port_group_lock);
>> +	list_del(&pg->node);
>> +	spin_unlock(&port_group_lock);
>> +	if (pg->buff && pg->inq != pg->buff)
>> +		kfree(pg->buff);
>> +	kfree(pg);
>> +}
> 
> Does that message really need level "WARNING" ?
> 
>> +	sdev_printk(KERN_INFO, sdev,
>> +		    "%s: port group %02x rel port %02x\n",
>> +		    ALUA_DH_NAME, group_id, h->rel_port);
>> +	spin_lock(&port_group_lock);
>> +	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
>> +	if (!pg) {
>> +		sdev_printk(KERN_WARNING, sdev,
>> +			    "%s: kzalloc port group failed\n",
>> +			    ALUA_DH_NAME);
>> +		/* Temporary failure, bypass */
>> +		spin_unlock(&port_group_lock);
>> +		err = SCSI_DH_DEV_TEMP_BUSY;
>> +		goto out;
>> +	}
>> +	pg->group_id = group_id;
>> +	pg->buff = pg->inq;
>> +	pg->bufflen = ALUA_INQUIRY_SIZE;
>> +	pg->tpgs = h->tpgs;
>> +	pg->state = TPGS_STATE_OPTIMIZED;
>> +	pg->flags = h->flags;
>> +	kref_init(&pg->kref);
>> +	list_add(&pg->node, &port_group_list);
>> +	h->pg = pg;
>> +	spin_unlock(&port_group_lock);
>> +	err = SCSI_DH_OK;
> 
> A GFP_KERNEL allocation with a spin lock held ?? Please move that
> allocation outside the critical section and also the code for
> initializing *pg. What's not clear to me is why no uniqueness check is
> performed before invoking list_add() ? Does that mean that information
> for the same port group ID can end up multiple times in the
> port_group_list ? Such a uniqueness check can only be performed if a
> storage array identification is available so patches 07 and 08 may have
> to be swapped.
> 
With this patch I'm allocating one pg per sdev, so I won't need any
uniqueness check here (sdevs are already unique).
I only need to check for uniqueness once I have array identifiers.

But yes, the allocation can be moved to outside the critical section.

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 08/16] scsi_dh_alua: parse target device id

[Hannes Reinecke]

On 02/14/2014 01:09 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> +	if (!target_id_size) {
>> +		/* Check for EMC Clariion extended inquiry */
>> +		if (!strncmp(sdev->vendor, "DGC     ", 8) &&
>> +		    sdev->inquiry_len > 160) {
>> +			target_id_size = sdev->inquiry[160];
>> +			target_id = sdev->inquiry + 161;
>> +			strcpy(target_id_str, "emc.");
>> +			memcpy(target_id_str + 4, target_id, target_id_size);
>> +		}
>> +		/* Check for HP EVA extended inquiry */
>> +		if (!strncmp(sdev->vendor, "HP      ", 8) &&
>> +		    !strncmp(sdev->model, "HSV", 3) &&
>> +		    sdev->inquiry_len > 170) {
>> +			target_id_size = 16;
>> +			target_id = sdev->inquiry + 154;
>> +			strcpy(target_id_str, "naa.");
>> +			memcpy(target_id_str + 4, target_id, target_id_size);
>> +		}
>> +	}
> 
> Being able to identify a storage array unambiguously is essential for
> the new ALUA device handler algorithm introduced by this patch series.
> What if a new storage array is introduced that is not covered by one of
> the heuristics in this patch ? Has it been considered to let storage
> array identification occur in user space instead of in the kernel ?
> 
As noted in the other mail, if we cannot decipher the array
identifier the whole thing degrades into one pg per sdev.
IE we cannot bunch RTPG / STPG for those.
The idea here is that those arrays do not have any limitations,
so that no special treatment is required.

Adding identifiers from userspace are a bit tricky; there is no
actual sysfs structure for the device handler.
Which we would need if we were to use these tricks.

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 07/16] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke]

On 02/14/2014 12:56 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> +static void release_port_group(struct kref *kref)
>> +{
>> +	struct alua_port_group *pg;
>> +
>> +	pg = container_of(kref, struct alua_port_group, kref);
>> +	printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
>> +	spin_lock(&port_group_lock);
>> +	list_del(&pg->node);
>> +	spin_unlock(&port_group_lock);
>> +	if (pg->buff && pg->inq != pg->buff)
>> +		kfree(pg->buff);
>> +	kfree(pg);
>> +}
> 
> Does that message really need level "WARNING" ?
No, probably not.

> 
>> +	sdev_printk(KERN_INFO, sdev,
>> +		    "%s: port group %02x rel port %02x\n",
>> +		    ALUA_DH_NAME, group_id, h->rel_port);
>> +	spin_lock(&port_group_lock);
>> +	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
>> +	if (!pg) {
>> +		sdev_printk(KERN_WARNING, sdev,
>> +			    "%s: kzalloc port group failed\n",
>> +			    ALUA_DH_NAME);
>> +		/* Temporary failure, bypass */
>> +		spin_unlock(&port_group_lock);
>> +		err = SCSI_DH_DEV_TEMP_BUSY;
>> +		goto out;
>> +	}
>> +	pg->group_id = group_id;
>> +	pg->buff = pg->inq;
>> +	pg->bufflen = ALUA_INQUIRY_SIZE;
>> +	pg->tpgs = h->tpgs;
>> +	pg->state = TPGS_STATE_OPTIMIZED;
>> +	pg->flags = h->flags;
>> +	kref_init(&pg->kref);
>> +	list_add(&pg->node, &port_group_list);
>> +	h->pg = pg;
>> +	spin_unlock(&port_group_lock);
>> +	err = SCSI_DH_OK;
> 
> A GFP_KERNEL allocation with a spin lock held ?? Please move that
> allocation outside the critical section and also the code for
> initializing *pg. What's not clear to me is why no uniqueness check is
> performed before invoking list_add() ? Does that mean that information
> for the same port group ID can end up multiple times in the
> port_group_list ? Such a uniqueness check can only be performed if a
> storage array identification is available so patches 07 and 08 may have
> to be swapped.
> 
The reason I did this was that I don't have to allocate memory
unnecesarily.
If I move the allocation out of the spinlock I'll have to recheck
the list upon insertion to ensure no duplicates are present.
Upon hitting a duplicate I would have to release the memory again.

I do agree that GFP_KERNEL is probably not the correct thing here;
so either I move it to GFP_ATOMIC or we may run into a chance of
having to release the memory again afterwards.

Personally I'm inclined to use GFP_ATOMIC, but I'm not sure what'd
be best.

What would you suggest here?

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 04/16] scsi_dh_alua: Make stpg synchronous

[Mike Christie]

On 2/13/14 3:31 AM, Hannes Reinecke wrote:
> On 02/12/2014 06:31 PM, Mike Christie wrote:
>> On 2/12/14 10:26 AM, Mike Christie wrote:
>>> On 2/12/14 10:11 AM, Mike Christie wrote:
>>>> On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>>>>> On 02/07/2014 02:54 AM, Mike Christie wrote:
>>>>>> On 02/06/2014 07:24 PM, Mike Christie wrote:
>>>>>>> On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>>>>>>> We should be issuing STPG synchronously as we need to
>>>>>>>> evaluate the return code on failure.
>>>>>>>>
>>>>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>>>>
>>>>>>> I think we need to also make dm-mpath.c use a non-ordered
>>>>>>> workqueue
>>>>>>> for
>>>>>>> kmpath_handlerd. With this patch and the current ordered
>>>>>>> workqueue in
>>>>>>> dm-mpath I think we will only be able to do one STPG at a time. I
>>>>>>> think
>>>>>>> if we use a normal old non-ordered workqueue then we would be
>>>>>>> limited to
>>>>>>> have max_active STPGs executing.
>>>>>>
>>>>>> I goofed and commented in the order I saw the patches :) I take
>>>>>> this
>>>>>> comment back for this patch, because I see in 16/16 you added a
>>>>>> new
>>>>>> workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>>>>>
>>>>>> For 16/16 though, do we want to make kmpath_aluad a non single
>>>>>> threaded
>>>>>> workqueue? It looks like max_active for single threaded is only
>>>>>> one
>>>>>> work
>>>>>> at a time too.
>>>>>>
>>>>> Well, that was by intention.
>>>>>
>>>>> The workqueue will be triggered very infrequently (basically for
>>>>> every path switch).
>>>>> For implicit ALUA we just need to issue a RTPG to get the new path
>>>>> status; there we might be suffering from single threaded behaviour.
>>>>> But we need to issue it only once and it should be processed
>>>>> reasonably fast.
>>>>> For explicit ALUA we'll have to send an STPG, which has potentially
>>>>> system-wide implications. So sending several to (supposedly)
>>>>> different targets might actually be contraproductive, as the first
>>>>> might have already set the status for the second call.
>>>>> Here we most definitely _want_ serialisation to avoid
>>>>> superfluous STPGs.
>>>>>
>>>>
>>>> What target is this?
>>>>
>>>> For our target it adds a regression. It only affects devices on
>>>> the same
>>>> port group. We then have multiple groups. Before the patch, we could
>>>> failover/failback multiple devices in parallel. To do 64 devices
>>>> it took
>>>> about 3 seconds. With your patch it takes around 3 minutes.
>>>>
>>>
>>> Also, with your pg change patch, I think we can serialize based on
>>> group
>>> and it will do what you want and also allow us to do STPGs to
>>> different
>>> groups in parallel.
>>
>> I guess that would work for me, but I think if you had the same
>> target port in multiple port groups then you could hit the issue you
>> described, right?
>>
> Yes, we might. But it's worth a shot anyway.
>
> So to alleviate all this, this patch:
>
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
> b/drivers/scsi/device_ha
> ndler/scsi_dh_alua.c
> index 569af9d..46cc1ef 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -1353,7 +1353,7 @@ static int __init alua_init(void)
>   {
>          int r;
>
> -       kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
> +       kmpath_aluad = create_workqueue("kmpath_aluad");
>          if (!kmpath_aluad) {
>                  printk(KERN_ERR "kmpath_aluad creation failed.\n");
>                  return -EINVAL;
>
> should be sufficient, right?
>

I think you need to do

alloc_workqueue("kmpath_aluad", WQ_MEM_RECLAIM, 0);

If you use create_workqueue then it sets max_active to 1 like is done 
for create_singlethread_workqueue.

> Could you test and see if it makes a difference?
>

Testing both.

Re: [PATCH 07/16] scsi_dh_alua: Use separate alua_port_group structure

[Bart Van Assche]

On 02/14/14 17:12, Hannes Reinecke wrote:
> The reason I did this was that I don't have to allocate memory
> unnecesarily.
> If I move the allocation out of the spinlock I'll have to recheck
> the list upon insertion to ensure no duplicates are present.
> Upon hitting a duplicate I would have to release the memory again.
> 
> I do agree that GFP_KERNEL is probably not the correct thing here;
> so either I move it to GFP_ATOMIC or we may run into a chance of
> having to release the memory again afterwards.
> 
> Personally I'm inclined to use GFP_ATOMIC, but I'm not sure what'd
> be best.
> 
> What would you suggest here?

I have not yet had a chance to audit all code where list_lock is used.
Is sleeping allowed in each function where list_lock is used ? If so,
how about using a mutex instead of a spinlock ?

Bart.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Mike Christie]

On 2/14/14 5:37 AM, Bart Van Assche wrote:
> On 02/13/14 10:31, Hannes Reinecke wrote:
>> >On 02/12/2014 06:31 PM, Mike Christie wrote:
>>> >>On 2/12/14 10:26 AM, Mike Christie wrote:
>>>> >>>On 2/12/14 10:11 AM, Mike Christie wrote:
>>>>> >>>>On 2/12/14 9:29 AM, Hannes Reinecke wrote:
>>>>>> >>>>>On 02/07/2014 02:54 AM, Mike Christie wrote:
>>>>>>> >>>>>>On 02/06/2014 07:24 PM, Mike Christie wrote:
>>>>>>>> >>>>>>>On 01/31/2014 03:29 AM, Hannes Reinecke wrote:
>>>>>>>>> >>>>>>>>We should be issuing STPG synchronously as we need to
>>>>>>>>> >>>>>>>>evaluate the return code on failure.
>>>>>>>>> >>>>>>>>
>>>>>>>>> >>>>>>>>Signed-off-by: Hannes Reinecke<hare@suse.de>
>>>>>>>> >>>>>>>
>>>>>>>> >>>>>>>I think we need to also make dm-mpath.c use a non-ordered
>>>>>>>> >>>>>>>workqueue
>>>>>>>> >>>>>>>for
>>>>>>>> >>>>>>>kmpath_handlerd. With this patch and the current ordered
>>>>>>>> >>>>>>>workqueue in
>>>>>>>> >>>>>>>dm-mpath I think we will only be able to do one STPG at a time. I
>>>>>>>> >>>>>>>think
>>>>>>>> >>>>>>>if we use a normal old non-ordered workqueue then we would be
>>>>>>>> >>>>>>>limited to
>>>>>>>> >>>>>>>have max_active STPGs executing.
>>>>>>> >>>>>>
>>>>>>> >>>>>>I goofed and commented in the order I saw the patches:)  I take
>>>>>>> >>>>>>this
>>>>>>> >>>>>>comment back for this patch, because I see in 16/16 you added a
>>>>>>> >>>>>>new
>>>>>>> >>>>>>workqueue to scsi_dh_alua to do rtpgs and stpgs.
>>>>>>> >>>>>>
>>>>>>> >>>>>>For 16/16 though, do we want to make kmpath_aluad a non single
>>>>>>> >>>>>>threaded
>>>>>>> >>>>>>workqueue? It looks like max_active for single threaded is only
>>>>>>> >>>>>>one
>>>>>>> >>>>>>work
>>>>>>> >>>>>>at a time too.
>>>>>>> >>>>>>
>>>>>> >>>>>Well, that was by intention.
>>>>>> >>>>>
>>>>>> >>>>>The workqueue will be triggered very infrequently (basically for
>>>>>> >>>>>every path switch).
>>>>>> >>>>>For implicit ALUA we just need to issue a RTPG to get the new path
>>>>>> >>>>>status; there we might be suffering from single threaded behaviour.
>>>>>> >>>>>But we need to issue it only once and it should be processed
>>>>>> >>>>>reasonably fast.
>>>>>> >>>>>For explicit ALUA we'll have to send an STPG, which has potentially
>>>>>> >>>>>system-wide implications. So sending several to (supposedly)
>>>>>> >>>>>different targets might actually be contraproductive, as the first
>>>>>> >>>>>might have already set the status for the second call.
>>>>>> >>>>>Here we most definitely_want_  serialisation to avoid
>>>>>> >>>>>superfluous STPGs.
>>>>>> >>>>>
>>>>> >>>>
>>>>> >>>>What target is this?
>>>>> >>>>
>>>>> >>>>For our target it adds a regression. It only affects devices on
>>>>> >>>>the same
>>>>> >>>>port group. We then have multiple groups. Before the patch, we could
>>>>> >>>>failover/failback multiple devices in parallel. To do 64 devices
>>>>> >>>>it took
>>>>> >>>>about 3 seconds. With your patch it takes around 3 minutes.
>>>>> >>>>
>>>> >>>
>>>> >>>Also, with your pg change patch, I think we can serialize based on
>>>> >>>group
>>>> >>>and it will do what you want and also allow us to do STPGs to
>>>> >>>different
>>>> >>>groups in parallel.
>>> >>
>>> >>I guess that would work for me, but I think if you had the same
>>> >>target port in multiple port groups then you could hit the issue you
>>> >>described, right?
>>> >>
>> >Yes, we might. But it's worth a shot anyway.
>> >
>> >So to alleviate all this, this patch:
>> >
>> >diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
>> >b/drivers/scsi/device_ha
>> >ndler/scsi_dh_alua.c
>> >index 569af9d..46cc1ef 100644
>> >--- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> >+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> >@@ -1353,7 +1353,7 @@ static int __init alua_init(void)
>> >  {
>> >         int r;
>> >
>> >-       kmpath_aluad = create_singlethread_workqueue("kmpath_aluad");
>> >+       kmpath_aluad = create_workqueue("kmpath_aluad");
>> >         if (!kmpath_aluad) {
>> >                 printk(KERN_ERR "kmpath_aluad creation failed.\n");
>> >                 return -EINVAL;
>> >
>> >should be sufficient, right?
> I think this will only be sufficient if there are more CPU threads
> available than the number of LUNs for which an STPG has to be issued.

Ah shoot, you reminded me of some other issue. With the above command we 
only can do 1 unit of work at a time. With the alloc_workqueue example I 
sent in the other mail, I think it would create a new problem.

alloc_workqueue with max_active=0 looks like it has the wq code 
dynamically create worker threads as needed, so with that we could end 
up with a lot of threads and can failover lots of devices (512 with the 
current max) in parallel. The threads will even get reaped when they are 
not needed. However, trying to create lots of threads at failover time 
might have issues, and in general creating 32 or 64 threads or more is 
probably not something we want to do.

Re: [PATCH 16/16] scsi_dh_alua: Use workqueue for RTPG

[Bart Van Assche]

On 01/31/14 10:30, Hannes Reinecke wrote:
> +static void alua_check(struct scsi_device *sdev)
> +{
> +	struct alua_dh_data *h = get_alua_data(sdev);
> +	struct alua_port_group *pg;
> +
> +	if (!h)
> +		return;
> +
> +	rcu_read_lock();
> +	pg = rcu_dereference(h->pg);
> +	if (pg) {
> +		kref_get(&pg->kref);
> +		rcu_read_unlock();
> +		alua_rtpg_queue(pg, sdev, NULL);
> +		kref_put(&pg->kref, release_port_group);
> +	} else
> +		rcu_read_unlock();
> +}

>From the implementation it seems to me like the target port group data
is kept just as long as it is in use ? Has it been considered to keep
that data as long as the sdev exists instead ? I think that would result
in far fewer reference count manipulations and hence in code that is
easier to read and to verify.

Thanks,

Bart.

Re: [PATCH 08/16] scsi_dh_alua: parse target device id

[Bart Van Assche]

On 02/14/14 13:21, Hannes Reinecke wrote:
> On 02/14/2014 01:09 PM, Bart Van Assche wrote:
>> On 01/31/14 10:29, Hannes Reinecke wrote:
>>> +	if (!target_id_size) {
>>> +		/* Check for EMC Clariion extended inquiry */
>>> +		if (!strncmp(sdev->vendor, "DGC     ", 8) &&
>>> +		    sdev->inquiry_len > 160) {
>>> +			target_id_size = sdev->inquiry[160];
>>> +			target_id = sdev->inquiry + 161;
>>> +			strcpy(target_id_str, "emc.");
>>> +			memcpy(target_id_str + 4, target_id, target_id_size);
>>> +		}
>>> +		/* Check for HP EVA extended inquiry */
>>> +		if (!strncmp(sdev->vendor, "HP      ", 8) &&
>>> +		    !strncmp(sdev->model, "HSV", 3) &&
>>> +		    sdev->inquiry_len > 170) {
>>> +			target_id_size = 16;
>>> +			target_id = sdev->inquiry + 154;
>>> +			strcpy(target_id_str, "naa.");
>>> +			memcpy(target_id_str + 4, target_id, target_id_size);
>>> +		}
>>> +	}
>>
>> Being able to identify a storage array unambiguously is essential for
>> the new ALUA device handler algorithm introduced by this patch series.
>> What if a new storage array is introduced that is not covered by one of
>> the heuristics in this patch ? Has it been considered to let storage
>> array identification occur in user space instead of in the kernel ?
>
> As noted in the other mail, if we cannot decipher the array
> identifier the whole thing degrades into one pg per sdev.
> IE we cannot bunch RTPG / STPG for those.
> The idea here is that those arrays do not have any limitations,
> so that no special treatment is required.
> 
> Adding identifiers from userspace are a bit tricky; there is no
> actual sysfs structure for the device handler.
> Which we would need if we were to use these tricks.

The reason I started asking about the target device ID is because for
Fusion-io ION devices we also encode the device ID in the INQUIRY
response (bytes 36..55; left aligned; padded with '\0'). The vendor name
is "FUSIONIO" and the first three characters of the product name are
"ION". Can you add  the necessary support in the device identification
code ? Seems like it's time we get you a pair of those devices for
testing :-) An example:

# sg_inq -rl256 /dev/sdb | dd bs=1 count=20 skip=36 2>/dev/null | od -c
0000000   U   S   E   2   4   0   F   W   6   5  \0  \0  \0  \0  \0  \0
0000020  \0  \0  \0  \0
0000024

Thanks,

Bart.

Re: [PATCH 08/16] scsi_dh_alua: parse target device id

[Hannes Reinecke]

On 02/17/2014 03:05 PM, Bart Van Assche wrote:
> On 02/14/14 13:21, Hannes Reinecke wrote:
>> On 02/14/2014 01:09 PM, Bart Van Assche wrote:
>>> On 01/31/14 10:29, Hannes Reinecke wrote:
>>>> +	if (!target_id_size) {
>>>> +		/* Check for EMC Clariion extended inquiry */
>>>> +		if (!strncmp(sdev->vendor, "DGC     ", 8) &&
>>>> +		    sdev->inquiry_len > 160) {
>>>> +			target_id_size = sdev->inquiry[160];
>>>> +			target_id = sdev->inquiry + 161;
>>>> +			strcpy(target_id_str, "emc.");
>>>> +			memcpy(target_id_str + 4, target_id, target_id_size);
>>>> +		}
>>>> +		/* Check for HP EVA extended inquiry */
>>>> +		if (!strncmp(sdev->vendor, "HP      ", 8) &&
>>>> +		    !strncmp(sdev->model, "HSV", 3) &&
>>>> +		    sdev->inquiry_len > 170) {
>>>> +			target_id_size = 16;
>>>> +			target_id = sdev->inquiry + 154;
>>>> +			strcpy(target_id_str, "naa.");
>>>> +			memcpy(target_id_str + 4, target_id, target_id_size);
>>>> +		}
>>>> +	}
>>>
>>> Being able to identify a storage array unambiguously is essential for
>>> the new ALUA device handler algorithm introduced by this patch series.
>>> What if a new storage array is introduced that is not covered by one of
>>> the heuristics in this patch ? Has it been considered to let storage
>>> array identification occur in user space instead of in the kernel ?
>>
>> As noted in the other mail, if we cannot decipher the array
>> identifier the whole thing degrades into one pg per sdev.
>> IE we cannot bunch RTPG / STPG for those.
>> The idea here is that those arrays do not have any limitations,
>> so that no special treatment is required.
>>
>> Adding identifiers from userspace are a bit tricky; there is no
>> actual sysfs structure for the device handler.
>> Which we would need if we were to use these tricks.
> 
> The reason I started asking about the target device ID is because for
> Fusion-io ION devices we also encode the device ID in the INQUIRY
> response (bytes 36..55; left aligned; padded with '\0'). The vendor name
> is "FUSIONIO" and the first three characters of the product name are
> "ION". Can you add  the necessary support in the device identification
> code ? Seems like it's time we get you a pair of those devices for
> testing :-) An example:
> 
> # sg_inq -rl256 /dev/sdb | dd bs=1 count=20 skip=36 2>/dev/null | od -c
> 0000000   U   S   E   2   4   0   F   W   6   5  \0  \0  \0  \0  \0  \0
> 0000020  \0  \0  \0  \0
> 0000024
> 
If the product ID is actually the array identification and not a
generic name, sure.
But if all ION arrays with the same FW level identify itself with
the same identifier it's kinda pointless :-)
Otherwise, yeah, that's easily done.

But you might want to talk to your FW folks; if they were to update
VPD page 0x83 to include an target-specific identifier we'd be all
set ...

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 08/16] scsi_dh_alua: parse target device id

[Bart Van Assche]

On 02/17/14 15:09, Hannes Reinecke wrote:
> If the product ID is actually the array identification and not a
> generic name, sure.
> But if all ION arrays with the same FW level identify itself with
> the same identifier it's kinda pointless :-)
> Otherwise, yeah, that's easily done.
> 
> But you might want to talk to your FW folks; if they were to update
> VPD page 0x83 to include an target-specific identifier we'd be all
> set ...

The product ID indeed identifies an array.

Updating VPD page 0x83 can be done easily for new FW releases. However,
the target identification approach explained in the previous e-mail is
already in use at several customer sites and I'm not sure it will be
possible to convince all of them to upgrade ...

Bart.

Re: [PATCH 08/16] scsi_dh_alua: parse target device id

[Hannes Reinecke]

On 02/17/2014 03:27 PM, Bart Van Assche wrote:
> On 02/17/14 15:09, Hannes Reinecke wrote:
>> If the product ID is actually the array identification and not a
>> generic name, sure.
>> But if all ION arrays with the same FW level identify itself with
>> the same identifier it's kinda pointless :-)
>> Otherwise, yeah, that's easily done.
>>
>> But you might want to talk to your FW folks; if they were to update
>> VPD page 0x83 to include an target-specific identifier we'd be all
>> set ...
> 
> The product ID indeed identifies an array.
> 
> Updating VPD page 0x83 can be done easily for new FW releases. However,
> the target identification approach explained in the previous e-mail is
> already in use at several customer sites and I'm not sure it will be
> possible to convince all of them to upgrade ...
> 
Ok, fair enough.

Will be including it.

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 08/16] scsi_dh_alua: parse target device id

[Bart Van Assche]

On 02/17/14 15:33, Hannes Reinecke wrote:
> On 02/17/2014 03:27 PM, Bart Van Assche wrote:
>> Updating VPD page 0x83 can be done easily for new FW releases. However,
>> the target identification approach explained in the previous e-mail is
>> already in use at several customer sites and I'm not sure it will be
>> possible to convince all of them to upgrade ...
>>
> Ok, fair enough.
> 
> Will be including it.

This has been discussed further internally. The outcome of the
discussion is that a target specific identifier will be added in the
Device Identification VPD page and also that we will take care of
upgrading the customer systems. In other words, no further action is
needed from your side. Sorry for the noise.

Bart.

Re: [PATCH 04/16] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

On 02/14/2014 07:21 PM, Mike Christie wrote:
> On 2/14/14 5:37 AM, Bart Van Assche wrote:
[ .. ]
>> I think this will only be sufficient if there are more CPU threads
>> available than the number of LUNs for which an STPG has to be issued.
> 
> Ah shoot, you reminded me of some other issue. With the above
> command we only can do 1 unit of work at a time. With the
> alloc_workqueue example I sent in the other mail, I think it would
> create a new problem.
> 
> alloc_workqueue with max_active=0 looks like it has the wq code
> dynamically create worker threads as needed, so with that we could
> end up with a lot of threads and can failover lots of devices (512
> with the current max) in parallel. The threads will even get reaped
> when they are not needed. However, trying to create lots of threads
> at failover time might have issues, and in general creating 32 or 64
> threads or more is probably not something we want to do.

Another followup: Mike is, as usual, right.
I need to use 'alloc_workqueue' to be able to have more than one
worker at a time.
And also Bart is right in that it makes sense to have STPG run
asynchronously.

However, in doing so I'll be bunching together STPG activations from
several request_queues, and will be deferring I/O on those
queues during STPG update.
So to avoid stalled I/O I'll need to kick each of the queues again
after STPG is finished, right?

Hmm. Let's see how this will look like.

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: [PATCHv2 00/16] scsi_dh_alua updates

[Bart Van Assche]

On 01/31/14 10:29, Hannes Reinecke wrote:
> here's an update for the ALUA device handler I've been hoarding
> for quite some time. The major bit here is the asynchronous
> RTPG handling. With the original design we would treat every
> LUN independently, despite the fact that several LUNs might
> in fact belong to the same target port group. So any
> change on one LUN will affect the others, too.
> And we now can treat LUNs in 'transitioning' ALUA mode
> correctly, as now we'll be blocking any I/O in the prep_fn()
> until the controller is in a working state again.

Also for this patch series, I think the cached INQUIRY data should be
refreshed at least after an INQUIRY DATA HAS CHANGED unit attention code
has been received.

Bart.

Re: [PATCH 01/16] scsi_dh_alua: Improve error handling

[Jeremy Linton]

On 1/31/2014 3:29 AM, Hannes Reinecke wrote:
> Improve error handling and use standard logging functions
> instead of hand-crafted ones.
> 
> @@ -182,11 +185,13 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
>  			    bool rtpg_ext_hdr_req)

	Can you shorten the timeouts for this?

	I think submit_rtpg failures are causing a boot failure on a SLES 11 SP2
machine (3.0.101-0.7.17-default). The basic configuration is an ACNC jetstor,
mapped as a raw LUN through a vmware (esxi 5.5) virtual mptsas adapter. The sd
driver seems to be jamming up udev's creation of the boot /dev entries with a
~360 second delay probing /dev/sdb. This causes the /dev/sda entry for the root
partition (on another device) not to show up in time.


(output from a screen capture sent to me/ocr'ed)

[ 2.918164] sdev dma_alignment 511
[ 3.917810] mptsas2 ioc02 attaching ssp device2 fw_channel 0, fu_id 1, phg 1,
sas_addr 0x5000
c29f6c78b0af
[ 3.920204] scsi target0:0:1: mptsas2: ioc02: add device: fw_channel 0, fw_id 1,
phy 1, sas_add
r 0x5000c29f6c78b0af
[ 3.921923] scsi 02021202 Direct-Access JetStor VMB-00 R001 PQ: 0 ANSI: 5
[ 4.824734] scsi 02021202 mptscsih: ioc0: qdepth=64, tagged=1, simple=1,
ordered:0, scsi_level:6, cmd_que=1
[ 4.827184] sdev dma_alignment 511
[ 4.828006] scsi 02021202 alua: supports implicit TPGS
[ 4.828897] scsi 02021202 alua: target naa.2000001B4D01CA59 port group 01 rel
port 1b
[ 4.830084] scsi 02021202 alua: Attached
[ 4.834297] sd 0:0:0:0: [sda] 33554432 512-byte logical blocks: (17.1 GB/16.0 GiB)
[ 4.835141] sd 02020202 [sda] Write Protect is off
[ 4.835954] sd 02020202 [sda] Mode Sense2 61 00 00 00
[ 4.835994] sd 02020202 [sda] Cache data unavailable
[ 4.836808] sd 02020202 [sda] Assuming drive cache2 write through
[ 4.837718] sd 0:0:1:02 [sdb] 390623744 512-byte logical blocks: (199 GB/186 GiB)
[ 4.837756] sd 02020202 [sda] Cache data unavailable
[ 4.837758] sd 02020202 [sda] Assuming drive cache2 write through
[ 4.870098] sda2 sdal sda2
[ 4.871675] sd 02020202 [sda] Cache data unavailable
[ 4.872461] sd 02020202 [sda] Assuming drive cache2 write through
[ 4.873243] sd 02020202 [sda] Attached SCSI disk
[ 5.622838] sd 02021202 [sdb] Write Protect is off
[ 5.624410] sd 02021202 [sdb] Mode Sense2 bf 00 00 08
[ 5.624654] sd 02021202 [sdb] Write cache2 enabled, read cache2 enabled, doesn’t
support DPO or FUA
[ 364.290276] sd 02021202 timing out command, waited 360s
[ 364.291495] sd 02021202 alua2 rtpg failed, Result2 hostbgte=DID_OK
driuerbgte:DRIVER_OK
[ 364.309198] sdb: sdbl
[ 367.890274] sd 02021202 [sdb] Attached SCSI disk


vmware says this about the disk:

naa.6001b4d4373468630000000000000000
   Device Display Name: JetStor Fibre Channel Disk (naa.6001b4d43734686300000000
                                            00000000)
   Storage Array Type: VMW_SATP_ALUA
   Storage Array Type Device Config: {implicit_support=on;explicit_support=off;

explicit_allow=on;alua_followover=on;{TPG_id=1,TPG_state=ANO}}
   Path Selection Policy: VMW_PSP_MRU
   Path Selection Policy Device Config: Current Path=vmhba5:C0:T0:L1
   Path Selection Policy Device Custom Config:
   Working Paths: vmhba5:C0:T0:L1
   Is Local SAS Device: false
   Is Boot USB Device: false


--
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 01/16] scsi_dh_alua: Improve error handling

[Hannes Reinecke]

On 03/07/2014 12:43 AM, Jeremy Linton wrote:
> On 1/31/2014 3:29 AM, Hannes Reinecke wrote:
>> Improve error handling and use standard logging functions
>> instead of hand-crafted ones.
>>
>> @@ -182,11 +185,13 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
>>  			    bool rtpg_ext_hdr_req)
> 
> 	Can you shorten the timeouts for this?
> 
> 	I think submit_rtpg failures are causing a boot failure on a SLES 11 SP2
> machine (3.0.101-0.7.17-default). The basic configuration is an ACNC jetstor,
> mapped as a raw LUN through a vmware (esxi 5.5) virtual mptsas adapter. The sd
> driver seems to be jamming up udev's creation of the boot /dev entries with a
> ~360 second delay probing /dev/sdb. This causes the /dev/sda entry for the root
> partition (on another device) not to show up in time.
> 
> 
> (output from a screen capture sent to me/ocr'ed)
> 
> [ 2.918164] sdev dma_alignment 511
> [ 3.917810] mptsas2 ioc02 attaching ssp device2 fw_channel 0, fu_id 1, phg 1,
> sas_addr 0x5000
> c29f6c78b0af
> [ 3.920204] scsi target0:0:1: mptsas2: ioc02: add device: fw_channel 0, fw_id 1,
> phy 1, sas_add
> r 0x5000c29f6c78b0af
> [ 3.921923] scsi 02021202 Direct-Access JetStor VMB-00 R001 PQ: 0 ANSI: 5
> [ 4.824734] scsi 02021202 mptscsih: ioc0: qdepth=64, tagged=1, simple=1,
> ordered:0, scsi_level:6, cmd_que=1
> [ 4.827184] sdev dma_alignment 511
> [ 4.828006] scsi 02021202 alua: supports implicit TPGS
> [ 4.828897] scsi 02021202 alua: target naa.2000001B4D01CA59 port group 01 rel
> port 1b
> [ 4.830084] scsi 02021202 alua: Attached
> [ 4.834297] sd 0:0:0:0: [sda] 33554432 512-byte logical blocks: (17.1 GB/16.0 GiB)
> [ 4.835141] sd 02020202 [sda] Write Protect is off
> [ 4.835954] sd 02020202 [sda] Mode Sense2 61 00 00 00
> [ 4.835994] sd 02020202 [sda] Cache data unavailable
> [ 4.836808] sd 02020202 [sda] Assuming drive cache2 write through
> [ 4.837718] sd 0:0:1:02 [sdb] 390623744 512-byte logical blocks: (199 GB/186 GiB)
> [ 4.837756] sd 02020202 [sda] Cache data unavailable
> [ 4.837758] sd 02020202 [sda] Assuming drive cache2 write through
> [ 4.870098] sda2 sdal sda2
> [ 4.871675] sd 02020202 [sda] Cache data unavailable
> [ 4.872461] sd 02020202 [sda] Assuming drive cache2 write through
> [ 4.873243] sd 02020202 [sda] Attached SCSI disk
> [ 5.622838] sd 02021202 [sdb] Write Protect is off
> [ 5.624410] sd 02021202 [sdb] Mode Sense2 bf 00 00 08
> [ 5.624654] sd 02021202 [sdb] Write cache2 enabled, read cache2 enabled, doesn’t
> support DPO or FUA
> [ 364.290276] sd 02021202 timing out command, waited 360s
> [ 364.291495] sd 02021202 alua2 rtpg failed, Result2 hostbgte=DID_OK
> driuerbgte:DRIVER_OK
> [ 364.309198] sdb: sdbl
> [ 367.890274] sd 02021202 [sdb] Attached SCSI disk
> 
> 
> vmware says this about the disk:
> 
> naa.6001b4d4373468630000000000000000
>    Device Display Name: JetStor Fibre Channel Disk (naa.6001b4d43734686300000000
>                                             00000000)
>    Storage Array Type: VMW_SATP_ALUA
>    Storage Array Type Device Config: {implicit_support=on;explicit_support=off;
> 
> explicit_allow=on;alua_followover=on;{TPG_id=1,TPG_state=ANO}}
>    Path Selection Policy: VMW_PSP_MRU
>    Path Selection Policy Device Config: Current Path=vmhba5:C0:T0:L1
>    Path Selection Policy Device Custom Config:
>    Working Paths: vmhba5:C0:T0:L1
>    Is Local SAS Device: false
>    Is Boot USB Device: false
> 
> 
Hehe. Infamous 'timing out command' message.
That smells more like a weird interaction going on there.

Can you file a bug with bugzilla.novell.com and assign it to me?

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 01/16] scsi_dh_alua: Improve error handling

[Jeremy Linton]

On 3/7/2014 1:12 AM, Hannes Reinecke wrote:
> Can you file a bug with bugzilla.novell.com and assign it to me?

	Thanks, its bug 867371 https://bugzilla.novell.com/show_bug.cgi?id=867371.

Re: [PATCH 16/16] scsi_dh_alua: Use workqueue for RTPG

[Stewart, Sean]

On Fri, 2014-01-31 at 10:30 +0100, Hannes Reinecke wrote:
> @@ -797,37 +838,40 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  		off = 8 + (ucp[7] * 4);
>  	}
>  
> -	sdev_printk(KERN_INFO, sdev,
> -		    "%s: port group %02x state %c %s supports %c%c%c%c%c%c%c\n",
> -		    ALUA_DH_NAME, pg->group_id, print_alua_state(pg->state),
> -		    pg->pref ? "preferred" : "non-preferred",
> -		    valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
> -		    valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
> -		    valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
> -		    valid_states&TPGS_SUPPORT_UNAVAILABLE?'U':'u',
> -		    valid_states&TPGS_SUPPORT_STANDBY?'S':'s',
> -		    valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
> -		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
Hannes,

I was wondering why this was changed from an sdev_printk to a printk? I
can see the AAS of a TPG on a target, but with it this way I do not know
with respect to what logical unit it is.

> +	printk(KERN_INFO "%s: target %s port group %02x state %c %s "
> +	       "supports %c%c%c%c%c%c%c\n", ALUA_DH_NAME, pg->target_id_str,
> +	       pg->group_id, print_alua_state(pg->state),
> +	       pg->pref ? "preferred" : "non-preferred",
> +	       valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
> +	       valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
> +	       valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
> +	       valid_states&TPGS_SUPPORT_UNAVAILABLE?'U':'u',
> +	       valid_states&TPGS_SUPPORT_STANDBY?'S':'s',
> +	       valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
> +	       valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
>  
>  

Thanks,
Sean

Re: [PATCH 16/16] scsi_dh_alua: Use workqueue for RTPG

[Hannes Reinecke]

On 04/29/2014 11:47 PM, Stewart, Sean wrote:
> On Fri, 2014-01-31 at 10:30 +0100, Hannes Reinecke wrote:
>> @@ -797,37 +838,40 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>>   		off = 8 + (ucp[7] * 4);
>>   	}
>>
>> -	sdev_printk(KERN_INFO, sdev,
>> -		    "%s: port group %02x state %c %s supports %c%c%c%c%c%c%c\n",
>> -		    ALUA_DH_NAME, pg->group_id, print_alua_state(pg->state),
>> -		    pg->pref ? "preferred" : "non-preferred",
>> -		    valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
>> -		    valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
>> -		    valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
>> -		    valid_states&TPGS_SUPPORT_UNAVAILABLE?'U':'u',
>> -		    valid_states&TPGS_SUPPORT_STANDBY?'S':'s',
>> -		    valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
>> -		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
> Hannes,
>
> I was wondering why this was changed from an sdev_printk to a printk? I
> can see the AAS of a TPG on a target, but with it this way I do not know
> with respect to what logical unit it is.
>
>> +	printk(KERN_INFO "%s: target %s port group %02x state %c %s "
>> +	       "supports %c%c%c%c%c%c%c\n", ALUA_DH_NAME, pg->target_id_str,
>> +	       pg->group_id, print_alua_state(pg->state),
>> +	       pg->pref ? "preferred" : "non-preferred",
>> +	       valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
>> +	       valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
>> +	       valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
>> +	       valid_states&TPGS_SUPPORT_UNAVAILABLE?'U':'u',
>> +	       valid_states&TPGS_SUPPORT_STANDBY?'S':'s',
>> +	       valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
>> +	       valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
>>
>>
>
Reasoning was the the 'target port' is actually independent on the 
scsi device; there might be several scsi devices pointing to the 
same target port. So using 'sdev_printk' here wouldn't be entirely 
correct.

But you are right, we're losing the information about the LUN here.
So I'll be reverting that bit.

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 11/16] scsi_dh_alua: multipath failover fails with error 15

[Bart Van Assche]

On 01/31/14 10:29, Hannes Reinecke wrote:
> When a path is already optimized multipath failover will fail
> with the message
> Could not failover device X:Y: Handler scsi_dh_alua Error 15
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index a1c69bb..8ea35a9 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -851,6 +851,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  		return SCSI_DH_RETRY;
>  	}
>  	switch (pg->state) {
> +	case TPGS_STATE_OPTIMIZED:
> +		return SCSI_DH_OK;
>  	case TPGS_STATE_NONOPTIMIZED:
>  		if ((pg->flags & ALUA_OPTIMIZE_STPG) &&
>  		    (!pg->pref) &&
> @@ -865,10 +867,11 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  		break;
>  	case TPGS_STATE_TRANSITIONING:
>  		return SCSI_DH_RETRY;
> -		break;
>  	default:
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: stpg failed, unhandled TPGS state %d",
> +			    ALUA_DH_NAME, pg->state);
>  		return SCSI_DH_NOSYS;
> -		break;
>  	}
>  	/* Set state to transitioning */
>  	pg->state = TPGS_STATE_TRANSITIONING;

(replying to an e-mail of last year)

Hello Hannes,

Our Q.A. team started to run into the issue that is fixed by this patch.
Do you have the time to resend this patch series or would you rather
prefer that I split out this patch, test it and post it with a "Cc:
stable" tag ?

Thanks,

Bart.

Re: [PATCH 11/16] scsi_dh_alua: multipath failover fails with error 15

[Hannes Reinecke]

On 03/30/2015 02:30 PM, Bart Van Assche wrote:
> On 01/31/14 10:29, Hannes Reinecke wrote:
>> When a path is already optimized multipath failover will fail
>> with the message
>> Could not failover device X:Y: Handler scsi_dh_alua Error 15
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/device_handler/scsi_dh_alua.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
>> index a1c69bb..8ea35a9 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -851,6 +851,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
>>  		return SCSI_DH_RETRY;
>>  	}
>>  	switch (pg->state) {
>> +	case TPGS_STATE_OPTIMIZED:
>> +		return SCSI_DH_OK;
>>  	case TPGS_STATE_NONOPTIMIZED:
>>  		if ((pg->flags & ALUA_OPTIMIZE_STPG) &&
>>  		    (!pg->pref) &&
>> @@ -865,10 +867,11 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
>>  		break;
>>  	case TPGS_STATE_TRANSITIONING:
>>  		return SCSI_DH_RETRY;
>> -		break;
>>  	default:
>> +		sdev_printk(KERN_INFO, sdev,
>> +			    "%s: stpg failed, unhandled TPGS state %d",
>> +			    ALUA_DH_NAME, pg->state);
>>  		return SCSI_DH_NOSYS;
>> -		break;
>>  	}
>>  	/* Set state to transitioning */
>>  	pg->state = TPGS_STATE_TRANSITIONING;
> 
> (replying to an e-mail of last year)
> 
> Hello Hannes,
> 
> Our Q.A. team started to run into the issue that is fixed by this patch.
> Do you have the time to resend this patch series or would you rather
> prefer that I split out this patch, test it and post it with a "Cc:
> stable" tag ?
> 
Well, I'll have to repost the patch-series anyway; unfortunately
it's being held off by some unresolved issues.
Best I'll split if off into 'real' bugfixes and the actual
scsi_dh_alua updates.

Cheers,

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

[PATCH 09/16] scsi_dh_alua: simplify sense code handling

[Hannes Reinecke]

Most sense code is already handled in the generic
code, so we shouldn't be adding special cases here.
However, when doing so we need to check for
unit attention whenever we're sending an internal
command.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 9d712be..e297851 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -388,6 +388,8 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			goto out;
 		}
 		err = alua_check_sense(sdev, &sense_hdr);
+		if (sense_hdr.sense_key == UNIT_ATTENTION)
+			err = ADD_TO_MLQUEUE;
 		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
 			goto retry;
 		if (err != SUCCESS) {
@@ -617,21 +619,6 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * LUN Not Accessible - ALUA state transition
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0b)
-			/*
-			 * LUN Not Accessible -- Target port in standby state
-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0c)
-			/*
-			 * LUN Not Accessible -- Target port in unavailable state
-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x12)
-			/*
-			 * LUN Not Ready -- Offline
-			 */
-			return SUCCESS;
 		break;
 	case UNIT_ATTENTION:
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
@@ -646,7 +633,7 @@ static int alua_check_sense(struct scsi_device *sdev,
 			return ADD_TO_MLQUEUE;
 		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x01)
 			/*
-			 * Mode Parameters Changed
+			 * Mode parameter changed
 			 */
 			return ADD_TO_MLQUEUE;
 		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06)
@@ -659,18 +646,6 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * Implicit ALUA state transition failed
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x03)
-			/*
-			 * Inquiry data has changed
-			 */
-			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x0e)
-			/*
-			 * REPORTED_LUNS_DATA_HAS_CHANGED is reported
-			 * when switching controllers on targets like
-			 * Intel Multi-Flex. We can just retry.
-			 */
-			return ADD_TO_MLQUEUE;
 		break;
 	}
 
@@ -735,6 +710,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		}
 
 		err = alua_check_sense(sdev, &sense_hdr);
+		if (sense_hdr.sense_key == UNIT_ATTENTION)
+			err = ADD_TO_MLQUEUE;
 		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry, ",
 				    ALUA_DH_NAME);
-- 
1.7.12.4