[PATCH 00/20] ALUA device handler update, part II

[PATCH 00/20] ALUA device handler update, part II

[Hannes Reinecke]

Hi all,

as promised here is now the second part of my ALUA device handler update.
This contains a major rework of the ALUA device handler as execution is
moved onto a workqueue. This has the advantage that we avoid having to
do multiple calls to the same LUN (as happens frequently when failing
over a LUN with several paths) and finally retries are handled correctly.
As some arrays are only capable of handling one STPG at a time I've added
a module parameter 'sync_stpg' which switches to a singlethreaded
workqueue, thereby effectively synchronize STPG handling.
Thanks to Bart for this suggestion.

As usual, comments and reviews are welcome.

Hannes Reinecke (20):
  scsi_dh_alua: Pass buffer as function argument
  scsi_dh_alua: separate out alua_stpg()
  scsi_dh_alua: Make stpg synchronous
  scsi_dh_alua: call alua_rtpg() if stpg fails
  scsi_dh_alua: switch to scsi_execute_req_flags()
  scsi_dh_alua: Use separate alua_port_group structure
  scsi_dh_alua: allocate RTPG buffer separately
  scsi_dh_alua: use unique device id
  scsi_dh_alua: simplify alua_initialize()
  revert commit a8e5a2d593cb ("[SCSI] scsi_dh_alua: ALUA handler attach
    should succeed while TPG is transitioning")
  scsi_dh_alua: Use workqueue for RTPG
  scsi_dh_alua: Allow workqueue to run synchronously
  scsi_dh_alua: Recheck state on unit attention
  scsi_dh_alua: update all port states
  scsi_dh_alua: Send TEST UNIT READY to poll for transitioning
  scsi_dh: add 'rescan' callback
  scsi: Add 'access_state' attribute
  scsi_dh_alua: use common definitions for ALUA state
  scsi_dh_alua: update 'access_state' field
  scsi_dh_alua: Update version to 2.0

 drivers/scsi/device_handler/scsi_dh_alua.c | 977 ++++++++++++++++++++---------
 drivers/scsi/scsi_lib.c                    |   1 +
 drivers/scsi/scsi_scan.c                   |   9 +-
 drivers/scsi/scsi_sysfs.c                  |  49 ++
 include/scsi/scsi_device.h                 |   1 +
 include/scsi/scsi_dh.h                     |   2 +
 include/scsi/scsi_proto.h                  |  13 +
 7 files changed, 745 insertions(+), 307 deletions(-)

-- 
1.8.5.6

[PATCH 01/20] scsi_dh_alua: Pass buffer as function argument

[Hannes Reinecke]

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

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 5a328bf..df71e85 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -135,12 +135,13 @@ static struct request *get_alua_req(struct scsi_device *sdev,
  * 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 = 0;
 
-	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;
@@ -148,14 +149,14 @@ 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;
-	put_unaligned_be32(h->bufflen, &rq->cmd[6]);
+	put_unaligned_be32(bufflen, &rq->cmd[6]);
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
 
-	rq->sense = h->sense;
+	rq->sense = sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;
 
@@ -446,7 +447,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);
+	retval = submit_rtpg(sdev, h->buff, h->bufflen, h->sense, h->flags);
 	if (retval) {
 		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {
-- 
1.8.5.6

[PATCH 02/20] scsi_dh_alua: separate out alua_stpg()

[Hannes Reinecke]

Separate out SET TARGET PORT GROUP functionality into a separate
function alua_stpg().

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index df71e85..7c66e4a 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -579,6 +579,65 @@ 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 if the target port group
+ * state is found to be in 'transitioning'.
+ * If SCSI_DH_OK is returned the passed-in 'fn' function
+ * this function will take care of executing 'fn'.
+ * Otherwise 'fn' should be executed by the caller with the
+ * returned error code.
+ */
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h,
+			  activate_complete fn, void *data)
+{
+	int err = SCSI_DH_OK;
+	int stpg = 0;
+
+	if (!(h->tpgs & TPGS_MODE_EXPLICIT))
+		/* Only implicit ALUA supported */
+		goto out;
+
+	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)
+			h->callback_fn = h->callback_data = NULL;
+		else
+			fn = NULL;
+	}
+out:
+	if (fn)
+		fn(data, err);
+
+	return err;
+}
+
+/*
  * alua_initialize - Initialize ALUA state
  * @sdev: the device to be initialized
  *
@@ -655,7 +714,6 @@ static int alua_activate(struct scsi_device *sdev,
 {
 	struct alua_dh_data *h = sdev->handler_data;
 	int err = SCSI_DH_OK;
-	int stpg = 0;
 
 	err = alua_rtpg(sdev, h, 1);
 	if (err != SCSI_DH_OK)
@@ -664,41 +722,10 @@ 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, fn, data);
 
 out:
-	if (fn)
+	if (err != SCSI_DH_OK && fn)
 		fn(data, err);
 	return 0;
 }
-- 
1.8.5.6

Re: [PATCH 02/20] scsi_dh_alua: separate out alua_stpg()

[Christoph Hellwig]

The stpg variable could be a bool.

Otherwise looks good,

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

[PATCH 03/20] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

The 'activate_complete' function needs to be executed after
stpg has finished, so we can as well execute stpg synchronously
and call the function directly.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 7c66e4a..609691f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -169,81 +169,28 @@ 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 (scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-				 &sense_hdr)) {
-		if (sense_hdr.sense_key == NOT_READY &&
-		    sense_hdr.asc == 0x04 && sense_hdr.ascq == 0x0a) {
-			/* ALUA state transition already in progress */
-			err = SCSI_DH_OK;
-			goto done;
-		}
-		if (sense_hdr.sense_key == UNIT_ATTENTION) {
-			err = SCSI_DH_RETRY;
-			goto done;
-		}
-		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
-			    ALUA_DH_NAME);
-		scsi_print_sense_hdr(h->sdev, ALUA_DH_NAME, &sense_hdr);
-		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 = 0;
 
 	/* Prepare the data buffer */
-	memset(h->buff, 0, stpg_len);
-	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
-	put_unaligned_be16(h->group_id, &h->buff[6]);
+	memset(stpg_data, 0, stpg_len);
+	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
+	put_unaligned_be16(group_id, &stpg_data[6]);
 
-	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;
+		return DRIVER_BUSY << 24;
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_OUT;
@@ -251,13 +198,17 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 	put_unaligned_be32(stpg_len, &rq->cmd[6]);
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_OUT);
 
-	rq->sense = h->sense;
+	rq->sense = sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;
-	rq->end_io_data = h;
 
-	blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio);
-	return SCSI_DH_OK;
+	blk_execute_rq(rq->q, NULL, rq, 1);
+	if (rq->errors)
+		err = rq->errors;
+
+	blk_put_request(rq);
+
+	return err;
 }
 
 /*
@@ -582,59 +533,63 @@ 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 if the target port group
- * state is found to be in 'transitioning'.
- * If SCSI_DH_OK is returned the passed-in 'fn' function
- * this function will take care of executing 'fn'.
- * Otherwise 'fn' should be executed by the caller with the
- * returned error code.
+ * response. Returns SCSI_DH_RETRY per default to trigger
+ * a re-evaluation of the target group state or SCSI_DH_OK
+ * if no further action needs to be taken.
  */
-static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h,
-			  activate_complete fn, void *data)
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int err = SCSI_DH_OK;
-	int stpg = 0;
-
-	if (!(h->tpgs & TPGS_MODE_EXPLICIT))
-		/* Only implicit ALUA supported */
-		goto out;
+	int retval;
+	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_OPTIMIZED:
+		return SCSI_DH_OK;
 	case TPGS_STATE_NONOPTIMIZED:
-		stpg = 1;
 		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
 		    !h->pref &&
 		    (h->tpgs & TPGS_MODE_IMPLICIT))
-			stpg = 0;
+			return SCSI_DH_OK;
 		break;
 	case TPGS_STATE_STANDBY:
 	case TPGS_STATE_UNAVAILABLE:
-		stpg = 1;
 		break;
 	case TPGS_STATE_OFFLINE:
-		err = SCSI_DH_IO;
+		return SCSI_DH_IO;
 		break;
 	case TPGS_STATE_TRANSITIONING:
-		err = SCSI_DH_RETRY;
 		break;
 	default:
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: stpg failed, unhandled TPGS state %d",
+			    ALUA_DH_NAME, h->state);
+		return SCSI_DH_NOSYS;
 		break;
 	}
+	/* Set state to transitioning */
+	h->state = TPGS_STATE_TRANSITIONING;
+	retval = submit_stpg(sdev, h->group_id, h->sense);
 
-	if (stpg) {
-		h->callback_fn = fn;
-		h->callback_data = data;
-		err = submit_stpg(h);
-		if (err != SCSI_DH_OK)
-			h->callback_fn = h->callback_data = NULL;
-		else
-			fn = NULL;
+	if (retval) {
+		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: stpg failed, result %d",
+				    ALUA_DH_NAME, retval);
+			if (driver_byte(retval) == DRIVER_BUSY)
+				return SCSI_DH_DEV_TEMP_BUSY;
+		} else {
+			sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+				    ALUA_DH_NAME);
+			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		}
 	}
-out:
-	if (fn)
-		fn(data, err);
-
-	return err;
+	/* Retry RTPG */
+	return SCSI_DH_RETRY;
 }
 
 /*
@@ -722,10 +677,9 @@ static int alua_activate(struct scsi_device *sdev,
 	if (optimize_stpg)
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
-	err = alua_stpg(sdev, h, fn, data);
-
+	err = alua_stpg(sdev, h);
 out:
-	if (err != SCSI_DH_OK && fn)
+	if (fn)
 		fn(data, err);
 	return 0;
 }
-- 
1.8.5.6

Re: [PATCH 03/20] scsi_dh_alua: Make stpg synchronous

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 08:37:23AM +0100, Hannes Reinecke wrote:
> The 'activate_complete' function needs to be executed after
> stpg has finished, so we can as well execute stpg synchronously
> and call the function directly.

We could also execute that from the end_io handler.  I think that
was Bart's main issue.  I have to admit that I'd prefer synchronous
execution if possible, and with a work item only per port group
and not every SCSI device I think we are fine.

Otherwise looks fine to me:

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

[PATCH 04/20] scsi_dh_alua: call alua_rtpg() if stpg fails

[Hannes Reinecke]

If the call to SET TARGET PORT GROUPS fails we have no idea what
state the array is left in, so we need to issue a call to
REPORT TARGET PORT GROUPS in these cases.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 609691f..ba3d23e 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -678,6 +678,8 @@ static int alua_activate(struct scsi_device *sdev,
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
 	err = alua_stpg(sdev, h);
+	if (err == SCSI_DH_RETRY)
+		err = alua_rtpg(sdev, h, 1);
 out:
 	if (fn)
 		fn(data, err);
-- 
1.8.5.6

Re: [PATCH 04/20] scsi_dh_alua: call alua_rtpg() if stpg fails

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 08:37:24AM +0100, Hannes Reinecke wrote:
> If the call to SET TARGET PORT GROUPS fails we have no idea what
> state the array is left in, so we need to issue a call to
> REPORT TARGET PORT GROUPS in these cases.

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

[PATCH 05/20] scsi_dh_alua: switch to scsi_execute_req_flags()

[Hannes Reinecke]

All commands are issued synchronously, so no need to open-code
scsi_execute_req_flags() anymore. And we can get rid of the
static sense code structure element. scsi_execute_req_flags()
will be setting REQ_QUIET and REQ_PREEMPT, but that is
perfectly fine as we're evaluating and logging any errors
ourselves and we really need to send the command even if
the device is quiesced.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ewan Milne <emilne@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 125 +++++++++--------------------
 1 file changed, 36 insertions(+), 89 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index ba3d23e..063c03a 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -75,7 +75,6 @@ struct alua_dh_data {
 	unsigned char		*buff;
 	int			bufflen;
 	unsigned char		transition_tmo;
-	unsigned char		sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;
 	void			*callback_data;
@@ -101,71 +100,30 @@ static int realloc_buffer(struct alua_dh_data *h, unsigned len)
 	return 0;
 }
 
-static struct request *get_alua_req(struct scsi_device *sdev,
-				    void *buffer, unsigned buflen, int rw)
-{
-	struct request *rq;
-	struct request_queue *q = sdev->request_queue;
-
-	rq = blk_get_request(q, rw, GFP_NOIO);
-
-	if (IS_ERR(rq)) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: blk_get_request failed\n", __func__);
-		return NULL;
-	}
-	blk_rq_set_block_pc(rq);
-
-	if (buflen && blk_rq_map_kern(q, rq, buffer, buflen, GFP_NOIO)) {
-		blk_put_request(rq);
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: blk_rq_map_kern failed\n", __func__);
-		return NULL;
-	}
-
-	rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-			 REQ_FAILFAST_DRIVER;
-	rq->retries = ALUA_FAILOVER_RETRIES;
-	rq->timeout = ALUA_FAILOVER_TIMEOUT * HZ;
-
-	return rq;
-}
-
 /*
  * 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, unsigned char *buff,
-			    int bufflen, unsigned char *sense, int flags)
+static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
+		       int bufflen, struct scsi_sense_hdr *sshdr, int flags)
 {
-	struct request *rq;
-	int err = 0;
-
-	rq = get_alua_req(sdev, buff, bufflen, READ);
-	if (!rq) {
-		err = DRIVER_BUSY << 24;
-		goto done;
-	}
+	u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)];
+	int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
+		REQ_FAILFAST_DRIVER;
 
 	/* Prepare the command. */
-	rq->cmd[0] = MAINTENANCE_IN;
+	memset(cdb, 0x0, COMMAND_SIZE(MAINTENANCE_IN));
+	cdb[0] = MAINTENANCE_IN;
 	if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP))
-		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
+		cdb[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
-		rq->cmd[1] = MI_REPORT_TARGET_PGS;
-	put_unaligned_be32(bufflen, &rq->cmd[6]);
-	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
-
-	rq->sense = sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = 0;
-
-	blk_execute_rq(rq->q, NULL, rq, 1);
-	if (rq->errors)
-		err = rq->errors;
-	blk_put_request(rq);
-done:
-	return err;
+		cdb[1] = MI_REPORT_TARGET_PGS;
+	put_unaligned_be32(bufflen, &cdb[6]);
+
+	return scsi_execute_req_flags(sdev, cdb, DMA_FROM_DEVICE,
+				      buff, bufflen, sshdr,
+				      ALUA_FAILOVER_TIMEOUT * HZ,
+				      ALUA_FAILOVER_RETRIES, NULL, req_flags);
 }
 
 /*
@@ -175,40 +133,30 @@ done:
  * to 'active/optimized' and let the array firmware figure out
  * the states of the remaining groups.
  */
