[PATCH 00/17] asynchronous ALUA device handler

[PATCH 00/17] asynchronous ALUA device handler

[Hannes Reinecke]

Hi all,

here is an update to the ALUA device handler. The main
features are:

- Topology discovery: the device handler creates a separate
  port_group structure, which is used to update all paths to
  the same port group. With that we achieve a significant
  reduction of the number of RTPGs.
- Asynchronous state update: The ALUA state is now updated
  from a workqueue item, so all concurrent RTPG calls are
  buffered with that.
- Use the existing vpd page 0x83 to detect device IDs

The patchset is relative to hch's scsi_dh update.

As usual, reviews and comments are welcome.

Hannes Reinecke (17):
  scsi_dh: return individual errors in scsi_dh_activate()
  scsi_dh_alua: Disable ALUA handling for non-disk devices
  scsi_dh_alua: Use vpd_pg83 information
  scsi_dh_alua: Improve error handling
  scsi: remove scsi_show_sense_hdr()
  scsi_dh_alua: use flag for RTPG extended header
  scsi_dh_alua: Pass buffer as function argument
  scsi_dh_alua: Make stpg synchronous
  scsi_dh_alua: switch to scsi_execute()
  scsi_dh_alua: Use separate alua_port_group structure
  scsi_dh_alua: simplify sense code handling
  scsi_dh_alua: parse target device id
  scsi_dh_alua: revert commit a8e5a2d593cbfccf530c3382c2c328d2edaa7b66
  scsi_dh_alua: Use workqueue for RTPG
  scsi_dh_alua: Recheck state on unit attention
  scsi_dh_alua: update all port states
  scsi_dh_alua: Update version to 2.0

 drivers/scsi/device_handler/scsi_dh.c      |    3 +-
 drivers/scsi/device_handler/scsi_dh_alua.c | 1099 +++++++++++++++++-----------
 include/scsi/scsi_dbg.h                    |    2 -
 3 files changed, 693 insertions(+), 411 deletions(-)

-- 
1.8.5.2

[PATCH 01/17] scsi_dh: return individual errors in scsi_dh_activate()

[Hannes Reinecke]

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

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

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 155abeab..a4afd30 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -315,9 +315,10 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 
 	if (!sdev->handler)
 		goto out_fn;
+	err = SCSI_DH_NOTCONN;
 	if (sdev->sdev_state == SDEV_CANCEL ||
 	    sdev->sdev_state == SDEV_DEL)
-	    	goto out_fn;
+		goto out_fn;
 
 	err = SCSI_DH_DEV_OFFLINED;
 	if (sdev->sdev_state == SDEV_OFFLINE)
-- 
1.8.5.2

Re: [PATCH 01/17] scsi_dh: return individual errors in scsi_dh_activate()

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> When calling scsi_dh_activate() we should be returning
> individual errors and not lumping all into one.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/device_handler/scsi_dh.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
> index 155abeab..a4afd30 100644
> --- a/drivers/scsi/device_handler/scsi_dh.c
> +++ b/drivers/scsi/device_handler/scsi_dh.c
> @@ -315,9 +315,10 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
>
>   	if (!sdev->handler)
>   		goto out_fn;
> +	err = SCSI_DH_NOTCONN;
>   	if (sdev->sdev_state == SDEV_CANCEL ||
>   	    sdev->sdev_state == SDEV_DEL)
> -	    	goto out_fn;
> +		goto out_fn;
>
>   	err = SCSI_DH_DEV_OFFLINED;
>   	if (sdev->sdev_state == SDEV_OFFLINE)
>

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Re: [PATCH 01/17] scsi_dh: return individual errors in scsi_dh_activate()

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:07PM +0200, Hannes Reinecke wrote:
> When calling scsi_dh_activate() we should be returning
> individual errors and not lumping all into one.

Looks good,

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

(although you might need a rebase, my v2 post has some white space changes
that git-am complained about, sorry)

[PATCH 02/17] scsi_dh_alua: Disable ALUA handling for non-disk devices

[Hannes Reinecke]

Non-disk devices should be ignored when detecting
ALUA capabilities.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index e418849..834e80f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -320,6 +320,23 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	int err = SCSI_DH_OK;
 
+	if (scsi_is_wlun(sdev->lun)) {
+		h->tpgs = TPGS_MODE_NONE;
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: disable for WLUN\n",
+			    ALUA_DH_NAME);
+		return SCSI_DH_DEV_UNSUPP;
+	}
+	if (sdev->type != TYPE_DISK &&
+	    sdev->type != TYPE_RBC &&
+	    sdev->type != TYPE_OSD) {
+		h->tpgs = TPGS_MODE_NONE;
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: disable for non-disk devices\n",
+			    ALUA_DH_NAME);
+		return SCSI_DH_DEV_UNSUPP;
+	}
+
 	h->tpgs = scsi_device_tpgs(sdev);
 	switch (h->tpgs) {
 	case TPGS_MODE_EXPLICIT|TPGS_MODE_IMPLICIT:
-- 
1.8.5.2

Re: [PATCH 02/17] scsi_dh_alua: Disable ALUA handling for non-disk devices

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> Non-disk devices should be ignored when detecting
> ALUA capabilities.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/device_handler/scsi_dh_alua.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index e418849..834e80f 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -320,6 +320,23 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
>   {
>   	int err = SCSI_DH_OK;
>
> +	if (scsi_is_wlun(sdev->lun)) {
> +		h->tpgs = TPGS_MODE_NONE;
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: disable for WLUN\n",
> +			    ALUA_DH_NAME);
> +		return SCSI_DH_DEV_UNSUPP;
> +	}
> +	if (sdev->type != TYPE_DISK &&
> +	    sdev->type != TYPE_RBC &&
> +	    sdev->type != TYPE_OSD) {
> +		h->tpgs = TPGS_MODE_NONE;
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: disable for non-disk devices\n",
> +			    ALUA_DH_NAME);
> +		return SCSI_DH_DEV_UNSUPP;
> +	}
> +
>   	h->tpgs = scsi_device_tpgs(sdev);
>   	switch (h->tpgs) {
>   	case TPGS_MODE_EXPLICIT|TPGS_MODE_IMPLICIT:
>

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Re: [PATCH 02/17] scsi_dh_alua: Disable ALUA handling for non-disk devices

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:08PM +0200, Hannes Reinecke wrote:
> Non-disk devices should be ignored when detecting
> ALUA capabilities.

Hmm.  I don't think we should even attach the alua handler in this case,
e.g. refine the check in scsi_dh_find_driver.  But then again I don't
remember how the spec wording actually disallows this, and a quick grep
of SPC-4 can't find anything either.  Can you put a reference to the spec
section that disallows attachment to non-disk devices into the code?

> +	if (sdev->type != TYPE_DISK &&
> +	    sdev->type != TYPE_RBC &&
> +	    sdev->type != TYPE_OSD) {

And does it really allow RBC (but not ZBC)?

Re: [PATCH 02/17] scsi_dh_alua: Disable ALUA handling for non-disk devices

[Hannes Reinecke]

On 05/11/2015 08:46 AM, Christoph Hellwig wrote:
> On Mon, May 04, 2015 at 02:42:08PM +0200, Hannes Reinecke wrote:
>> Non-disk devices should be ignored when detecting
>> ALUA capabilities.
> 
> Hmm.  I don't think we should even attach the alua handler in this case,
> e.g. refine the check in scsi_dh_find_driver.  But then again I don't
> remember how the spec wording actually disallows this, and a quick grep
> of SPC-4 can't find anything either.  Can you put a reference to the spec
> section that disallows attachment to non-disk devices into the code?
> 
The spec wording doesn't explicitly disallowing you from doing so
(unless you're using referrals).
But in realiter we're using the ALUA device handler only when
multipathing is used, which in turn runs on disk devices only.
So we can limit ourselves to disk devices, too.

Everything else (like tapes or tape changers) will have to
implement their own logic anyway; tape support in multipathing
with or without ALUA is too horrible to contemplate ...


>> +	if (sdev->type != TYPE_DISK &&
>> +	    sdev->type != TYPE_RBC &&
>> +	    sdev->type != TYPE_OSD) {
> 
> And does it really allow RBC (but not ZBC)?
> 
Thought so, but I'll have to cross-check.
Maybe it's time to implement a 'scsi_is_disk_type()' macro...

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

Re: [PATCH 02/17] scsi_dh_alua: Disable ALUA handling for non-disk devices

[Christoph Hellwig]

On Mon, May 11, 2015 at 12:25:51PM +0200, Hannes Reinecke wrote:
> The spec wording doesn't explicitly disallowing you from doing so
> (unless you're using referrals).
> But in realiter we're using the ALUA device handler only when
> multipathing is used, which in turn runs on disk devices only.
> So we can limit ourselves to disk devices, too.
> 
> Everything else (like tapes or tape changers) will have to
> implement their own logic anyway; tape support in multipathing
> with or without ALUA is too horrible to contemplate ...

So what is the problem you're trying to solve here?  We were attaching
to other nodes and so far I haven't heard about that being a problem.

Basically we'll disallow I/O for offlines paths to non-disk devices
once the ALUA handler is attached, which actually seems useful
even without a multipath handler on top.

> >> +	if (sdev->type != TYPE_DISK &&
> >> +	    sdev->type != TYPE_RBC &&
> >> +	    sdev->type != TYPE_OSD) {
> > 
> > And does it really allow RBC (but not ZBC)?
> > 
> Thought so, but I'll have to cross-check.
> Maybe it's time to implement a 'scsi_is_disk_type()' macro...

If the reason to disallow anything that we can't use dm-mpath on
both OSD and ZBC in it's current form are excluded.  RBC would
be included, although I doubt there is a multi ported RBC
device available.

Re: [PATCH 02/17] scsi_dh_alua: Disable ALUA handling for non-disk devices

[Hannes Reinecke]

On 05/11/2015 01:34 PM, Christoph Hellwig wrote:
> On Mon, May 11, 2015 at 12:25:51PM +0200, Hannes Reinecke wrote:
>> The spec wording doesn't explicitly disallowing you from doing so
>> (unless you're using referrals).
>> But in realiter we're using the ALUA device handler only when
>> multipathing is used, which in turn runs on disk devices only.
>> So we can limit ourselves to disk devices, too.
>>
>> Everything else (like tapes or tape changers) will have to
>> implement their own logic anyway; tape support in multipathing
>> with or without ALUA is too horrible to contemplate ...
> 
> So what is the problem you're trying to solve here?  We were attaching
> to other nodes and so far I haven't heard about that being a problem.
> 
> Basically we'll disallow I/O for offlines paths to non-disk devices
> once the ALUA handler is attached, which actually seems useful
> even without a multipath handler on top.
> 
I've had far too many issues with ALUA attaching to eg enclosure
devices; some enclosures have the habit of returning 'NOT READY,
CAUSE NOT REPORTABLE' when sending an RTPG to it.

Other (broken) devices set the 'TPGS' bit in the inquiry data,
but fail to implement support for everything else.

Currently we're just doing a retry for these cases, and would
have to wait for the timeout to kick in.

We _could_ implement a dedicated error handling here, but
then we're pretty much guaranteed to miss the odd case.
So I've decided to just skip them altogether.

If you insist we could remove this patch, and straighten out
error handling here.

>>>> +	if (sdev->type != TYPE_DISK &&
>>>> +	    sdev->type != TYPE_RBC &&
>>>> +	    sdev->type != TYPE_OSD) {
>>>
>>> And does it really allow RBC (but not ZBC)?
>>>
>> Thought so, but I'll have to cross-check.
>> Maybe it's time to implement a 'scsi_is_disk_type()' macro...
> 
> If the reason to disallow anything that we can't use dm-mpath on
> both OSD and ZBC in it's current form are excluded.  RBC would
> be included, although I doubt there is a multi ported RBC
> device available.
> 
As said above; we _could_ drop this patch and update error
handling, but then we'll risk of having more bug reports
due to invalid ALUA implementations.
Might be worth it, though, as then we could get in touch
with the respective vendors. But then, maybe not, as not
every vendor is willing to listen to us...

I don't really have a fixed opinion here; either way is
fine with me.

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

Re: [PATCH 02/17] scsi_dh_alua: Disable ALUA handling for non-disk devices

[Christoph Hellwig]

On Mon, May 11, 2015 at 01:55:15PM +0200, Hannes Reinecke wrote:
> As said above; we _could_ drop this patch and update error
> handling, but then we'll risk of having more bug reports
> due to invalid ALUA implementations.
> Might be worth it, though, as then we could get in touch
> with the respective vendors. But then, maybe not, as not
> every vendor is willing to listen to us...
> 
> I don't really have a fixed opinion here; either way is
> fine with me.

For now mayeb reject a everything but TYPE_SBC and add a comment
explaining why, i.e. a short form of the reasons in this mail.

[PATCH 03/17] scsi_dh_alua: Use vpd_pg83 information

[Hannes Reinecke]

The SCSI device now has the VPD page 0x83 information attached,
so there is no need to query it again.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 834e80f..b5903eb 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -131,43 +131,6 @@ static struct request *get_alua_req(struct scsi_device *sdev,
 }
 
 /*
- * submit_vpd_inquiry - Issue an INQUIRY VPD page 0x83 command
- * @sdev: sdev the command should be sent to
- */
-static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
-{
-	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
-
-	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
-		goto done;
-
-	/* Prepare the command. */
-	rq->cmd[0] = INQUIRY;
-	rq->cmd[1] = 1;
-	rq->cmd[2] = 0x83;
-	rq->cmd[4] = h->bufflen;
-	rq->cmd_len = COMMAND_SIZE(INQUIRY);
-
-	rq->sense = h->sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
-
-	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: evpd inquiry failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
-		h->senselen = rq->sense_len;
-		err = SCSI_DH_IO;
-	}
-	blk_put_request(rq);
-done:
-	return err;
-}
-
-/*
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
  */
@@ -352,55 +315,42 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 		sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n",
 			    ALUA_DH_NAME);
 		break;
-	default:
-		h->tpgs = TPGS_MODE_NONE;
+	case TPGS_MODE_NONE:
 		sdev_printk(KERN_INFO, sdev, "%s: not supported\n",
 			    ALUA_DH_NAME);
 		err = SCSI_DH_DEV_UNSUPP;
 		break;
+	default:
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: unsupported TPGS setting %d\n",
+			    ALUA_DH_NAME, h->tpgs);
+		h->tpgs = TPGS_MODE_NONE;
+		err = SCSI_DH_DEV_UNSUPP;
+		break;
 	}
 
 	return err;
 }
 
 /*
- * alua_vpd_inquiry - Evaluate INQUIRY vpd page 0x83
+ * alua_check_vpd - Evaluate INQUIRY vpd page 0x83
  * @sdev: device to be checked
  *
  * Extract the relative target port and the target port group
  * descriptor from the list of identificators.
  */
-static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
+static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int len;
-	unsigned err;
 	unsigned char *d;
 
- retry:
-	err = submit_vpd_inquiry(sdev, h);
-
-	if (err != SCSI_DH_OK)
-		return err;
-
-	/* Check if vpd page exceeds initial buffer */
-	len = (h->buff[2] << 8) + h->buff[3] + 4;
-	if (len > h->bufflen) {
-		/* Resubmit with the correct length */
-		if (realloc_buffer(h, len)) {
-			sdev_printk(KERN_WARNING, sdev,
-				    "%s: kmalloc buffer failed\n",
-				    ALUA_DH_NAME);
-			/* Temporary failure, bypass */
-			return SCSI_DH_DEV_TEMP_BUSY;
-		}
-		goto retry;
-	}
+	if (!sdev->vpd_pg83)
+		return SCSI_DH_DEV_UNSUPP;
 
 	/*
-	 * Now look for the correct descriptor.
+	 * Look for the correct descriptor.
 	 */
-	d = h->buff + 4;
-	while (d < h->buff + len) {
+	d = sdev->vpd_pg83 + 4;
+	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
 		switch (d[1] & 0xf) {
 		case 0x4:
 			/* Relative target port */
@@ -427,14 +377,13 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 			    ALUA_DH_NAME);
 		h->state = TPGS_STATE_OPTIMIZED;
 		h->tpgs = TPGS_MODE_NONE;
-		err = SCSI_DH_DEV_UNSUPP;
-	} else {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: port group %02x rel port %02x\n",
-			    ALUA_DH_NAME, h->group_id, h->rel_port);
+		return SCSI_DH_DEV_UNSUPP;
 	}
+	sdev_printk(KERN_INFO, sdev,
+		    "%s: port group %02x rel port %02x\n",
+		    ALUA_DH_NAME, h->group_id, h->rel_port);
 
-	return err;
+	return 0;
 }
 
 static char print_alua_state(int state)
@@ -697,7 +646,7 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 	if (err != SCSI_DH_OK)
 		goto out;
 
-	err = alua_vpd_inquiry(sdev, h);
+	err = alua_check_vpd(sdev, h);
 	if (err != SCSI_DH_OK)
 		goto out;
 
-- 
1.8.5.2

Re: [PATCH 03/17] scsi_dh_alua: Use vpd_pg83 information

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> -/*
>    * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
>    * @sdev: sdev the command should be sent to
>    */
> @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
>   		sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n",
>   			    ALUA_DH_NAME);
>   		break;
> -	default:
> -		h->tpgs = TPGS_MODE_NONE;
> +	case TPGS_MODE_NONE:
>   		sdev_printk(KERN_INFO, sdev, "%s: not supported\n",
>   			    ALUA_DH_NAME);
>   		err = SCSI_DH_DEV_UNSUPP;
>   		break;
> +	default:
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: unsupported TPGS setting %d\n",
> +			    ALUA_DH_NAME, h->tpgs);
> +		h->tpgs = TPGS_MODE_NONE;
> +		err = SCSI_DH_DEV_UNSUPP;
> +		break;
>   	}
>
>   	return err;
>   }

