scsi_dh_alua: add missing transitioning state support

PATCH In review Mike Snitzer 4 participants 14 messages Aug 17, 2010 – Sep 23, 2010 SCSI

lore

Share & tools

lore

Mike Snitzer

author

2010-08-17 19:06 UTC #0 lore raw

PATCH 1/1 scsi_dh_alua: add missing transitioning state support

Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.

These gaps were identified during controller failover testing of an ALUA array.

Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +7 -3

index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		    h->state == TPGS_STATE_STANDBY)
 			/* Useable path if active */
 			err = SCSI_DH_OK;
+		else if (h->state == TPGS_STATE_TRANSITIONING)
+			/* State transition, retry */
+			goto retry;
 		else
 			/* Path unuseable for unavailable/offline */
 			err = SCSI_DH_DEV_OFFLINED;

@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED &&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->state == TPGS_STATE_TRANSITIONING)
+		ret = BLKPREP_DEFER;
+	else if (h->state != TPGS_STATE_OPTIMIZED &&
+		 h->state != TPGS_STATE_NONOPTIMIZED) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}
 	return ret;
-
 }
 
 static const struct scsi_dh_devlist alua_dev_list[] = {

Nicholas A. Bellinger

Acked-by on v1 1/1

2010-08-17 19:27 UTC #1 lore raw

Quoted 32 lines · Mike Snitzer

Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.

These gaps were identified during controller failover testing of an ALUA array.

Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +4 -2

index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		    h->state == TPGS_STATE_STANDBY)
 			/* Useable path if active */
 			err = SCSI_DH_OK;
+		else if (h->state == TPGS_STATE_TRANSITIONING)
+			/* State transition, retry */
+			goto retry;
 		else
 			/* Path unuseable for unavailable/offline */
 			err = SCSI_DH_DEV_OFFLINED;

@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED &&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->state == TPGS_STATE_TRANSITIONING)

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device *sdev, struct request *req) · line 713

Quoted 10 lines

drivers/scsi/device_handler/scsi_dh_alua.c +3 -1

+		ret = BLKPREP_DEFER;
+	else if (h->state != TPGS_STATE_OPTIMIZED &&
+		 h->state != TPGS_STATE_NONOPTIMIZED) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}
 	return ret;
-
 }

Makes sense to me..

Acked-by Nicholas A. Bellinger <[email protected]>

Hannes Reinecke

2010-08-30 09:36 UTC #2 lore raw

Quoted 46 lines · Nicholas A. Bellinger

Quoted 32 lines · Mike Snitzer

Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.

These gaps were identified during controller failover testing of an ALUA array.

Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

drivers/scsi/device_handler/scsi_dh_alua.c +4 -2

index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		    h->state == TPGS_STATE_STANDBY)
 			/* Useable path if active */
 			err = SCSI_DH_OK;
+		else if (h->state == TPGS_STATE_TRANSITIONING)
+			/* State transition, retry */
+			goto retry;
 		else
 			/* Path unuseable for unavailable/offline */
 			err = SCSI_DH_DEV_OFFLINED;

@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED &&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->state == TPGS_STATE_TRANSITIONING)

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device *sdev, struct request *req) · line 713

Quoted 10 lines

drivers/scsi/device_handler/scsi_dh_alua.c +3 -1

+		ret = BLKPREP_DEFER;
+	else if (h->state != TPGS_STATE_OPTIMIZED &&
+		 h->state != TPGS_STATE_NONOPTIMIZED) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}
 	return ret;
-
 }

Makes sense to me..

Quoted 2 lines

Acked-by Nicholas A. Bellinger <[email protected]>

Not so fast. There are two problems with this approach:

The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning' only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer', too.

Secondly this path fails with 'directio' multipath checker. Remember that 'directio' is using 'fs' requests, not block-pc ones. Hence for all I/O the prep_fn() callback is evaluated, which will return 'DEFER' here once the path is in transitioning. And the state is never updated as RTPG is never called.

I'm currently preparing a patch which addressed these situations, too.

Cheers,

Hannes

Signature · 8 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mike Snitzer

author

2010-08-31 15:11 UTC #3 lore raw

Quoted 47 lines · Hannes Reinecke

Quoted 46 lines · Nicholas A. Bellinger
Quoted 32 lines · Mike Snitzer
Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.
These gaps were identified during controller failover testing of an ALUA array.
Signed-off-by Mike Snitzer <[email protected]>
---
 drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +4 -2
index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		    h->state == TPGS_STATE_STANDBY)
 			/* Useable path if active */
 			err = SCSI_DH_OK;
+		else if (h->state == TPGS_STATE_TRANSITIONING)
+			/* State transition, retry */
+			goto retry;
 		else
 			/* Path unuseable for unavailable/offline */
 			err = SCSI_DH_DEV_OFFLINED;
@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED &&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->state == TPGS_STATE_TRANSITIONING)
drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device *sdev, struct request *req) · line 713
Quoted 10 lines
drivers/scsi/device_handler/scsi_dh_alua.c +3 -1
+		ret = BLKPREP_DEFER;
+	else if (h->state != TPGS_STATE_OPTIMIZED &&
+		 h->state != TPGS_STATE_NONOPTIMIZED) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}
 	return ret;