-static unsigned submit_stpg(struct scsi_device *sdev, int group_id,
-			    unsigned char *sense)
+static int submit_stpg(struct scsi_device *sdev, int group_id,
+		       struct scsi_sense_hdr *sshdr)
 {
-	struct request *rq;
+	u8 cdb[COMMAND_SIZE(MAINTENANCE_OUT)];
 	unsigned char stpg_data[8];
 	int stpg_len = 8;
-	int err = 0;
+	int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
+		REQ_FAILFAST_DRIVER;
 
 	/* Prepare the data buffer */
 	memset(stpg_data, 0, stpg_len);
 	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
 	put_unaligned_be16(group_id, &stpg_data[6]);
 
-	rq = get_alua_req(sdev, stpg_data, stpg_len, WRITE);
-	if (!rq)
-		return DRIVER_BUSY << 24;
-
 	/* Prepare the command. */
-	rq->cmd[0] = MAINTENANCE_OUT;
-	rq->cmd[1] = MO_SET_TARGET_PGS;
-	put_unaligned_be32(stpg_len, &rq->cmd[6]);
-	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_OUT);
-
-	rq->sense = sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = 0;
-
-	blk_execute_rq(rq->q, NULL, rq, 1);
-	if (rq->errors)
-		err = rq->errors;
-
-	blk_put_request(rq);
-
-	return err;
+	memset(cdb, 0x0, COMMAND_SIZE(MAINTENANCE_OUT));
+	cdb[0] = MAINTENANCE_OUT;
+	cdb[1] = MO_SET_TARGET_PGS;
+	put_unaligned_be32(stpg_len, &cdb[6]);
+
+	return scsi_execute_req_flags(sdev, cdb, DMA_TO_DEVICE,
+				      stpg_data, stpg_len,
+				      sshdr, ALUA_FAILOVER_TIMEOUT * HZ,
+				      ALUA_FAILOVER_RETRIES, NULL, req_flags);
 }
 
 /*
@@ -398,14 +346,14 @@ 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_hdr, h->flags);
+
 	if (retval) {
-		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					  &sense_hdr)) {
+		if (!scsi_sense_valid(&sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: rtpg failed, result %d\n",
 				    ALUA_DH_NAME, retval);
-			if (driver_byte(retval) == DRIVER_BUSY)
+			if (driver_byte(retval) == DRIVER_ERROR)
 				return SCSI_DH_DEV_TEMP_BUSY;
 			return SCSI_DH_IO;
 		}
@@ -572,15 +520,14 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	}
 	/* Set state to transitioning */
 	h->state = TPGS_STATE_TRANSITIONING;
-	retval = submit_stpg(sdev, h->group_id, h->sense);
+	retval = submit_stpg(sdev, h->group_id, &sense_hdr);
 
 	if (retval) {
-		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					  &sense_hdr)) {
+		if (!scsi_sense_valid(&sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: stpg failed, result %d",
 				    ALUA_DH_NAME, retval);
-			if (driver_byte(retval) == DRIVER_BUSY)
+			if (driver_byte(retval) == DRIVER_ERROR)
 				return SCSI_DH_DEV_TEMP_BUSY;
 		} else {
 			sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
-- 
1.8.5.6

[PATCH 06/20] 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.

Reviewed-by: Ewan Milne <emilne@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 221 ++++++++++++++++++++---------
 include/scsi/scsi_dh.h                     |   1 +
 2 files changed, 151 insertions(+), 71 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 063c03a..f3191f7 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -64,9 +64,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;
@@ -75,6 +79,12 @@ struct alua_dh_data {
 	unsigned char		*buff;
 	int			bufflen;
 	unsigned char		transition_tmo;
+};
+
+struct alua_dh_data {
+	struct alua_port_group	*pg;
+	int			group_id;
+	int			rel_port;
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;
 	void			*callback_data;
@@ -85,21 +95,34 @@ struct alua_dh_data {
 
 static char print_alua_state(int);
 
-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;
 }
 
+static void release_port_group(struct kref *kref)
+{
+	struct alua_port_group *pg;
+
+	pg = container_of(kref, struct alua_port_group, kref);
+	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_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
@@ -160,6 +183,37 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 }
 
 /*
+ * alua_get_pg - Allocate a new port_group structure
+ * @sdev: scsi device
+ * @h: alua device_handler data
+ * @group_id: port group id
+ *
+ * Allocate a new port_group structure for a given
+ * device.
+ */
+struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
+				    int group_id, int tpgs)
+{
+	struct alua_port_group *pg = NULL;
+
+	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
+	if (!pg)
+		return NULL;
+
+	pg->group_id = group_id;
+	pg->buff = pg->inq;
+	pg->bufflen = ALUA_INQUIRY_SIZE;
+	pg->tpgs = tpgs;
+	pg->state = TPGS_STATE_OPTIMIZED;
+	kref_init(&pg->kref);
+	spin_lock(&port_group_lock);
+	list_add(&pg->node, &port_group_list);
+	spin_unlock(&port_group_lock);
+
+	return pg;
+}
+
+/*
  * alua_check_tpgs - Evaluate TPGS setting
  * @sdev: device to be checked
  *
@@ -330,7 +384,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)
 {
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;
@@ -340,13 +394,14 @@ 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_hdr, h->flags);
+	retval = submit_rtpg(sdev, pg->buff, pg->bufflen,
+			     &sense_hdr, pg->flags);
 
 	if (retval) {
 		if (!scsi_sense_valid(&sense_hdr)) {
@@ -366,10 +421,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;
 		}
 		/*
@@ -393,11 +448,11 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		return SCSI_DH_IO;
 	}
 
-	len = get_unaligned_be32(&h->buff[0]) + 4;
+	len = get_unaligned_be32(&pg->buff[0]) + 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 */
@@ -406,31 +461,33 @@ 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 == get_unaligned_be16(&ucp[2])) {
-			h->state = ucp[0] & 0x0f;
-			h->pref = ucp[0] >> 7;
+		if (pg->group_id == get_unaligned_be16(&ucp[2])) {
+			pg->state = ucp[0] & 0x0f;
+			pg->pref = ucp[0] >> 7;
 			valid_states = ucp[1];
 		}
 		off = 8 + (ucp[7] * 4);
@@ -438,8 +495,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',
@@ -448,7 +505,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)) {
@@ -463,7 +520,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 */
@@ -485,22 +542,22 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
  * a re-evaluation of the target group state or SCSI_DH_OK
  * if no further action needs to be taken.
  */
-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;
 	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_OPTIMIZED:
 		return SCSI_DH_OK;
 	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:
@@ -514,13 +571,13 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	default:
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: stpg failed, unhandled TPGS state %d",
-			    ALUA_DH_NAME, h->state);
+			    ALUA_DH_NAME, pg->state);
 		return SCSI_DH_NOSYS;
 		break;
 	}
 	/* Set state to transitioning */
-	h->state = TPGS_STATE_TRANSITIONING;
-	retval = submit_stpg(sdev, h->group_id, &sense_hdr);
+	pg->state = TPGS_STATE_TRANSITIONING;
+	retval = submit_stpg(sdev, pg->group_id, &sense_hdr);
 
 	if (retval) {
 		if (!scsi_sense_valid(&sense_hdr)) {
@@ -530,7 +587,7 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 			if (driver_byte(retval) == DRIVER_ERROR)
 				return SCSI_DH_DEV_TEMP_BUSY;
 		} else {
-			sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+			sdev_printk(KERN_INFO, sdev, "%s: stpg failed\n",
 				    ALUA_DH_NAME);
 			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 		}
@@ -548,20 +605,24 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
  */
 static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int err = SCSI_DH_DEV_UNSUPP;
+	int err = SCSI_DH_DEV_UNSUPP, tpgs;
 
-	h->tpgs = alua_check_tpgs(sdev);
-	if (h->tpgs == TPGS_MODE_NONE)
+	tpgs = alua_check_tpgs(sdev);
+	if (tpgs == TPGS_MODE_NONE)
 		goto out;
 
 	err = alua_check_vpd(sdev, h);
 	if (err != SCSI_DH_OK)
 		goto out;
 
-	err = alua_rtpg(sdev, h, 0);
-	if (err != SCSI_DH_OK)
+	h->pg = alua_get_pg(sdev, h->group_id, tpgs);
+	if (!h->pg) {
+		err = SCSI_DH_NOMEM;
 		goto out;
-
+	}
+	kref_get(&h->pg->kref);
+	err = alua_rtpg(sdev, h->pg, 0);
+	kref_put(&h->pg->kref, release_port_group);
 out:
 	return err;
 }
@@ -577,6 +638,7 @@ out:
 static int alua_set_params(struct scsi_device *sdev, const char *params)
 {
 	struct alua_dh_data *h = sdev->handler_data;
+	struct alua_port_group *pg = NULL;
 	unsigned int optimize = 0, argc;
 	const char *p = params;
 	int result = SCSI_DH_OK;
@@ -589,10 +651,14 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	if ((sscanf(p, "%u", &optimize) != 1) || (optimize > 1))
 		return -EINVAL;
 
+	pg = h->pg;
+	if (!pg)
+		return -ENXIO;
+
 	if (optimize)
-		h->flags |= ALUA_OPTIMIZE_STPG;
+		pg->flags |= ALUA_OPTIMIZE_STPG;
 	else
-		h->flags &= ~ALUA_OPTIMIZE_STPG;
+		pg->flags |= ~ALUA_OPTIMIZE_STPG;
 
 	return result;
 }
@@ -617,16 +683,23 @@ static int alua_activate(struct scsi_device *sdev,
 	struct alua_dh_data *h = sdev->handler_data;
 	int err = SCSI_DH_OK;
 
-	err = alua_rtpg(sdev, h, 1);
-	if (err != SCSI_DH_OK)
+	if (!h->pg)
 		goto out;
 
+	kref_get(&h->pg->kref);
+
 	if (optimize_stpg)
-		h->flags |= ALUA_OPTIMIZE_STPG;
+		h->pg->flags |= ALUA_OPTIMIZE_STPG;
 
-	err = alua_stpg(sdev, h);
+	err = alua_rtpg(sdev, h->pg, 1);
+	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, 1);
+		err = alua_rtpg(sdev, h->pg, 1);
+	kref_put(&h->pg->kref, release_port_group);
 out:
 	if (fn)
 		fn(data, err);
@@ -642,13 +715,19 @@ out:
 static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 {
 	struct alua_dh_data *h = sdev->handler_data;
+	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;
 	}
@@ -663,20 +742,18 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 static int alua_bus_attach(struct scsi_device *sdev)
 {
 	struct alua_dh_data *h;
-	int err;
+	int err, ret = -EINVAL;
 
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
 	if (!h)
 		return -ENOMEM;
-	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);
+	if (err == SCSI_DH_NOMEM)
+		ret = -ENOMEM;
 	if (err != SCSI_DH_OK && err != SCSI_DH_DEV_OFFLINED)
 		goto failed;
 
@@ -684,7 +761,7 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	return 0;
 failed:
 	kfree(h);
-	return -EINVAL;
+	return ret;
 }
 
 /*
@@ -695,8 +772,10 @@ static void alua_bus_detach(struct scsi_device *sdev)
 {
 	struct alua_dh_data *h = sdev->handler_data;
 
-	if (h->buff && h->inq != h->buff)
-		kfree(h->buff);
+	if (h->pg) {
+		kref_put(&h->pg->kref, release_port_group);
+		h->pg = NULL;
+	}
 	sdev->handler_data = NULL;
 	kfree(h);
 }
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 85d7317..7e184c6 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -52,6 +52,7 @@ enum {
 	SCSI_DH_TIMED_OUT,
 	SCSI_DH_RES_TEMP_UNAVAIL,
 	SCSI_DH_DEV_OFFLINED,
+	SCSI_DH_NOMEM,
 	SCSI_DH_NOSYS,
 	SCSI_DH_DRIVER_MAX,
 };
-- 
1.8.5.6

Re: [PATCH 06/20] scsi_dh_alua: Use separate alua_port_group structure

[Christoph Hellwig]

> +struct alua_dh_data {
> +	struct alua_port_group	*pg;
> +	int			group_id;

Keeping the group id here seem odd.  It gets cleaned up later
in the series, so this is just a nitpick.

> -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;
>  }

All this disappears in the next patch, wouldn't it have been smarted
to move the next one before this one?

>  /*
> + * alua_get_pg - Allocate a new port_group structure
> + * @sdev: scsi device
> + * @h: alua device_handler data
> + * @group_id: port group id
> + *
> + * Allocate a new port_group structure for a given
> + * device.
> + */
> +struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
> +				    int group_id, int tpgs)

Can you call this alua_alloc_pg, please?

Otherwise looks fine:

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

Re: [PATCH 06/20] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke]

On 12/30/2015 12:17 PM, Christoph Hellwig wrote:
>> +struct alua_dh_data {
>> +	struct alua_port_group	*pg;
>> +	int			group_id;
>
> Keeping the group id here seem odd.  It gets cleaned up later
> in the series, so this is just a nitpick.
>
>> -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;
>>   }
>
> All this disappears in the next patch, wouldn't it have been smarted
> to move the next one before this one?
>
Could've. Will be doing so when posting the next round,

>>   /*
>> + * alua_get_pg - Allocate a new port_group structure
>> + * @sdev: scsi device
>> + * @h: alua device_handler data
>> + * @group_id: port group id
>> + *
>> + * Allocate a new port_group structure for a given
>> + * device.
>> + */
>> +struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
>> +				    int group_id, int tpgs)
>
> Can you call this alua_alloc_pg, please?
>
Sure.

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

[PATCH 07/20] scsi_dh_alua: allocate RTPG buffer separately

[Hannes Reinecke]

The RTPG buffer will only evaluated within alua_rtpg(),
so we can allocate it locally there and avoid having to
put it into the global structure.

Reviewed-by: Ewan Milne <emilne@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 56 +++++++++++-------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index f3191f7..ca6322d 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -56,7 +56,7 @@
 #define TPGS_MODE_IMPLICIT		0x1
 #define TPGS_MODE_EXPLICIT		0x2
 
-#define ALUA_INQUIRY_SIZE		36
+#define ALUA_RTPG_SIZE			128
 #define ALUA_FAILOVER_TIMEOUT		60
 #define ALUA_FAILOVER_RETRIES		5
 
@@ -75,9 +75,6 @@ struct alua_port_group {
 	int			state;
 	int			pref;
 	unsigned		flags; /* used for optimizing STPG */
-	unsigned char		inq[ALUA_INQUIRY_SIZE];
-	unsigned char		*buff;
-	int			bufflen;
 	unsigned char		transition_tmo;
 };
 
@@ -95,21 +92,6 @@ struct alua_dh_data {
 
 static char print_alua_state(int);
 
-static int realloc_buffer(struct alua_port_group *pg, unsigned len)
-{
-	if (pg->buff && pg->buff != pg->inq)
-		kfree(pg->buff);
-
-	pg->buff = kmalloc(len, GFP_NOIO);
-	if (!pg->buff) {
-		pg->buff = pg->inq;
-		pg->bufflen = ALUA_INQUIRY_SIZE;
-		return 1;
-	}
-	pg->bufflen = len;
-	return 0;
-}
-
 static void release_port_group(struct kref *kref)
 {
 	struct alua_port_group *pg;
@@ -118,8 +100,6 @@ static void release_port_group(struct kref *kref)
 	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);
 }
 
@@ -201,8 +181,6 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
 		return NULL;
 
 	pg->group_id = group_id;
-	pg->buff = pg->inq;
-	pg->bufflen = ALUA_INQUIRY_SIZE;
 	pg->tpgs = tpgs;
 	pg->state = TPGS_STATE_OPTIMIZED;
 	kref_init(&pg->kref);