The function scsi_device_tpgs() returns a value between 0 and 3. So why 
to add a fifth case in this switch statement ?

Bart.

Re: [PATCH 03/17] scsi_dh_alua: Use vpd_pg83 information

[Hannes Reinecke]

On 05/07/2015 01:41 PM, Bart Van Assche wrote:
> On 05/04/15 14:42, Hannes Reinecke wrote:
>> -/*
>>    * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
>>    * @sdev: sdev the command should be sent to
>>    */
>> @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct
>> scsi_device *sdev, struct alua_dh_data *h)
>>           sdev_printk(KERN_INFO, sdev, "%s: supports implicit
>> TPGS\n",
>>                   ALUA_DH_NAME);
>>           break;
>> -    default:
>> -        h->tpgs = TPGS_MODE_NONE;
>> +    case TPGS_MODE_NONE:
>>           sdev_printk(KERN_INFO, sdev, "%s: not supported\n",
>>                   ALUA_DH_NAME);
>>           err = SCSI_DH_DEV_UNSUPP;
>>           break;
>> +    default:
>> +        sdev_printk(KERN_INFO, sdev,
>> +                "%s: unsupported TPGS setting %d\n",
>> +                ALUA_DH_NAME, h->tpgs);
>> +        h->tpgs = TPGS_MODE_NONE;
>> +        err = SCSI_DH_DEV_UNSUPP;
>> +        break;
>>       }
>>
>>       return err;
>>   }
> 
> The function scsi_device_tpgs() returns a value between 0 and 3. So
> why to add a fifth case in this switch statement ?
> 
Because I'm paranoid?

'h->tpgs' is an integer, so _in principle_ it could take any value.
We can only safely restrict this by turning 'h->tpgs' into
an enum.
_And_ 'h->tpgs' is being set to '-1' initially, so this is to catch
any logic / initialisation issues.

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

Re: [PATCH 03/17] scsi_dh_alua: Use vpd_pg83 information

[Christoph Hellwig]

> @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
>  		sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n",
>  			    ALUA_DH_NAME);
>  		break;
> -	default:
> -		h->tpgs = TPGS_MODE_NONE;
> +	case TPGS_MODE_NONE:
>  		sdev_printk(KERN_INFO, sdev, "%s: not supported\n",
>  			    ALUA_DH_NAME);
>  		err = SCSI_DH_DEV_UNSUPP;
>  		break;
> +	default:
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: unsupported TPGS setting %d\n",
> +			    ALUA_DH_NAME, h->tpgs);
> +		h->tpgs = TPGS_MODE_NONE;
> +		err = SCSI_DH_DEV_UNSUPP;
> +		break;

Can you split this into a separate patch, please?

Otherwise looks fine, so:

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

for both resulting patches.

Re: [PATCH 03/17] scsi_dh_alua: Use vpd_pg83 information

[Hannes Reinecke]

On 05/11/2015 08:48 AM, Christoph Hellwig wrote:
>> @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
>>  		sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n",
>>  			    ALUA_DH_NAME);
>>  		break;
>> -	default:
>> -		h->tpgs = TPGS_MODE_NONE;
>> +	case TPGS_MODE_NONE:
>>  		sdev_printk(KERN_INFO, sdev, "%s: not supported\n",
>>  			    ALUA_DH_NAME);
>>  		err = SCSI_DH_DEV_UNSUPP;
>>  		break;
>> +	default:
>> +		sdev_printk(KERN_INFO, sdev,
>> +			    "%s: unsupported TPGS setting %d\n",
>> +			    ALUA_DH_NAME, h->tpgs);
>> +		h->tpgs = TPGS_MODE_NONE;
>> +		err = SCSI_DH_DEV_UNSUPP;
>> +		break;
> 
> Can you split this into a separate patch, please?
> 
> Otherwise looks fine, so:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> for both resulting patches.
> 
Okay, will do.

Cheers,

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

[PATCH 04/17] scsi_dh_alua: Improve error handling

[Hannes Reinecke]

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

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index b5903eb..f5d5859 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <scsi/scsi.h>
+#include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_dh.h>
 
@@ -138,11 +139,13 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 			    bool rtpg_ext_hdr_req)
 {
 	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err;
 
 	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
+	if (!rq) {
+		err = DRIVER_BUSY << 24;
 		goto done;
+	}
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
@@ -161,12 +164,12 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 	rq->sense_len = h->senselen = 0;
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err == -EIO) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg failed with %x\n",
-			    ALUA_DH_NAME, rq->errors);
+	if (err < 0) {
+		if (!rq->errors)
+			err = DID_ERROR << 16;
+		else
+			err = rq->errors;
 		h->senselen = rq->sense_len;
-		err = SCSI_DH_IO;
 	}
 	blk_put_request(rq);
 done:
@@ -174,13 +177,11 @@ done:
 }
 
 /*
- * alua_stpg - Evaluate SET TARGET GROUP STATES
+ * stpg_endio - Evaluate SET TARGET GROUP STATES
  * @sdev: the device to be evaluated
  * @state: the new target group state
  *
- * Send a SET TARGET GROUP STATES command to the device.
- * We only have to test here if we should resubmit the command;
- * any other error is assumed as a failure.
+ * Evaluate a SET TARGET GROUP STATES command response.
  */
 static void stpg_endio(struct request *req, int error)
 {
@@ -195,9 +196,8 @@ static void stpg_endio(struct request *req, int error)
 	}
 
 	if (req->sense_len > 0) {
-		err = scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					   &sense_hdr);
-		if (!err) {
+		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
 			err = SCSI_DH_IO;
 			goto done;
 		}
@@ -206,10 +206,9 @@ static void stpg_endio(struct request *req, int error)
 			err = SCSI_DH_RETRY;
 			goto done;
 		}
-		sdev_printk(KERN_INFO, h->sdev,
-			    "%s: stpg sense code: %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\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;
@@ -499,7 +498,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
-	unsigned err;
+	unsigned err, retval;
 	bool rtpg_ext_hdr_req = 1;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
@@ -511,13 +510,21 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	err = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
-
-	if (err == SCSI_DH_IO && h->senselen > 0) {
-		err = scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					   &sense_hdr);
-		if (!err)
-			return SCSI_DH_IO;
+	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+
+	if (retval) {
+		if (h->senselen == 0 ||
+		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: rtpg failed, result %d\n",
+				    ALUA_DH_NAME, retval);
+			if (driver_byte(retval) == DRIVER_BUSY)
+				err = SCSI_DH_DEV_TEMP_BUSY;
+			else
+				err = SCSI_DH_IO;
+			return err;
+		}
 
 		/*
 		 * submit_rtpg() has failed on existing arrays
@@ -535,16 +542,17 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		}
 
 		err = alua_check_sense(sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
+		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
+			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
+				    ALUA_DH_NAME);
+			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 			goto retry;
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: rtpg sense code %02x/%02x/%02x\n",
-			    ALUA_DH_NAME, sense_hdr.sense_key,
-			    sense_hdr.asc, sense_hdr.ascq);
-		err = SCSI_DH_IO;
+		}
+		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
+			    ALUA_DH_NAME);
+		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		return SCSI_DH_IO;
 	}
-	if (err != SCSI_DH_OK)
-		return err;
 
 	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
 		(h->buff[2] << 8) + h->buff[3] + 4;
-- 
1.8.5.2

Re: [PATCH 04/17] scsi_dh_alua: Improve error handling

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> @@ -161,12 +164,12 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
>   	rq->sense_len = h->senselen = 0;
>
>   	err = blk_execute_rq(rq->q, NULL, rq, 1);
> -	if (err == -EIO) {
> -		sdev_printk(KERN_INFO, sdev,
> -			    "%s: rtpg failed with %x\n",
> -			    ALUA_DH_NAME, rq->errors);
> +	if (err < 0) {
> +		if (!rq->errors)
> +			err = DID_ERROR << 16;
> +		else
> +			err = rq->errors;
>   		h->senselen = rq->sense_len;
> -		err = SCSI_DH_IO;
>   	}
>   	blk_put_request(rq);
>   done:

Running the grep query "->errors = " over the Linux kernel source tree 
shows that sometimes a SCSI error code is written into that field and 
sometimes a negative error code. Does this mean that the test 
!rq->errors should be modified into !rq->errors && 
!IS_ERR_VALUE(rq->errors) ?

Bart.

Re: [PATCH 04/17] scsi_dh_alua: Improve error handling

[Hannes Reinecke]

On 05/07/2015 01:48 PM, Bart Van Assche wrote:
> On 05/04/15 14:42, Hannes Reinecke wrote:
>> @@ -161,12 +164,12 @@ static unsigned submit_rtpg(struct
>> scsi_device *sdev, struct alua_dh_data *h,
>>       rq->sense_len = h->senselen = 0;
>>
>>       err = blk_execute_rq(rq->q, NULL, rq, 1);
>> -    if (err == -EIO) {
>> -        sdev_printk(KERN_INFO, sdev,
>> -                "%s: rtpg failed with %x\n",
>> -                ALUA_DH_NAME, rq->errors);
>> +    if (err < 0) {
>> +        if (!rq->errors)
>> +            err = DID_ERROR << 16;
>> +        else
>> +            err = rq->errors;
>>           h->senselen = rq->sense_len;
>> -        err = SCSI_DH_IO;
>>       }
>>       blk_put_request(rq);
>>   done:
> 
> Running the grep query "->errors = " over the Linux kernel source
> tree shows that sometimes a SCSI error code is written into that
> field and sometimes a negative error code. Does this mean that the
> test !rq->errors should be modified into !rq->errors &&
> !IS_ERR_VALUE(rq->errors) ?
> 
Hmm. I'm going to review this. 'rq->errors' should be used consistently.

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

Re: [PATCH 04/17] scsi_dh_alua: Improve error handling

[Hannes Reinecke]

On 05/07/2015 01:52 PM, Hannes Reinecke wrote:
> On 05/07/2015 01:48 PM, Bart Van Assche wrote:
>> On 05/04/15 14:42, Hannes Reinecke wrote:
>>> @@ -161,12 +164,12 @@ static unsigned submit_rtpg(struct
>>> scsi_device *sdev, struct alua_dh_data *h,
>>>       rq->sense_len = h->senselen = 0;
>>>
>>>       err = blk_execute_rq(rq->q, NULL, rq, 1);
>>> -    if (err == -EIO) {
>>> -        sdev_printk(KERN_INFO, sdev,
>>> -                "%s: rtpg failed with %x\n",
>>> -                ALUA_DH_NAME, rq->errors);
>>> +    if (err < 0) {
>>> +        if (!rq->errors)
>>> +            err = DID_ERROR << 16;
>>> +        else
>>> +            err = rq->errors;
>>>           h->senselen = rq->sense_len;
>>> -        err = SCSI_DH_IO;
>>>       }
>>>       blk_put_request(rq);
>>>   done:
>>
>> Running the grep query "->errors = " over the Linux kernel source
>> tree shows that sometimes a SCSI error code is written into that
>> field and sometimes a negative error code. Does this mean that the
>> test !rq->errors should be modified into !rq->errors &&
>> !IS_ERR_VALUE(rq->errors) ?
>>
> Hmm. I'm going to review this. 'rq->errors' should be used consistently.
> 
drivers/scsi/scsi_lib.c:scsi_execute() has this comment in the
header:

 * returns the req->errors value which is the scsi_cmnd result
 * field.

And later on 'req->errors' is used verbatim as the return value:

	if (resid)
		*resid = req->resid_len;
	ret = req->errors;
 out:
	blk_put_request(req);

	return ret;
}

As the above patch is modeled according to this, any issue here
would affect scsi_execute, too.

Cheers,

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

[PATCH 05/17] scsi: remove scsi_show_sense_hdr()

[Hannes Reinecke]

Last caller is gone, so remove it.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 include/scsi/scsi_dbg.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h
index f8170e9..56710e0 100644
--- a/include/scsi/scsi_dbg.h
+++ b/include/scsi/scsi_dbg.h
@@ -12,8 +12,6 @@ extern size_t __scsi_format_command(char *, size_t,
 				   const unsigned char *, size_t);
 extern void scsi_show_extd_sense(const struct scsi_device *, const char *,
 				 unsigned char, unsigned char);
-extern void scsi_show_sense_hdr(const struct scsi_device *, const char *,
-				const struct scsi_sense_hdr *);
 extern void scsi_print_sense_hdr(const struct scsi_device *, const char *,
 				 const struct scsi_sense_hdr *);
 extern void scsi_print_sense(const struct scsi_cmnd *);
-- 
1.8.5.2

Re: [PATCH 05/17] scsi: remove scsi_show_sense_hdr()

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> Last caller is gone, so remove it.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   include/scsi/scsi_dbg.h | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/include/scsi/scsi_dbg.h b/include/scsi/scsi_dbg.h
> index f8170e9..56710e0 100644
> --- a/include/scsi/scsi_dbg.h
> +++ b/include/scsi/scsi_dbg.h
> @@ -12,8 +12,6 @@ extern size_t __scsi_format_command(char *, size_t,
>   				   const unsigned char *, size_t);
>   extern void scsi_show_extd_sense(const struct scsi_device *, const char *,
>   				 unsigned char, unsigned char);
> -extern void scsi_show_sense_hdr(const struct scsi_device *, const char *,
> -				const struct scsi_sense_hdr *);
>   extern void scsi_print_sense_hdr(const struct scsi_device *, const char *,
>   				 const struct scsi_sense_hdr *);
>   extern void scsi_print_sense(const struct scsi_cmnd *);

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Re: [PATCH 05/17] scsi: remove scsi_show_sense_hdr()

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:11PM +0200, Hannes Reinecke wrote:
> Last caller is gone, so remove it.

And the implementation as well is gone as well..  Looks fine,

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

[PATCH 06/17] scsi_dh_alua: use flag for RTPG extended header

[Hannes Reinecke]

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

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index f5d5859..69cad2a 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -61,6 +61,7 @@
 
 /* flags passed from user level */
 #define ALUA_OPTIMIZE_STPG		1
+#define ALUA_RTPG_EXT_HDR_UNSUPP	2
 
 struct alua_dh_data {
 	int			group_id;
@@ -135,8 +136,7 @@ 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,
-			    bool rtpg_ext_hdr_req)
+static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	struct request *rq;
 	int err;
@@ -149,7 +149,7 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
-	if (rtpg_ext_hdr_req)
+	if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP))
 		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
 		rq->cmd[1] = MI_REPORT_TARGET_PGS;