-
 }
 
Makes sense to me..

Quoted 7 lines

Quoted 2 lines
Acked-by Nicholas A. Bellinger <[email protected]>
Not so fast. There are two problems with this approach:
The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning' only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer', too.

And what is the problem with that? The IO will eventually time out.

Quoted 4 lines

Secondly this path fails with 'directio' multipath checker. Remember that 'directio' is using 'fs' requests, not block-pc ones. Hence for all I/O the prep_fn() callback is evaluated, which will return 'DEFER' here once the path is in transitioning. And the state is never updated as RTPG is never called.

Testing ALUA with directio path checker did not result in such immutable state in the few instances that TPGS_STATE_TRANSITIONING was seen in alua_prep_fn.

Quoted 1 line

I'm currently preparing a patch which addressed these situations, too.

OK, please share.

Thanks,
Mike

Mike Snitzer

author

2010-09-20 15:35 UTC #4 lore raw

Hi Hannes,

Quoted 52 lines · Mike Snitzer

Quoted 49 lines · Hannes Reinecke
Quoted 48 lines · Nicholas A. Bellinger
Quoted 32 lines · Mike Snitzer
Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.
These gaps were identified during controller failover testing of an ALUA array.
Signed-off-by Mike Snitzer <[email protected]>
---
 drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +4 -2
index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		    h->state == TPGS_STATE_STANDBY)
 			/* Useable path if active */
 			err = SCSI_DH_OK;
+		else if (h->state == TPGS_STATE_TRANSITIONING)
+			/* State transition, retry */
+			goto retry;
 		else
 			/* Path unuseable for unavailable/offline */
 			err = SCSI_DH_DEV_OFFLINED;
@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED &&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->state == TPGS_STATE_TRANSITIONING)
drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device *sdev, struct request *req) · line 713
Quoted 10 lines
drivers/scsi/device_handler/scsi_dh_alua.c +3 -1
+		ret = BLKPREP_DEFER;
+	else if (h->state != TPGS_STATE_OPTIMIZED &&
+		 h->state != TPGS_STATE_NONOPTIMIZED) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}
 	return ret;
-
 }
 
Makes sense to me..
Acked-by Nicholas A. Bellinger <[email protected]>

Quoted 7 lines

Quoted 5 lines
Not so fast. There are two problems with this approach:
The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning' only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer', too.
And what is the problem with that? The IO will eventually time out.

To restate as a question: even though we'll retry in alua_rtpg(); shouldn't the SCSI command eventually time out (via scsi_attempt_requeue_command)?

Note that my proposed change to alua_rtpg() just adds a TPGS_STATE_TRANSITIONING handler for implicit ALUA -- explicit ALUA already has such a handler.

Allowing implicit ALUA to fall through (as we do currently) causes a return alua_rtpg() of SCSI_DH_DEV_OFFLINED -- which isn't the correct state.

Quoted 8 lines

Quoted 4 lines
Secondly this path fails with 'directio' multipath checker. Remember that 'directio' is using 'fs' requests, not block-pc ones. Hence for all I/O the prep_fn() callback is evaluated, which will return 'DEFER' here once the path is in transitioning. And the state is never updated as RTPG is never called.
Testing ALUA with directio path checker did not result in such immutable state in the few instances that TPGS_STATE_TRANSITIONING was seen in alua_prep_fn.

I had another look and I see what you're saying. Thanks for catching this!

Quoted 3 lines

Quoted 1 line
I'm currently preparing a patch which addressed these situations, too.
OK, please share.

Do you have that patch you were preparing? I look forward to seeing your solution to this.

Thanks,
Mike

Mike Christie

2010-09-21 02:21 UTC #5 lore raw

Quoted 63 lines · Mike Snitzer

Hi Hannes,
Quoted 58 lines · Mike Snitzer
Quoted 54 lines · Hannes Reinecke
Quoted 46 lines · Nicholas A. Bellinger
Quoted 32 lines · Mike Snitzer
Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.
These gaps were identified during controller failover testing of an ALUA array.
Signed-off-by Mike Snitzer<[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
  1 files changed, 7 insertions(+), 3 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +4 -2
index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
  		    h->state == TPGS_STATE_STANDBY)
  			/* Useable path if active */
  			err = SCSI_DH_OK;
+		else if (h->state == TPGS_STATE_TRANSITIONING)
+			/* State transition, retry */
+			goto retry;
  		else
  			/* Path unuseable for unavailable/offline */
  			err = SCSI_DH_DEV_OFFLINED;
@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
  	struct alua_dh_data *h = get_alua_data(sdev);
  	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED&&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->state == TPGS_STATE_TRANSITIONING)
drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device *sdev, struct request *req) · line 713
Quoted 10 lines
drivers/scsi/device_handler/scsi_dh_alua.c +3 -1
+		ret = BLKPREP_DEFER;
+	else if (h->state != TPGS_STATE_OPTIMIZED&&
+		 h->state != TPGS_STATE_NONOPTIMIZED) {
  		ret = BLKPREP_KILL;
  		req->cmd_flags |= REQ_QUIET;
  	}
  	return ret;