@@ -387,8 +365,8 @@ static int alua_check_sense(struct scsi_device *sdev,
 static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int wait_for_transition)
 {
 	struct scsi_sense_hdr sense_hdr;
-	int len, k, off, valid_states = 0;
-	unsigned char *ucp;
+	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
+	unsigned char *ucp, *buff;
 	unsigned err, retval;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
@@ -399,15 +377,19 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 	else
 		expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
 
+	buff = kzalloc(bufflen, GFP_KERNEL);
+	if (!buff)
+		return SCSI_DH_DEV_TEMP_BUSY;
+
  retry:
-	retval = submit_rtpg(sdev, pg->buff, pg->bufflen,
-			     &sense_hdr, pg->flags);
+	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
 
 	if (retval) {
 		if (!scsi_sense_valid(&sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: rtpg failed, result %d\n",
 				    ALUA_DH_NAME, retval);
+			kfree(buff);
 			if (driver_byte(retval) == DRIVER_ERROR)
 				return SCSI_DH_DEV_TEMP_BUSY;
 			return SCSI_DH_IO;
@@ -445,14 +427,18 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
 			    ALUA_DH_NAME);
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		kfree(buff);
 		return SCSI_DH_IO;
 	}
 
-	len = get_unaligned_be32(&pg->buff[0]) + 4;
+	len = get_unaligned_be32(&buff[0]) + 4;
 
-	if (len > pg->bufflen) {
+	if (len > bufflen) {
 		/* Resubmit with the correct length */
-		if (realloc_buffer(pg, len)) {
+		kfree(buff);
+		bufflen = len;
+		buff = kmalloc(bufflen, GFP_KERNEL);
+		if (!buff) {
 			sdev_printk(KERN_WARNING, sdev,
 				    "%s: kmalloc buffer failed\n",__func__);
 			/* Temporary failure, bypass */
@@ -462,9 +448,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 	}
 
 	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];
+	if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR && buff[5] != 0)
+		pg->transition_tmo = buff[5];
 	else
 		pg->transition_tmo = ALUA_FAILOVER_TIMEOUT;
 
@@ -476,12 +461,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		expiry = jiffies + pg->transition_tmo * HZ;
 	}
 
-	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
+	if ((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 = pg->buff + tpg_desc_tbl_off;
+	for (k = tpg_desc_tbl_off, ucp = buff + tpg_desc_tbl_off;
 	     k < len;
 	     k += off, ucp += off) {
 
@@ -531,6 +516,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		err = SCSI_DH_OK;
 		break;
 	}
+	kfree(buff);
 	return err;
 }
 
-- 
1.8.5.6

[PATCH 08/20] scsi_dh_alua: use unique device id

[Hannes Reinecke]

Use scsi_vpd_lun_id() to assign a unique device identification
to the alua port group structure.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index ca6322d..688e0f7 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -70,6 +70,8 @@ static DEFINE_SPINLOCK(port_group_lock);
 struct alua_port_group {
 	struct kref		kref;
 	struct list_head	node;
+	unsigned char		device_id_str[256];
+	int			device_id_len;
 	int			group_id;
 	int			tpgs;
 	int			state;
@@ -162,6 +164,25 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 				      ALUA_FAILOVER_RETRIES, NULL, req_flags);
 }
 
+struct alua_port_group *alua_lookup_pg(char *id_str, size_t id_size,
+				       int group_id)
+{
+	struct alua_port_group *pg = NULL;
+
+	list_for_each_entry(pg, &port_group_list, node) {
+		if (pg->group_id != group_id)
+			continue;
+		if (pg->device_id_len != id_size)
+			continue;
+		if (strncmp(pg->device_id_str, id_str, id_size))
+			continue;
+		kref_get(&pg->kref);
+		return pg;
+	}
+
+	return NULL;
+}
+
 /*
  * alua_get_pg - Allocate a new port_group structure
  * @sdev: scsi device
@@ -172,19 +193,37 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
  * device.
  */
 struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
-				    int group_id, int tpgs)
+				    int group_id, int tpgs,
+				    char *id_str, size_t id_size)
 {
-	struct alua_port_group *pg = NULL;
+	struct alua_port_group *pg = NULL, *tmp_pg;
+
+	spin_lock(&port_group_lock);
+	pg = alua_lookup_pg(id_str, id_size, group_id);
+	spin_unlock(&port_group_lock);
+	if (pg)
+		return pg;
 
 	pg = kzalloc(sizeof(struct alua_port_group), GFP_KERNEL);
 	if (!pg)
 		return NULL;
 
+	strncpy(pg->device_id_str, id_str, sizeof(pg->device_id_str));
+
+	pg->device_id_len = id_size;
 	pg->group_id = group_id;
 	pg->tpgs = tpgs;
 	pg->state = TPGS_STATE_OPTIMIZED;
 	kref_init(&pg->kref);
+
+	/* Re-check list again to catch concurrent updates */
 	spin_lock(&port_group_lock);
+	tmp_pg = alua_lookup_pg(id_str, id_size, group_id);
+	if (tmp_pg) {
+		spin_unlock(&port_group_lock);
+		kfree(pg);
+		return tmp_pg;
+	}
 	list_add(&pg->node, &port_group_list);
 	spin_unlock(&port_group_lock);
 
@@ -592,6 +631,8 @@ 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 = SCSI_DH_DEV_UNSUPP, tpgs;
+	char device_id_str[256];
+	int device_id_len;
 
 	tpgs = alua_check_tpgs(sdev);
 	if (tpgs == TPGS_MODE_NONE)
@@ -601,7 +642,24 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 	if (err != SCSI_DH_OK)
 		goto out;
 
-	h->pg = alua_get_pg(sdev, h->group_id, tpgs);
+	device_id_len = scsi_vpd_lun_id(sdev, device_id_str,
+					sizeof(device_id_str));
+	if (device_id_len <= 0) {
+		/*
+		 * Internal error: TPGS supported but no device
+		 * identifcation found. Disable ALUA support.
+		 */
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: No device descriptors found\n",
+			    ALUA_DH_NAME);
+		goto out;
+	}
+	sdev_printk(KERN_INFO, sdev,
+		    "%s: device %s port group %02x rel port %02x\n",
+		    ALUA_DH_NAME, device_id_str, h->group_id, h->rel_port);
+
+	h->pg = alua_get_pg(sdev, h->group_id, tpgs,
+			    device_id_str, device_id_len);
 	if (!h->pg) {
 		err = SCSI_DH_NOMEM;
 		goto out;
-- 
1.8.5.6

Re: [PATCH 08/20] scsi_dh_alua: use unique device id

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 08:37:28AM +0100, Hannes Reinecke wrote:
> Use scsi_vpd_lun_id() to assign a unique device identification
> to the alua port group structure.

I really hate the fix length arrays and how they are passed around.

Can't you move the call to scsi_vpd_lun_id into alua_get_pg (alua_alloc_pg)?

Also I think the new alua_lookup_pg should be alua_find_get_pg or
similar as it takes a reference on the pg.

Re: [PATCH 08/20] scsi_dh_alua: use unique device id

[Hannes Reinecke]

On 12/30/2015 12:20 PM, Christoph Hellwig wrote:
> On Tue, Dec 08, 2015 at 08:37:28AM +0100, Hannes Reinecke wrote:
>> Use scsi_vpd_lun_id() to assign a unique device identification
>> to the alua port group structure.
>
> I really hate the fix length arrays and how they are passed around.
>
You and me both.
I would love to be able to identify port group states by a simple 64bit 
identifier, but SCSI Names throws everything into disarray,

> Can't you move the call to scsi_vpd_lun_id into alua_get_pg (alua_alloc_pg)?
>
Hmm. Think so, yes,

> Also I think the new alua_lookup_pg should be alua_find_get_pg or
> similar as it takes a reference on the pg.
>
Sure, no problem with that,

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/20] scsi_dh_alua: simplify alua_initialize()

[Hannes Reinecke]

Rework alua_check_vpd() to use scsi_vpd_get_tpg()
and move the port group selection into the function, too.
With that we can simplify alua_initialize() to just
call alua_check_tpgs() and alua_check_vpd();

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 688e0f7..1eacf3f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -82,7 +82,6 @@ struct alua_port_group {
 
 struct alua_dh_data {
 	struct alua_port_group	*pg;
-	int			group_id;
 	int			rel_port;
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;
@@ -92,6 +91,7 @@ struct alua_dh_data {
 #define ALUA_POLICY_SWITCH_CURRENT	0
 #define ALUA_POLICY_SWITCH_ALL		1
 
+static int alua_rtpg(struct scsi_device *, struct alua_port_group *, int);
 static char print_alua_state(int);
 
 static void release_port_group(struct kref *kref)
@@ -288,10 +288,15 @@ static int alua_check_tpgs(struct scsi_device *sdev)
  *
  * Extract the relative target port and the target port group
  * descriptor from the list of identificators.
+ *
+ * Returns 0 or SCSI_DH_ error code on failure.
  */
-static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
+static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
+			  int tpgs)
 {
 	int rel_port = -1, group_id;
+	char id_str[256];
+	int id_size;
 
 	group_id = scsi_vpd_tpg_id(sdev, &rel_port);
 	if (group_id < 0) {
@@ -305,14 +310,28 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 			    ALUA_DH_NAME);
 		return SCSI_DH_DEV_UNSUPP;
 	}
-	h->state = TPGS_STATE_OPTIMIZED;
-	h->group_id = group_id;
+	h->rel_port = rel_port;
 
+	id_size = scsi_vpd_lun_id(sdev, id_str, 256);
+	if (id_size <= 0) {
+		/*
+		 * Internal error: TPGS supported but no device
+		 * identifcation found. Disable ALUA support.
+		 */
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: No device descriptors found\n",
+			    ALUA_DH_NAME);
+		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);
+		    "%s: device %s port group %02x rel port %02x\n",
+		    ALUA_DH_NAME, id_str, group_id, h->rel_port);
 
-	return 0;
+	h->pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
+	if (!h->pg)
+		return SCSI_DH_NOMEM;
+
+	return alua_rtpg(sdev, h->pg, 0);
 }
 
 static char print_alua_state(int state)
@@ -631,45 +650,14 @@ 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 = SCSI_DH_DEV_UNSUPP, tpgs;
-	char device_id_str[256];
-	int device_id_len;
 
 	tpgs = alua_check_tpgs(sdev);
-	if (tpgs == TPGS_MODE_NONE)
-		goto out;
-
-	err = alua_check_vpd(sdev, h);
-	if (err != SCSI_DH_OK)
-		goto out;
+	if (tpgs != TPGS_MODE_NONE)
+		err = alua_check_vpd(sdev, h, tpgs);
 
-	device_id_len = scsi_vpd_lun_id(sdev, device_id_str,
-					sizeof(device_id_str));
-	if (device_id_len <= 0) {
-		/*
-		 * Internal error: TPGS supported but no device
-		 * identifcation found. Disable ALUA support.
-		 */
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: No device descriptors found\n",
-			    ALUA_DH_NAME);
-		goto out;
-	}
-	sdev_printk(KERN_INFO, sdev,
-		    "%s: device %s port group %02x rel port %02x\n",
-		    ALUA_DH_NAME, device_id_str, h->group_id, h->rel_port);
-
-	h->pg = alua_get_pg(sdev, h->group_id, tpgs,
-			    device_id_str, device_id_len);
-	if (!h->pg) {
-		err = SCSI_DH_NOMEM;
-		goto out;
-	}
-	kref_get(&h->pg->kref);
-	err = alua_rtpg(sdev, h->pg, 0);
-	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
-- 
1.8.5.6

Re: [PATCH 09/20] scsi_dh_alua: simplify alua_initialize()

[Christoph Hellwig]

Looks fine,

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

[PATCH 10/20] revert commit a8e5a2d593cb ("[SCSI] scsi_dh_alua: ALUA handler attach should succeed while TPG is transitioning")

[Hannes Reinecke]

Obsoleted by the next patch.

Reviewed-by: Ewan Milne <emilne@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 31 ++++++++++++------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 1eacf3f..8d80ac6 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -91,7 +91,7 @@ struct alua_dh_data {
 #define ALUA_POLICY_SWITCH_CURRENT	0
 #define ALUA_POLICY_SWITCH_ALL		1
 
-static int alua_rtpg(struct scsi_device *, struct alua_port_group *, int);
+static int alua_rtpg(struct scsi_device *, struct alua_port_group *);
 static char print_alua_state(int);
 
 static void release_port_group(struct kref *kref)
@@ -331,7 +331,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 	if (!h->pg)
 		return SCSI_DH_NOMEM;
 
-	return alua_rtpg(sdev, h->pg, 0);
+	return alua_rtpg(sdev, h->pg);
 }
 
 static char print_alua_state(int state)
@@ -414,13 +414,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)
 {
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
@@ -511,8 +510,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);
@@ -550,19 +548,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:
@@ -723,14 +716,14 @@ static int alua_activate(struct scsi_device *sdev,
 	if (optimize_stpg)
 		h->pg->flags |= ALUA_OPTIMIZE_STPG;
 
-	err = alua_rtpg(sdev, h->pg, 1);
+	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->pg, 1);
+		err = alua_rtpg(sdev, h->pg);
 	kref_put(&h->pg->kref, release_port_group);
 out:
 	if (fn)
-- 
1.8.5.6

Re: [PATCH 10/20] revert commit a8e5a2d593cb ("[SCSI] scsi_dh_alua: ALUA handler attach should succeed while TPG is transitioning")

[Christoph Hellwig]

Maybe this should be folded into the next patch?

Otherwise looks fine:

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

Re: [PATCH 10/20] revert commit a8e5a2d593cb ("[SCSI] scsi_dh_alua: ALUA handler attach should succeed while TPG is transitioning")

[Hannes Reinecke]

On 12/30/2015 12:22 PM, Christoph Hellwig wrote:
> Maybe this should be folded into the next patch?
>
I did it on purpose, as this really is a clean revert,

> Otherwise looks fine:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
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

[PATCH 11/20] 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 per-portgroup
workqueue. This reduces the number of 'REPORT TARGET PORT GROUP'
and 'SET TARGET PORT GROUPS' sent to the controller as we're sending
them now per port group, and not per device as previously.
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 | 331 +++++++++++++++++++++++------
 1 file changed, 270 insertions(+), 61 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 8d80ac6..6fcdcd5 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -59,13 +59,23 @@
 #define ALUA_RTPG_SIZE			128
 #define ALUA_FAILOVER_TIMEOUT		60
 #define ALUA_FAILOVER_RETRIES		5
+#define ALUA_RTPG_DELAY_MSECS		5
 
 /* device handler flags */
-#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_RUNNING			0x40
+
+static uint optimize_stpg;
+module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");
 
 static LIST_HEAD(port_group_list);
 static DEFINE_SPINLOCK(port_group_lock);