@@ -499,7 +499,6 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
 	unsigned err, retval;
-	bool rtpg_ext_hdr_req = 1;
 	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
@@ -510,7 +509,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h, rtpg_ext_hdr_req);
+	retval = submit_rtpg(sdev, h);
 
 	if (retval) {
 		if (h->senselen == 0 ||
@@ -534,10 +533,10 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		 * The retry without rtpg_ext_hdr_req set
 		 * handles this.
 		 */
-		if (rtpg_ext_hdr_req == 1 &&
+		if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP) &&
 		    sense_hdr.sense_key == ILLEGAL_REQUEST &&
 		    sense_hdr.asc == 0x24 && sense_hdr.ascq == 0) {
-			rtpg_ext_hdr_req = 0;
+			h->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
 			goto retry;
 		}
 
-- 
1.8.5.2

Re: [PATCH 06/17] scsi_dh_alua: use flag for RTPG extended header

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> We should be using a flag when RTPG extended header is not
> supported, that saves us sending RTPG twice for older arrays.

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Re: [PATCH 06/17] scsi_dh_alua: use flag for RTPG extended header

[Christoph Hellwig]

>  /* flags passed from user level */
>  #define ALUA_OPTIMIZE_STPG		1
> +#define ALUA_RTPG_EXT_HDR_UNSUPP	2

The comment is now incorrect.

Otherwise looks good,

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

[PATCH 07/17] scsi_dh_alua: Pass buffer as function argument

[Hannes Reinecke]

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

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 69cad2a..1bfc7a4 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -136,12 +136,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;
 
-	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;
@@ -149,19 +150,19 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_IN;
-	if (!(h->flags & ALUA_RTPG_EXT_HDR_UNSUPP))
+	if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP))
 		rq->cmd[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT;
 	else
 		rq->cmd[1] = MI_REPORT_TARGET_PGS;
-	rq->cmd[6] = (h->bufflen >> 24) & 0xff;
-	rq->cmd[7] = (h->bufflen >> 16) & 0xff;
-	rq->cmd[8] = (h->bufflen >>  8) & 0xff;
-	rq->cmd[9] = h->bufflen & 0xff;
+	rq->cmd[6] = (bufflen >> 24) & 0xff;
+	rq->cmd[7] = (bufflen >> 16) & 0xff;
+	rq->cmd[8] = (bufflen >>  8) & 0xff;
+	rq->cmd[9] = bufflen & 0xff;
 	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
 
-	rq->sense = h->sense;
+	rq->sense = sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = h->senselen = 0;
+	rq->sense_len = 0;
 
 	err = blk_execute_rq(rq->q, NULL, rq, 1);
 	if (err < 0) {
@@ -169,7 +170,8 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 			err = DID_ERROR << 16;
 		else
 			err = rq->errors;
-		h->senselen = rq->sense_len;
+		if (rq->sense_len)
+			err |= (DRIVER_SENSE << 24);
 	}
 	blk_put_request(rq);
 done:
@@ -509,10 +511,10 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		expiry = round_jiffies_up(jiffies + h->transition_tmo * HZ);
 
  retry:
-	retval = submit_rtpg(sdev, h);
+	retval = submit_rtpg(sdev, h->buff, h->bufflen, h->sense, h->flags);
 
 	if (retval) {
-		if (h->senselen == 0 ||
+	    if (!(driver_byte(retval) & DRIVER_SENSE) ||
 		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
-- 
1.8.5.2

Re: [PATCH 07/17] scsi_dh_alua: Pass buffer as function argument

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> -	rq->cmd[6] = (h->bufflen >> 24) & 0xff;
> -	rq->cmd[7] = (h->bufflen >> 16) & 0xff;
> -	rq->cmd[8] = (h->bufflen >>  8) & 0xff;
> -	rq->cmd[9] = h->bufflen & 0xff;
> +	rq->cmd[6] = (bufflen >> 24) & 0xff;
> +	rq->cmd[7] = (bufflen >> 16) & 0xff;
> +	rq->cmd[8] = (bufflen >>  8) & 0xff;
> +	rq->cmd[9] = bufflen & 0xff;

This would have been a good opportunity to introduce 
get_unaligned_be32(). But even without that change:

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>

Re: [PATCH 07/17] scsi_dh_alua: Pass buffer as function argument

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:13PM +0200, Hannes Reinecke wrote:
> Pass in the buffer as a function argument for submit_vpd() and
> submit_rtpg().

Looks good,

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

[PATCH 08/17] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

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

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 1bfc7a4..0b8e111 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <asm/unaligned.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_dbg.h>
 #include <scsi/scsi_eh.h>
@@ -179,98 +180,51 @@ done:
 }
 
 /*
- * stpg_endio - Evaluate SET TARGET GROUP STATES
- * @sdev: the device to be evaluated
- * @state: the new target group state
- *
- * Evaluate a SET TARGET GROUP STATES command response.
- */
-static void stpg_endio(struct request *req, int error)
-{
-	struct alua_dh_data *h = req->end_io_data;
-	struct scsi_sense_hdr sense_hdr;
-	unsigned err = SCSI_DH_OK;
-
-	if (host_byte(req->errors) != DID_OK ||
-	    msg_byte(req->errors) != COMMAND_COMPLETE) {
-		err = SCSI_DH_IO;
-		goto done;
-	}
-
-	if (req->sense_len > 0) {
-		if (!scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
-					  &sense_hdr)) {
-			err = SCSI_DH_IO;
-			goto done;
-		}
-		err = alua_check_sense(h->sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE) {
-			err = SCSI_DH_RETRY;
-			goto done;
-		}
-		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\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;
 
 	/* Prepare the data buffer */
-	memset(h->buff, 0, stpg_len);
-	h->buff[4] = TPGS_STATE_OPTIMIZED & 0x0f;
-	h->buff[6] = (h->group_id >> 8) & 0xff;
-	h->buff[7] = h->group_id & 0xff;
+	memset(stpg_data, 0, stpg_len);
+	stpg_data[4] = TPGS_STATE_OPTIMIZED & 0x0f;
+	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;
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_OUT;
 	rq->cmd[1] = MO_SET_TARGET_PGS;
-	rq->cmd[6] = (stpg_len >> 24) & 0xff;
-	rq->cmd[7] = (stpg_len >> 16) & 0xff;
-	rq->cmd[8] = (stpg_len >>  8) & 0xff;
-	rq->cmd[9] = stpg_len & 0xff;
+	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 = h->senselen = 0;
-	rq->end_io_data = h;
+	rq->sense_len = 0;
+
+	err = blk_execute_rq(rq->q, NULL, rq, 1);
+	if (err < 0) {
+		if (!rq->errors)
+			err = DID_ERROR << 16;
+		else
+			err = rq->errors;
+		if (rq->sense_len)
+			err |= (DRIVER_SENSE << 24);
+	}
+	blk_put_request(rq);
 
-	blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio);
-	return SCSI_DH_OK;
+	return err;
 }
 
 /*
@@ -641,6 +595,70 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 }
 
 /*
+ * alua_stpg - Issue a SET TARGET GROUP STATES command
+ *
+ * Issue a SET TARGET GROUP STATES command and evaluate the
+ * response. Returns SCSI_DH_RETRY per default to trigger
+ * a re-evaluation of the target group state.
+ */
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
+{
+	int retval, err = SCSI_DH_RETRY;
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	struct scsi_sense_hdr sense_hdr;
+
+	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
+		/* Only implicit ALUA supported, retry */
+		return SCSI_DH_RETRY;
+	}
+	switch (h->state) {
+	case TPGS_STATE_OPTIMIZED:
+		return SCSI_DH_OK;
+	case TPGS_STATE_NONOPTIMIZED:
+		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
+		    (!h->pref) &&
+		    (h->tpgs & TPGS_MODE_IMPLICIT))
+			return SCSI_DH_OK;
+		break;
+	case TPGS_STATE_STANDBY:
+	case TPGS_STATE_UNAVAILABLE:
+		break;
+	case TPGS_STATE_OFFLINE:
+		return SCSI_DH_IO;
+		break;
+	case TPGS_STATE_TRANSITIONING:
+		break;
+	default:
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: stpg failed, unhandled TPGS state %d",
+			    ALUA_DH_NAME, pg->state);
+		return SCSI_DH_NOSYS;
+		break;
+	}
+	/* Set state to transitioning */
+	h->state = TPGS_STATE_TRANSITIONING;
+	retval = submit_stpg(sdev, h->group_id, sense);
+
+	if (retval) {
+		if (!(driver_byte(retval) & DRIVER_SENSE) ||
+		    !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
+					  &sense_hdr)) {
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: stpg failed, result %d",
+				    ALUA_DH_NAME, retval);
+			/* Retry RTPG */
+			return err;
+		}
+		err = alua_check_sense(h->sdev, &sense_hdr);
+		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+			    ALUA_DH_NAME);
+		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		err = SCSI_DH_RETRY;
+	}
+	return err;
+}
+
+/*
  * alua_initialize - Initialize ALUA state
  * @sdev: the device to be initialized
  *
@@ -717,7 +735,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)
@@ -726,39 +743,9 @@ static int alua_activate(struct scsi_device *sdev,
 	if (optimize_stpg)
 		h->flags |= ALUA_OPTIMIZE_STPG;
 
-	if (h->tpgs & TPGS_MODE_EXPLICIT) {
-		switch (h->state) {
-		case TPGS_STATE_NONOPTIMIZED:
-			stpg = 1;
-			if ((h->flags & ALUA_OPTIMIZE_STPG) &&
-			    (!h->pref) &&
-			    (h->tpgs & TPGS_MODE_IMPLICIT))
-				stpg = 0;
-			break;
-		case TPGS_STATE_STANDBY:
-		case TPGS_STATE_UNAVAILABLE:
-			stpg = 1;
-			break;
-		case TPGS_STATE_OFFLINE:
-			err = SCSI_DH_IO;
-			break;
-		case TPGS_STATE_TRANSITIONING:
-			err = SCSI_DH_RETRY;
-			break;
-		default:
-			break;
-		}
-	}
-
-	if (stpg) {
-		h->callback_fn = fn;
-		h->callback_data = data;
-		err = submit_stpg(h);
-		if (err == SCSI_DH_OK)
-			return 0;
-		h->callback_fn = h->callback_data = NULL;
-	}
-
+	err = alua_stpg(sdev, h);
+	if (err == SCSI_DH_RETRY)
+		err = alua_rtpg(sdev, h, 1);
 out:
 	if (fn)
 		fn(data, err);
-- 
1.8.5.2

Re: [PATCH 08/17] scsi_dh_alua: Make stpg synchronous

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> +static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
> +{
> +	int retval, err = SCSI_DH_RETRY;
> +	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> +	struct scsi_sense_hdr sense_hdr;
> +
> +	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
> +		/* Only implicit ALUA supported, retry */
> +		return SCSI_DH_RETRY;
> +	}
> +	switch (h->state) {
> +	case TPGS_STATE_OPTIMIZED:
> +		return SCSI_DH_OK;
> +	case TPGS_STATE_NONOPTIMIZED:
> +		if ((h->flags & ALUA_OPTIMIZE_STPG) &&
> +		    (!h->pref) &&
> +		    (h->tpgs & TPGS_MODE_IMPLICIT))
> +			return SCSI_DH_OK;
> +		break;
> +	case TPGS_STATE_STANDBY:
> +	case TPGS_STATE_UNAVAILABLE:
> +		break;
> +	case TPGS_STATE_OFFLINE:
> +		return SCSI_DH_IO;
> +		break;
> +	case TPGS_STATE_TRANSITIONING:
> +		break;
> +	default:
> +		sdev_printk(KERN_INFO, sdev,
> +			    "%s: stpg failed, unhandled TPGS state %d",
> +			    ALUA_DH_NAME, pg->state);
> +		return SCSI_DH_NOSYS;
> +		break;
> +	}
> +	/* Set state to transitioning */
> +	h->state = TPGS_STATE_TRANSITIONING;
> +	retval = submit_stpg(sdev, h->group_id, sense);
> +
> +	if (retval) {
> +		if (!(driver_byte(retval) & DRIVER_SENSE) ||
> +		    !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
> +					  &sense_hdr)) {
> +			sdev_printk(KERN_INFO, sdev,
> +				    "%s: stpg failed, result %d",
> +				    ALUA_DH_NAME, retval);
> +			/* Retry RTPG */
> +			return err;
> +		}
> +		err = alua_check_sense(h->sdev, &sense_hdr);
> +		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
> +			    ALUA_DH_NAME);
> +		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
> +		err = SCSI_DH_RETRY;
> +	}
> +	return err;
> +}

