[PATCH 0/3] failover fixes for scsi_dh_alua

[PATCH 0/3] failover fixes for scsi_dh_alua

[Martin Wilck]

Here are 3 fixes we came up with at SUSE to fix problems we encountered
in multipath failover tests. Feedback welcome.

Best regards,
Martin

Hannes Reinecke (2):
  scsi_dh_alua: Do not modify the interval value for retries
  scsi_dh_alua: Do not retry for unmapped device

Martin Wilck (1):
  scsi_dh_alua: do not call BUG_ON when updating port group

 drivers/scsi/device_handler/scsi_dh_alua.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

-- 
2.12.2

[PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries

[Martin Wilck]

From: Hannes Reinecke <hare@suse.de>

We shouldn't modify the interval value, as the struct is accessed
from different devices and hence we might end up scheduling too
early.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index c01b47e5b55a..b90a5dec199f 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -681,7 +681,6 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	case SCSI_ACCESS_STATE_TRANSITIONING:
 		if (time_before(jiffies, pg->expiry)) {
 			/* State transition, retry */
-			pg->interval = 2;
 			err = SCSI_DH_RETRY;
 		} else {
 			struct alua_dh_data *h;
@@ -836,11 +835,9 @@ static void alua_rtpg_work(struct work_struct *work)
 		spin_lock_irqsave(&pg->lock, flags);
 		if (err == SCSI_DH_RETRY || pg->flags & ALUA_PG_RUN_RTPG) {
 			pg->flags |= ALUA_PG_RUN_RTPG;
-			pg->interval = 0;
 			pg->flags &= ~ALUA_PG_RUNNING;
 			spin_unlock_irqrestore(&pg->lock, flags);
-			queue_delayed_work(alua_wq, &pg->rtpg_work,
-					   pg->interval * HZ);
+			queue_delayed_work(alua_wq, &pg->rtpg_work, 0);
 			return;
 		}
 	}
@@ -886,7 +883,7 @@ static bool alua_rtpg_queue(struct alua_port_group *pg,
 		force = true;
 	}
 	if (pg->rtpg_sdev == NULL) {
-		pg->interval = 0;
+		pg->interval = 2;
 		pg->flags |= ALUA_PG_RUN_RTPG;
 		kref_get(&pg->kref);
 		pg->rtpg_sdev = sdev;
-- 
2.12.2

[PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device

[Martin Wilck]

From: Hannes Reinecke <hare@suse.de>

If a device becomes unmapped on the target we should be returning
SCSI_DH_DEV_OFFLINED.

Signed-off-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index b90a5dec199f..501855bde633 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -522,7 +522,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	struct alua_port_group *tmp_pg;
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
 	unsigned char *desc, *buff;
-	unsigned err, retval;
+	unsigned err = SCSI_DH_OK, retval;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 	unsigned long flags;
@@ -541,7 +541,6 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		return SCSI_DH_DEV_TEMP_BUSY;
 
  retry:
-	err = 0;
 	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
 
 	if (retval) {
@@ -569,6 +568,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			pg->flags |= ALUA_RTPG_EXT_HDR_UNSUPP;
 			goto retry;
 		}
+		err = SCSI_DH_IO;
 		/*
 		 * Retry on ALUA state transition or if any
 		 * UNIT ATTENTION occurred.
@@ -576,6 +576,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		if (sense_hdr.sense_key == NOT_READY &&
 		    sense_hdr.asc == 0x04 && sense_hdr.ascq == 0x0a)
 			err = SCSI_DH_RETRY;
+		if (sense_hdr.sense_key == ILLEGAL_REQUEST &&
+		    sense_hdr.asc == 0x25 && sense_hdr.ascq == 0x00)
+			err = SCSI_DH_DEV_OFFLINED;
 		else if (sense_hdr.sense_key == UNIT_ATTENTION)
 			err = SCSI_DH_RETRY;
 		if (err == SCSI_DH_RETRY &&
@@ -591,7 +594,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
 		kfree(buff);
 		pg->expiry = 0;
-		return SCSI_DH_IO;
+		return err;
 	}
 
 	len = get_unaligned_be32(&buff[0]) + 4;
-- 
2.12.2

[PATCH 3/3] scsi_dh_alua: do not call BUG_ON when updating port group

[Martin Wilck]

alua_rtpg() can race with alua_bus_detach(). The assertion that
alua_dh_data *h->sdev must be non-NULL is not guaranteed because
alua_bus_detach sets this field to NULL before removing the entry
from the port group's dh_list.

This happens when a device is about to be removed, so don't BUG out
but continue silently.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 501855bde633..274fb49d0801 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -652,9 +652,13 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 					rcu_read_lock();
 					list_for_each_entry_rcu(h,
 						&tmp_pg->dh_list, node) {
-						/* h->sdev should always be valid */
-						BUG_ON(!h->sdev);
-						h->sdev->access_state = desc[0];
+						/*
+						 * We might be racing with
+						 * alua_bus_detach here
+						 */
+						if (h->sdev)
+							h->sdev->access_state =
+								desc[0];
 					}
 					rcu_read_unlock();
 				}
@@ -694,7 +698,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 			pg->expiry = 0;
 			rcu_read_lock();
 			list_for_each_entry_rcu(h, &pg->dh_list, node) {
-				BUG_ON(!h->sdev);
+				if (!h->sdev)
+					continue;
 				h->sdev->access_state =
 					(pg->state & SCSI_ACCESS_STATE_MASK);
 				if (pg->pref)
-- 
2.12.2

Re: [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries

[Bart Van Assche]

On Fri, 2017-04-28 at 15:06 +0200, Martin Wilck wrote:
> @@ -886,7 +883,7 @@ static bool alua_rtpg_queue(struct alua_port_group *pg,
>  		force = true;
>  	}
>  	if (pg->rtpg_sdev == NULL) {
> -		pg->interval = 0;
> +		pg->interval = 2;
>  		pg->flags |= ALUA_PG_RUN_RTPG;
>  		kref_get(&pg->kref);
>  		pg->rtpg_sdev = sdev;

Hello Hannes and Martin,

Why is .interval initialized in alua_rtpg_queue() instead of in
alua_alloc_pg()? I think initializing it in alua_alloc_pg() would
make more clear that .interval is constant.

Thanks,

Bart.

Re: [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device

[Bart Van Assche]

On Fri, 2017-04-28 at 15:06 +0200, Martin Wilck wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> If a device becomes unmapped on the target we should be returning
> SCSI_DH_DEV_OFFLINED.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> Reviewed-by: Martin Wilck <mwilck@suse.com>

Hello Hannes and Martin,

Have you considered to add "Cc: stable" to this patch and other patches in
this series?

> 
>  		 * Retry on ALUA state transition or if any
>  		 * UNIT ATTENTION occurred.
> @@ -576,6 +576,9 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  		if (sense_hdr.sense_key == NOT_READY &&
>  		    sense_hdr.asc == 0x04 && sense_hdr.ascq == 0x0a)
>  			err = SCSI_DH_RETRY;
> +		if (sense_hdr.sense_key == ILLEGAL_REQUEST &&
> +		    sense_hdr.asc == 0x25 && sense_hdr.ascq == 0x00)
> +			err = SCSI_DH_DEV_OFFLINED;
>  		else if (sense_hdr.sense_key == UNIT_ATTENTION)
>  			err = SCSI_DH_RETRY;

Please be consistent in the code that checks the sense codes - either use
"else if" for the test that has been added or changed the existing "else if"
into "if". Otherwise this patch looks fine to me.

Thanks,

Bart.

Re: [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries

[Martin Wilck]

On Fri, 2017-04-28 at 18:35 +0000, Bart Van Assche wrote:
> On Fri, 2017-04-28 at 15:06 +0200, Martin Wilck wrote:
> > @@ -886,7 +883,7 @@ static bool alua_rtpg_queue(struct
> > alua_port_group *pg,
> >  		force = true;
> >  	}
> >  	if (pg->rtpg_sdev == NULL) {
> > -		pg->interval = 0;
> > +		pg->interval = 2;
> >  		pg->flags |= ALUA_PG_RUN_RTPG;
> >  		kref_get(&pg->kref);
> >  		pg->rtpg_sdev = sdev;
> 
> Hello Hannes and Martin,
> 
> Why is .interval initialized in alua_rtpg_queue() instead of in
> alua_alloc_pg()? I think initializing it in alua_alloc_pg() would
> make more clear that .interval is constant.

Thinking about it - since "interval" has now become a global constant,
we might as well declare it as a constant rather than carrying in
around in the alua_port_group struct.

It's kind of funny how this evolved from a geometric series via
an arithmetic series (bc97f4bb) and a per port-group variable with just
two values (03197b61) to a global constant ... an example of kernel
code gradually getting simpler over time :-)

However: 03197b61 ("scsi_dh_alua: Use workqueue for RTPG") has removed
the progression sort of silently. It was still present in Hannes's
first version of the patch (http://marc.info/?l=linux-scsi&m=1391160640
32031&w=2) but seems to have been dropped in later versions: 

@@ -546,23 +593,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;

Can someone confirm that using a constant value here is sufficient?

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device

[Martin Wilck]

Hi Bart,

On Fri, 2017-04-28 at 18:42 +0000, Bart Van Assche wrote:
> On Fri, 2017-04-28 at 15:06 +0200, Martin Wilck wrote:
> > From: Hannes Reinecke <hare@suse.de>
> > 
> > If a device becomes unmapped on the target we should be returning
> > SCSI_DH_DEV_OFFLINED.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.com>
> > Reviewed-by: Martin Wilck <mwilck@suse.com>
> 
> Hello Hannes and Martin,
> 
> Have you considered to add "Cc: stable" to this patch and other
> patches in
> this series?

Sorry, I forgot. But a v2 will be necessary anyway as it seems.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/3] scsi_dh_alua: do not call BUG_ON when updating port group

[Bart Van Assche]

On Fri, 2017-04-28 at 15:06 +0200, Martin Wilck wrote:
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 501855bde633..274fb49d0801 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -652,9 +652,13 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  					rcu_read_lock();
>  					list_for_each_entry_rcu(h,
>  						&tmp_pg->dh_list, node) {
> -						/* h->sdev should always be valid */
> -						BUG_ON(!h->sdev);
> -						h->sdev->access_state = desc[0];
> +						/*
> +						 * We might be racing with
> +						 * alua_bus_detach here
> +						 */
> +						if (h->sdev)
> +							h->sdev->access_state =
> +								desc[0];
>  					}
>  					rcu_read_unlock();
>  				}

Hello Hannes and Martin,

What will happen if h->sdev is cleared after it has been tested and before
it is dereferenced? Additionally, even if h->sdev would be cached, can the
following sequence of events happen?
* alua_rtpg() tests h->sdev.
* alua_bus_detach() clears h->sdev.
* h->sdev is freed.
* alua_rtpg() dereferences h->sdev.

Thanks,

Bart.

Re: [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries

[Hannes Reinecke]

On 04/28/2017 08:35 PM, Bart Van Assche wrote:
> On Fri, 2017-04-28 at 15:06 +0200, Martin Wilck wrote:
>> @@ -886,7 +883,7 @@ static bool alua_rtpg_queue(struct alua_port_group *pg,
>>  		force = true;
>>  	}
>>  	if (pg->rtpg_sdev == NULL) {
>> -		pg->interval = 0;
>> +		pg->interval = 2;
>>  		pg->flags |= ALUA_PG_RUN_RTPG;
>>  		kref_get(&pg->kref);
>>  		pg->rtpg_sdev = sdev;
> 
> Hello Hannes and Martin,
> 
> Why is .interval initialized in alua_rtpg_queue() instead of in
> alua_alloc_pg()? I think initializing it in alua_alloc_pg() would
> make more clear that .interval is constant.
> 
Yes, valid point.
Will be doing so.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
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)