+static struct workqueue_struct *kaluad_wq;
 
 struct alua_port_group {
 	struct kref		kref;
@@ -78,12 +88,25 @@ struct alua_port_group {
 	int			pref;
 	unsigned		flags; /* used for optimizing STPG */
 	unsigned char		transition_tmo;
+	unsigned long		expiry;
+	unsigned long		interval;
+	struct delayed_work	rtpg_work;
+	spinlock_t		lock;
+	struct list_head	rtpg_list;
+	struct scsi_device	*rtpg_sdev;
 };
 
 struct alua_dh_data {
 	struct alua_port_group	*pg;
 	int			rel_port;
+	spinlock_t		pg_lock;
 	struct scsi_device	*sdev;
+	int			init_error;
+	struct mutex		init_mutex;
+};
+
+struct alua_queue_data {
+	struct list_head	entry;
 	activate_complete	callback_fn;
 	void			*callback_data;
 };
@@ -91,8 +114,10 @@ struct alua_dh_data {
 #define ALUA_POLICY_SWITCH_CURRENT	0
 #define ALUA_POLICY_SWITCH_ALL		1
 
-static int alua_rtpg(struct scsi_device *, struct alua_port_group *);
-static char print_alua_state(int);
+static void alua_rtpg_work(struct work_struct *work);
+static void alua_rtpg_queue(struct alua_port_group *pg,
+			    struct scsi_device *sdev,
+			    struct alua_queue_data *qdata);
 
 static void release_port_group(struct kref *kref)
 {
@@ -102,6 +127,7 @@ static void release_port_group(struct kref *kref)
 	spin_lock(&port_group_lock);
 	list_del(&pg->node);
 	spin_unlock(&port_group_lock);
+	WARN_ON(pg->rtpg_sdev);
 	kfree(pg);
 }
 
@@ -167,7 +193,7 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 struct alua_port_group *alua_lookup_pg(char *id_str, size_t id_size,
 				       int group_id)
 {
-	struct alua_port_group *pg = NULL;
+	struct alua_port_group *pg;
 
 	list_for_each_entry(pg, &port_group_list, node) {
 		if (pg->group_id != group_id)
@@ -214,18 +240,26 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
 	pg->group_id = group_id;
 	pg->tpgs = tpgs;
 	pg->state = TPGS_STATE_OPTIMIZED;
+	if (optimize_stpg)
+		pg->flags |= ALUA_OPTIMIZE_STPG;
 	kref_init(&pg->kref);
+	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
+	INIT_LIST_HEAD(&pg->rtpg_list);
+	INIT_LIST_HEAD(&pg->node);
+	spin_lock_init(&pg->lock);
 
 	/* Re-check list again to catch concurrent updates */
 	spin_lock(&port_group_lock);
 	tmp_pg = alua_lookup_pg(id_str, id_size, group_id);
 	if (tmp_pg) {
 		spin_unlock(&port_group_lock);
-		kfree(pg);
-		return tmp_pg;
+		kref_put(&pg->kref, release_port_group);
+		pg = tmp_pg;
+		tmp_pg = NULL;
+	} else {
+		list_add(&pg->node, &port_group_list);
+		spin_unlock(&port_group_lock);
 	}
-	list_add(&pg->node, &port_group_list);
-	spin_unlock(&port_group_lock);
 
 	return pg;
 }
@@ -289,7 +323,7 @@ static int alua_check_tpgs(struct scsi_device *sdev)
  * Extract the relative target port and the target port group
  * descriptor from the list of identificators.
  *
- * Returns 0 or SCSI_DH_ error code on failure.
+ * Returns the target port group id or -1 on failure
  */
 static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 			  int tpgs)
@@ -297,6 +331,8 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 	int rel_port = -1, group_id;
 	char id_str[256];
 	int id_size;
+	struct alua_port_group *pg = NULL, *old_pg = NULL;
+	bool pg_found = false;
 
 	group_id = scsi_vpd_tpg_id(sdev, &rel_port);
 	if (group_id < 0) {
@@ -327,11 +363,37 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 		    "%s: device %s port group %02x rel port %02x\n",
 		    ALUA_DH_NAME, id_str, group_id, h->rel_port);
 
-	h->pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
-	if (!h->pg)
+	pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
+	if (!pg)
 		return SCSI_DH_NOMEM;
 
-	return alua_rtpg(sdev, h->pg);
+	/* Check for existing port_group references */
+	spin_lock(&h->pg_lock);
+	if (h->pg) {
+		old_pg = pg;
+		/* port_group has changed. Update to new port group */
+		if (h->pg != pg) {
+			old_pg = h->pg;
+			rcu_assign_pointer(h->pg, pg);
+			h->pg->expiry = 0;
+			pg_found = true;
+		}
+	} else {
+		rcu_assign_pointer(h->pg, pg);
+		pg_found = true;
+	}
+	alua_rtpg_queue(h->pg, sdev, NULL);
+	spin_unlock(&h->pg_lock);
+
+	if (pg_found)
+		synchronize_rcu();
+	if (old_pg) {
+		if (old_pg->rtpg_sdev)
+			flush_delayed_work(&old_pg->rtpg_work);
+		kref_put(&old_pg->kref, release_port_group);
+	}
+
+	return SCSI_DH_OK;
 }
 
 static char print_alua_state(int state)
@@ -425,14 +487,17 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
 	unsigned char *ucp, *buff;
 	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) {
+		unsigned long transition_tmo = ALUA_FAILOVER_TIMEOUT * HZ;
+
+		if (pg->transition_tmo)
+			transition_tmo = pg->transition_tmo * HZ;
+
+		pg->expiry = round_jiffies_up(jiffies + transition_tmo);
+	}
 
 	buff = kzalloc(bufflen, GFP_KERNEL);
 	if (!buff)
@@ -475,16 +540,18 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			err = SCSI_DH_RETRY;
 		else if (sense_hdr.sense_key == UNIT_ATTENTION)
 			err = SCSI_DH_RETRY;
-		if (err == SCSI_DH_RETRY && time_before(jiffies, expiry)) {
+		if (err == SCSI_DH_RETRY &&
+		    pg->expiry != 0 && time_before(jiffies, pg->expiry)) {
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
 				    ALUA_DH_NAME);
 			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
-			goto retry;
+			return err;
 		}
 		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
 			    ALUA_DH_NAME);
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 		kfree(buff);
+		pg->expiry = 0;
 		return SCSI_DH_IO;
 	}
 
@@ -499,6 +566,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;
@@ -514,7 +582,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		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;
+		pg->expiry = jiffies + pg->transition_tmo * HZ;
 	}
 
 	if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
@@ -548,23 +616,26 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 
 	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;
 	}
 	kfree(buff);
@@ -633,6 +704,106 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	return SCSI_DH_RETRY;
 }
 
+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;
+	LIST_HEAD(qdata_list);
+	int err = SCSI_DH_OK;
+	struct alua_queue_data *qdata, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pg->lock, flags);
+	sdev = pg->rtpg_sdev;
+	if (!sdev) {
+		WARN_ON(pg->flags & ALUA_PG_RUN_RTPG ||
+			pg->flags & ALUA_PG_RUN_STPG);
+		spin_unlock_irqrestore(&pg->lock, flags);
+		return;
+	}
+	pg->flags |= ALUA_PG_RUNNING;
+	if (pg->flags & ALUA_PG_RUN_RTPG) {
+		spin_unlock_irqrestore(&pg->lock, flags);
+		err = alua_rtpg(sdev, pg);
+		spin_lock_irqsave(&pg->lock, flags);
+		if (err == SCSI_DH_RETRY) {
+			pg->flags &= ~ALUA_PG_RUNNING;
+			spin_unlock_irqrestore(&pg->lock, flags);
+			queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+		pg->flags &= ~ALUA_PG_RUN_RTPG;
+		if (err != SCSI_DH_OK)
+			pg->flags &= ~ALUA_PG_RUN_STPG;
+	}
+	if (pg->flags & ALUA_PG_RUN_STPG) {
+		spin_unlock_irqrestore(&pg->lock, flags);
+		err = alua_stpg(sdev, pg);
+		spin_lock_irqsave(&pg->lock, flags);
+		pg->flags &= ~ALUA_PG_RUN_STPG;
+		if (err == SCSI_DH_RETRY) {
+			pg->flags |= ALUA_PG_RUN_RTPG;
+			pg->interval = 0;
+			pg->flags &= ~ALUA_PG_RUNNING;
+			spin_unlock_irqrestore(&pg->lock, flags);
+			queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+	}
+
+	list_splice_init(&pg->rtpg_list, &qdata_list);
+	pg->rtpg_sdev = NULL;
+	spin_unlock_irqrestore(&pg->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);
+	}
+	spin_lock_irqsave(&pg->lock, flags);
+	pg->flags &= ~ALUA_PG_RUNNING;
+	spin_unlock_irqrestore(&pg->lock, flags);
+	scsi_device_put(sdev);
+	kref_put(&pg->kref, release_port_group);
+}
+
+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;
+
+	spin_lock_irqsave(&pg->lock, flags);
+	if (qdata) {
+		list_add_tail(&qdata->entry, &pg->rtpg_list);
+		pg->flags |= ALUA_PG_RUN_STPG;
+	}
+	if (pg->rtpg_sdev == NULL) {
+		pg->interval = 0;
+		pg->flags |= ALUA_PG_RUN_RTPG;
+		kref_get(&pg->kref);
+		pg->rtpg_sdev = sdev;
+		scsi_device_get(sdev);
+		start_queue = 1;
+	}
+	spin_unlock_irqrestore(&pg->lock, flags);
+
+	if (start_queue &&
+	    !queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+				msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS))) {
+		scsi_device_put(sdev);
+		kref_put(&pg->kref, release_port_group);
+	}
+}
+
 /*
  * alua_initialize - Initialize ALUA state
  * @sdev: the device to be initialized
@@ -644,10 +815,12 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int err = SCSI_DH_DEV_UNSUPP, tpgs;
 
+	mutex_lock(&h->init_mutex);
 	tpgs = alua_check_tpgs(sdev);
 	if (tpgs != TPGS_MODE_NONE)
 		err = alua_check_vpd(sdev, h, tpgs);
-
+	h->init_error = err;
+	mutex_unlock(&h->init_mutex);
 	return err;
 }
 
@@ -667,6 +840,7 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	unsigned int optimize = 0, argc;
 	const char *p = params;
 	int result = SCSI_DH_OK;
+	unsigned long flags;
 
 	if ((sscanf(params, "%u", &argc) != 1) || (argc != 1))
 		return -EINVAL;
@@ -676,22 +850,23 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	if ((sscanf(p, "%u", &optimize) != 1) || (optimize > 1))
 		return -EINVAL;
 
-	pg = h->pg;
-	if (!pg)
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
 		return -ENXIO;
-
+	}
+	spin_lock_irqsave(&pg->lock, flags);
 	if (optimize)
 		pg->flags |= ALUA_OPTIMIZE_STPG;
 	else
 		pg->flags |= ~ALUA_OPTIMIZE_STPG;
+	spin_unlock_irqrestore(&pg->lock, flags);
+	rcu_read_unlock();
 
 	return result;
 }
 
-static uint optimize_stpg;
-module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
-MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");
-
 /*
  * alua_activate - activate a path
  * @sdev: device on the path to be activated
@@ -707,24 +882,34 @@ static int alua_activate(struct scsi_device *sdev,
 {
 	struct alua_dh_data *h = sdev->handler_data;
 	int err = SCSI_DH_OK;
+	struct alua_queue_data *qdata;
+	struct alua_port_group *pg;
 
-	if (!h->pg)
+	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
+	if (!qdata) {
+		err = SCSI_DH_RES_TEMP_UNAVAIL;
 		goto out;
-
-	kref_get(&h->pg->kref);
-
-	if (optimize_stpg)
-		h->pg->flags |= ALUA_OPTIMIZE_STPG;
-
-	err = alua_rtpg(sdev, h->pg);
-	if (err != SCSI_DH_OK) {
-		kref_put(&h->pg->kref, release_port_group);
+	}
+	qdata->callback_fn = fn;
+	qdata->callback_data = data;
+
+	mutex_lock(&h->init_mutex);
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
+		kfree(qdata);
+		err = h->init_error;
+		mutex_unlock(&h->init_mutex);
 		goto out;
 	}
-	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);
+	mutex_unlock(&h->init_mutex);
+	fn = NULL;
+	kref_get(&pg->kref);
+	rcu_read_unlock();
+
+	alua_rtpg_queue(pg, sdev, qdata);
+	kref_put(&pg->kref, release_port_group);
 out:
 	if (fn)
 		fn(data, err);
@@ -740,14 +925,19 @@ out:
 static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 {
 	struct alua_dh_data *h = sdev->handler_data;
-	int state;
+	struct alua_port_group *pg;
+	int state = TPGS_STATE_OPTIMIZED;
 	int ret = BLKPREP_OK;
 
-	if (!h->pg)
-		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 &&
@@ -772,10 +962,13 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
 	if (!h)
 		return -ENOMEM;
-	h->pg = NULL;
+	spin_lock_init(&h->pg_lock);
+	rcu_assign_pointer(h->pg, NULL);
 	h->rel_port = -1;
+	h->init_error = SCSI_DH_OK;
 	h->sdev = sdev;
 
+	mutex_init(&h->init_mutex);
 	err = alua_initialize(sdev, h);
 	if (err == SCSI_DH_NOMEM)
 		ret = -ENOMEM;
@@ -796,10 +989,18 @@ failed:
 static void alua_bus_detach(struct scsi_device *sdev)
 {
 	struct alua_dh_data *h = sdev->handler_data;
+	struct alua_port_group *pg;
 
-	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);
+	h->sdev = NULL;
+	spin_unlock(&h->pg_lock);
+	if (pg) {
+		synchronize_rcu();
+		if (pg->rtpg_sdev)
+			flush_delayed_work(&pg->rtpg_work);
+		kref_put(&pg->kref, release_port_group);
 	}
 	sdev->handler_data = NULL;
 	kfree(h);
@@ -820,16 +1021,24 @@ static int __init alua_init(void)
 {
 	int r;
 
+	kaluad_wq = alloc_workqueue("kaluad", WQ_MEM_RECLAIM, 0);
+	if (!kaluad_wq) {
+		/* Temporary failure, bypass */
+		return SCSI_DH_DEV_TEMP_BUSY;
+	}
 	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(kaluad_wq);
+	}
 	return r;
 }
 
 static void __exit alua_exit(void)
 {
 	scsi_unregister_device_handler(&alua_dh);
+	destroy_workqueue(kaluad_wq);
 }
 
 module_init(alua_init);
-- 
1.8.5.6

Re: [PATCH 11/20] scsi_dh_alua: Use workqueue for RTPG

[Christoph Hellwig]

This looks good in general, but a couple nitpicks below:

> +static uint optimize_stpg;
> +module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");

why is this moved around in this patch?  It doesn't seem related to the
rest of it, and isn't documented in the changelog either.