The value returned by alua_check_sense() is assigned to the variable 
'err' but that value is not used. Otherwise in this function the 
variable 'err' always has the value SCSI_DH_RETRY. Does that mean that 
that variable can be removed ? Additionally, why is 'retval' only 
printed if normalizing the sense data succeeds and not if normalizing 
the sense data fails ? Has it been considered to print the ASC and ASCQ 
values upon STPG failure ? Having these values available can be a big 
help during debugging.

Thanks,

Bart.

Re: [PATCH 08/17] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

On 05/07/2015 02:18 PM, Bart Van Assche wrote:
> On 05/04/15 14:42, Hannes Reinecke wrote:
>> +static unsigned alua_stpg(struct scsi_device *sdev, struct
>> alua_dh_data *h)
>> +{
>> +    int retval, err = SCSI_DH_RETRY;
>> +    unsigned char sense[SCSI_SENSE_BUFFERSIZE];
>> +    struct scsi_sense_hdr sense_hdr;
>> +
>> +    if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
>> +        /* Only implicit ALUA supported, retry */
>> +        return SCSI_DH_RETRY;
>> +    }
>> +    switch (h->state) {
>> +    case TPGS_STATE_OPTIMIZED:
>> +        return SCSI_DH_OK;
>> +    case TPGS_STATE_NONOPTIMIZED:
>> +        if ((h->flags & ALUA_OPTIMIZE_STPG) &&
>> +            (!h->pref) &&
>> +            (h->tpgs & TPGS_MODE_IMPLICIT))
>> +            return SCSI_DH_OK;
>> +        break;
>> +    case TPGS_STATE_STANDBY:
>> +    case TPGS_STATE_UNAVAILABLE:
>> +        break;
>> +    case TPGS_STATE_OFFLINE:
>> +        return SCSI_DH_IO;
>> +        break;
>> +    case TPGS_STATE_TRANSITIONING:
>> +        break;
>> +    default:
>> +        sdev_printk(KERN_INFO, sdev,
>> +                "%s: stpg failed, unhandled TPGS state %d",
>> +                ALUA_DH_NAME, pg->state);
>> +        return SCSI_DH_NOSYS;
>> +        break;
>> +    }
>> +    /* Set state to transitioning */
>> +    h->state = TPGS_STATE_TRANSITIONING;
>> +    retval = submit_stpg(sdev, h->group_id, sense);
>> +
>> +    if (retval) {
>> +        if (!(driver_byte(retval) & DRIVER_SENSE) ||
>> +            !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
>> +                      &sense_hdr)) {
>> +            sdev_printk(KERN_INFO, sdev,
>> +                    "%s: stpg failed, result %d",
>> +                    ALUA_DH_NAME, retval);
>> +            /* Retry RTPG */
>> +            return err;
>> +        }
>> +        err = alua_check_sense(h->sdev, &sense_hdr);
>> +        sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
>> +                ALUA_DH_NAME);
>> +        scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
>> +        err = SCSI_DH_RETRY;
>> +    }
>> +    return err;
>> +}
> 
> The value returned by alua_check_sense() is assigned to the variable
> 'err' but that value is not used. Otherwise in this function the
> variable 'err' always has the value SCSI_DH_RETRY. Does that mean
> that that variable can be removed ? 
Hmm. Yes, we could.

> Additionally, why is 'retval'
> only printed if normalizing the sense data succeeds and not if
> normalizing the sense data fails ? Has it been considered to print
> the ASC and ASCQ values upon STPG failure ? Having these values
> available can be a big help during debugging.
> 
We do print the ASC / ASCQ values; that's what scsi_print_sense_hdr
does. And the above line works on the assumption that iff
DRIVER_SENSE is set in the retval we'll have a sense code, and
this sense code will give more information than any other
(internally generated) error status. So 'retval' isn't printed
in that case, only if no sense code is provided or if we fail
to decode it.

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

Re: [PATCH 08/17] scsi_dh_alua: Make stpg synchronous

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:14PM +0200, Hannes Reinecke wrote:
> We should be issuing STPG synchronously as we need to
> evaluate the return code on failure.

How does this fit into the calling convention?  It seems like generally
dm-mpath expects async execution of ->activate.  If we don't actually need
asynchronous activation we should remove it from all the drivers to have
a consistent calling convention.

Re: [PATCH 08/17] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

On 05/11/2015 08:55 AM, Christoph Hellwig wrote:
> On Mon, May 04, 2015 at 02:42:14PM +0200, Hannes Reinecke wrote:
>> We should be issuing STPG synchronously as we need to
>> evaluate the return code on failure.
> 
> How does this fit into the calling convention?  It seems like generally
> dm-mpath expects async execution of ->activate.  If we don't actually need
> asynchronous activation we should remove it from all the drivers to have
> a consistent calling convention.
> 
This is just an intermediate step, before the entire thing moves off
to a workqueue and becomes asynchronous again.
It's not meant to be as a standalone patch; I've just split it off
for easier review.

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

Re: [PATCH 08/17] scsi_dh_alua: Make stpg synchronous

[Christoph Hellwig]

On Mon, May 11, 2015 at 11:59:16AM +0200, Hannes Reinecke wrote:
> This is just an intermediate step, before the entire thing moves off
> to a workqueue and becomes asynchronous again.
> It's not meant to be as a standalone patch; I've just split it off
> for easier review.

It actually made the review a bit harder.  Also I don't really undertand
what the benfit of running a synchronous STPG in a workqueue is over
running it asynchronously.

Re: [PATCH 08/17] scsi_dh_alua: Make stpg synchronous

[Hannes Reinecke]

On 05/11/2015 03:50 PM, Christoph Hellwig wrote:
> On Mon, May 11, 2015 at 11:59:16AM +0200, Hannes Reinecke wrote:
>> This is just an intermediate step, before the entire thing moves off
>> to a workqueue and becomes asynchronous again.
>> It's not meant to be as a standalone patch; I've just split it off
>> for easier review.
> 
> It actually made the review a bit harder.  Also I don't really undertand
> what the benfit of running a synchronous STPG in a workqueue is over
> running it asynchronously.
> 
Because STPG might fail, at which point we need to re-run RTPG to
figure out the current status.
But that might clash with a scheduled RTPG from another task.
_And_ issuing RTPG while STPG is active is pointless, too, as
the information is pretty much guaranteed to be incorrect.

Hence the use of a workqueue to serialize RTPG and STPG against each
other.

Cheers,

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

[PATCH 09/17] scsi_dh_alua: switch to scsi_execute()

[Hannes Reinecke]

All commands are issued synchronously, so no need to open-code
scsi_execute anymore.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 0b8e111..0cc18e3 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -103,80 +103,29 @@ 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,
+static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff,
 			    int bufflen, unsigned char *sense, int flags)
 {
-	struct request *rq;
-	int err;
-
-	rq = get_alua_req(sdev, buff, bufflen, READ);
-	if (!rq) {
-		err = DRIVER_BUSY << 24;
-		goto done;
-	}
+	u8 cdb[16];
+	int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
+		REQ_FAILFAST_DRIVER;
 
 	/* Prepare the command. */
-	rq->cmd[0] = MAINTENANCE_IN;
+	memset(cdb, 0x0, 16);
+	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;
-	rq->cmd[6] = (bufflen >> 24) & 0xff;
-	rq->cmd[7] = (bufflen >> 16) & 0xff;
-	rq->cmd[8] = (bufflen >>  8) & 0xff;
-	rq->cmd[9] = bufflen & 0xff;
-	rq->cmd_len = COMMAND_SIZE(MAINTENANCE_IN);
-
-	rq->sense = sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = 0;
-
-	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err < 0) {
-		if (!rq->errors)
-			err = DID_ERROR << 16;
-		else
-			err = rq->errors;
-		if (rq->sense_len)
-			err |= (DRIVER_SENSE << 24);
-	}
-	blk_put_request(rq);
-done:
-	return err;
+		cdb[1] = MI_REPORT_TARGET_PGS;
+	put_unaligned_be32(bufflen, &cdb[6]);
+
+	return scsi_execute(sdev, cdb, DMA_FROM_DEVICE, buff, bufflen,
+			    sense, ALUA_FAILOVER_TIMEOUT * HZ,
+			    ALUA_FAILOVER_RETRIES, req_flags, NULL);
 }
 
 /*
@@ -186,45 +135,29 @@ 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,
+		       unsigned char *sense)
 {
-	struct request *rq;
+	u8 cdb[COMMAND_SIZE(MAINTENANCE_OUT)];
 	unsigned char stpg_data[8];
 	int stpg_len = 8;
-	int err;
+	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 SCSI_DH_RES_TEMP_UNAVAIL;
-
 	/* 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;
-
-	err = blk_execute_rq(rq->q, NULL, rq, 1);
-	if (err < 0) {
-		if (!rq->errors)
-			err = DID_ERROR << 16;
-		else
-			err = rq->errors;
-		if (rq->sense_len)
-			err |= (DRIVER_SENSE << 24);
-	}
-	blk_put_request(rq);
-
-	return err;
+	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(sdev, cdb, DMA_TO_DEVICE, stpg_data, stpg_len,
+			    sense, ALUA_FAILOVER_TIMEOUT * HZ,
+			    ALUA_FAILOVER_RETRIES, req_flags, NULL);
 }
 
 /*
-- 
1.8.5.2

Re: [PATCH 09/17] scsi_dh_alua: switch to scsi_execute()

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:15PM +0200, Hannes Reinecke wrote:
> All commands are issued synchronously, so no need to open-code
> scsi_execute anymore.

Currently the alua codes doesn't set REQ_PREEMPT, and uses a GFP_NOIO
allocation.  Please document why changing these is safe/and or desirable.

Also it would be good to a change like this to the other device handlers,
as the code is copy and pasted everywhere, and only hp_sw has an asynchronous
case that can be handled separately.

Re: [PATCH 09/17] scsi_dh_alua: switch to scsi_execute()

[Hannes Reinecke]

On 05/06/2015 11:26 AM, Christoph Hellwig wrote:
> On Mon, May 04, 2015 at 02:42:15PM +0200, Hannes Reinecke wrote:
>> All commands are issued synchronously, so no need to open-code
>> scsi_execute anymore.
> 
> Currently the alua codes doesn't set REQ_PREEMPT, and uses a GFP_NOIO
> allocation.  Please document why changing these is safe/and or desirable.
> 
> Also it would be good to a change like this to the other device handlers,
> as the code is copy and pasted everywhere, and only hp_sw has an asynchronous
> case that can be handled separately.
> 
Well, yes, of course.
However, for ALUA this will only work once the previous alua patches
are in. So I'd prefer to have another patchset on top of this,
converting the other device handlers.

As for the REQ_PREEMPT / GFP_NOIO I'll have a look and document
or fix is as appropriate.

Cheers,

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

[PATCH 10/17] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke]

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

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 0cc18e3..492b721 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,8 +79,12 @@ struct alua_dh_data {
 	unsigned char		*buff;
 	int			bufflen;
 	unsigned char		transition_tmo;
-	unsigned char		sense[SCSI_SENSE_BUFFERSIZE];
-	int			senselen;
+};
+
+struct alua_dh_data {
+	struct alua_port_group	*pg;
+	int			rel_port;
+	int			tpgs;
 	struct scsi_device	*sdev;
 	activate_complete	callback_fn;
 	void			*callback_data;
@@ -88,21 +96,35 @@ struct alua_dh_data {
 static char print_alua_state(int);
 static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);
 
-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);
+	printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);
+	spin_lock(&port_group_lock);
+	list_del(&pg->node);
+	spin_unlock(&port_group_lock);
+	if (pg->buff && pg->inq != pg->buff)
+		kfree(pg->buff);
+	kfree(pg);
+}
+
 /*
  * submit_rtpg - Issue a REPORT TARGET GROUP STATES command
  * @sdev: sdev the command should be sent to
@@ -230,6 +252,8 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h)
 static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	unsigned char *d;
+	int group_id = -1;
+	struct alua_port_group *pg = NULL;
 
 	if (!sdev->vpd_pg83)
 		return SCSI_DH_DEV_UNSUPP;
@@ -246,7 +270,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 			break;
 		case 0x5:
 			/* Target port group */
-			h->group_id = (d[6] << 8) + d[7];
+			group_id = (d[6] << 8) + d[7];
 			break;
 		default:
 			break;
@@ -254,7 +278,7 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 		d += d[3] + 4;
 	}
 
-	if (h->group_id == -1) {
+	if (group_id == -1) {
 		/*
 		 * Internal error; TPGS supported but required
 		 * VPD identification descriptors not present.
@@ -263,15 +287,34 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: No target port descriptors found\n",
 			    ALUA_DH_NAME);
-		h->state = TPGS_STATE_OPTIMIZED;
 		h->tpgs = TPGS_MODE_NONE;
 		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);
+		    ALUA_DH_NAME, group_id, h->rel_port);
 
-	return 0;
+	spin_lock(&port_group_lock);
+	pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
+	if (!pg) {
+		sdev_printk(KERN_WARNING, sdev,
+			    "%s: kzalloc port group failed\n",
+			    ALUA_DH_NAME);
+		/* Temporary failure, bypass */
+		spin_unlock(&port_group_lock);
+		return SCSI_DH_DEV_TEMP_BUSY;
+	}
+	pg->group_id = group_id;
+	pg->buff = pg->inq;
+	pg->bufflen = ALUA_INQUIRY_SIZE;
+	pg->tpgs = h->tpgs;
+	pg->state = TPGS_STATE_OPTIMIZED;
+	kref_init(&pg->kref);
+	list_add(&pg->node, &port_group_list);
+	h->pg = pg;
+	spin_unlock(&port_group_lock);
+
+	return SCSI_DH_OK;
 }
 
 static char print_alua_state(int state)