-
  }
 
Makes sense to me..
Quoted 2 lines
Acked-by Nicholas A. Bellinger<[email protected]>
Not so fast. There are two problems with this approach:
The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning' only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer', too.

Quoted 5 lines

Quoted 1 line
And what is the problem with that? The IO will eventually time out.
To restate as a question: even though we'll retry in alua_rtpg(); shouldn't the SCSI command eventually time out (via scsi_attempt_requeue_command)?

That function is only in RHEL. Requests that are prepd and sent to the scsi layer and driver would eventually timeout in scsi_softirq_done in upstream.

alua_prep_fn prevents the IO from getting sent to the scsi layer so we do not hit the check in scsi_softirq_done though.

Mike Christie

2010-09-21 02:23 UTC #6 lore raw

Quoted 71 lines · Mike Christie

Quoted 70 lines · Mike Snitzer
Hi Hannes,
Quoted 64 lines · Mike Snitzer
Quoted 52 lines · Hannes Reinecke
Quoted 49 lines · Nicholas A. Bellinger
Quoted 35 lines · Mike Snitzer
Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.
These gaps were identified during controller failover testing of an ALUA array.
Signed-off-by Mike Snitzer<[email protected]>
---
drivers/scsi/device_handler/scsi_dh_alua.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +3 -0
index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev,
 struct alua_dh_data *h)
 h->state == TPGS_STATE_STANDBY)
 /* Useable path if active */
 err = SCSI_DH_OK;
+ else if (h->state == TPGS_STATE_TRANSITIONING)
+ /* State transition, retry */
+ goto retry;
else
/* Path unuseable for unavailable/offline */
err = SCSI_DH_DEV_OFFLINED;
drivers/scsi/device_handler/scsi_dh_alua.c +1 -2
@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device
 *sdev, struct request *req)
 struct alua_dh_data *h = get_alua_data(sdev);
 int ret = BLKPREP_OK;
 
- if (h->state != TPGS_STATE_OPTIMIZED&&
- h->state != TPGS_STATE_NONOPTIMIZED) {
+ if (h->state == TPGS_STATE_TRANSITIONING)
drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device · line 714
Quoted 10 lines
drivers/scsi/device_handler/scsi_dh_alua.c +3 -1
+ ret = BLKPREP_DEFER;
+ else if (h->state != TPGS_STATE_OPTIMIZED&&
+ h->state != TPGS_STATE_NONOPTIMIZED) {
 ret = BLKPREP_KILL;
 req->cmd_flags |= REQ_QUIET;
 }
 return ret;
-
 }
 
Makes sense to me..
Quoted 2 lines
Acked-by Nicholas A. Bellinger<[email protected]>
Quoted 7 lines
Not so fast. There are two problems with this approach:
The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning'
only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer',
too.
And what is the problem with that? The IO will eventually time out.

Quoted 7 lines

Quoted 3 lines
To restate as a question: even though we'll retry in alua_rtpg(); shouldn't the SCSI command eventually time out (via scsi_attempt_requeue_command)?
That function is only in RHEL. Requests that are prepd and sent to the scsi layer and driver would eventually timeout in scsi_softirq_done in upstream.

Did we want to move this check to the block layer btw? If so it could solve the problem.

Mike Snitzer

author

2010-09-21 19:33 UTC #7 lore raw

In reply to Mike Christie

Quoted 70 lines · Mike Christie

Quoted 63 lines · Mike Snitzer
Hi Hannes,
Quoted 58 lines · Mike Snitzer
Quoted 54 lines · Hannes Reinecke
Quoted 46 lines · Nicholas A. Bellinger
Quoted 32 lines · Mike Snitzer
Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.
These gaps were identified during controller failover testing of an ALUA array.
Signed-off-by Mike Snitzer<[email protected]>
---
 drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +4 -2
index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 		    h->state == TPGS_STATE_STANDBY)
 			/* Useable path if active */
 			err = SCSI_DH_OK;
+		else if (h->state == TPGS_STATE_TRANSITIONING)
+			/* State transition, retry */
+			goto retry;
 		else
 			/* Path unuseable for unavailable/offline */
 			err = SCSI_DH_DEV_OFFLINED;
@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED&&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->state == TPGS_STATE_TRANSITIONING)
drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device *sdev, struct request *req) · line 713
Quoted 10 lines
drivers/scsi/device_handler/scsi_dh_alua.c +3 -1
+		ret = BLKPREP_DEFER;
+	else if (h->state != TPGS_STATE_OPTIMIZED&&
+		 h->state != TPGS_STATE_NONOPTIMIZED) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}
 	return ret;
-
 }
 
Makes sense to me..
Quoted 2 lines
Acked-by Nicholas A. Bellinger<[email protected]>
Not so fast. There are two problems with this approach:
The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning' only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer', too.
Quoted 5 lines
Quoted 1 line
And what is the problem with that? The IO will eventually time out.
To restate as a question: even though we'll retry in alua_rtpg(); shouldn't the SCSI command eventually time out (via scsi_attempt_requeue_command)?