>  {
> -	struct alua_port_group *pg = NULL;
> +	struct alua_port_group *pg;

looks like this should be folded into the patch that introduces the
unessecary NULL assignment.

>  	list_for_each_entry(pg, &port_group_list, node) {
>  		if (pg->group_id != group_id)
> @@ -214,18 +240,26 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
>  	pg->group_id = group_id;
>  	pg->tpgs = tpgs;
>  	pg->state = TPGS_STATE_OPTIMIZED;
> +	if (optimize_stpg)
> +		pg->flags |= ALUA_OPTIMIZE_STPG;


why is this moved earlier here?  Doing it from the beginning seems
useful to me, but I'd expect it in a separate patch with a proper
description.

>  	kref_init(&pg->kref);
> +	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
> +	INIT_LIST_HEAD(&pg->rtpg_list);
> +	INIT_LIST_HEAD(&pg->node);
> +	spin_lock_init(&pg->lock);
>  
>  	/* Re-check list again to catch concurrent updates */
>  	spin_lock(&port_group_lock);
>  	tmp_pg = alua_lookup_pg(id_str, id_size, group_id);
>  	if (tmp_pg) {
>  		spin_unlock(&port_group_lock);
> -		kfree(pg);
> -		return tmp_pg;
> +		kref_put(&pg->kref, release_port_group);
> +		pg = tmp_pg;
> +		tmp_pg = NULL;

The only thing release_port_group does in addition to the kfree
is a list_del on pg->entry.  But given that we never added
the pg to a list it doesn't need to be deleted, and it can't
have another reference.  Why this change?

While we're at it, there are 6 calls to
'kref_put(&pg->kref, release_port_group)' in addition to this one,
so it might make sense to add a helper for it to the patch introducing
struct alua_port_group.

>   * Extract the relative target port and the target port group
>   * descriptor from the list of identificators.
>   *
> - * Returns 0 or SCSI_DH_ error code on failure.
> + * Returns the target port group id or -1 on failure

That's not how I interpret the code below, it seems to always return
SCSI_DH_* values.

> +	struct alua_port_group *pg = NULL, *old_pg = NULL;
> +	bool pg_found = false;

> +	pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
> +	if (!pg)
>  		return SCSI_DH_NOMEM;
> +	/* Check for existing port_group references */
> +	spin_lock(&h->pg_lock);
> +	if (h->pg) {
> +		old_pg = pg;
> +		/* port_group has changed. Update to new port group */
> +		if (h->pg != pg) {
> +			old_pg = h->pg;
> +			rcu_assign_pointer(h->pg, pg);
> +			h->pg->expiry = 0;

why do we set expiry to 0 here, but not if we didn't have a pg
yet?  This could use a comment not just here but in all the places
that set it to zero.

> +			pg_found = true;

pg_found should be pg_updated, right?

> +	if (pg_found)
> +		synchronize_rcu();
> +	if (old_pg) {
> +		if (old_pg->rtpg_sdev)
> +			flush_delayed_work(&old_pg->rtpg_work);
> +		kref_put(&old_pg->kref, release_port_group);
> +	}

This code looks odd.  I can't see why we need a synchronize_rcu here.
The only thing we should need is a kfree_rcu for the final free in
release_port_group.  I also don't quite understand the flush_delayed_work.
As far as I can tell we only need it if rtpg_sdev is the sdev passed
in, so it we probably should check for that.

Re: [PATCH 11/20] scsi_dh_alua: Use workqueue for RTPG

[Hannes Reinecke]

On 12/30/2015 02:19 PM, Christoph Hellwig wrote:
> This looks good in general, but a couple nitpicks below:
>
>> +static uint optimize_stpg;
>> +module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
>> +MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");
>
> why is this moved around in this patch?  It doesn't seem related to the
> rest of it, and isn't documented in the changelog either.
>
Yeah, can move it to a separate patch.

>>   {
>> -	struct alua_port_group *pg = NULL;
>> +	struct alua_port_group *pg;
>
> looks like this should be folded into the patch that introduces the
> unessecary NULL assignment.
>
Ok.

>>   	list_for_each_entry(pg, &port_group_list, node) {
>>   		if (pg->group_id != group_id)
>> @@ -214,18 +240,26 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
>>   	pg->group_id = group_id;
>>   	pg->tpgs = tpgs;
>>   	pg->state = TPGS_STATE_OPTIMIZED;
>> +	if (optimize_stpg)
>> +		pg->flags |= ALUA_OPTIMIZE_STPG;
>
>
> why is this moved earlier here?  Doing it from the beginning seems
> useful to me, but I'd expect it in a separate patch with a proper
> description.
>
Ok.

>>   	kref_init(&pg->kref);
>> +	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
>> +	INIT_LIST_HEAD(&pg->rtpg_list);
>> +	INIT_LIST_HEAD(&pg->node);
>> +	spin_lock_init(&pg->lock);
>>
>>   	/* Re-check list again to catch concurrent updates */
>>   	spin_lock(&port_group_lock);
>>   	tmp_pg = alua_lookup_pg(id_str, id_size, group_id);
>>   	if (tmp_pg) {
>>   		spin_unlock(&port_group_lock);
>> -		kfree(pg);
>> -		return tmp_pg;
>> +		kref_put(&pg->kref, release_port_group);
>> +		pg = tmp_pg;
>> +		tmp_pg = NULL;
>
> The only thing release_port_group does in addition to the kfree
> is a list_del on pg->entry.  But given that we never added
> the pg to a list it doesn't need to be deleted, and it can't
> have another reference.  Why this change?
>
Mainly for symmetry; with this patch we're always calling kref_put() to 
delete the port_group structure.

> While we're at it, there are 6 calls to
> 'kref_put(&pg->kref, release_port_group)' in addition to this one,
> so it might make sense to add a helper for it to the patch introducing
> struct alua_port_group.
>
Ok.

>>    * Extract the relative target port and the target port group
>>    * descriptor from the list of identificators.
>>    *
>> - * Returns 0 or SCSI_DH_ error code on failure.
>> + * Returns the target port group id or -1 on failure
>
> That's not how I interpret the code below, it seems to always return
> SCSI_DH_* values.
>
>> +	struct alua_port_group *pg = NULL, *old_pg = NULL;
>> +	bool pg_found = false;
>
>> +	pg = alua_get_pg(sdev, group_id, tpgs, id_str, id_size);
>> +	if (!pg)
>>   		return SCSI_DH_NOMEM;
>> +	/* Check for existing port_group references */
>> +	spin_lock(&h->pg_lock);
>> +	if (h->pg) {
>> +		old_pg = pg;
>> +		/* port_group has changed. Update to new port group */
>> +		if (h->pg != pg) {
>> +			old_pg = h->pg;
>> +			rcu_assign_pointer(h->pg, pg);
>> +			h->pg->expiry = 0;
>
> why do we set expiry to 0 here, but not if we didn't have a pg
> yet?  This could use a comment not just here but in all the places
> that set it to zero.
>
>> +			pg_found = true;
>
> pg_found should be pg_updated, right?
>
>> +	if (pg_found)
>> +		synchronize_rcu();
>> +	if (old_pg) {
>> +		if (old_pg->rtpg_sdev)
>> +			flush_delayed_work(&old_pg->rtpg_work);
>> +		kref_put(&old_pg->kref, release_port_group);
>> +	}
>
> This code looks odd.  I can't see why we need a synchronize_rcu here.
> The only thing we should need is a kfree_rcu for the final free in
> release_port_group.  I also don't quite understand the flush_delayed_work.
> As far as I can tell we only need it if rtpg_sdev is the sdev passed
> in, so it we probably should check for that.
>
rtpg_sdev is set whenever the port_group structure needs or is scheduled 
for alua_rtpg_work(), ie whenever some action needs to be taken.
As the sdev might be a different sdev than that one we've been called 
from (a port_group might have several sdevs pointing to it), the 
existence of an sdev signals that we need to flush the workqueue item.

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/20] scsi_dh_alua: Use workqueue for RTPG

[Christoph Hellwig]

On Thu, Dec 31, 2015 at 02:01:43PM +0100, Hannes Reinecke wrote:
>>>   	if (tmp_pg) {
>>>   		spin_unlock(&port_group_lock);
>>> -		kfree(pg);
>>> -		return tmp_pg;
>>> +		kref_put(&pg->kref, release_port_group);
>>> +		pg = tmp_pg;
>>> +		tmp_pg = NULL;
>>
>> The only thing release_port_group does in addition to the kfree
>> is a list_del on pg->entry.  But given that we never added
>> the pg to a list it doesn't need to be deleted, and it can't
>> have another reference.  Why this change?
>>
> Mainly for symmetry; with this patch we're always calling kref_put() to 
> delete the port_group structure.

Given that it has never been added it's not actuallt symmetric.

Either way it shouldn't be added in this patch - either use kref_put
from the patch introducing the kref or not use it at all.  I would
prefer the second option.

>>
>> pg_found should be pg_updated, right?
>>
>>> +	if (pg_found)
>>> +		synchronize_rcu();
>>> +	if (old_pg) {
>>> +		if (old_pg->rtpg_sdev)
>>> +			flush_delayed_work(&old_pg->rtpg_work);
>>> +		kref_put(&old_pg->kref, release_port_group);
>>> +	}
>>
>> This code looks odd.  I can't see why we need a synchronize_rcu here.
>> The only thing we should need is a kfree_rcu for the final free in
>> release_port_group.  I also don't quite understand the flush_delayed_work.
>> As far as I can tell we only need it if rtpg_sdev is the sdev passed
>> in, so it we probably should check for that.
>>
> rtpg_sdev is set whenever the port_group structure needs or is scheduled 
> for alua_rtpg_work(), ie whenever some action needs to be taken.
> As the sdev might be a different sdev than that one we've been called from 
> (a port_group might have several sdevs pointing to it), the existence of an 
> sdev signals that we need to flush the workqueue item.

So why do we only need to flush it for some kref_put callers and not the
others?

[PATCH 12/20] scsi_dh_alua: Allow workqueue to run synchronously

[Hannes Reinecke]

Some arrays may only capable of handling one STPG at a time,
so this patch implements a module option 'sync_stpg' to have
STPGs submitted synchronously.

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 6fcdcd5..525449f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -73,6 +73,10 @@ static uint optimize_stpg;
 module_param(optimize_stpg, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(optimize_stpg, "Allow use of a non-optimized path, rather than sending a STPG, when implicit TPGS is supported (0=No,1=Yes). Default is 0.");
 
+static uint sync_stpg;
+module_param(sync_stpg, uint, S_IRUGO);
+MODULE_PARM_DESC(sync_stpg, "Issue STPG synchronously (0=No,1=Yes). Default is 0.");
+
 static LIST_HEAD(port_group_list);
 static DEFINE_SPINLOCK(port_group_lock);
 static struct workqueue_struct *kaluad_wq;
@@ -1019,9 +1023,11 @@ static struct scsi_device_handler alua_dh = {
 
 static int __init alua_init(void)
 {
-	int r;
+	int r, max_active = 0;
 
-	kaluad_wq = alloc_workqueue("kaluad", WQ_MEM_RECLAIM, 0);
+	if (sync_stpg)
+		max_active = 1;
+	kaluad_wq = alloc_workqueue("kaluad", WQ_MEM_RECLAIM, max_active);
 	if (!kaluad_wq) {
 		/* Temporary failure, bypass */
 		return SCSI_DH_DEV_TEMP_BUSY;
-- 
1.8.5.6

Re: [PATCH 12/20] scsi_dh_alua: Allow workqueue to run synchronously

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 08:37:32AM +0100, Hannes Reinecke wrote:
> Some arrays may only capable of handling one STPG at a time,

Which arrays?  We should have a quirk for them in the devinfo database
instead of a module option.

Re: [PATCH 12/20] scsi_dh_alua: Allow workqueue to run synchronously

[Hannes Reinecke]

On 12/30/2015 02:20 PM, Christoph Hellwig wrote:
> On Tue, Dec 08, 2015 at 08:37:32AM +0100, Hannes Reinecke wrote:
>> Some arrays may only capable of handling one STPG at a time,
>
> Which arrays?  We should have a quirk for them in the devinfo database
> instead of a module option.
>
ATM the only ones I know of is NetApp (both FAS and E-series; E-series 
requires it, and FAS benefits greatly).
But this is not to say that these are the only ones, _and_ the more 
obvious approach would be to add the handling to multipath-tools 
settings, so I really would like to keep the module option for now.

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 12/20] scsi_dh_alua: Allow workqueue to run synchronously

[Christoph Hellwig]

On Thu, Dec 31, 2015 at 02:54:18PM +0100, Hannes Reinecke wrote:
> ATM the only ones I know of is NetApp (both FAS and E-series; E-series 
> requires it, and FAS benefits greatly).
> But this is not to say that these are the only ones, _and_ the more obvious 
> approach would be to add the handling to multipath-tools settings, so I 
> really would like to keep the module option for now.

Anything involving multipath-tools settings is a giant nightmare.  We
need to get it rifght in the kernel so that multipathing can work
out of the box.  And the right place for SCSI-level quirks are the
devlist entries.

Module options for hardware specific behavior are a horrible idea - you
can be connect to different devies, which is a very common setup.

[PATCH 13/20] scsi_dh_alua: Recheck state on unit attention

[Hannes Reinecke]

When we receive a unit attention code of 'ALUA state changed'
we should recheck the state, as it might be due to an implicit
ALUA state transition. This allows us to return NEEDS_RETRY
instead of ADD_TO_MLQUEUE, allowing to terminate the retries
after a certain time.
At the same time a workqueue item might already be queued, which
should be started immediately to avoid any delays.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 525449f..04a3a543 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -121,7 +121,8 @@ struct alua_queue_data {
 static void alua_rtpg_work(struct work_struct *work);
 static void alua_rtpg_queue(struct alua_port_group *pg,
 			    struct scsi_device *sdev,
-			    struct alua_queue_data *qdata);
+			    struct alua_queue_data *qdata, bool force);
+static void alua_check(struct scsi_device *sdev, bool force);
 
 static void release_port_group(struct kref *kref)
 {
@@ -386,7 +387,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 		rcu_assign_pointer(h->pg, pg);
 		pg_found = true;
 	}
-	alua_rtpg_queue(h->pg, sdev, NULL);
+	alua_rtpg_queue(h->pg, sdev, NULL, true);
 	spin_unlock(&h->pg_lock);
 
 	if (pg_found)
@@ -427,18 +428,24 @@ 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
 			 */
-			return ADD_TO_MLQUEUE;
+			alua_check(sdev, false);
+			return NEEDS_RETRY;
+		}
 		break;
 	case UNIT_ATTENTION:
-		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
+		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00) {
 			/*
-			 * Power On, Reset, or Bus Device Reset, just retry.
+			 * Power On, Reset, or Bus Device Reset.
+			 * Might have obscured a state transition,
+			 * so schedule a recheck.
 			 */
+			alua_check(sdev, true);
 			return ADD_TO_MLQUEUE;
+		}
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x04)
 			/*
 			 * Device internal reset
@@ -449,16 +456,20 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * Mode Parameters Changed
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06)
+		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06) {
 			/*
 			 * ALUA state changed
 			 */
+			alua_check(sdev, true);
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x07)
+		}
+		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x07) {
 			/*
 			 * Implicit ALUA state transition failed
 			 */
+			alua_check(sdev, true);
 			return ADD_TO_MLQUEUE;
+		}
 		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x03)
 			/*
 			 * Inquiry data has changed
@@ -777,7 +788,7 @@ static void alua_rtpg_work(struct work_struct *work)
 
 static void alua_rtpg_queue(struct alua_port_group *pg,
 			    struct scsi_device *sdev,
-			    struct alua_queue_data *qdata)
+			    struct alua_queue_data *qdata, bool force)
 {
 	int start_queue = 0;
 	unsigned long flags;
@@ -797,7 +808,9 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
 		pg->rtpg_sdev = sdev;
 		scsi_device_get(sdev);
 		start_queue = 1;
-	}
+	} else if (!(pg->flags & ALUA_PG_RUN_RTPG) && force)
+		start_queue = 1;
+
 	spin_unlock_irqrestore(&pg->lock, flags);
 
 	if (start_queue &&
@@ -912,7 +925,7 @@ static int alua_activate(struct scsi_device *sdev,
 	kref_get(&pg->kref);
 	rcu_read_unlock();
 
-	alua_rtpg_queue(pg, sdev, qdata);
+	alua_rtpg_queue(pg, sdev, qdata, true);
 	kref_put(&pg->kref, release_port_group);
 out:
 	if (fn)
@@ -921,6 +934,29 @@ out:
 }
 
 /*
+ * 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, bool force)
+{
+	struct alua_dh_data *h = sdev->handler_data;
+	struct alua_port_group *pg;
+
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
+		return;
+	}
+	kref_get(&pg->kref);
+	rcu_read_unlock();
+	alua_rtpg_queue(pg, sdev, NULL, force);
+	kref_put(&pg->kref, release_port_group);
+}
+
+/*
  * alua_prep_fn - request callback
  *
  * Fail I/O to all paths not in state
-- 
1.8.5.6

Re: [PATCH 13/20] scsi_dh_alua: Recheck state on unit attention

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 08:37:33AM +0100, Hannes Reinecke wrote:
> When we receive a unit attention code of 'ALUA state changed'
> we should recheck the state, as it might be due to an implicit
> ALUA state transition. This allows us to return NEEDS_RETRY
> instead of ADD_TO_MLQUEUE, allowing to terminate the retries
> after a certain time.
> At the same time a workqueue item might already be queued, which
> should be started immediately to avoid any delays.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 58 ++++++++++++++++++++++++------
>  1 file changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 525449f..04a3a543 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -121,7 +121,8 @@ struct alua_queue_data {
>  static void alua_rtpg_work(struct work_struct *work);
>  static void alua_rtpg_queue(struct alua_port_group *pg,
>  			    struct scsi_device *sdev,
> -			    struct alua_queue_data *qdata);
> +			    struct alua_queue_data *qdata, bool force);
> +static void alua_check(struct scsi_device *sdev, bool force);
>  
>  static void release_port_group(struct kref *kref)
>  {
> @@ -386,7 +387,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
>  		rcu_assign_pointer(h->pg, pg);
>  		pg_found = true;
>  	}
> -	alua_rtpg_queue(h->pg, sdev, NULL);
> +	alua_rtpg_queue(h->pg, sdev, NULL, true);
>  	spin_unlock(&h->pg_lock);
>  
>  	if (pg_found)
> @@ -427,18 +428,24 @@ 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
>  			 */
> -			return ADD_TO_MLQUEUE;
> +			alua_check(sdev, false);
> +			return NEEDS_RETRY;
> +		}
>  		break;
>  	case UNIT_ATTENTION:
> -		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
> +		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00) {
>  			/*
> -			 * Power On, Reset, or Bus Device Reset, just retry.
> +			 * Power On, Reset, or Bus Device Reset.
> +			 * Might have obscured a state transition,
> +			 * so schedule a recheck.
>  			 */
> +			alua_check(sdev, true);
>  			return ADD_TO_MLQUEUE;
> +		}
>  		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x04)
>  			/*
>  			 * Device internal reset
> @@ -449,16 +456,20 @@ static int alua_check_sense(struct scsi_device *sdev,
>  			 * Mode Parameters Changed
>  			 */
>  			return ADD_TO_MLQUEUE;
> -		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06)
> +		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06) {
>  			/*
>  			 * ALUA state changed
>  			 */
> +			alua_check(sdev, true);
>  			return ADD_TO_MLQUEUE;
> -		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x07)
> +		}
> +		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x07) {
>  			/*
>  			 * Implicit ALUA state transition failed
>  			 */
> +			alua_check(sdev, true);
>  			return ADD_TO_MLQUEUE;
> +		}
>  		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x03)
>  			/*
>  			 * Inquiry data has changed
> @@ -777,7 +788,7 @@ static void alua_rtpg_work(struct work_struct *work)
>  
>  static void alua_rtpg_queue(struct alua_port_group *pg,
>  			    struct scsi_device *sdev,
> -			    struct alua_queue_data *qdata)
> +			    struct alua_queue_data *qdata, bool force)
>  {
>  	int start_queue = 0;
>  	unsigned long flags;
> @@ -797,7 +808,9 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
>  		pg->rtpg_sdev = sdev;
>  		scsi_device_get(sdev);
>  		start_queue = 1;
> -	}
> +	} else if (!(pg->flags & ALUA_PG_RUN_RTPG) && force)
> +		start_queue = 1;
> +
>  	spin_unlock_irqrestore(&pg->lock, flags);
>  
>  	if (start_queue &&
> @@ -912,7 +925,7 @@ static int alua_activate(struct scsi_device *sdev,
>  	kref_get(&pg->kref);
>  	rcu_read_unlock();
>  
> -	alua_rtpg_queue(pg, sdev, qdata);
> +	alua_rtpg_queue(pg, sdev, qdata, true);
>  	kref_put(&pg->kref, release_port_group);
>  out:
>  	if (fn)
> @@ -921,6 +934,29 @@ out:
>  }
>  
>  /*
> + * 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, bool force)
> +{
> +	struct alua_dh_data *h = sdev->handler_data;
> +	struct alua_port_group *pg;
> +
> +	rcu_read_lock();
> +	pg = rcu_dereference(h->pg);
> +	if (!pg) {
> +		rcu_read_unlock();
> +		return;
> +	}
> +	kref_get(&pg->kref);

What protects us from pg->kref beeing released?  I think the whole
refcounting scheme needs an audit to see where kref_get is called
without synchronization and use kref_get_unless_zero where needed.

Re: [PATCH 13/20] scsi_dh_alua: Recheck state on unit attention

[Hannes Reinecke]

On 12/30/2015 02:22 PM, Christoph Hellwig wrote:
> On Tue, Dec 08, 2015 at 08:37:33AM +0100, Hannes Reinecke wrote:
>> When we receive a unit attention code of 'ALUA state changed'
>> we should recheck the state, as it might be due to an implicit
>> ALUA state transition. This allows us to return NEEDS_RETRY
>> instead of ADD_TO_MLQUEUE, allowing to terminate the retries
>> after a certain time.
>> At the same time a workqueue item might already be queued, which
>> should be started immediately to avoid any delays.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/device_handler/scsi_dh_alua.c | 58 ++++++++++++++++++++++++------
>>   1 file changed, 47 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
>> index 525449f..04a3a543 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -121,7 +121,8 @@ struct alua_queue_data {
>>   static void alua_rtpg_work(struct work_struct *work);
>>   static void alua_rtpg_queue(struct alua_port_group *pg,
>>   			    struct scsi_device *sdev,
>> -			    struct alua_queue_data *qdata);
>> +			    struct alua_queue_data *qdata, bool force);
>> +static void alua_check(struct scsi_device *sdev, bool force);
>>
>>   static void release_port_group(struct kref *kref)
>>   {
>> @@ -386,7 +387,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
>>   		rcu_assign_pointer(h->pg, pg);
>>   		pg_found = true;
>>   	}
>> -	alua_rtpg_queue(h->pg, sdev, NULL);
>> +	alua_rtpg_queue(h->pg, sdev, NULL, true);
>>   	spin_unlock(&h->pg_lock);
>>
>>   	if (pg_found)
>> @@ -427,18 +428,24 @@ 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
>>   			 */
>> -			return ADD_TO_MLQUEUE;
>> +			alua_check(sdev, false);
>> +			return NEEDS_RETRY;
>> +		}
>>   		break;
>>   	case UNIT_ATTENTION:
>> -		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
>> +		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00) {
>>   			/*
>> -			 * Power On, Reset, or Bus Device Reset, just retry.
>> +			 * Power On, Reset, or Bus Device Reset.
>> +			 * Might have obscured a state transition,
>> +			 * so schedule a recheck.
>>   			 */
>> +			alua_check(sdev, true);
>>   			return ADD_TO_MLQUEUE;
>> +		}
>>   		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x04)
>>   			/*
>>   			 * Device internal reset
>> @@ -449,16 +456,20 @@ static int alua_check_sense(struct scsi_device *sdev,
>>   			 * Mode Parameters Changed
>>   			 */
>>   			return ADD_TO_MLQUEUE;
>> -		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06)
>> +		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06) {
>>   			/*
>>   			 * ALUA state changed
>>   			 */
>> +			alua_check(sdev, true);
>>   			return ADD_TO_MLQUEUE;
>> -		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x07)
>> +		}
>> +		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x07) {
>>   			/*
>>   			 * Implicit ALUA state transition failed
>>   			 */
>> +			alua_check(sdev, true);
>>   			return ADD_TO_MLQUEUE;
>> +		}
>>   		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x03)
>>   			/*
>>   			 * Inquiry data has changed
>> @@ -777,7 +788,7 @@ static void alua_rtpg_work(struct work_struct *work)
>>
>>   static void alua_rtpg_queue(struct alua_port_group *pg,
>>   			    struct scsi_device *sdev,
>> -			    struct alua_queue_data *qdata)
>> +			    struct alua_queue_data *qdata, bool force)
>>   {
>>   	int start_queue = 0;
>>   	unsigned long flags;
>> @@ -797,7 +808,9 @@ static void alua_rtpg_queue(struct alua_port_group *pg,
>>   		pg->rtpg_sdev = sdev;
>>   		scsi_device_get(sdev);
>>   		start_queue = 1;
>> -	}
>> +	} else if (!(pg->flags & ALUA_PG_RUN_RTPG) && force)
>> +		start_queue = 1;
>> +
>>   	spin_unlock_irqrestore(&pg->lock, flags);
>>
>>   	if (start_queue &&
>> @@ -912,7 +925,7 @@ static int alua_activate(struct scsi_device *sdev,
>>   	kref_get(&pg->kref);
>>   	rcu_read_unlock();
>>
>> -	alua_rtpg_queue(pg, sdev, qdata);
>> +	alua_rtpg_queue(pg, sdev, qdata, true);
>>   	kref_put(&pg->kref, release_port_group);
>>   out:
>>   	if (fn)
>> @@ -921,6 +934,29 @@ out:
>>   }
>>
>>   /*
>> + * 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, bool force)
>> +{
>> +	struct alua_dh_data *h = sdev->handler_data;
>> +	struct alua_port_group *pg;
>> +
>> +	rcu_read_lock();
>> +	pg = rcu_dereference(h->pg);
>> +	if (!pg) {
>> +		rcu_read_unlock();
>> +		return;
>> +	}
>> +	kref_get(&pg->kref);
>
> What protects us from pg->kref beeing released?  I think the whole
> refcounting scheme needs an audit to see where kref_get is called
> without synchronization and use kref_get_unless_zero where needed.
>
Hehe. This really is a bit of an awkward point. The overall idea here is 
that rcu_dereference() will give us a port_group structure.
But seeing that this pointer is not necessarily synchronized we need to 
use kref_get_unless_zero here, true.

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

[PATCH 14/20] scsi_dh_alua: update all port states

[Hannes Reinecke]

When we read in the target port group state we should be
updating all affected port groups, otherwise we risk
running out of sync.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 04a3a543..6ddbb88 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -499,11 +499,13 @@ static int alua_check_sense(struct scsi_device *sdev,
 static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	struct scsi_sense_hdr sense_hdr;
+	struct alua_port_group *tmp_pg;
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
-	unsigned char *ucp, *buff;
+	unsigned char *desc, *buff;
 	unsigned err, retval;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
+	unsigned long flags;
 
 	if (!pg->expiry) {
 		unsigned long transition_tmo = ALUA_FAILOVER_TIMEOUT * HZ;
@@ -605,16 +607,27 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	else
 		tpg_desc_tbl_off = 4;
 
-	for (k = tpg_desc_tbl_off, ucp = buff + tpg_desc_tbl_off;
+	for (k = tpg_desc_tbl_off, desc = buff + tpg_desc_tbl_off;
 	     k < len;
-	     k += off, ucp += off) {
-
-		if (pg->group_id == get_unaligned_be16(&ucp[2])) {
-			pg->state = ucp[0] & 0x0f;
-			pg->pref = ucp[0] >> 7;
-			valid_states = ucp[1];
+	     k += off, desc += off) {
+		u16 group_id = get_unaligned_be16(&desc[2]);
+
+		spin_lock_irqsave(&port_group_lock, flags);
+		list_for_each_entry(tmp_pg, &port_group_list, node) {
+			if (tmp_pg->group_id != group_id)
+				continue;
+			if (tmp_pg->device_id_len != pg->device_id_len)
+				continue;
+			if (strncmp(tmp_pg->device_id_str, pg->device_id_str,
+				    tmp_pg->device_id_len))
+				continue;
+			tmp_pg->state = desc[0] & 0x0f;
+			tmp_pg->pref = desc[0] >> 7;
+			if (tmp_pg == pg)
+				valid_states = desc[1];
 		}
-		off = 8 + (ucp[7] * 4);
+		spin_unlock_irqrestore(&port_group_lock, flags);
+		off = 8 + (desc[7] * 4);
 	}
 
 	sdev_printk(KERN_INFO, sdev,
-- 
1.8.5.6

Re: [PATCH 14/20] scsi_dh_alua: update all port states

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 08:37:34AM +0100, Hannes Reinecke wrote:
> When we read in the target port group state we should be
> updating all affected port groups, otherwise we risk
> running out of sync.

Why would we ever have multiple alua_port_group structures for the
same port that we'd need to iterate here?

Re: [PATCH 14/20] scsi_dh_alua: update all port states

[Hannes Reinecke]

On 12/30/2015 02:23 PM, Christoph Hellwig wrote:
> On Tue, Dec 08, 2015 at 08:37:34AM +0100, Hannes Reinecke wrote:
>> When we read in the target port group state we should be
>> updating all affected port groups, otherwise we risk
>> running out of sync.
>
> Why would we ever have multiple alua_port_group structures for the
> same port that we'd need to iterate here?
>
That is the whole point :-)
The index into the port_group structure is the tuple
(LUN ID, group ID), where each tuple corresponds to a different/separate 
port group state.
So for each LUN ID we will have at least two port_group structures, the 
states of which will be returned per call to REPORT TARGET PORT GROUPS.
And as the other paths might be unavailable we might not be able to 
execute REPORT TARGET PORT GROUPS at all. So we need to update it 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

[PATCH 15/20] scsi_dh_alua: Send TEST UNIT READY to poll for transitioning

[Hannes Reinecke]

Sending a 'REPORT TARGET PORT GROUP' command is a costly operation,
as the array has to gather information about all ports.
So instead of using RTPG to poll for a status update when a port
is in transitioning we should be sending a TEST UNIT READY, and
wait for the sense code to report success.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Ewan Milne <emilne@redhat.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 37 ++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 6ddbb88..6014750 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -489,6 +489,30 @@ static int alua_check_sense(struct scsi_device *sdev,
 }
 
 /*
+ * alua_tur - Send a TEST UNIT READY
+ * @sdev: device to which the TEST UNIT READY command should be send
+ *
+ * Send a TEST UNIT READY to @sdev to figure out the device state
+ * Returns SCSI_DH_RETRY if the sense code is NOT READY/ALUA TRANSITIONING,
+ * SCSI_DH_OK if no error occurred, and SCSI_DH_IO otherwise.
+ */
+static int alua_tur(struct scsi_device *sdev)
+{
+	struct scsi_sense_hdr sense_hdr;
+	int retval;
+
+	retval = scsi_test_unit_ready(sdev, ALUA_FAILOVER_TIMEOUT * HZ,
+				      ALUA_FAILOVER_RETRIES, &sense_hdr);
+	if (sense_hdr.sense_key == NOT_READY &&
+	    sense_hdr.asc == 0x04 && sense_hdr.ascq == 0x0a)
+		return SCSI_DH_RETRY;
+	else if (retval)
+		return SCSI_DH_IO;
+	else
+		return SCSI_DH_OK;
+}
+
+/*
  * alua_rtpg - Evaluate REPORT TARGET GROUP STATES
  * @sdev: the device to be evaluated.
  *
@@ -752,7 +776,20 @@ static void alua_rtpg_work(struct work_struct *work)
 	}
 	pg->flags |= ALUA_PG_RUNNING;
 	if (pg->flags & ALUA_PG_RUN_RTPG) {
+		int state = pg->state;
+
 		spin_unlock_irqrestore(&pg->lock, flags);
+		if (state == TPGS_STATE_TRANSITIONING) {
+			if (alua_tur(sdev) == SCSI_DH_RETRY) {
+				spin_lock_irqsave(&pg->lock, flags);
+				pg->flags &= ~ALUA_PG_RUNNING;
+				spin_unlock_irqrestore(&pg->lock, flags);
+				queue_delayed_work(kaluad_wq, &pg->rtpg_work,
+						   pg->interval * HZ);
+				return;
+			}
+			/* Send RTPG on failure or if TUR indicates SUCCESS */
+		}
 		err = alua_rtpg(sdev, pg);
 		spin_lock_irqsave(&pg->lock, flags);
 		if (err == SCSI_DH_RETRY) {
-- 
1.8.5.6

Re: [PATCH 15/20] scsi_dh_alua: Send TEST UNIT READY to poll for transitioning

[Christoph Hellwig]

Looks fine,

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

[PATCH 16/20] scsi_dh: add 'rescan' callback

[Hannes Reinecke]

If a device needs to be rescanned the device_handler might need
to be rechecked, too.
So add a 'rescan' callback to the device handler and call it
upon scsi_rescan_device().

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 8 ++++++++
 drivers/scsi/scsi_lib.c                    | 1 +
 drivers/scsi/scsi_scan.c                   | 8 +++++++-
 include/scsi/scsi_dh.h                     | 1 +
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 6014750..58c51ad 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -1040,6 +1040,13 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 
 }
 