@@ -382,8 +425,9 @@ static int alua_check_sense(struct scsi_device *sdev,
  * Returns SCSI_DH_DEV_OFFLINED if the path is
  * found to be unusable.
  */
-static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_for_transition)
+static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int wait_for_transition)
 {
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
@@ -392,17 +436,17 @@ 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, h->sense, h->flags);
+	retval = submit_rtpg(sdev, pg->buff, pg->bufflen, sense, pg->flags);
 
 	if (retval) {
 	    if (!(driver_byte(retval) & DRIVER_SENSE) ||
-		    !scsi_normalize_sense(h->sense, SCSI_SENSE_BUFFERSIZE,
+		    !scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE,
 					  &sense_hdr)) {
 			sdev_printk(KERN_INFO, sdev,
 				    "%s: rtpg failed, result %d\n",
@@ -422,10 +466,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;
 		}
 
@@ -442,12 +486,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		return SCSI_DH_IO;
 	}
 
-	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
-		(h->buff[2] << 8) + h->buff[3] + 4;
+	len = (pg->buff[0] << 24) + (pg->buff[1] << 16) +
+		(pg->buff[2] << 8) + pg->buff[3] + 4;
 
-	if (len > h->bufflen) {
+	if (len > pg->bufflen) {
 		/* Resubmit with the correct length */
-		if (realloc_buffer(h, len)) {
+		if (realloc_buffer(pg, len)) {
 			sdev_printk(KERN_WARNING, sdev,
 				    "%s: kmalloc buffer failed\n",__func__);
 			/* Temporary failure, bypass */
@@ -456,31 +500,32 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
 		goto retry;
 	}
 
-	orig_transition_tmo = h->transition_tmo;
-	if ((h->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR && h->buff[5] != 0)
-		h->transition_tmo = h->buff[5];
+	orig_transition_tmo = pg->transition_tmo;
+	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR &&
+	    pg->buff[5] != 0)
+		pg->transition_tmo = pg->buff[5];
 	else
-		h->transition_tmo = ALUA_FAILOVER_TIMEOUT;
+		pg->transition_tmo = ALUA_FAILOVER_TIMEOUT;
 
-	if (wait_for_transition && (orig_transition_tmo != h->transition_tmo)) {
+	if (wait_for_transition && (orig_transition_tmo != pg->transition_tmo)) {
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: transition timeout set to %d seconds\n",
-			    ALUA_DH_NAME, h->transition_tmo);
-		expiry = jiffies + h->transition_tmo * HZ;
+			    ALUA_DH_NAME, pg->transition_tmo);
+		expiry = jiffies + pg->transition_tmo * HZ;
 	}
 
-	if ((h->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
+	if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
 		tpg_desc_tbl_off = 8;
 	else
 		tpg_desc_tbl_off = 4;
 
-	for (k = tpg_desc_tbl_off, ucp = h->buff + tpg_desc_tbl_off;
+	for (k = tpg_desc_tbl_off, ucp = pg->buff + tpg_desc_tbl_off;
 	     k < len;
 	     k += off, ucp += off) {
 
-		if (h->group_id == (ucp[2] << 8) + ucp[3]) {
-			h->state = ucp[0] & 0x0f;
-			h->pref = ucp[0] >> 7;
+		if (pg->group_id == (ucp[2] << 8) + ucp[3]) {
+			pg->state = ucp[0] & 0x0f;
+			pg->pref = ucp[0] >> 7;
 			valid_states = ucp[1];
 		}
 		off = 8 + (ucp[7] * 4);
@@ -488,8 +533,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',
@@ -498,7 +543,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)) {
@@ -513,7 +558,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 */
@@ -534,23 +579,23 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h, int wait_
  * response. Returns SCSI_DH_RETRY per default to trigger
  * a re-evaluation of the target group state.
  */
-static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
+static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	int retval, err = SCSI_DH_RETRY;
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
 
-	if (!(h->tpgs & TPGS_MODE_EXPLICIT)) {
+	if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
 		/* Only implicit ALUA supported, retry */
 		return SCSI_DH_RETRY;
 	}
-	switch (h->state) {
+	switch (pg->state) {
 	case TPGS_STATE_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:
@@ -569,8 +614,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		break;
 	}
 	/* Set state to transitioning */
-	h->state = TPGS_STATE_TRANSITIONING;
-	retval = submit_stpg(sdev, h->group_id, sense);
+	pg->state = TPGS_STATE_TRANSITIONING;
+	retval = submit_stpg(sdev, pg->group_id, sense);
 
 	if (retval) {
 		if (!(driver_byte(retval) & DRIVER_SENSE) ||
@@ -582,8 +627,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_dh_data *h)
 			/* Retry RTPG */
 			return err;
 		}
-		err = alua_check_sense(h->sdev, &sense_hdr);
-		sdev_printk(KERN_INFO, h->sdev, "%s: stpg failed\n",
+		err = alua_check_sense(sdev, &sense_hdr);
+		sdev_printk(KERN_INFO, sdev, "%s: stpg failed\n",
 			    ALUA_DH_NAME);
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 		err = SCSI_DH_RETRY;
@@ -607,13 +652,12 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 		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)
+	if (err != SCSI_DH_OK || !h->pg)
 		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;
 }
@@ -629,6 +673,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;
@@ -641,10 +686,18 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	if ((sscanf(p, "%u", &optimize) != 1) || (optimize > 1))
 		return -EINVAL;
 
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
+		return -ENXIO;
+	}
+	rcu_read_unlock();
+
 	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;
 }
@@ -669,16 +722,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);
@@ -694,13 +754,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;
 	}
@@ -721,11 +787,8 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	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);
@@ -747,8 +810,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;
+	}
 	kfree(h);
 }
 
-- 
1.8.5.2

Re: [PATCH 10/17] scsi_dh_alua: Use separate alua_port_group structure

[Bart Van Assche]

On 05/04/15 14:42, Hannes Reinecke wrote:
> +static void release_port_group(struct kref *kref)
> +{
> +	struct alua_port_group *pg;
> +
> +	pg = container_of(kref, struct alua_port_group, kref);
> +	printk(KERN_WARNING "alua: release port group %d\n", pg->group_id);

Does this really need to be a warning ?

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

Sorry but it's not clear to me why the kzalloc() statement happens under 
port_group_lock. Please consider to perform only the list_add() 
statement under port_group_lock, to perform all other assignments 
without holding that lock and to change GFP_ATOMIC into GFP_KERNEL.

> -	len = (h->buff[0] << 24) + (h->buff[1] << 16) +
> -		(h->buff[2] << 8) + h->buff[3] + 4;
> +	len = (pg->buff[0] << 24) + (pg->buff[1] << 16) +
> +		(pg->buff[2] << 8) + pg->buff[3] + 4;

Has it been considered to use get_unaligned_be32() instead ?

Bart.

Re: [PATCH 10/17] scsi_dh_alua: Use separate alua_port_group structure

[Bart Van Assche]

On 05/07/15 14:34, Bart Van Assche wrote:
> On 05/04/15 14:42, Hannes Reinecke wrote:
>> +    spin_lock(&port_group_lock);
>> +    pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
>> +    if (!pg) {
>> +        sdev_printk(KERN_WARNING, sdev,
>> +                "%s: kzalloc port group failed\n",
>> +                ALUA_DH_NAME);
>> +        /* Temporary failure, bypass */
>> +        spin_unlock(&port_group_lock);
>> +        return SCSI_DH_DEV_TEMP_BUSY;
>> +    }
>> +    pg->group_id = group_id;
>> +    pg->buff = pg->inq;
>> +    pg->bufflen = ALUA_INQUIRY_SIZE;
>> +    pg->tpgs = h->tpgs;
>> +    pg->state = TPGS_STATE_OPTIMIZED;
>> +    kref_init(&pg->kref);
>> +    list_add(&pg->node, &port_group_list);
>> +    h->pg = pg;
>> +    spin_unlock(&port_group_lock);
>
> Sorry but it's not clear to me why the kzalloc() statement happens under
> port_group_lock. Please consider to perform only the list_add()
> statement under port_group_lock, to perform all other assignments
> without holding that lock and to change GFP_ATOMIC into GFP_KERNEL.

(replying to my own e-mail)

Hello Hannes,

Is it correct that port_group_list is only accessed from thread context 
? If so, has it been considered to change port_group_lock into a mutex 
instead ?

Thanks,

Bart.

Re: [PATCH 10/17] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke]

On 05/07/2015 03:36 PM, Bart Van Assche wrote:
> On 05/07/15 14:34, Bart Van Assche wrote:
>> On 05/04/15 14:42, Hannes Reinecke wrote:
>>> +    spin_lock(&port_group_lock);
>>> +    pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
>>> +    if (!pg) {
>>> +        sdev_printk(KERN_WARNING, sdev,
>>> +                "%s: kzalloc port group failed\n",
>>> +                ALUA_DH_NAME);
>>> +        /* Temporary failure, bypass */
>>> +        spin_unlock(&port_group_lock);
>>> +        return SCSI_DH_DEV_TEMP_BUSY;
>>> +    }
>>> +    pg->group_id = group_id;
>>> +    pg->buff = pg->inq;
>>> +    pg->bufflen = ALUA_INQUIRY_SIZE;
>>> +    pg->tpgs = h->tpgs;
>>> +    pg->state = TPGS_STATE_OPTIMIZED;
>>> +    kref_init(&pg->kref);
>>> +    list_add(&pg->node, &port_group_list);
>>> +    h->pg = pg;
>>> +    spin_unlock(&port_group_lock);
>>
>> Sorry but it's not clear to me why the kzalloc() statement happens
>> under
>> port_group_lock. Please consider to perform only the list_add()
>> statement under port_group_lock, to perform all other assignments
>> without holding that lock and to change GFP_ATOMIC into GFP_KERNEL.
> 
> (replying to my own e-mail)
> 
> Hello Hannes,
> 
> Is it correct that port_group_list is only accessed from thread
> context ? If so, has it been considered to change port_group_lock
> into a mutex instead ?
> 
The lock is only taken for list modifications; the actual
accesses are done via RCU references.
So converting this to a mutex would give an insignificant
advantage. Plus I'm not sure if using a mutex in the face
for RCU accesses is valid; the documentation only uses
spinlocks here.

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

Re: [PATCH 10/17] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke]

On 05/07/2015 02:34 PM, Bart Van Assche wrote:
> On 05/04/15 14:42, Hannes Reinecke wrote:
>> +static void release_port_group(struct kref *kref)
>> +{
>> +    struct alua_port_group *pg;
>> +
>> +    pg = container_of(kref, struct alua_port_group, kref);
>> +    printk(KERN_WARNING "alua: release port group %d\n",
>> pg->group_id);
> 
> Does this really need to be a warning ?
> 
No, you are right. This it mainly for debugging.

>> +    spin_lock(&port_group_lock);
>> +    pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
>> +    if (!pg) {
>> +        sdev_printk(KERN_WARNING, sdev,
>> +                "%s: kzalloc port group failed\n",
>> +                ALUA_DH_NAME);
>> +        /* Temporary failure, bypass */
>> +        spin_unlock(&port_group_lock);
>> +        return SCSI_DH_DEV_TEMP_BUSY;
>> +    }
>> +    pg->group_id = group_id;
>> +    pg->buff = pg->inq;
>> +    pg->bufflen = ALUA_INQUIRY_SIZE;
>> +    pg->tpgs = h->tpgs;
>> +    pg->state = TPGS_STATE_OPTIMIZED;
>> +    kref_init(&pg->kref);
>> +    list_add(&pg->node, &port_group_list);
>> +    h->pg = pg;
>> +    spin_unlock(&port_group_lock);
> 
> Sorry but it's not clear to me why the kzalloc() statement happens
> under port_group_lock. Please consider to perform only the
> list_add() statement under port_group_lock, to perform all other
> assignments without holding that lock and to change GFP_ATOMIC into
> GFP_KERNEL.
> 
Right, kzalloc() doesn't need to happen under the lock.

>> -    len = (h->buff[0] << 24) + (h->buff[1] << 16) +
>> -        (h->buff[2] << 8) + h->buff[3] + 4;
>> +    len = (pg->buff[0] << 24) + (pg->buff[1] << 16) +
>> +        (pg->buff[2] << 8) + pg->buff[3] + 4;
> 
> Has it been considered to use get_unaligned_be32() instead ?
> 
Okay, will do.

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

Re: [PATCH 10/17] scsi_dh_alua: Use separate alua_port_group structure

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:16PM +0200, Hannes Reinecke wrote:
> The port group needs to be a separate structure as several
> LUNs might belong to the same group.

The switch to on-stack sense data in alua_rtpg probably should be
a separate patch.

> -static int realloc_buffer(struct alua_dh_data *h, unsigned len)
> +static int realloc_buffer(struct alua_port_group *pg, unsigned len)
>  {

Not directly related to this patch, but what's the reason to have
a buffer hang off the dh_data or now port group?  Seems like a local
allocation in alua_rtpg would be more sensible.

> +	spin_lock(&port_group_lock);
> +	pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
> +	if (!pg) {
> +		sdev_printk(KERN_WARNING, sdev,
> +			    "%s: kzalloc port group failed\n",
> +			    ALUA_DH_NAME);
> +		/* Temporary failure, bypass */
> +		spin_unlock(&port_group_lock);
> +		return SCSI_DH_DEV_TEMP_BUSY;
> +	}

As bart mentioned it should be simply enough to move the allocation
and initialization outside the spinlock.  

Re: [PATCH 10/17] scsi_dh_alua: Use separate alua_port_group structure

[Hannes Reinecke]

On 05/11/2015 02:32 PM, Christoph Hellwig wrote:
> On Mon, May 04, 2015 at 02:42:16PM +0200, Hannes Reinecke wrote:
>> The port group needs to be a separate structure as several
>> LUNs might belong to the same group.
> 
> The switch to on-stack sense data in alua_rtpg probably should be
> a separate patch.
> 
>> -static int realloc_buffer(struct alua_dh_data *h, unsigned len)
>> +static int realloc_buffer(struct alua_port_group *pg, unsigned len)
>>  {
> 
> Not directly related to this patch, but what's the reason to have
> a buffer hang off the dh_data or now port group?  Seems like a local
> allocation in alua_rtpg would be more sensible.
> 
Hmm. Okay, can do this.

>> +	spin_lock(&port_group_lock);
>> +	pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
>> +	if (!pg) {
>> +		sdev_printk(KERN_WARNING, sdev,
>> +			    "%s: kzalloc port group failed\n",
>> +			    ALUA_DH_NAME);
>> +		/* Temporary failure, bypass */
>> +		spin_unlock(&port_group_lock);
>> +		return SCSI_DH_DEV_TEMP_BUSY;
>> +	}
> 
> As bart mentioned it should be simply enough to move the allocation
> and initialization outside the spinlock.  
> 
Yes, will be doing so. But I'm still waiting for a review for the
remaining bits before sending a new version.

Cheers,

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

[PATCH 11/17] scsi_dh_alua: simplify sense code handling

[Hannes Reinecke]

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

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 492b721..98d4a22 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -349,28 +349,6 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * LUN Not Accessible - ALUA state transition
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0b)
-			/*
-			 * LUN Not Accessible -- Target port in standby state
-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0c)
-			/*
-			 * LUN Not Accessible -- Target port in unavailable state
-			 */
-			return SUCCESS;
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x12)
-			/*
-			 * LUN Not Ready -- Offline
-			 */
-			return SUCCESS;
-		if (sdev->allow_restart &&
-		    sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x02)
-			/*
-			 * if the device is not started, we need to wake
-			 * the error handler to start the motor
-			 */
-			return FAILED;
 		break;
 	case UNIT_ATTENTION:
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
@@ -385,7 +363,7 @@ static int alua_check_sense(struct scsi_device *sdev,
 			return ADD_TO_MLQUEUE;
 		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x01)
 			/*
-			 * Mode Parameters Changed
+			 * Mode parameter changed
 			 */
 			return ADD_TO_MLQUEUE;
 		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x06)