Quoted 6 lines

That function is only in RHEL. Requests that are prepd and sent to the scsi layer and driver would eventually timeout in scsi_softirq_done in upstream.
alua_prep_fn prevents the IO from getting sent to the scsi layer so we do not hit the check in scsi_softirq_done though.

That is only the case if alua_prep_fn were to return BLKPREP_DEFER right?

Taking a step back:
1) the proposed alua_prep_fn BLKPREP_DEFER change is not correct and is no longer being proposed...

2) the patch also modified alua_rtpg() so implicit ALUA would retry (just like explicit ALUA currently does) if TPGS_STATE_TRANSITIONING
- so why should we avoid retry for implicit but do it for explicit?

Mike Christie

2010-09-21 21:08 UTC #8 lore raw

Quoted 77 lines · Mike Snitzer

Quoted 74 lines · Mike Christie
Quoted 63 lines · Mike Snitzer
Hi Hannes,
Quoted 58 lines · Mike Snitzer
Quoted 54 lines · Hannes Reinecke
Quoted 46 lines · Nicholas A. Bellinger
Quoted 32 lines · Mike Snitzer
Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.
These gaps were identified during controller failover testing of an ALUA array.
Signed-off-by Mike Snitzer<[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
  1 files changed, 7 insertions(+), 3 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +4 -2
index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
  		    h->state == TPGS_STATE_STANDBY)
  			/* Useable path if active */
  			err = SCSI_DH_OK;
+		else if (h->state == TPGS_STATE_TRANSITIONING)
+			/* State transition, retry */
+			goto retry;
  		else
  			/* Path unuseable for unavailable/offline */
  			err = SCSI_DH_DEV_OFFLINED;
@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
  	struct alua_dh_data *h = get_alua_data(sdev);
  	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED&&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->state == TPGS_STATE_TRANSITIONING)
drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device *sdev, struct request *req) · line 713
Quoted 10 lines
drivers/scsi/device_handler/scsi_dh_alua.c +3 -1
+		ret = BLKPREP_DEFER;
+	else if (h->state != TPGS_STATE_OPTIMIZED&&
+		 h->state != TPGS_STATE_NONOPTIMIZED) {
  		ret = BLKPREP_KILL;
  		req->cmd_flags |= REQ_QUIET;
  	}
  	return ret;
-
  }
 
Makes sense to me..
Quoted 2 lines
Acked-by Nicholas A. Bellinger<[email protected]>
Not so fast. There are two problems with this approach:
The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning' only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer', too.
Quoted 5 lines
Quoted 1 line
And what is the problem with that? The IO will eventually time out.
To restate as a question: even though we'll retry in alua_rtpg(); shouldn't the SCSI command eventually time out (via scsi_attempt_requeue_command)?
That function is only in RHEL. Requests that are prepd and sent to the scsi layer and driver would eventually timeout in scsi_softirq_done in upstream.

Quoted 5 lines

Quoted 2 lines
alua_prep_fn prevents the IO from getting sent to the scsi layer so we do not hit the check in scsi_softirq_done though.
That is only the case if alua_prep_fn were to return BLKPREP_DEFER right?

Yeah. I misread the email above. Just to clarify..

For the alua case, we will always retry due the prep_fn issue I mentioned.

For the alua_rtpg() retry case you were discussing we will also always retry, because the timer check in scsi_attempt_requeue_command/scsi_softirq_done will break us from retrying forever in that execution of the request started by the submit_rtpg call. However, the added goto retry will would just end up starting a another execution. To handle Hannes comment I think you would want to add some retry/timer checks in alua_rtpg to prevent that from retrying forever.

Quoted 4 lines

2) the patch also modified alua_rtpg() so implicit ALUA would retry (just like explicit ALUA currently does) if TPGS_STATE_TRANSITIONING
- so why should we avoid retry for implicit but do it for explicit?

Leaving that for Hannes. I cannot think of a reason. Probably just did not do it.

Hannes Reinecke

2010-09-22 10:13 UTC #9 lore raw

Quoted 103 lines · Mike Christie

Quoted 82 lines · Mike Snitzer
Quoted 79 lines · Mike Christie
Quoted 68 lines · Mike Snitzer
Hi Hannes,
Quoted 63 lines · Mike Snitzer
Quoted 59 lines · Hannes Reinecke
Quoted 49 lines · Nicholas A. Bellinger
Quoted 35 lines · Mike Snitzer
Handle transitioning in the prep_fn.
Handle transitioning in alua_rtpg's implicit alua code too.
These gaps were identified during controller failover testing of an ALUA array.
Signed-off-by Mike Snitzer<[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_alua.c |   10 +++++++---
  1 files changed, 7 insertions(+), 3 deletions(-)
drivers/scsi/device_handler/scsi_dh_alua.c +3 -0
index 1a970a7..c1eedc5 100644
@@ -616,6 +616,9 @@ static int alua_rtpg(struct scsi_device
 *sdev, struct alua_dh_data *h)
              h->state == TPGS_STATE_STANDBY)
              /* Useable path if active */
              err = SCSI_DH_OK;