+static void alua_rescan(struct scsi_device *sdev)
+{
+	struct alua_dh_data *h = sdev->handler_data;
+
+	alua_initialize(sdev, h);
+}
+
 /*
  * alua_bus_attach - Attach device handler
  * @sdev: device to be attached to
@@ -1104,6 +1111,7 @@ static struct scsi_device_handler alua_dh = {
 	.prep_fn = alua_prep_fn,
 	.check_sense = alua_check_sense,
 	.activate = alua_activate,
+	.rescan = alua_rescan,
 	.set_params = alua_set_params,
 };
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fa6b2c4..d46193a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2699,6 +2699,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt)
 		envp[idx++] = "SDEV_MEDIA_CHANGE=1";
 		break;
 	case SDEV_EVT_INQUIRY_CHANGE_REPORTED:
+		scsi_rescan_device(&sdev->sdev_gendev);
 		envp[idx++] = "SDEV_UA=INQUIRY_DATA_HAS_CHANGED";
 		break;
 	case SDEV_EVT_CAPACITY_CHANGE_REPORTED:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index a1c195d..05dc1aa 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -43,6 +43,7 @@
 #include <scsi/scsi_devinfo.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_transport.h>
+#include <scsi/scsi_dh.h>
 #include <scsi/scsi_eh.h>
 
 #include "scsi_priv.h"
@@ -1516,9 +1517,14 @@ EXPORT_SYMBOL(scsi_add_device);
 
 void scsi_rescan_device(struct device *dev)
 {
+	struct scsi_device *sdev = to_scsi_device(dev);
+
 	device_lock(dev);
 
-	scsi_attach_vpd(to_scsi_device(dev));
+	scsi_attach_vpd(sdev);
+
+	if (sdev->handler && sdev->handler->rescan)
+		sdev->handler->rescan(sdev);
 
 	if (dev->driver && try_module_get(dev->driver->owner)) {
 		struct scsi_driver *drv = to_scsi_driver(dev->driver);
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 7e184c6..c7bba2b 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -71,6 +71,7 @@ struct scsi_device_handler {
 	int (*activate)(struct scsi_device *, activate_complete, void *);
 	int (*prep_fn)(struct scsi_device *, struct request *);
 	int (*set_params)(struct scsi_device *, const char *);
+	void (*rescan)(struct scsi_device *);
 };
 
 #ifdef CONFIG_SCSI_DH
-- 
1.8.5.6

Re: [PATCH 16/20] scsi_dh: add 'rescan' callback

[Christoph Hellwig]

Looks fine,

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

[PATCH 17/20] scsi: Add 'access_state' attribute

[Hannes Reinecke]

Add an 'access_state' attribute to struct scsi_device to
display the asymmetric LUN access state.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_scan.c   |  1 +
 drivers/scsi/scsi_sysfs.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  1 +
 include/scsi/scsi_proto.h  | 13 ++++++++++++
 4 files changed, 64 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 05dc1aa..9fc1531 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	sdev->lun = lun;
 	sdev->channel = starget->channel;
 	sdev->sdev_state = SDEV_CREATED;
+	sdev->access_state = SCSI_ACCESS_STATE_UNKNOWN;
 	INIT_LIST_HEAD(&sdev->siblings);
 	INIT_LIST_HEAD(&sdev->same_target_siblings);
 	INIT_LIST_HEAD(&sdev->cmd_list);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ef36053..80b4c67 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -81,6 +81,34 @@ const char *scsi_host_state_name(enum scsi_host_state state)
 	return name;
 }
 
+static const struct {
+	enum scsi_access_state	value;
+	char			*name;
+} sdev_access_states[] = {
+	{ SCSI_ACCESS_STATE_OPTIMAL, "active/optimized" },
+	{ SCSI_ACCESS_STATE_ACTIVE, "active/non-optimized" },
+	{ SCSI_ACCESS_STATE_STANDBY, "standby" },
+	{ SCSI_ACCESS_STATE_UNAVAILABLE, "unavailable" },
+	{ SCSI_ACCESS_STATE_LBA, "lba-dependent" },
+	{ SCSI_ACCESS_STATE_OFFLINE, "offline" },
+	{ SCSI_ACCESS_STATE_TRANSITIONING, "transitioning" },
+	{ SCSI_ACCESS_STATE_UNKNOWN, "unknown" },
+};
+
+const char *scsi_access_state_name(enum scsi_access_state state)
+{
+	int i;
+	char *name = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(sdev_access_states); i++) {
+		if (sdev_access_states[i].value == state) {
+			name = sdev_access_states[i].name;
+			break;
+		}
+	}
+	return name;
+}
+
 static int check_set(unsigned long long *val, char *src)
 {
 	char *last;
@@ -973,6 +1001,26 @@ sdev_store_dh_state(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR(dh_state, S_IRUGO | S_IWUSR, sdev_show_dh_state,
 		   sdev_store_dh_state);
+
+static ssize_t
+sdev_show_access_state(struct device *dev,
+		       struct device_attribute *attr,
+		       char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	enum scsi_access_state access_state;
+	bool pref = false;
+
+	if (sdev->access_state & SCSI_ACCESS_STATE_PREFERRED)
+		pref = true;
+
+	access_state = (sdev->access_state & SCSI_ACCESS_STATE_MASK);
+
+	return snprintf(buf, 32, "%s%s\n",
+			scsi_access_state_name(access_state),
+			pref ? " preferred" : "");
+}
+static DEVICE_ATTR(access_state, S_IRUGO, sdev_show_access_state, NULL);
 #endif
 
 static ssize_t
@@ -1047,6 +1095,7 @@ static struct attribute *scsi_sdev_attrs[] = {
 	&dev_attr_wwid.attr,
 #ifdef CONFIG_SCSI_DH
 	&dev_attr_dh_state.attr,
+	&dev_attr_access_state.attr,
 #endif
 	&dev_attr_queue_ramp_up_period.attr,
 	REF_EVT(media_change),
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index f63a167..5dd00e9f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -200,6 +200,7 @@ struct scsi_device {
 	struct scsi_device_handler *handler;
 	void			*handler_data;
 
+	enum scsi_access_state	access_state;
 	enum scsi_device_state sdev_state;
 	unsigned long		sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index a9fbf1b..80e85e7 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -277,5 +277,18 @@ struct scsi_lun {
 	__u8 scsi_lun[8];
 };
 
+/* SPC asymmetric access states */
+enum scsi_access_state {
+	SCSI_ACCESS_STATE_OPTIMAL = 0,
+	SCSI_ACCESS_STATE_ACTIVE,
+	SCSI_ACCESS_STATE_STANDBY,
+	SCSI_ACCESS_STATE_UNAVAILABLE,
+	SCSI_ACCESS_STATE_LBA,
+	SCSI_ACCESS_STATE_OFFLINE = 0xe,
+	SCSI_ACCESS_STATE_TRANSITIONING = 0xf,
+	SCSI_ACCESS_STATE_UNKNOWN = 0x70,
+};
+#define SCSI_ACCESS_STATE_MASK 0x0f
+#define SCSI_ACCESS_STATE_PREFERRED 0x80
 
 #endif /* _SCSI_PROTO_H_ */