@@ -398,18 +376,6 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * Implicit ALUA state transition failed
 			 */
 			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x03)
-			/*
-			 * Inquiry data has changed
-			 */
-			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x3f && sense_hdr->ascq == 0x0e)
-			/*
-			 * REPORTED_LUNS_DATA_HAS_CHANGED is reported
-			 * when switching controllers on targets like
-			 * Intel Multi-Flex. We can just retry.
-			 */
-			return ADD_TO_MLQUEUE;
 		break;
 	}
 
@@ -474,6 +440,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 		}
 
 		err = alua_check_sense(sdev, &sense_hdr);
+		if (sense_hdr.sense_key == UNIT_ATTENTION)
+			err = ADD_TO_MLQUEUE;
 		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
 				    ALUA_DH_NAME);
-- 
1.8.5.2

Re: [PATCH 11/17] scsi_dh_alua: simplify sense code handling

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:17PM +0200, Hannes Reinecke wrote:
> Most sense code is already handled in the generic
> code, so we shouldn't be adding special cases here.
> However, when doing so we need to check for
> unit attention whenever we're sending an internal
> command.

Shouldn't we move handling of all these sense codes to common code?  They
are part of the generic SPC list of sense codes, so splitting them up
into two functions is rather confusing.

> @@ -474,6 +440,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
>  		}
>  
>  		err = alua_check_sense(sdev, &sense_hdr);
> +		if (sense_hdr.sense_key == UNIT_ATTENTION)
> +			err = ADD_TO_MLQUEUE;

And this really should be a separate patch.

Re: [PATCH 11/17] scsi_dh_alua: simplify sense code handling

[Hannes Reinecke]

On 05/11/2015 08:58 AM, Christoph Hellwig wrote:
> On Mon, May 04, 2015 at 02:42:17PM +0200, Hannes Reinecke wrote:
>> Most sense code is already handled in the generic
>> code, so we shouldn't be adding special cases here.
>> However, when doing so we need to check for
>> unit attention whenever we're sending an internal
>> command.
> 
> Shouldn't we move handling of all these sense codes to common code?  They
> are part of the generic SPC list of sense codes, so splitting them up
> into two functions is rather confusing.
> 
Sigh. This is one of the sore topics with the SCSI stack.

The default sense code handling is correct only for filesystem
I/O; BLOCK_PC callers are expected to handle all errors themselves.
Which typically is a pain as one always forgets the one or the
other issue.

The device handlers have a callout into that generic function
to handle and device handler specific sense codes.

So with that I do agree that calling alua_check_sense() here
is dubious as it should have been run from the generic path already.

Will be checking and fixing it up.

>> @@ -474,6 +440,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
>>  		}
>>  
>>  		err = alua_check_sense(sdev, &sense_hdr);
>> +		if (sense_hdr.sense_key == UNIT_ATTENTION)
>> +			err = ADD_TO_MLQUEUE;
> 
> And this really should be a separate patch.
> 
Okay, will be doing so.

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

Re: [PATCH 11/17] scsi_dh_alua: simplify sense code handling

[Christoph Hellwig]

On Mon, May 11, 2015 at 04:52:14PM +0200, Hannes Reinecke wrote:
> Sigh. This is one of the sore topics with the SCSI stack.
> 
> The default sense code handling is correct only for filesystem
> I/O; BLOCK_PC callers are expected to handle all errors themselves.
> Which typically is a pain as one always forgets the one or the
> other issue.
> 
> The device handlers have a callout into that generic function
> to handle and device handler specific sense codes.
> 
> So with that I do agree that calling alua_check_sense() here
> is dubious as it should have been run from the generic path already.
> 
> Will be checking and fixing it up.

What I meant is that we really shouldn't handle the sense codes in
the ALUA handler - they are generic SCSІ sense codes and we'd better
handle them in the core code, ditto for the other device handlers actually.

Now the problem of BLOCK_PC ignoring the sense handling is a different
one, but why don't we export scsi_check_sense and allow BLOCK_PC callers
like the device handlers reuse the logic instead of duplicating it?
--
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 12/17] scsi_dh_alua: parse target device id

[Hannes Reinecke]

Parse VPD descriptor to figure out the device identification.
As devices might implement several descriptors the order
of preference is:
- NAA IEE Registered Extended
- EUI-64 based 16-byte
- EUI-64 based 12-byte
- NAA IEEE Registered
- NAA IEEE Extended
A SCSI name string descriptor is preferred to all of them
if the identification is longer than 16 bytes.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 98d4a22..2dd9578 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -70,6 +70,9 @@ static DEFINE_SPINLOCK(port_group_lock);
 struct alua_port_group {
 	struct kref		kref;
 	struct list_head	node;
+	unsigned char		device_id[256];
+	unsigned char		device_id_str[256];
+	int			device_id_size;
 	int			group_id;
 	int			tpgs;
 	int			state;
@@ -253,17 +256,84 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	unsigned char *d;
 	int group_id = -1;
-	struct alua_port_group *pg = NULL;
+	char device_id_str[256], *device_id = NULL;
+	int device_id_size, device_id_type = 0;
+	struct alua_port_group *tmp_pg, *pg = NULL;
 
 	if (!sdev->vpd_pg83)
 		return SCSI_DH_DEV_UNSUPP;
 
 	/*
 	 * Look for the correct descriptor.
+	 * Order of preference for lun descriptor:
+	 * - SCSI name string
+	 * - NAA IEEE Registered Extended
+	 * - EUI-64 based 16-byte
+	 * - EUI-64 based 12-byte
+	 * - NAA IEEE Registered
+	 * - NAA IEEE Extended
+	 * as longer descriptors reduce the likelyhood
+	 * of identification clashes.
 	 */
+	memset(device_id_str, 0, 256);
+	device_id_size = 0;
 	d = sdev->vpd_pg83 + 4;
 	while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) {
 		switch (d[1] & 0xf) {
+		case 0x2:
+			/* EUI-64 */
+			if ((d[1] & 0x30) == 0x00) {
+				if (device_id_size > d[3])
+					break;
+				/* Prefer NAA IEEE Registered Extended */
+				if (device_id_type == 0x3 &&
+				    device_id_size == d[3])
+					break;
+				device_id_size = d[3];
+				device_id = d + 4;
+				device_id_type = d[1] & 0xf;
+				switch (device_id_size) {
+				case 8:
+					sprintf(device_id_str,
+						"eui.%8phN", d + 4);
+					break;
+				case 12:
+					sprintf(device_id_str,
+						"eui.%12phN", d + 4);
+					break;
+				case 16:
+					sprintf(device_id_str,
+						"eui.%16phN", d + 4);
+					break;
+				default:
+					device_id_size = 0;
+					break;
+				}
+			}
+			break;
+		case 0x3:
+			/* NAA */
+			if ((d[1] & 0x30) == 0x00) {
+				if (device_id_size > d[3])
+					break;
+				device_id_size = d[3];
+				device_id = d + 4;
+				device_id_type = d[1] & 0xf;
+				switch (device_id_size) {
+				case 8:
+					sprintf(device_id_str,
+						"naa.%8phN", d + 4);
+					break;
+				case 16:
+					sprintf(device_id_str,
+						"naa.%16phN", d + 4);
+					break;
+				default:
+					device_id_size = 0;
+					break;
+				}
+			}
+			break;
 		case 0x4:
 			/* Relative target port */
 			h->rel_port = (d[6] << 8) + d[7];
@@ -272,6 +342,21 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 			/* Target port group */
 			group_id = (d[6] << 8) + d[7];
 			break;
+		case 0x8:
+			/* SCSI name string */
+			if ((d[1] & 0x30) == 0x00) {
+				/* SCSI name */
+				if (device_id_size > d[3])
+					break;
+				device_id_size = d[3];
+				device_id = d + 4;
+				device_id_type = d[1] & 0xf;
+				strncpy(device_id_str, d + 4, 256);
+				if (device_id_size > 255)
+					device_id_size = 255;
+				device_id_str[device_id_size] = '\0';
+			}
+			break;
 		default:
 			break;
 		}
@@ -290,11 +375,35 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 		h->tpgs = TPGS_MODE_NONE;
 		return SCSI_DH_DEV_UNSUPP;
 	}
-	sdev_printk(KERN_INFO, sdev,
-		    "%s: port group %02x rel port %02x\n",
-		    ALUA_DH_NAME, group_id, h->rel_port);
 
 	spin_lock(&port_group_lock);
+	if (device_id_size) {
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: device %s port group %02x "
+			    "rel port %02x\n", ALUA_DH_NAME,
+			    device_id_str, group_id, h->rel_port);
+		list_for_each_entry(tmp_pg, &port_group_list, node) {
+			if (tmp_pg->group_id != group_id)
+				continue;
+			if (tmp_pg->device_id_size != device_id_size)
+				continue;
+			if (memcmp(tmp_pg->device_id, device_id,
+				   device_id_size))
+				continue;
+			pg = tmp_pg;
+			break;
+		}
+	} else {
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: port group %02x rel port %02x\n",
+			    ALUA_DH_NAME, group_id, h->rel_port);
+	}
+	if (pg) {
+		h->pg = pg;
+		kref_get(&pg->kref);
+		spin_unlock(&port_group_lock);
+		return SCSI_DH_OK;
+	}
 	pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
 	if (!pg) {
 		sdev_printk(KERN_WARNING, sdev,
@@ -304,6 +413,14 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 		spin_unlock(&port_group_lock);
 		return SCSI_DH_DEV_TEMP_BUSY;
 	}
+	if (device_id_size) {
+		memcpy(pg->device_id, device_id, device_id_size);
+		strncpy(pg->device_id_str, device_id_str, 256);
+	} else {
+		memset(pg->device_id, 0, 256);
+		pg->device_id_str[0] = '\0';
+	}
+	pg->device_id_size = device_id_size;
 	pg->group_id = group_id;
 	pg->buff = pg->inq;
 	pg->bufflen = ALUA_INQUIRY_SIZE;
-- 
1.8.5.2

[PATCH 13/17] scsi_dh_alua: revert commit a8e5a2d593cbfccf530c3382c2c328d2edaa7b66

[Hannes Reinecke]

Obsoleted by the next patch.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 2dd9578..2a5e2b2 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -502,13 +502,12 @@ static int alua_check_sense(struct scsi_device *sdev,
 /*
  * alua_rtpg - Evaluate REPORT TARGET GROUP STATES
  * @sdev: the device to be evaluated.
- * @wait_for_transition: if nonzero, wait ALUA_FAILOVER_TIMEOUT seconds for device to exit transitioning state
  *
  * Evaluate the Target Port Group State.
  * Returns SCSI_DH_DEV_OFFLINED if the path is
  * found to be unusable.
  */
-static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int wait_for_transition)
+static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 	struct scsi_sense_hdr sense_hdr;
@@ -592,7 +591,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);
@@ -630,19 +629,14 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, int w
 
 	switch (pg->state) {
 	case TPGS_STATE_TRANSITIONING:
-		if (wait_for_transition) {
-			if (time_before(jiffies, expiry)) {
-				/* State transition, retry */
-				interval += 2000;
-				msleep(interval);
-				goto retry;
-			}
-			err = SCSI_DH_RETRY;
-		} else {
-			err = SCSI_DH_OK;
+		if (time_before(jiffies, expiry)) {
+			/* State transition, retry */
+			interval += 2000;
+			msleep(interval);
+			goto retry;
 		}
-
 		/* Transitioning time exceeded, set port to standby */
+		err = SCSI_DH_RETRY;
 		pg->state = TPGS_STATE_STANDBY;
 		break;
 	case TPGS_STATE_OFFLINE:
-- 
1.8.5.2

Re: [PATCH 13/17] scsi_dh_alua: revert commit a8e5a2d593cbfccf530c3382c2c328d2edaa7b66

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:19PM +0200, Hannes Reinecke wrote:
> Obsoleted by the next patch.

Please use a descriptive subject, and the mention the commit name in the
message body in the canonical format:  commit a8e5a2 ("[SCSI] scsi_dh_alua:
ALUA handler attach should succeed while TPG is transitioning");

Re: [PATCH 13/17] scsi_dh_alua: revert commit a8e5a2d593cbfccf530c3382c2c328d2edaa7b66

[Hannes Reinecke]

On 05/11/2015 09:00 AM, Christoph Hellwig wrote:
> On Mon, May 04, 2015 at 02:42:19PM +0200, Hannes Reinecke wrote:
>> Obsoleted by the next patch.
> 
> Please use a descriptive subject, and the mention the commit name in the
> message body in the canonical format:  commit a8e5a2 ("[SCSI] scsi_dh_alua:
> ALUA handler attach should succeed while TPG is transitioning");
> 
Okay.

Cheers,

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

[PATCH 14/17] scsi_dh_alua: Use workqueue for RTPG

[Hannes Reinecke]

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

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

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 2a5e2b2..142c95b 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -22,6 +22,8 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/rcupdate.h>
 #include <asm/unaligned.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_dbg.h>
@@ -59,13 +61,19 @@
 #define ALUA_INQUIRY_SIZE		36
 #define ALUA_FAILOVER_TIMEOUT		60
 #define ALUA_FAILOVER_RETRIES		5
+#define ALUA_RTPG_DELAY_MSECS		5
 
 /* flags passed from user level */
-#define ALUA_OPTIMIZE_STPG		1
-#define ALUA_RTPG_EXT_HDR_UNSUPP	2
+#define ALUA_OPTIMIZE_STPG		0x01
+#define ALUA_RTPG_EXT_HDR_UNSUPP	0x02
+/* State machine flags */
+#define ALUA_PG_RUN_RTPG		0x10
+#define ALUA_PG_RUN_STPG		0x20
+
 
 static LIST_HEAD(port_group_list);
 static DEFINE_SPINLOCK(port_group_lock);
+static struct workqueue_struct *scsidh_aluad;
 
 struct alua_port_group {
 	struct kref		kref;
@@ -82,13 +90,25 @@ struct alua_port_group {
 	unsigned char		*buff;
 	int			bufflen;
 	unsigned char		transition_tmo;
+	unsigned long		expiry;
+	unsigned long		interval;
+	struct delayed_work	rtpg_work;
+	spinlock_t		rtpg_lock;
+	struct list_head	rtpg_list;
+	struct scsi_device	*rtpg_sdev;
 };
 
 struct alua_dh_data {
 	struct alua_port_group	*pg;
+	spinlock_t		pg_lock;
 	int			rel_port;
 	int			tpgs;
-	struct scsi_device	*sdev;
+	int			error;
+	struct completion       init_complete;
+};
+
+struct alua_queue_data {
+	struct list_head	entry;
 	activate_complete	callback_fn;
 	void			*callback_data;
 };
@@ -98,6 +118,8 @@ struct alua_dh_data {
 
 static char print_alua_state(int);
 static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);
+static void alua_rtpg_work(struct work_struct *work);
+static void alua_check(struct scsi_device *sdev);
 
 static int realloc_buffer(struct alua_port_group *pg, unsigned len)
 {
@@ -399,9 +421,12 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 			    ALUA_DH_NAME, group_id, h->rel_port);
 	}
 	if (pg) {
-		h->pg = pg;
 		kref_get(&pg->kref);
 		spin_unlock(&port_group_lock);
+		spin_lock(&h->pg_lock);
+		rcu_assign_pointer(h->pg, pg);
+		spin_unlock(&h->pg_lock);
+		synchronize_rcu();
 		return SCSI_DH_OK;
 	}
 	pg = kzalloc(sizeof(struct alua_port_group), GFP_ATOMIC);
@@ -427,9 +452,14 @@ static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h)
 	pg->tpgs = h->tpgs;
 	pg->state = TPGS_STATE_OPTIMIZED;
 	kref_init(&pg->kref);
+	INIT_DELAYED_WORK(&pg->rtpg_work, alua_rtpg_work);
+	INIT_LIST_HEAD(&pg->rtpg_list);
+	spin_lock_init(&pg->rtpg_lock);
 	list_add(&pg->node, &port_group_list);
-	h->pg = pg;
 	spin_unlock(&port_group_lock);
+	spin_lock(&h->pg_lock);
+	rcu_assign_pointer(h->pg, pg);
+	spin_unlock(&h->pg_lock);
 
 	return SCSI_DH_OK;
 }
@@ -461,11 +491,14 @@ static int alua_check_sense(struct scsi_device *sdev,
 {
 	switch (sense_hdr->sense_key) {
 	case NOT_READY:
-		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0a)
+		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x0a) {
 			/*
 			 * LUN Not Accessible - ALUA state transition
+			 * Kickoff worker to update internal state.
 			 */
+			alua_check(sdev);
 			return ADD_TO_MLQUEUE;
+		}
 		break;
 	case UNIT_ATTENTION:
 		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
@@ -514,15 +547,15 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	int len, k, off, valid_states = 0;
 	unsigned char *ucp;
 	unsigned err, retval;
-	unsigned long expiry, interval = 0;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 
-	if (!pg->transition_tmo)
-		expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT * HZ);
-	else
-		expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
-
+	if (!pg->expiry) {
+		if (!pg->transition_tmo)
+			pg->expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT * HZ);
+		else
+			pg->expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ);
+	}
  retry:
 	retval = submit_rtpg(sdev, pg->buff, pg->bufflen, sense, pg->flags);
 