+        else if (h->state == TPGS_STATE_TRANSITIONING)
+            /* State transition, retry */
+            goto retry;
          else
              /* Path unuseable for unavailable/offline */
              err = SCSI_DH_DEV_OFFLINED;
drivers/scsi/device_handler/scsi_dh_alua.c +1 -2
@@ -698,13 +701,14 @@ static int alua_prep_fn(struct scsi_device
 *sdev, struct request *req)
      struct alua_dh_data *h = get_alua_data(sdev);
      int ret = BLKPREP_OK;
 
-    if (h->state != TPGS_STATE_OPTIMIZED&&
-        h->state != TPGS_STATE_NONOPTIMIZED) {
+    if (h->state == TPGS_STATE_TRANSITIONING)
drivers/scsi/device_handler/scsi_dh_alua.c static int alua_prep_fn(struct scsi_device · line 714
Quoted 10 lines
drivers/scsi/device_handler/scsi_dh_alua.c +3 -1
+        ret = BLKPREP_DEFER;
+    else if (h->state != TPGS_STATE_OPTIMIZED&&
+         h->state != TPGS_STATE_NONOPTIMIZED) {
          ret = BLKPREP_KILL;
          req->cmd_flags |= REQ_QUIET;
      }
      return ret;
-
  }
 
Makes sense to me..
Quoted 2 lines
Acked-by Nicholas A. Bellinger<[email protected]>
Not so fast. There are two problems with this approach:
The path is retried indefinitely. Arrays are _supposed_ to be in 'transitioning'
only temporary; however, if the array is stuck due to a fw error we're stuck in 'defer',
too.
Quoted 5 lines
Quoted 1 line
And what is the problem with that? The IO will eventually time out.
To restate as a question: even though we'll retry in alua_rtpg(); shouldn't the SCSI command eventually time out (via scsi_attempt_requeue_command)?
That function is only in RHEL. Requests that are prepd and sent to the scsi layer and driver would eventually timeout in scsi_softirq_done in upstream.
Quoted 5 lines
Quoted 2 lines
alua_prep_fn prevents the IO from getting sent to the scsi layer so we do not hit the check in scsi_softirq_done though.
That is only the case if alua_prep_fn were to return BLKPREP_DEFER right?
Yeah. I misread the email above. Just to clarify..
For the alua case, we will always retry due the prep_fn issue I mentioned.
For the alua_rtpg() retry case you were discussing we will also always retry, because the timer check in scsi_attempt_requeue_command/scsi_softirq_done will break us from retrying forever in that execution of the request started by the submit_rtpg call. However, the added goto retry will would just end up starting a another execution. To handle Hannes comment I think you would want to add some retry/timer checks in alua_rtpg to prevent that from retrying forever.
Quoted 1 line

Quoted 6 lines

Quoted 3 lines
2) the patch also modified alua_rtpg() so implicit ALUA would retry (just like explicit ALUA currently does) if TPGS_STATE_TRANSITIONING
- so why should we avoid retry for implicit but do it for explicit?
Leaving that for Hannes. I cannot think of a reason. Probably just did not do it.

Finally I got around to answering this.

I've attached a patch which I made the other day which seems to work reasonably well.
Looks better from my side, so if you agree I'll be sending it upstream properly.

Cheers,

Hannes

Signature · 4 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Quoted 1 line

From d3f02c90db3e3177309b78726d082e17dd772ee2 Mon Sep 17 00:00:00 2001

From: Hannes Reinecke <[email protected]>
Date: Wed, 22 Sep 2010 12:09:07 +0200
Subject: [PATCH] scsi_dh_alua: Handle all states correctly

For ALUA we should be handling all states, independent of whether is explicit or implicit. For 'Transitioning' we should be retry for a certain amount of time; after that an error should be returned.

Signed-off-by Hannes Reinecke <[email protected]>

drivers/scsi/device_handler/scsi_dh_alua.c +34 ~~-25~~

index 1a970a7..c6f57e3 100644
@@ -31,6 +31,7 @@
 #define TPGS_STATE_NONOPTIMIZED		0x1
 #define TPGS_STATE_STANDBY		0x2
 #define TPGS_STATE_UNAVAILABLE		0x3
+#define TPGS_STATE_LBA_DEPENDENT	0x4
 #define TPGS_STATE_OFFLINE		0xe
 #define TPGS_STATE_TRANSITIONING	0xf

@@ -39,6 +40,7 @@
 #define TPGS_SUPPORT_NONOPTIMIZED	0x02
 #define TPGS_SUPPORT_STANDBY		0x04
 #define TPGS_SUPPORT_UNAVAILABLE	0x08
+#define TPGS_SUPPORT_LBA_DEPENDENT	0x10
 #define TPGS_SUPPORT_OFFLINE		0x40
 #define TPGS_SUPPORT_TRANSITION		0x80

@@ -460,6 +462,8 @@ static char print_alua_state(int state)
 		return 'S';
 	case TPGS_STATE_UNAVAILABLE:
 		return 'U';
+	case TPGS_STATE_LBA_DEPENDENT:
+		return 'L';
 	case TPGS_STATE_OFFLINE:
 		return 'O';
 	case TPGS_STATE_TRANSITIONING:

@@ -542,7 +546,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	int len, k, off, valid_states = 0;
 	char *ucp;
 	unsigned err;
+	unsigned long expiry, interval = 10;
 
+	expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT);
  retry:
 	err = submit_rtpg(sdev, h);

@@ -553,7 +559,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 			return SCSI_DH_IO;
 
 		err = alua_check_sense(sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE)
+		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
 			goto retry;
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: rtpg sense code %02x/%02x/%02x\n",

@@ -587,38 +593,36 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	}
 
 	sdev_printk(KERN_INFO, sdev,
-		    "%s: port group %02x state %c supports %c%c%c%c%c%c\n",
+		    "%s: port group %02x state %c supports %c%c%c%c%c%c%c\n",
 		    ALUA_DH_NAME, h->group_id, print_alua_state(h->state),
 		    valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
 		    valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
+		    valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
 		    valid_states&TPGS_SUPPORT_UNAVAILABLE?'U':'u',
 		    valid_states&TPGS_SUPPORT_STANDBY?'S':'s',
 		    valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
 		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
 
-	if (h->tpgs & TPGS_MODE_EXPLICIT) {
-		switch (h->state) {
-		case TPGS_STATE_TRANSITIONING:
+	switch (h->state) {
+	case TPGS_STATE_TRANSITIONING:
+		if (time_before(jiffies, expiry)) {
 			/* State transition, retry */
+			interval *= 10;
+			msleep(interval);
 			goto retry;
-			break;
-		case TPGS_STATE_OFFLINE:
-			/* Path is offline, fail */
-			err = SCSI_DH_DEV_OFFLINED;
-			break;
-		default:
-			break;
 		}
-	} else {
-		/* Only Implicit ALUA support */
-		if (h->state == TPGS_STATE_OPTIMIZED ||
-		    h->state == TPGS_STATE_NONOPTIMIZED ||
-		    h->state == TPGS_STATE_STANDBY)
-			/* Useable path if active */
-			err = SCSI_DH_OK;
-		else
-			/* Path unuseable for unavailable/offline */
-			err = SCSI_DH_DEV_OFFLINED;
+		/* Transitioning time exceeded */
+		err = SCSI_DH_RETRY;
+		break;
+	case TPGS_STATE_OFFLINE:
+	case TPGS_STATE_UNAVAILABLE:
+		/* Path unuseable for unavailable/offline */
+		err = SCSI_DH_DEV_OFFLINED;
+		break;
+	default:
+		/* Useable path if active */
+		err = SCSI_DH_OK;
+		break;
 	}
 	return err;
 }

@@ -672,7 +676,9 @@ static int alua_activate(struct scsi_device *sdev,
 			goto out;
 	}
 
-	if (h->tpgs & TPGS_MODE_EXPLICIT && h->state != TPGS_STATE_OPTIMIZED) {
+	if (h->tpgs & TPGS_MODE_EXPLICIT &&
+	    h->state != TPGS_STATE_OPTIMIZED &&
+	    h->state != TPGS_STATE_LBA_DEPENDENT) {
 		h->callback_fn = fn;
 		h->callback_data = data;
 		err = submit_stpg(h);

@@ -698,8 +704,11 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED &&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->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) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}

Mike Snitzer

author

2010-09-22 12:29 UTC #10 lore raw

Quoted 9 lines · Hannes Reinecke

Quoted 7 lines · Mike Christie
Quoted 3 lines · Mike Snitzer
2) the patch also modified alua_rtpg() so implicit ALUA would retry (just like explicit ALUA currently does) if TPGS_STATE_TRANSITIONING
- so why should we avoid retry for implicit but do it for explicit?
Leaving that for Hannes. I cannot think of a reason. Probably just did not do it.

Quoted 6 lines

Finally I got around to answering this.
I've attached a patch which I made the other day which seems to work reasonably well.
Looks better from my side, so if you agree I'll be sending it upstream properly.

Looks good for covering this "2)" change above.

But "1)" change you had concern with (in my previous patch) was that alua_prep_fn would return BLKPREP_DEFER if TPGS_STATE_TRANSITIONING. This was bad in that FS requests (like the directio path checker) would always call ->prep_fn and that if TPGS_STATE_TRANSITIONING the RTPG state would never be reevaluated.. leaving us stuck in BLKPREP_DEFER.

Seems I'm missing a new flow that proves this is no longer a concern.

Please advise, thanks.
Mike

Quoted 16 lines

From d3f02c90db3e3177309b78726d082e17dd772ee2 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <[email protected]>
Date: Wed, 22 Sep 2010 12:09:07 +0200
Subject: [PATCH] scsi_dh_alua: Handle all states correctly
For ALUA we should be handling all states, independent of whether is explicit or implicit. For 'Transitioning' we should be retry for a certain amount of time; after that an error should be returned.
Signed-off-by Hannes Reinecke <[email protected]>
drivers/scsi/device_handler/scsi_dh_alua.c +0 -0
index 1a970a7..c6f57e3 100644

...

Quoted 14 lines

drivers/scsi/device_handler/scsi_dh_alua.c +5 -2

@@ -698,8 +704,11 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED &&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->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) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}