-- 
1.8.5.6

Re: [PATCH 17/20] scsi: Add 'access_state' attribute

[Christoph Hellwig]

Looks fine,

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

Re: [PATCH 17/20] scsi: Add 'access_state' attribute

[Christoph Hellwig]

Looks fine,

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

[PATCH 18/20] scsi_dh_alua: use common definitions for ALUA state

[Hannes Reinecke]

scsi_proto.h now contains definitions for the ALUA state,
so we don't have to carry them in the device handler.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 58c51ad..6c6ff0b 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -31,14 +31,6 @@
 #define ALUA_DH_NAME "alua"
 #define ALUA_DH_VER "1.3"
 
-#define TPGS_STATE_OPTIMIZED		0x0
-#define TPGS_STATE_NONOPTIMIZED		0x1
-#define TPGS_STATE_STANDBY		0x2
-#define TPGS_STATE_UNAVAILABLE		0x3
-#define TPGS_STATE_LBA_DEPENDENT	0x4
-#define TPGS_STATE_OFFLINE		0xe
-#define TPGS_STATE_TRANSITIONING	0xf
-
 #define TPGS_SUPPORT_NONE		0x00
 #define TPGS_SUPPORT_OPTIMIZED		0x01
 #define TPGS_SUPPORT_NONOPTIMIZED	0x02
@@ -180,7 +172,7 @@ static int submit_stpg(struct scsi_device *sdev, int group_id,
 
 	/* Prepare the data buffer */
 	memset(stpg_data, 0, stpg_len);
-	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
+	stpg_data[4] = SCSI_ACCESS_STATE_OPTIMAL & 0x0f;
 	put_unaligned_be16(group_id, &stpg_data[6]);
 
 	/* Prepare the command. */
@@ -244,7 +236,7 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
 	pg->device_id_len = id_size;
 	pg->group_id = group_id;
 	pg->tpgs = tpgs;
-	pg->state = TPGS_STATE_OPTIMIZED;
+	pg->state = SCSI_ACCESS_STATE_OPTIMAL;
 	if (optimize_stpg)
 		pg->flags |= ALUA_OPTIMIZE_STPG;
 	kref_init(&pg->kref);
@@ -401,22 +393,22 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 	return SCSI_DH_OK;
 }
 
