* [PATCH 0/3] failover fixes for scsi_dh_alua
@ 2017-04-28 13:06 Martin Wilck
2017-04-28 13:06 ` [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries Martin Wilck
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Martin Wilck @ 2017-04-28 13:06 UTC (permalink / raw)
To: Martin K. Petersen, Hannes Reinecke
Cc: Bart Van Assche, Mauricio Faria de Oliveira, linux-scsi
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries
2017-04-28 13:06 [PATCH 0/3] failover fixes for scsi_dh_alua Martin Wilck
@ 2017-04-28 13:06 ` Martin Wilck
2017-04-28 18:35 ` Bart Van Assche
2017-04-28 13:06 ` [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device Martin Wilck
2017-04-28 13:06 ` [PATCH 3/3] scsi_dh_alua: do not call BUG_ON when updating port group Martin Wilck
2 siblings, 1 reply; 10+ messages in thread
From: Martin Wilck @ 2017-04-28 13:06 UTC (permalink / raw)
To: Martin K. Petersen, Hannes Reinecke
Cc: Bart Van Assche, Mauricio Faria de Oliveira, linux-scsi
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device
2017-04-28 13:06 [PATCH 0/3] failover fixes for scsi_dh_alua Martin Wilck
2017-04-28 13:06 ` [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries Martin Wilck
@ 2017-04-28 13:06 ` Martin Wilck
2017-04-28 18:42 ` Bart Van Assche
2017-04-28 13:06 ` [PATCH 3/3] scsi_dh_alua: do not call BUG_ON when updating port group Martin Wilck
2 siblings, 1 reply; 10+ messages in thread
From: Martin Wilck @ 2017-04-28 13:06 UTC (permalink / raw)
To: Martin K. Petersen, Hannes Reinecke
Cc: Bart Van Assche, Mauricio Faria de Oliveira, linux-scsi
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] scsi_dh_alua: do not call BUG_ON when updating port group
2017-04-28 13:06 [PATCH 0/3] failover fixes for scsi_dh_alua Martin Wilck
2017-04-28 13:06 ` [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries Martin Wilck
2017-04-28 13:06 ` [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device Martin Wilck
@ 2017-04-28 13:06 ` Martin Wilck
2017-04-28 19:58 ` Bart Van Assche
2 siblings, 1 reply; 10+ messages in thread
From: Martin Wilck @ 2017-04-28 13:06 UTC (permalink / raw)
To: Martin K. Petersen, Hannes Reinecke
Cc: Bart Van Assche, Mauricio Faria de Oliveira, linux-scsi
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries
2017-04-28 13:06 ` [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries Martin Wilck
@ 2017-04-28 18:35 ` Bart Van Assche
2017-04-28 19:49 ` Martin Wilck
2017-05-02 6:17 ` Hannes Reinecke
0 siblings, 2 replies; 10+ messages in thread
From: Bart Van Assche @ 2017-04-28 18:35 UTC (permalink / raw)
To: mwilck, hare, martin.petersen; +Cc: mauricfo, linux-scsi
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device
2017-04-28 13:06 ` [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device Martin Wilck
@ 2017-04-28 18:42 ` Bart Van Assche
2017-04-28 19:52 ` Martin Wilck
0 siblings, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2017-04-28 18:42 UTC (permalink / raw)
To: mwilck, hare, martin.petersen; +Cc: mauricfo, linux-scsi
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries
2017-04-28 18:35 ` Bart Van Assche
@ 2017-04-28 19:49 ` Martin Wilck
2017-05-02 6:17 ` Hannes Reinecke
1 sibling, 0 replies; 10+ messages in thread
From: Martin Wilck @ 2017-04-28 19:49 UTC (permalink / raw)
To: Bart Van Assche, hare, martin.petersen; +Cc: mauricfo, linux-scsi
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)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device
2017-04-28 18:42 ` Bart Van Assche
@ 2017-04-28 19:52 ` Martin Wilck
0 siblings, 0 replies; 10+ messages in thread
From: Martin Wilck @ 2017-04-28 19:52 UTC (permalink / raw)
To: Bart Van Assche, hare, martin.petersen; +Cc: mauricfo, linux-scsi
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)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] scsi_dh_alua: do not call BUG_ON when updating port group
2017-04-28 13:06 ` [PATCH 3/3] scsi_dh_alua: do not call BUG_ON when updating port group Martin Wilck
@ 2017-04-28 19:58 ` Bart Van Assche
0 siblings, 0 replies; 10+ messages in thread
From: Bart Van Assche @ 2017-04-28 19:58 UTC (permalink / raw)
To: mwilck, hare, martin.petersen; +Cc: mauricfo, linux-scsi
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries
2017-04-28 18:35 ` Bart Van Assche
2017-04-28 19:49 ` Martin Wilck
@ 2017-05-02 6:17 ` Hannes Reinecke
1 sibling, 0 replies; 10+ messages in thread
From: Hannes Reinecke @ 2017-05-02 6:17 UTC (permalink / raw)
To: Bart Van Assche, mwilck, martin.petersen; +Cc: mauricfo, linux-scsi
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)
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-05-02 6:18 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-28 13:06 [PATCH 0/3] failover fixes for scsi_dh_alua Martin Wilck
2017-04-28 13:06 ` [PATCH 1/3] scsi_dh_alua: Do not modify the interval value for retries Martin Wilck
2017-04-28 18:35 ` Bart Van Assche
2017-04-28 19:49 ` Martin Wilck
2017-05-02 6:17 ` Hannes Reinecke
2017-04-28 13:06 ` [PATCH 2/3] scsi_dh_alua: Do not retry for unmapped device Martin Wilck
2017-04-28 18:42 ` Bart Van Assche
2017-04-28 19:52 ` Martin Wilck
2017-04-28 13:06 ` [PATCH 3/3] scsi_dh_alua: do not call BUG_ON when updating port group Martin Wilck
2017-04-28 19:58 ` Bart Van Assche
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.