Hannes Reinecke

2010-09-23 07:15 UTC #11 lore raw

Quoted 20 lines · Mike Snitzer

Quoted 5 lines · Hannes Reinecke
Quoted 4 lines · Mike Christie
Quoted 3 lines · Mike Snitzer
2) the patch also modified alua_rtpg() so implicit ALUA would retry (just like explicit ALUA currently does) if TPGS_STATE_TRANSITIONING
- so why should we avoid retry for implicit but do it for explicit?
Quoted 8 lines
Quoted 2 lines
Leaving that for Hannes. I cannot think of a reason. Probably just did not do it.
Finally I got around to answering this.
I've attached a patch which I made the other day which seems to work reasonably well.
Looks better from my side, so if you agree I'll be sending it upstream properly.
Looks good for covering this "2)" change above.

Quoted 8 lines

But "1)" change you had concern with (in my previous patch) was that alua_prep_fn would return BLKPREP_DEFER if TPGS_STATE_TRANSITIONING. This was bad in that FS requests (like the directio path checker) would always call ->prep_fn and that if TPGS_STATE_TRANSITIONING the RTPG state would never be reevaluated.. leaving us stuck in BLKPREP_DEFER.
Seems I'm missing a new flow that proves this is no longer a concern.

The device_handler will keep retrying any path in 'transitioning'. After ALUA_FAILOVER_TIMEOUT the device handler will either have updated the port state to something other than 'transitioning' or returned with 'SCSI_DH_RETRY'.
But yes, you are right; we should be setting the port to something else than 'transitioning' then. Probably 'standby' is a good choice here.
New patch attached.

Thanks for the review.

Cheers,

Hannes

Signature · 4 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Quoted 1 line

From db02ba3c33a4a1dbfcdc4de1b73e378f7b3d8925 Mon Sep 17 00:00:00 2001

From: Hannes Reinecke <[email protected]>
Date: Wed, 22 Sep 2010 12:09:07 +0200
Subject: [PATCH] scsi_dh_alua: Handle all states correctly

For ALUA we should be handling all states, independent of whether is explicit or implicit. For 'Transitioning' we should be retry for a certain amount of time; after that we're setting the port to 'Standby' and return SCSI_DH_RETRY to signal upper layers a retry is in order here.

Signed-off-by Hannes Reinecke <[email protected]>

drivers/scsi/device_handler/scsi_dh_alua.c +35 ~~-25~~

index 1a970a7..c7e11ed 100644
@@ -31,6 +31,7 @@
 #define TPGS_STATE_NONOPTIMIZED		0x1
 #define TPGS_STATE_STANDBY		0x2
 #define TPGS_STATE_UNAVAILABLE		0x3
+#define TPGS_STATE_LBA_DEPENDENT	0x4
 #define TPGS_STATE_OFFLINE		0xe
 #define TPGS_STATE_TRANSITIONING	0xf

@@ -39,6 +40,7 @@
 #define TPGS_SUPPORT_NONOPTIMIZED	0x02
 #define TPGS_SUPPORT_STANDBY		0x04
 #define TPGS_SUPPORT_UNAVAILABLE	0x08
+#define TPGS_SUPPORT_LBA_DEPENDENT	0x10
 #define TPGS_SUPPORT_OFFLINE		0x40
 #define TPGS_SUPPORT_TRANSITION		0x80

@@ -460,6 +462,8 @@ static char print_alua_state(int state)
 		return 'S';
 	case TPGS_STATE_UNAVAILABLE:
 		return 'U';
+	case TPGS_STATE_LBA_DEPENDENT:
+		return 'L';
 	case TPGS_STATE_OFFLINE:
 		return 'O';
 	case TPGS_STATE_TRANSITIONING:

@@ -542,7 +546,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	int len, k, off, valid_states = 0;
 	char *ucp;
 	unsigned err;
+	unsigned long expiry, interval = 10;
 
+	expiry = round_jiffies_up(jiffies + ALUA_FAILOVER_TIMEOUT);
  retry:
 	err = submit_rtpg(sdev, h);

@@ -553,7 +559,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 			return SCSI_DH_IO;
 
 		err = alua_check_sense(sdev, &sense_hdr);
-		if (err == ADD_TO_MLQUEUE)
+		if (err == ADD_TO_MLQUEUE && time_before(jiffies, expiry))
 			goto retry;
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: rtpg sense code %02x/%02x/%02x\n",

@@ -587,38 +593,37 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_dh_data *h)
 	}
 
 	sdev_printk(KERN_INFO, sdev,
-		    "%s: port group %02x state %c supports %c%c%c%c%c%c\n",
+		    "%s: port group %02x state %c supports %c%c%c%c%c%c%c\n",
 		    ALUA_DH_NAME, h->group_id, print_alua_state(h->state),
 		    valid_states&TPGS_SUPPORT_TRANSITION?'T':'t',
 		    valid_states&TPGS_SUPPORT_OFFLINE?'O':'o',
+		    valid_states&TPGS_SUPPORT_LBA_DEPENDENT?'L':'l',
 		    valid_states&TPGS_SUPPORT_UNAVAILABLE?'U':'u',
 		    valid_states&TPGS_SUPPORT_STANDBY?'S':'s',
 		    valid_states&TPGS_SUPPORT_NONOPTIMIZED?'N':'n',
 		    valid_states&TPGS_SUPPORT_OPTIMIZED?'A':'a');
 