@@ -537,6 +570,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 				err = SCSI_DH_DEV_TEMP_BUSY;
 			else
 				err = SCSI_DH_IO;
+			pg->expiry = 0;
 			return err;
 		}
 
@@ -558,15 +592,17 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		err = alua_check_sense(sdev, &sense_hdr);
 		if (sense_hdr.sense_key == UNIT_ATTENTION)
 			err = ADD_TO_MLQUEUE;
-		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry)) {
+		if (err == ADD_TO_MLQUEUE &&
+		    pg->expiry != 0 && time_before(jiffies, pg->expiry)) {
 			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
 				    ALUA_DH_NAME);
 			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
-			goto retry;
+			return SCSI_DH_RETRY;
 		}
 		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
 			    ALUA_DH_NAME);
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
+		pg->expiry = 0;
 		return SCSI_DH_IO;
 	}
 
@@ -579,6 +615,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;
@@ -595,7 +632,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 ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR)
@@ -629,23 +666,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;
 	}
 	return err;
@@ -665,8 +705,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	struct scsi_sense_hdr sense_hdr;
 
 	if (!(pg->tpgs & TPGS_MODE_EXPLICIT)) {
-		/* Only implicit ALUA supported, retry */
-		return SCSI_DH_RETRY;
+		/* Only implicit ALUA supported */
+		return SCSI_DH_OK;
 	}
 	switch (pg->state) {
 	case TPGS_STATE_OPTIMIZED:
@@ -692,8 +732,6 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		return SCSI_DH_NOSYS;
 		break;
 	}
-	/* Set state to transitioning */
-	pg->state = TPGS_STATE_TRANSITIONING;
 	retval = submit_stpg(sdev, pg->group_id, sense);
 
 	if (retval) {
@@ -715,6 +753,98 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	return err;
 }
 
+static void alua_rtpg_work(struct work_struct *work)
+{
+	struct alua_port_group *pg =
+		container_of(work, struct alua_port_group, rtpg_work.work);
+	struct scsi_device *sdev;
+	LIST_HEAD(qdata_list);
+	int err = SCSI_DH_OK;
+	struct alua_queue_data *qdata, *tmp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pg->rtpg_lock, flags);
+	sdev = pg->rtpg_sdev;
+	if (!sdev) {
+		WARN_ON(pg->flags & ALUA_PG_RUN_RTPG ||
+			pg->flags & ALUA_PG_RUN_STPG);
+		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+		return;
+	}
+	if (pg->flags & ALUA_PG_RUN_RTPG) {
+		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+		err = alua_rtpg(sdev, pg);
+		if (err == SCSI_DH_RETRY) {
+			queue_delayed_work(scsidh_aluad, &pg->rtpg_work,
+					   pg->interval * HZ);
+			return;
+		}
+		spin_lock_irqsave(&pg->rtpg_lock, flags);
+		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->rtpg_lock, flags);
+		err = alua_stpg(sdev, pg);
+		spin_lock_irqsave(&pg->rtpg_lock, flags);
+		pg->flags &= ~ALUA_PG_RUN_STPG;
+		if (err == SCSI_DH_RETRY) {
+			pg->flags |= ALUA_PG_RUN_RTPG;
+			pg->interval = 0;
+			spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+			queue_delayed_work(scsidh_aluad, &pg->rtpg_work,
+				msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS));
+			return;
+		}
+	}
+
+	list_splice_init(&pg->rtpg_list, &qdata_list);
+	pg->rtpg_sdev = NULL;
+	spin_unlock_irqrestore(&pg->rtpg_lock, flags);
+
+	list_for_each_entry_safe(qdata, tmp, &qdata_list, entry) {
+		list_del(&qdata->entry);
+		if (qdata->callback_fn)
+			qdata->callback_fn(qdata->callback_data, err);
+		kfree(qdata);
+	}
+	kref_put(&pg->kref, release_port_group);
+	scsi_device_put(sdev);
+}
+
+static void alua_rtpg_queue(struct alua_port_group *pg,
+			    struct scsi_device *sdev,
+			    struct alua_queue_data *qdata)
+{
+	int start_queue = 0;
+	unsigned long flags;
+
+	if (!pg)
+		return;
+
+	kref_get(&pg->kref);
+	spin_lock_irqsave(&pg->rtpg_lock, flags);
+	if (qdata) {
+		list_add_tail(&qdata->entry, &pg->rtpg_list);
+		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->rtpg_lock, flags);
+
+	if (start_queue)
+		queue_delayed_work(scsidh_aluad, &pg->rtpg_work,
+				   msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS));
+	kref_put(&pg->kref, release_port_group);
+}
+
 /*
  * alua_initialize - Initialize ALUA state
  * @sdev: the device to be initialized
@@ -724,22 +854,32 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
  */
 static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 {
-	int err;
-
-	err = alua_check_tpgs(sdev, h);
-	if (err != SCSI_DH_OK)
-		goto out;
-
-	err = alua_check_vpd(sdev, h);
-	if (err != SCSI_DH_OK || !h->pg)
-		goto out;
+	struct alua_port_group *pg = NULL;
 
-	kref_get(&h->pg->kref);
-	err = alua_rtpg(sdev, h->pg, 0);
-	kref_put(&h->pg->kref, release_port_group);
-out:
-	return err;
+	h->error = alua_check_tpgs(sdev, h);
+	if (h->error == SCSI_DH_OK) {
+		h->error = alua_check_vpd(sdev, h);
+		rcu_read_lock();
+		pg = rcu_dereference(h->pg);
+		if (!pg) {
+			rcu_read_unlock();
+			h->tpgs = TPGS_MODE_NONE;
+			if (h->error == SCSI_DH_OK)
+				h->error = SCSI_DH_IO;
+		} else {
+			kref_get(&pg->kref);
+			rcu_read_unlock();
+		}
+	}
+	complete(&h->init_complete);
+	if (pg) {
+		pg->expiry = 0;
+		alua_rtpg_queue(pg, sdev, NULL);
+		kref_put(&pg->kref, release_port_group);
+	}
+	return h->error;
 }
+
 /*
  * alua_set_params - set/unset the optimize flag
  * @sdev: device on the path to be activated
@@ -756,6 +896,10 @@ 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 (!h)
+		return -ENXIO;
 
 	if ((sscanf(params, "%u", &argc) != 1) || (argc != 1))
 		return -EINVAL;
@@ -773,10 +917,12 @@ static int alua_set_params(struct scsi_device *sdev, const char *params)
 	}
 	rcu_read_unlock();
 
+	spin_lock_irqsave(&pg->rtpg_lock, flags);
 	if (optimize)
 		pg->flags |= ALUA_OPTIMIZE_STPG;
 	else
 		pg->flags |= ~ALUA_OPTIMIZE_STPG;
+	spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 
 	return result;
 }
@@ -800,24 +946,48 @@ 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)
+	if (!h) {
+		err = SCSI_DH_NOSYS;
 		goto out;
+	}
 
-	kref_get(&h->pg->kref);
+	qdata = kzalloc(sizeof(*qdata), GFP_KERNEL);
+	if (!qdata) {
+		err = SCSI_DH_NOSYS;
+		goto out;
+	}
+	qdata->callback_fn = fn;
+	qdata->callback_data = data;
 
-	if (optimize_stpg)
-		h->pg->flags |= ALUA_OPTIMIZE_STPG;
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (!pg) {
+		rcu_read_unlock();
+		wait_for_completion(&h->init_complete);
+		rcu_read_lock();
+		pg = rcu_dereference(h->pg);
+		if (!pg) {
+			rcu_read_unlock();
+			kfree(qdata);
+			err = h->error;
+			goto out;
+		}
+	}
+	kref_get(&pg->kref);
+	rcu_read_unlock();
 
-	err = alua_rtpg(sdev, h->pg, 1);
-	if (err != SCSI_DH_OK) {
-		kref_put(&h->pg->kref, release_port_group);
-		goto out;
+	if (optimize_stpg) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&pg->rtpg_lock, flags);
+		pg->flags |= ALUA_OPTIMIZE_STPG;
+		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 	}
-	err = alua_stpg(sdev, h->pg);
-	if (err == SCSI_DH_RETRY)
-		err = alua_rtpg(sdev, h->pg, 1);
-	kref_put(&h->pg->kref, release_port_group);
+	alua_rtpg_queue(pg, sdev, qdata);
+	kref_put(&pg->kref, release_port_group);
 out:
 	if (fn)
 		fn(data, err);
@@ -825,6 +995,28 @@ 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)
+{
+	struct alua_dh_data *h = sdev->handler_data;
+	struct alua_port_group *pg;
+
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (pg) {
+		kref_get(&pg->kref);
+		rcu_read_unlock();
+		alua_rtpg_queue(pg, sdev, NULL);
+		kref_put(&pg->kref, release_port_group);
+	} else
+		rcu_read_unlock();
+}
+
+/*
  * alua_prep_fn - request callback
  *
  * Fail I/O to all paths not in state
@@ -833,14 +1025,22 @@ 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)
+	if (!h)
 		return ret;
-	kref_get(&h->pg->kref);
-	state = h->pg->state;
-	kref_put(&h->pg->kref, release_port_group);
+
+	rcu_read_lock();
+	pg = rcu_dereference(h->pg);
+	if (pg) {
+		state = pg->state;
+		/* Defer I/O while rtpg_work is active */
+		if (pg->rtpg_sdev)
+			state = TPGS_STATE_TRANSITIONING;
+	}
+	rcu_read_unlock();
 	if (state == TPGS_STATE_TRANSITIONING)
 		ret = BLKPREP_DEFER;
 	else if (state != TPGS_STATE_OPTIMIZED &&
@@ -865,11 +1065,13 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
 	if (!h)
 		return -ENOMEM;
+	spin_lock_init(&h->pg_lock);
 	h->tpgs = TPGS_MODE_UNINITIALIZED;
-	h->pg = NULL;
+	rcu_assign_pointer(h->pg, NULL);
 	h->rel_port = -1;
-	h->sdev = sdev;
+	h->error = SCSI_DH_OK;
 
+	init_completion(&h->init_complete);
 	err = alua_initialize(sdev, h);
 	if (err != SCSI_DH_OK && err != SCSI_DH_DEV_OFFLINED)
 		goto failed;
@@ -888,10 +1090,17 @@ 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);
+	spin_unlock(&h->pg_lock);
+	synchronize_rcu();
+	if (pg) {
+		if (pg->rtpg_sdev)
+			flush_workqueue(scsidh_aluad);
+		kref_put(&pg->kref, release_port_group);
 	}
 	kfree(h);
 }