-static char print_alua_state(int state)
+static char print_alua_state(enum scsi_access_state state)
 {
 	switch (state) {
-	case TPGS_STATE_OPTIMIZED:
+	case SCSI_ACCESS_STATE_OPTIMAL:
 		return 'A';
-	case TPGS_STATE_NONOPTIMIZED:
+	case SCSI_ACCESS_STATE_ACTIVE:
 		return 'N';
-	case TPGS_STATE_STANDBY:
+	case SCSI_ACCESS_STATE_STANDBY:
 		return 'S';
-	case TPGS_STATE_UNAVAILABLE:
+	case SCSI_ACCESS_STATE_UNAVAILABLE:
 		return 'U';
-	case TPGS_STATE_LBA_DEPENDENT:
+	case SCSI_ACCESS_STATE_LBA:
 		return 'L';
-	case TPGS_STATE_OFFLINE:
+	case SCSI_ACCESS_STATE_OFFLINE:
 		return 'O';
-	case TPGS_STATE_TRANSITIONING:
+	case SCSI_ACCESS_STATE_TRANSITIONING:
 		return 'T';
 	default:
 		return 'X';
@@ -667,7 +659,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
 
 	switch (pg->state) {
-	case TPGS_STATE_TRANSITIONING:
+	case SCSI_ACCESS_STATE_TRANSITIONING:
 		if (time_before(jiffies, pg->expiry)) {
 			/* State transition, retry */
 			pg->interval = 2;
@@ -675,11 +667,11 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		} else {
 			/* Transitioning time exceeded, set port to standby */
 			err = SCSI_DH_IO;
-			pg->state = TPGS_STATE_STANDBY;
+			pg->state = SCSI_ACCESS_STATE_STANDBY;
 			pg->expiry = 0;
 		}
 		break;
-	case TPGS_STATE_OFFLINE:
+	case SCSI_ACCESS_STATE_OFFLINE:
 		/* Path unusable */
 		err = SCSI_DH_DEV_OFFLINED;
 		pg->expiry = 0;
@@ -712,21 +704,21 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		return SCSI_DH_RETRY;
 	}
 	switch (pg->state) {
-	case TPGS_STATE_OPTIMIZED:
+	case SCSI_ACCESS_STATE_OPTIMAL:
 		return SCSI_DH_OK;
-	case TPGS_STATE_NONOPTIMIZED:
+	case SCSI_ACCESS_STATE_ACTIVE:
 		if ((pg->flags & ALUA_OPTIMIZE_STPG) &&
 		    !pg->pref &&
 		    (pg->tpgs & TPGS_MODE_IMPLICIT))
 			return SCSI_DH_OK;
 		break;
-	case TPGS_STATE_STANDBY:
-	case TPGS_STATE_UNAVAILABLE:
+	case SCSI_ACCESS_STATE_STANDBY:
+	case SCSI_ACCESS_STATE_UNAVAILABLE:
 		break;
-	case TPGS_STATE_OFFLINE:
+	case SCSI_ACCESS_STATE_OFFLINE:
 		return SCSI_DH_IO;
 		break;
-	case TPGS_STATE_TRANSITIONING:
+	case SCSI_ACCESS_STATE_TRANSITIONING:
 		break;
 	default:
 		sdev_printk(KERN_INFO, sdev,
@@ -736,7 +728,7 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		break;
 	}
 	/* Set state to transitioning */
-	pg->state = TPGS_STATE_TRANSITIONING;
+	pg->state = SCSI_ACCESS_STATE_TRANSITIONING;
 	retval = submit_stpg(sdev, pg->group_id, &sense_hdr);
 
 	if (retval) {
@@ -779,7 +771,7 @@ static void alua_rtpg_work(struct work_struct *work)
 		int state = pg->state;
 
 		spin_unlock_irqrestore(&pg->lock, flags);
-		if (state == TPGS_STATE_TRANSITIONING) {
+		if (state == SCSI_ACCESS_STATE_TRANSITIONING) {
 			if (alua_tur(sdev) == SCSI_DH_RETRY) {
 				spin_lock_irqsave(&pg->lock, flags);
 				pg->flags &= ~ALUA_PG_RUNNING;
@@ -1016,7 +1008,7 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 {
 	struct alua_dh_data *h = sdev->handler_data;
 	struct alua_port_group *pg;
-	int state = TPGS_STATE_OPTIMIZED;
+	enum scsi_access_state state = SCSI_ACCESS_STATE_OPTIMAL;
 	int ret = BLKPREP_OK;
 
 	rcu_read_lock();
@@ -1025,14 +1017,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 		state = pg->state;
 		/* Defer I/O while rtpg_work is active */
 		if (pg->rtpg_sdev)
-			state = TPGS_STATE_TRANSITIONING;
+			state = SCSI_ACCESS_STATE_TRANSITIONING;
 	}
 	rcu_read_unlock();
-	if (state == TPGS_STATE_TRANSITIONING)
+	if (state == SCSI_ACCESS_STATE_TRANSITIONING)
 		ret = BLKPREP_DEFER;
-	else if (state != TPGS_STATE_OPTIMIZED &&
-		 state != TPGS_STATE_NONOPTIMIZED &&
-		 state != TPGS_STATE_LBA_DEPENDENT) {
+	else if (state != SCSI_ACCESS_STATE_OPTIMAL &&
+		 state != SCSI_ACCESS_STATE_ACTIVE &&
+		 state != SCSI_ACCESS_STATE_LBA) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}
-- 
1.8.5.6

Re: [PATCH 18/20] scsi_dh_alua: use common definitions for ALUA state

[Christoph Hellwig]

Looks fine,

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

[PATCH 19/20] scsi_dh_alua: update 'access_state' field

[Hannes Reinecke]

Track attached SCSI devices and update the 'access_state' field
whenever an ALUA state change has been detected.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 6c6ff0b..d01c86407 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -24,6 +24,7 @@
 #include <linux/module.h>
 #include <asm/unaligned.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_proto.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dh.h>
@@ -76,6 +77,7 @@ static struct workqueue_struct *kaluad_wq;
 struct alua_port_group {
 	struct kref		kref;
 	struct list_head	node;
+	struct list_head	dh_list;
 	unsigned char		device_id_str[256];
 	int			device_id_len;
 	int			group_id;
@@ -93,6 +95,7 @@ struct alua_port_group {
 };
 
 struct alua_dh_data {
+	struct list_head	node;
 	struct alua_port_group	*pg;
 	int			rel_port;
 	spinlock_t		pg_lock;
@@ -243,6 +246,7 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
 	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
 	INIT_LIST_HEAD(&pg->rtpg_list);
 	INIT_LIST_HEAD(&pg->node);
+	INIT_LIST_HEAD(&pg->dh_list);
 	spin_lock_init(&pg->lock);
 
 	/* Re-check list again to catch concurrent updates */
@@ -370,13 +374,26 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
 		old_pg = pg;
 		/* port_group has changed. Update to new port group */
 		if (h->pg != pg) {
+			unsigned long flags;
+
 			old_pg = h->pg;
 			rcu_assign_pointer(h->pg, pg);
+			spin_lock_irqsave(&old_pg->lock, flags);
+			list_del_rcu(&h->node);
+			spin_unlock_irqrestore(&old_pg->lock, flags);
+			spin_lock_irqsave(&pg->lock, flags);
+			list_add_rcu(&h->node, &pg->dh_list);
+			spin_unlock_irqrestore(&pg->lock, flags);
 			h->pg->expiry = 0;
 			pg_found = true;
 		}
 	} else {
+		unsigned long flags;
+
 		rcu_assign_pointer(h->pg, pg);
+		spin_lock_irqsave(&pg->lock, flags);
+		list_add_rcu(&h->node, &pg->dh_list);
+		spin_unlock_irqrestore(&pg->lock, flags);
 		pg_found = true;
 	}
 	alua_rtpg_queue(h->pg, sdev, NULL, true);
@@ -630,6 +647,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 
 		spin_lock_irqsave(&port_group_lock, flags);
 		list_for_each_entry(tmp_pg, &port_group_list, node) {
+			struct alua_dh_data *h;
+
 			if (tmp_pg->group_id != group_id)
 				continue;
 			if (tmp_pg->device_id_len != pg->device_id_len)
@@ -639,6 +658,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 				continue;
 			tmp_pg->state = desc[0] & 0x0f;
 			tmp_pg->pref = desc[0] >> 7;
+			rcu_read_lock();
+			list_for_each_entry_rcu(h, &tmp_pg->dh_list, node) {
+				if (h->sdev)
+					h->sdev->access_state = desc[0];
+			}
+			rcu_read_unlock();
 			if (tmp_pg == pg)
 				valid_states = desc[1];
 		}
@@ -1056,6 +1081,7 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	h->rel_port = -1;
 	h->init_error = SCSI_DH_OK;
 	h->sdev = sdev;
+	INIT_LIST_HEAD(&h->node);
 
 	mutex_init(&h->init_mutex);
 	err = alua_initialize(sdev, h);
@@ -1086,6 +1112,9 @@ static void alua_bus_detach(struct scsi_device *sdev)
 	h->sdev = NULL;
 	spin_unlock(&h->pg_lock);
 	if (pg) {
+		spin_lock(&pg->lock);
+		list_del_rcu(&h->node);
+		spin_unlock(&pg->lock);
 		synchronize_rcu();
 		if (pg->rtpg_sdev)
 			flush_delayed_work(&pg->rtpg_work);
-- 
1.8.5.6

Re: [PATCH 19/20] scsi_dh_alua: update 'access_state' field

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 08:37:39AM +0100, Hannes Reinecke wrote:
> Track attached SCSI devices and update the 'access_state' field
> whenever an ALUA state change has been detected.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 6c6ff0b..d01c86407 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -24,6 +24,7 @@
>  #include <linux/module.h>
>  #include <asm/unaligned.h>
>  #include <scsi/scsi.h>
> +#include <scsi/scsi_proto.h>
>  #include <scsi/scsi_dbg.h>
>  #include <scsi/scsi_eh.h>
>  #include <scsi/scsi_dh.h>
> @@ -76,6 +77,7 @@ static struct workqueue_struct *kaluad_wq;
>  struct alua_port_group {
>  	struct kref		kref;
>  	struct list_head	node;
> +	struct list_head	dh_list;
>  	unsigned char		device_id_str[256];
>  	int			device_id_len;
>  	int			group_id;
> @@ -93,6 +95,7 @@ struct alua_port_group {
>  };
>  
>  struct alua_dh_data {
> +	struct list_head	node;
>  	struct alua_port_group	*pg;
>  	int			rel_port;
>  	spinlock_t		pg_lock;
> @@ -243,6 +246,7 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
>  	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
>  	INIT_LIST_HEAD(&pg->rtpg_list);
>  	INIT_LIST_HEAD(&pg->node);
> +	INIT_LIST_HEAD(&pg->dh_list);
>  	spin_lock_init(&pg->lock);
>  
>  	/* Re-check list again to catch concurrent updates */
> @@ -370,13 +374,26 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
>  		old_pg = pg;
>  		/* port_group has changed. Update to new port group */
>  		if (h->pg != pg) {
> +			unsigned long flags;
> +
>  			old_pg = h->pg;
>  			rcu_assign_pointer(h->pg, pg);
> +			spin_lock_irqsave(&old_pg->lock, flags);
> +			list_del_rcu(&h->node);
> +			spin_unlock_irqrestore(&old_pg->lock, flags);
> +			spin_lock_irqsave(&pg->lock, flags);
> +			list_add_rcu(&h->node, &pg->dh_list);
> +			spin_unlock_irqrestore(&pg->lock, flags);

I think it makes more sense to remove it from the old list
before assining the new h->pg pointer to stay consistant.

Also the add code is not common with the other half of the outer
branch, it would be nice to find a way to share the code between
those two branches.

>  		list_for_each_entry(tmp_pg, &port_group_list, node) {
> +			struct alua_dh_data *h;
> +
>  			if (tmp_pg->group_id != group_id)
>  				continue;
>  			if (tmp_pg->device_id_len != pg->device_id_len)
> @@ -639,6 +658,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  				continue;
>  			tmp_pg->state = desc[0] & 0x0f;
>  			tmp_pg->pref = desc[0] >> 7;
> +			rcu_read_lock();
> +			list_for_each_entry_rcu(h, &tmp_pg->dh_list, node) {
> +				if (h->sdev)
> +					h->sdev->access_state = desc[0];
> +			}
> +			rcu_read_unlock();

І'd expect h->ѕdev to always be non-NULL.  With a little tweak (see below)
that should indeed be the case.

> @@ -1086,6 +1112,9 @@ static void alua_bus_detach(struct scsi_device *sdev)
>  	h->sdev = NULL;
>  	spin_unlock(&h->pg_lock);
>  	if (pg) {
> +		spin_lock(&pg->lock);
> +		list_del_rcu(&h->node);
> +		spin_unlock(&pg->lock);
>  		synchronize_rcu();
>  		if (pg->rtpg_sdev)
>  			flush_delayed_work(&pg->rtpg_work);

Here we'd just need to make sure to do the list_del before setting
h->sdev to NULL.
--
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 19/20] scsi_dh_alua: update 'access_state' field

[Hannes Reinecke]

On 12/30/2015 02:34 PM, Christoph Hellwig wrote:
> On Tue, Dec 08, 2015 at 08:37:39AM +0100, Hannes Reinecke wrote:
>> Track attached SCSI devices and update the 'access_state' field
>> whenever an ALUA state change has been detected.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/device_handler/scsi_dh_alua.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
>> index 6c6ff0b..d01c86407 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/module.h>
>>   #include <asm/unaligned.h>
>>   #include <scsi/scsi.h>
>> +#include <scsi/scsi_proto.h>
>>   #include <scsi/scsi_dbg.h>
>>   #include <scsi/scsi_eh.h>
>>   #include <scsi/scsi_dh.h>
>> @@ -76,6 +77,7 @@ static struct workqueue_struct *kaluad_wq;
>>   struct alua_port_group {
>>   	struct kref		kref;
>>   	struct list_head	node;
>> +	struct list_head	dh_list;
>>   	unsigned char		device_id_str[256];
>>   	int			device_id_len;
>>   	int			group_id;
>> @@ -93,6 +95,7 @@ struct alua_port_group {
>>   };
>>
>>   struct alua_dh_data {
>> +	struct list_head	node;
>>   	struct alua_port_group	*pg;
>>   	int			rel_port;
>>   	spinlock_t		pg_lock;
>> @@ -243,6 +246,7 @@ struct alua_port_group *alua_get_pg(struct scsi_device *sdev,
>>   	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
>>   	INIT_LIST_HEAD(&pg->rtpg_list);
>>   	INIT_LIST_HEAD(&pg->node);
>> +	INIT_LIST_HEAD(&pg->dh_list);
>>   	spin_lock_init(&pg->lock);
>>
>>   	/* Re-check list again to catch concurrent updates */
>> @@ -370,13 +374,26 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h,
>>   		old_pg = pg;
>>   		/* port_group has changed. Update to new port group */
>>   		if (h->pg != pg) {
>> +			unsigned long flags;
>> +
>>   			old_pg = h->pg;
>>   			rcu_assign_pointer(h->pg, pg);
>> +			spin_lock_irqsave(&old_pg->lock, flags);
>> +			list_del_rcu(&h->node);
>> +			spin_unlock_irqrestore(&old_pg->lock, flags);
>> +			spin_lock_irqsave(&pg->lock, flags);
>> +			list_add_rcu(&h->node, &pg->dh_list);
>> +			spin_unlock_irqrestore(&pg->lock, flags);
>
> I think it makes more sense to remove it from the old list
> before assining the new h->pg pointer to stay consistant.
>
Hmm. Yes, good point.

> Also the add code is not common with the other half of the outer
> branch, it would be nice to find a way to share the code between
> those two branches.
>
Indeed, it would. But so far I haven't found a nice way for that...

>>   		list_for_each_entry(tmp_pg, &port_group_list, node) {
>> +			struct alua_dh_data *h;
>> +
>>   			if (tmp_pg->group_id != group_id)
>>   				continue;
>>   			if (tmp_pg->device_id_len != pg->device_id_len)
>> @@ -639,6 +658,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>>   				continue;
>>   			tmp_pg->state = desc[0] & 0x0f;
>>   			tmp_pg->pref = desc[0] >> 7;
>> +			rcu_read_lock();
>> +			list_for_each_entry_rcu(h, &tmp_pg->dh_list, node) {
>> +				if (h->sdev)
>> +					h->sdev->access_state = desc[0];
>> +			}
>> +			rcu_read_unlock();
>
> І'd expect h->ѕdev to always be non-NULL.  With a little tweak (see below)
> that should indeed be the case.
>
>> @@ -1086,6 +1112,9 @@ static void alua_bus_detach(struct scsi_device *sdev)
>>   	h->sdev = NULL;
>>   	spin_unlock(&h->pg_lock);
>>   	if (pg) {
>> +		spin_lock(&pg->lock);
>> +		list_del_rcu(&h->node);
>> +		spin_unlock(&pg->lock);
>>   		synchronize_rcu();
>>   		if (pg->rtpg_sdev)
>>   			flush_delayed_work(&pg->rtpg_work);
>
> Here we'd just need to make sure to do the list_del before setting
> h->sdev to NULL.
>
Okay, true. Will do.

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

[PATCH 20/20] scsi_dh_alua: Update version to 2.0

[Hannes Reinecke]

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index d01c86407..1f97b65 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -30,7 +30,7 @@
 #include <scsi/scsi_dh.h>
 
 #define ALUA_DH_NAME "alua"
-#define ALUA_DH_VER "1.3"
+#define ALUA_DH_VER "2.0"
 
 #define TPGS_SUPPORT_NONE		0x00
 #define TPGS_SUPPORT_OPTIMIZED		0x01
-- 
1.8.5.6

Re: [PATCH 00/20] ALUA device handler update, part II

[Hannes Reinecke]

On 12/08/2015 08:37 AM, Hannes Reinecke wrote:
> Hi all,
>
> as promised here is now the second part of my ALUA device handler update.
> This contains a major rework of the ALUA device handler as execution is
> moved onto a workqueue. This has the advantage that we avoid having to
> do multiple calls to the same LUN (as happens frequently when failing
> over a LUN with several paths) and finally retries are handled correctly.
> As some arrays are only capable of handling one STPG at a time I've added
> a module parameter 'sync_stpg' which switches to a singlethreaded
> workqueue, thereby effectively synchronize STPG handling.
> Thanks to Bart for this suggestion.
>
> As usual, comments and reviews are welcome.
>
As I've been reminded a git reference would be welcome for review.
So you can find the entire patchset at

git://git.kernel.org/pub/scm/linux/kernel/git/hare/scsi-devel.git
branch alua.v8

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