-	if (h->tpgs & TPGS_MODE_EXPLICIT) {
-		switch (h->state) {
-		case TPGS_STATE_TRANSITIONING:
+	switch (h->state) {
+	case TPGS_STATE_TRANSITIONING:
+		if (time_before(jiffies, expiry)) {
 			/* State transition, retry */
+			interval *= 10;
+			msleep(interval);
 			goto retry;
-			break;
-		case TPGS_STATE_OFFLINE:
-			/* Path is offline, fail */
-			err = SCSI_DH_DEV_OFFLINED;
-			break;
-		default:
-			break;
 		}
-	} else {
-		/* Only Implicit ALUA support */
-		if (h->state == TPGS_STATE_OPTIMIZED ||
-		    h->state == TPGS_STATE_NONOPTIMIZED ||
-		    h->state == TPGS_STATE_STANDBY)
-			/* Useable path if active */
-			err = SCSI_DH_OK;
-		else
-			/* Path unuseable for unavailable/offline */
-			err = SCSI_DH_DEV_OFFLINED;
+		/* Transitioning time exceeded, set port to standby */
+		err = SCSI_DH_RETRY;
+		h->state = TPGS_STATE_STANDBY;
+		break;
+	case TPGS_STATE_OFFLINE:
+	case TPGS_STATE_UNAVAILABLE:
+		/* Path unuseable for unavailable/offline */
+		err = SCSI_DH_DEV_OFFLINED;
+		break;
+	default:
+		/* Useable path if active */
+		err = SCSI_DH_OK;
+		break;
 	}
 	return err;
 }

@@ -672,7 +677,9 @@ static int alua_activate(struct scsi_device *sdev,
 			goto out;
 	}
 
-	if (h->tpgs & TPGS_MODE_EXPLICIT && h->state != TPGS_STATE_OPTIMIZED) {
+	if (h->tpgs & TPGS_MODE_EXPLICIT &&
+	    h->state != TPGS_STATE_OPTIMIZED &&
+	    h->state != TPGS_STATE_LBA_DEPENDENT) {
 		h->callback_fn = fn;
 		h->callback_data = data;
 		err = submit_stpg(h);

@@ -698,8 +705,11 @@ static int alua_prep_fn(struct scsi_device *sdev, struct request *req)
 	struct alua_dh_data *h = get_alua_data(sdev);
 	int ret = BLKPREP_OK;
 
-	if (h->state != TPGS_STATE_OPTIMIZED &&
-	    h->state != TPGS_STATE_NONOPTIMIZED) {
+	if (h->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) {
 		ret = BLKPREP_KILL;
 		req->cmd_flags |= REQ_QUIET;
 	}

Mike Snitzer

author Acked-by

2010-09-23 13:44 UTC #12 lore raw

Quoted 9 lines · Hannes Reinecke

The device_handler will keep retrying any path in 'transitioning'. After ALUA_FAILOVER_TIMEOUT the device handler will either have updated the port state to something other than 'transitioning' or returned with 'SCSI_DH_RETRY'.
But yes, you are right; we should be setting the port to something else than 'transitioning' then. Probably 'standby' is a good choice here.
New patch attached.

Quoted 23 lines

Thanks for the review.

Cheers,

Hannes

Signature · 17 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

From db02ba3c33a4a1dbfcdc4de1b73e378f7b3d8925 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <[email protected]>
Date: Wed, 22 Sep 2010 12:09:07 +0200
Subject: [PATCH] scsi_dh_alua: Handle all states correctly

For ALUA we should be handling all states, independent of whether
is explicit or implicit. For 'Transitioning' we should be retry
for a certain amount of time; after that we're setting the port
to 'Standby' and return SCSI_DH_RETRY to signal upper layers
a retry is in order here.

Signed-off-by: Hannes Reinecke <[email protected]>

Acked-by Mike Snitzer <[email protected]>

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mike Snitzer

author

2010-09-23 18:53 UTC #13 lore raw

Quoted 11 lines · Mike Snitzer

Quoted 11 lines
From db02ba3c33a4a1dbfcdc4de1b73e378f7b3d8925 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <[email protected]>
Date: Wed, 22 Sep 2010 12:09:07 +0200
Subject: [PATCH] scsi_dh_alua: Handle all states correctly
For ALUA we should be handling all states, independent of whether is explicit or implicit. For 'Transitioning' we should be retry for a certain amount of time; after that we're setting the port to 'Standby' and return SCSI_DH_RETRY to signal upper layers a retry is in order here.

Quoted 3 lines

Quoted 1 line
Signed-off-by Hannes Reinecke <[email protected]>
Acked-by Mike Snitzer <[email protected]>

BTW, we're missing the include for the msleep() introduced in this patch:

#include <linux/delay.h>

James, any chance you'd add that?

Or should Hannes send a revised patch?

Mike