@@ -911,16 +1120,24 @@ static int __init alua_init(void)
 {
 	int r;
 
+	scsidh_aluad = create_singlethread_workqueue("scsi_dh_aluad");
+	if (!scsidh_aluad) {
+		printk(KERN_ERR "scsi_dh_aluad creation failed.\n");
+		return -EINVAL;
+	}
 	r = scsi_register_device_handler(&alua_dh);
-	if (r != 0)
+	if (r != 0) {
 		printk(KERN_ERR "%s: Failed to register scsi device handler",
 			ALUA_DH_NAME);
+		destroy_workqueue(scsidh_aluad);
+	}
 	return r;
 }
 
 static void __exit alua_exit(void)
 {
 	scsi_unregister_device_handler(&alua_dh);
+	destroy_workqueue(scsidh_aluad);
 }
 
 module_init(alua_init);
-- 
1.8.5.2

Re: [PATCH 14/17] scsi_dh_alua: Use workqueue for RTPG

[Christoph Hellwig]

On Mon, May 04, 2015 at 02:42:20PM +0200, Hannes Reinecke wrote:
> The current ALUA device_handler has two drawbacks:
> - We're sending a 'SET TARGET PORT GROUP' command to every LUN,
>   disregarding the fact that several LUNs might be in a port group
>   and will be automatically switched whenever _any_ LUN within
>   that port group receives the command.
> - Whenever a LUN is in 'transitioning' mode we cannot block I/O
>   to that LUN, instead the controller has to abort the command.
>   This leads to increased traffic across the wire and heavy load
>   on the controller during switchover.
> 
> With this patch the RTPG handling is moved to a workqueue, which
> is being run once per port group. This reduces the number of
> 'REPORT TARGET PORT GROUP' and 'SET TARGET PORT GROUPS' which
> will be send to the controller. It also allows us to block
> I/O to any LUN / port group found to be in 'transitioning' ALUA
> mode, as the workqueue item will be requeued until the controller
> moves out of transitioning.

I'm having a hard time understanding the workqueue use here.
What is the benefit of that one worker function to do everything?
It seems having a work struct in struct alua_queue_data to just
run STPG, and a different one to run RPTG in the port group structure
would be more sensible instead of interwinding them.

Also why do you need the sigle threaded workqueue?  That seems
like a possible limiting factor in a large enough system having
to deal with a cable disconnect cutting off multiple port groups,
or just during bootup.

Re: [PATCH 14/17] scsi_dh_alua: Use workqueue for RTPG

[Hannes Reinecke]

On 05/11/2015 03:49 PM, Christoph Hellwig wrote:
> On Mon, May 04, 2015 at 02:42:20PM +0200, Hannes Reinecke wrote:
>> The current ALUA device_handler has two drawbacks:
>> - We're sending a 'SET TARGET PORT GROUP' command to every LUN,
>>   disregarding the fact that several LUNs might be in a port group
>>   and will be automatically switched whenever _any_ LUN within
>>   that port group receives the command.
>> - Whenever a LUN is in 'transitioning' mode we cannot block I/O
>>   to that LUN, instead the controller has to abort the command.
>>   This leads to increased traffic across the wire and heavy load
>>   on the controller during switchover.
>>
>> With this patch the RTPG handling is moved to a workqueue, which
>> is being run once per port group. This reduces the number of
>> 'REPORT TARGET PORT GROUP' and 'SET TARGET PORT GROUPS' which
>> will be send to the controller. It also allows us to block
>> I/O to any LUN / port group found to be in 'transitioning' ALUA
>> mode, as the workqueue item will be requeued until the controller
>> moves out of transitioning.
> 
> I'm having a hard time understanding the workqueue use here.
> What is the benefit of that one worker function to do everything?
> It seems having a work struct in struct alua_queue_data to just
> run STPG, and a different one to run RPTG in the port group structure
> would be more sensible instead of interwinding them.
> 
Hmm. It _might_ be possible. However, we need to serialize
STPG and RTPG against each other; not sure if that's still possible
when using different workqueue items.

> Also why do you need the sigle threaded workqueue?  That seems
> like a possible limiting factor in a large enough system having
> to deal with a cable disconnect cutting off multiple port groups,
> or just during bootup.
> 
Okay, will be switching to a normal workqueue.

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

Re: [PATCH 14/17] scsi_dh_alua: Use workqueue for RTPG

[Christoph Hellwig]

So how about the following idea for GTPG and STPG handling:

 - we keep a single thread workqueue, but per target group instead
   of global to avoid concurrency issues hitting us too badly, after
   all workqueues are cheap these days.
 - GTPG keeps the per-group work_item, but instead of the separate
   lsit for STPG we just add the work item to the alua_queue_data
   structure, which at the point might also get a new name relecting
   the use a bit better.  STPG remains synchronous.

Btw, any chance to also publish a git tree to make reviewing easier?

Re: [PATCH 14/17] scsi_dh_alua: Use workqueue for RTPG

[Hannes Reinecke]

On 05/12/2015 10:16 AM, Christoph Hellwig wrote:
> So how about the following idea for GTPG and STPG handling:
> 
>  - we keep a single thread workqueue, but per target group instead
>    of global to avoid concurrency issues hitting us too badly, after
>    all workqueues are cheap these days.

Okay.

>  - GTPG keeps the per-group work_item, but instead of the separate
>    lsit for STPG we just add the work item to the alua_queue_data
>    structure, which at the point might also get a new name relecting
>    the use a bit better.  STPG remains synchronous.
> 
Well, I don't think that'll work.
qdata is per sdev, and STPG is per port group.

While it's true that we need to run STPG once a qdata item
has been created, it's not necessarily true that we need to
run STPG for _every_ qdata item; if we have more than one
path per group we need to run STPG only for one of them.
(Which is why there is a qdata structure in the first place.)

What I can do, though, is to split off stpg and rtpg in two
different routines, each with its own workqueue item.
That way the actual implementation becomes easier to follow.

Cheers,

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

[PATCH 15/17] 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.
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 | 65 +++++++++++++++++-------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 142c95b..cf83005 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -69,6 +69,7 @@
 /* State machine flags */
 #define ALUA_PG_RUN_RTPG		0x10
 #define ALUA_PG_RUN_STPG		0x20
+#define ALUA_PG_RUNNING			0x40
 
 
 static LIST_HEAD(port_group_list);
@@ -119,7 +120,7 @@ struct alua_queue_data {
 static char print_alua_state(int);
 static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *);
 static void alua_rtpg_work(struct work_struct *work);
-static void alua_check(struct scsi_device *sdev);
+static void alua_check(struct scsi_device *sdev, bool force);
 
 static int realloc_buffer(struct alua_port_group *pg, unsigned len)
 {
@@ -487,7 +488,7 @@ static char print_alua_state(int state)
 }
 
 static int alua_check_sense(struct scsi_device *sdev,
-			    struct scsi_sense_hdr *sense_hdr)
+			     struct scsi_sense_hdr *sense_hdr)
 {
 	switch (sense_hdr->sense_key) {
 	case NOT_READY:
@@ -496,36 +497,34 @@ static int alua_check_sense(struct scsi_device *sdev,
 			 * LUN Not Accessible - ALUA state transition
 			 * Kickoff worker to update internal state.
 			 */
-			alua_check(sdev);
-			return ADD_TO_MLQUEUE;
+			alua_check(sdev, false);
+			return NEEDS_RETRY;
 		}
 		break;
 	case UNIT_ATTENTION:
-		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
-			/*
-			 * Power On, Reset, or Bus Device Reset, just retry.
-			 */
-			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x04)
-			/*
-			 * Device internal reset
-			 */
-			return ADD_TO_MLQUEUE;
-		if (sense_hdr->asc == 0x2a && sense_hdr->ascq == 0x01)
+		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00) {
 			/*
-			 * Mode parameter changed
+			 * 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 == 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;
+		}
 		break;
 	}
 
@@ -589,7 +588,6 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			goto retry;
 		}
 
-		err = alua_check_sense(sdev, &sense_hdr);
 		if (sense_hdr.sense_key == UNIT_ATTENTION)
 			err = ADD_TO_MLQUEUE;
 		if (err == ADD_TO_MLQUEUE &&
@@ -744,7 +742,6 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			/* Retry RTPG */
 			return err;
 		}
-		err = alua_check_sense(sdev, &sense_hdr);
 		sdev_printk(KERN_INFO, sdev, "%s: stpg failed\n",
 			    ALUA_DH_NAME);
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
@@ -771,15 +768,18 @@ static void alua_rtpg_work(struct work_struct *work)
 		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 		return;
 	}
+	pg->flags |= ALUA_PG_RUNNING;
 	if (pg->flags & ALUA_PG_RUN_RTPG) {
 		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 		err = alua_rtpg(sdev, pg);
+		spin_lock_irqsave(&pg->rtpg_lock, flags);
 		if (err == SCSI_DH_RETRY) {
+			pg->flags &= ~ALUA_PG_RUNNING;
+			spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 			queue_delayed_work(scsidh_aluad, &pg->rtpg_work,
 					   pg->interval * HZ);
 			return;
 		}
-		spin_lock_irqsave(&pg->rtpg_lock, flags);
 		pg->flags &= ~ALUA_PG_RUN_RTPG;
 		if (err != SCSI_DH_OK)
 			pg->flags &= ~ALUA_PG_RUN_STPG;
@@ -792,6 +792,7 @@ static void alua_rtpg_work(struct work_struct *work)
 		if (err == SCSI_DH_RETRY) {
 			pg->flags |= ALUA_PG_RUN_RTPG;
 			pg->interval = 0;
+			pg->flags &= ~ALUA_PG_RUNNING;
 			spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 			queue_delayed_work(scsidh_aluad, &pg->rtpg_work,
 				msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS));
@@ -809,13 +810,16 @@ static void alua_rtpg_work(struct work_struct *work)
 			qdata->callback_fn(qdata->callback_data, err);
 		kfree(qdata);
 	}
+	spin_lock_irqsave(&pg->rtpg_lock, flags);
+	pg->flags &= ~ALUA_PG_RUNNING;
+	spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 	kref_put(&pg->kref, release_port_group);
 	scsi_device_put(sdev);
 }
 
 static void alua_rtpg_queue(struct alua_port_group *pg,
 			    struct scsi_device *sdev,
-			    struct alua_queue_data *qdata)
+			    struct alua_queue_data *qdata, bool force)
 {
 	int start_queue = 0;
 	unsigned long flags;
@@ -836,12 +840,15 @@ 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_RUNNING) && force)
+		start_queue = 1;
+
 	spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 
 	if (start_queue)
-		queue_delayed_work(scsidh_aluad, &pg->rtpg_work,
-				   msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS));
+		mod_delayed_work(scsidh_aluad, &pg->rtpg_work,
+				 msecs_to_jiffies(ALUA_RTPG_DELAY_MSECS));
+
 	kref_put(&pg->kref, release_port_group);
 }
 
@@ -874,7 +881,7 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h)
 	complete(&h->init_complete);
 	if (pg) {
 		pg->expiry = 0;
-		alua_rtpg_queue(pg, sdev, NULL);
+		alua_rtpg_queue(pg, sdev, NULL, true);
 		kref_put(&pg->kref, release_port_group);
 	}
 	return h->error;
@@ -986,7 +993,7 @@ static int alua_activate(struct scsi_device *sdev,
 		pg->flags |= ALUA_OPTIMIZE_STPG;
 		spin_unlock_irqrestore(&pg->rtpg_lock, flags);
 	}
-	alua_rtpg_queue(pg, sdev, qdata);
+	alua_rtpg_queue(pg, sdev, qdata, true);
 	kref_put(&pg->kref, release_port_group);
 out:
 	if (fn)
@@ -1000,7 +1007,7 @@ out:
  *
  * Check the device status
  */
-static void alua_check(struct scsi_device *sdev)
+static void alua_check(struct scsi_device *sdev, bool force)
 {
 	struct alua_dh_data *h = sdev->handler_data;
 	struct alua_port_group *pg;
@@ -1010,7 +1017,7 @@ static void alua_check(struct scsi_device *sdev)
 	if (pg) {
 		kref_get(&pg->kref);
 		rcu_read_unlock();
-		alua_rtpg_queue(pg, sdev, NULL);
+		alua_rtpg_queue(pg, sdev, NULL, force);
 		kref_put(&pg->kref, release_port_group);
 	} else
 		rcu_read_unlock();
-- 
1.8.5.2

[PATCH 16/17] 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 | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index cf83005..6c1eadf 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -548,6 +548,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	unsigned err, retval;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
+	struct alua_port_group *tmp_pg;
+	unsigned long flags;
 
 	if (!pg->expiry) {
 		if (!pg->transition_tmo)
@@ -638,17 +640,35 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	else
 		tpg_desc_tbl_off = 4;
 
+	spin_lock_irqsave(&port_group_lock, flags);
 	for (k = tpg_desc_tbl_off, ucp = pg->buff + tpg_desc_tbl_off;
 	     k < len;
 	     k += off, ucp += off) {
 
-		if (pg->group_id == (ucp[2] << 8) + ucp[3]) {
-			pg->state = ucp[0] & 0x0f;
-			pg->pref = ucp[0] >> 7;
-			valid_states = ucp[1];
+		list_for_each_entry(tmp_pg, &port_group_list, node) {
+			u16 group_id = get_unaligned_be16(&ucp[2]);
+			if (tmp_pg->group_id != group_id)
+				continue;
+			if (tmp_pg->device_id_size != pg->device_id_size)
+				continue;
+			if (memcmp(tmp_pg->device_id, pg->device_id,
+				   tmp_pg->device_id_size))
+				continue;
+			tmp_pg->state = ucp[0] & 0x0f;
+			tmp_pg->pref = ucp[0] >> 7;
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: device %s port group %02x "
+				    "state %c %s\n", ALUA_DH_NAME,
+				    tmp_pg->device_id_str, tmp_pg->group_id,
+				    print_alua_state(tmp_pg->state),
+				    tmp_pg->pref ?
+				    "preferred" : "non-preferred");
+			if (tmp_pg == pg)
+				valid_states = ucp[1];
 		}
 		off = 8 + (ucp[7] * 4);
 	}
+	spin_unlock_irqrestore(&port_group_lock, flags);
 
 	sdev_printk(KERN_INFO, sdev,
 		    "%s: port group %02x state %c %s supports %c%c%c%c%c%c%c\n",
-- 
1.8.5.2

[PATCH 17/17] 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 6c1eadf..a13f963 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -31,7 +31,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_STATE_OPTIMIZED		0x0
 #define TPGS_STATE_NONOPTIMIZED		0x1
-- 
1.8.5.2