[PATCH v2 0/4] failover fixes for scsi_dh_alua

[PATCH v2 0/4] failover fixes for scsi_dh_alua

[Martin Wilck]

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

Best regards,
Martin

Changes wrt v1:

- Use a constant value for the retry interval in patch 1/4
- Style fix in patch 2/4
- Safer coding in patch 3/4 as suggested by Bart
- Protect against sdev being released, as suggested by Bart (added patch 4/4)

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 (2):
  scsi_dh_alua: do not call BUG_ON when updating port group
  scsi_dh_alua: take sdev reference in alua_bus_attach

 drivers/scsi/device_handler/scsi_dh_alua.c | 51 +++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 18 deletions(-)

-- 
2.12.2

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

[Martin Wilck]

From: Hannes Reinecke <hare@suse.de>

We shouldn't set the interval value to 0 in alua_rtpg_work(), as the struct
is accessed from different devices and hence we might end up scheduling
too early.

With this change, pg->interval is now effectively constant, thus we might
as well replace it with a contant expression.

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

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index c01b47e5b55a..f17dccb2f72b 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -53,6 +53,7 @@
 #define ALUA_FAILOVER_TIMEOUT		60
 #define ALUA_FAILOVER_RETRIES		5
 #define ALUA_RTPG_DELAY_MSECS		5
+#define ALUA_RTPG_RETRY_INTERVAL	(2*HZ)
 
 /* device handler flags */
 #define ALUA_OPTIMIZE_STPG		0x01
@@ -86,7 +87,6 @@ struct alua_port_group {
 	unsigned		flags; /* used for optimizing STPG */
 	unsigned char		transition_tmo;
 	unsigned long		expiry;
-	unsigned long		interval;
 	struct delayed_work	rtpg_work;
 	spinlock_t		lock;
 	struct list_head	rtpg_list;
@@ -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;
@@ -811,7 +810,7 @@ static void alua_rtpg_work(struct work_struct *work)
 				pg->flags |= ALUA_PG_RUN_RTPG;
 				spin_unlock_irqrestore(&pg->lock, flags);
 				queue_delayed_work(alua_wq, &pg->rtpg_work,
-						   pg->interval * HZ);
+						   ALUA_RTPG_RETRY_INTERVAL);
 				return;
 			}
 			/* Send RTPG on failure or if TUR indicates SUCCESS */
@@ -823,7 +822,7 @@ static void alua_rtpg_work(struct work_struct *work)
 			pg->flags |= ALUA_PG_RUN_RTPG;
 			spin_unlock_irqrestore(&pg->lock, flags);
 			queue_delayed_work(alua_wq, &pg->rtpg_work,
-					   pg->interval * HZ);
+					   ALUA_RTPG_RETRY_INTERVAL);
 			return;
 		}
 		if (err != SCSI_DH_OK)
@@ -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,6 @@ static bool alua_rtpg_queue(struct alua_port_group *pg,
 		force = true;
 	}
 	if (pg->rtpg_sdev == NULL) {
-		pg->interval = 0;
 		pg->flags |= ALUA_PG_RUN_RTPG;
 		kref_get(&pg->kref);
 		pg->rtpg_sdev = sdev;
-- 
2.12.2

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

[Bart Van Assche]

On Fri, 2017-05-12 at 15:15 +0200, Martin Wilck wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> We shouldn't set the interval value to 0 in alua_rtpg_work(), as the struct
> is accessed from different devices and hence we might end up scheduling
> too early.
> 
> With this change, pg->interval is now effectively constant, thus we might
> as well replace it with a contant expression.

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

[PATCH v2 2/4] 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 f17dccb2f72b..2b60f493f90e 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;
+		else 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

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

[Bart Van Assche]

On Fri, 2017-05-12 at 15:15 +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.

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

[PATCH v2 3/4] 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 | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 2b60f493f90e..a59783020c66 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -652,9 +652,15 @@ 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
+						 */
+						struct scsi_device *sdev =
+							h->sdev;
+						if (sdev)
+							sdev->access_state =
+								desc[0];
 					}
 					rcu_read_unlock();
 				}
@@ -694,11 +700,13 @@ 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);
-				h->sdev->access_state =
+				struct scsi_device *sdev = h->sdev;
+				if (!sdev)
+					continue;
+				sdev->access_state =
 					(pg->state & SCSI_ACCESS_STATE_MASK);
 				if (pg->pref)
-					h->sdev->access_state |=
+					sdev->access_state |=
 						SCSI_ACCESS_STATE_PREFERRED;
 			}
 			rcu_read_unlock();
-- 
2.12.2

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

[Bart Van Assche]

On Fri, 2017-05-12 at 15:15 +0200, Martin Wilck wrote:
> 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 | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 2b60f493f90e..a59783020c66 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -652,9 +652,15 @@ 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
> +						 */
> +						struct scsi_device *sdev =
> +							h->sdev;
> +						if (sdev)
> +							sdev->access_state =
> +								desc[0];
>  					}
>  					rcu_read_unlock();
>  				}
> @@ -694,11 +700,13 @@ 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);
> -				h->sdev->access_state =
> +				struct scsi_device *sdev = h->sdev;
> +				if (!sdev)
> +					continue;
> +				sdev->access_state =
>  					(pg->state & SCSI_ACCESS_STATE_MASK);
>  				if (pg->pref)
> -					h->sdev->access_state |=
> +					sdev->access_state |=
>  						SCSI_ACCESS_STATE_PREFERRED;
>  			}
>  			rcu_read_unlock();

Hello Martin,

Allowing races like the one this patch tries to address to exist makes
the ALUA code harder to maintain than necessary. Have you considered to
make alua_bus_detach() wait until ALUA work has finished by using e.g.
cancel_work_sync() or rcu_synchronize()?

Bart.

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

[Martin Wilck]

On Fri, 2017-05-12 at 16:24 +0000, Bart Van Assche wrote:
> 
> Hello Martin,
> 
> Allowing races like the one this patch tries to address to exist
> makes
> the ALUA code harder to maintain than necessary. Have you considered
> to
> make alua_bus_detach() wait until ALUA work has finished by using
> e.g.
> cancel_work_sync() or rcu_synchronize()?
> 
> Bart.

Hello Bart,

to be honest, no, I didn't consider this yet. The current kernel
crashes with BUG() if an ALUA device is detached at an inopportune
point in time (not just theoretically, we actually observed this). The
goal of my patch was to fix this with minimum risk to introduce other
problems. The addition in patch 4/4 was an attempt to address the
concern you had expressed in your review of the v1 patch.

I'm not opposed to try to find a better solution, but could we maybe
get the fix for the BUG() (i.e. patch 3/4) applied in the first place?
AFAICS it would not conflict with a solution like the one you
suggested.

Best regards and thanks for the review,
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 v2 3/4] scsi_dh_alua: do not call BUG_ON when updating port group

[Bart Van Assche]

On Mon, 2017-05-15 at 10:16 +0200, Martin Wilck wrote:
> On Fri, 2017-05-12 at 16:24 +0000, Bart Van Assche wrote:
> > Allowing races like the one this patch tries to address to exist
> > makes the ALUA code harder to maintain than necessary. Have you
> > considered to make alua_bus_detach() wait until ALUA work has
> > finished by using e.g. cancel_work_sync() or rcu_synchronize()?
> 
> to be honest, no, I didn't consider this yet. The current kernel
> crashes with BUG() if an ALUA device is detached at an inopportune
> point in time (not just theoretically, we actually observed this). The
> goal of my patch was to fix this with minimum risk to introduce other
> problems. The addition in patch 4/4 was an attempt to address the
> concern you had expressed in your review of the v1 patch.
> 
> I'm not opposed to try to find a better solution, but could we maybe
> get the fix for the BUG() (i.e. patch 3/4) applied in the first place?
> AFAICS it would not conflict with a solution like the one you
> suggested.

Hello Martin,

Sorry but I don't think it's a good idea to merge patch 3/4 in the upstream
kernel. Even with that patch applied there is nothing that prevents that
h->handler_data would be freed while alua_rtpg() is in progress and hence
that h->sdev is a completely random pointer if alua_rtpg() is executed
concurrently with alua_bus_detach(). Please do not try to paper over race
conditions but fix these properly.

Bart.

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

[Martin Wilck]

On Mon, 2017-05-15 at 16:03 +0000, Bart Van Assche wrote:
> On Mon, 2017-05-15 at 10:16 +0200, Martin Wilck wrote:
> > On Fri, 2017-05-12 at 16:24 +0000, Bart Van Assche wrote:
> > > Allowing races like the one this patch tries to address to exist
> > > makes the ALUA code harder to maintain than necessary. Have you
> > > considered to make alua_bus_detach() wait until ALUA work has
> > > finished by using e.g. cancel_work_sync() or rcu_synchronize()?
> > 
> > to be honest, no, I didn't consider this yet. The current kernel
> > crashes with BUG() if an ALUA device is detached at an inopportune
> > point in time (not just theoretically, we actually observed this).
> > The
> > goal of my patch was to fix this with minimum risk to introduce
> > other
> > problems. The addition in patch 4/4 was an attempt to address the
> > concern you had expressed in your review of the v1 patch.
> > 
> > I'm not opposed to try to find a better solution, but could we
> > maybe
> > get the fix for the BUG() (i.e. patch 3/4) applied in the first
> > place?
> > AFAICS it would not conflict with a solution like the one you
> > suggested.
> 
> Hello Martin,
> 
> Sorry but I don't think it's a good idea to merge patch 3/4 in the
> upstream
> kernel. Even with that patch applied there is nothing that prevents
> that
> h->handler_data would be freed while alua_rtpg() is in progress and
> hence
> that h->sdev is a completely random pointer if alua_rtpg() is
> executed
> concurrently with alua_bus_detach(). Please do not try to paper over
> race
> conditions but fix these properly.

Hello Bart,

please be assured that I'm not trying to paper over anything. Your
concern about sdev->handler_data is justified. While I think that it's
a separate issue from what my patches were supposed to address, let me
see if I can come up with something more comprehensive.

It will take time, though, until I fully comprehend the locking concept
of scsi_dh_alua.c.

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 v2 3/4] scsi_dh_alua: do not call BUG_ON when updating port group

[Christoph Hellwig]

On Mon, May 15, 2017 at 08:30:19PM +0200, Martin Wilck wrote:
> please be assured that I'm not trying to paper over anything. Your
> concern about sdev->handler_data is justified. While I think that it's
> a separate issue from what my patches were supposed to address, let me
> see if I can come up with something more comprehensive.
> 
> It will take time, though, until I fully comprehend the locking concept
> of scsi_dh_alua.c.

I think a simple call_rcu for the cleanup in alua_bus_detach should
do the job.  And I agree with Bart that we need to sort this out
properly..

[PATCH v2 4/4] scsi_dh_alua: take sdev reference in alua_bus_attach

[Martin Wilck]

Modification of the access_state field in struct scsi_device
in alua_rtpg() may race with alua_bus_detach(). Avoid
the scsi_device struct to be freed while it's being processed
in the alua code by taking a reference.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/device_handler/scsi_dh_alua.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index a59783020c66..4b33e076d4f7 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -1108,6 +1108,10 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	h = kzalloc(sizeof(*h) , GFP_KERNEL);
 	if (!h)
 		return -ENOMEM;
+	if (scsi_device_get(sdev)) {
+		ret = -ENXIO;
+		goto failed;
+	}
 	spin_lock_init(&h->pg_lock);
 	rcu_assign_pointer(h->pg, NULL);
 	h->init_error = SCSI_DH_OK;
@@ -1119,10 +1123,13 @@ static int alua_bus_attach(struct scsi_device *sdev)
 	if (err == SCSI_DH_NOMEM)
 		ret = -ENOMEM;
 	if (err != SCSI_DH_OK && err != SCSI_DH_DEV_OFFLINED)
-		goto failed;
+		goto failed_put;
 
 	sdev->handler_data = h;
 	return 0;
+
+failed_put:
+	scsi_device_put(sdev);
 failed:
 	kfree(h);
 	return ret;
@@ -1140,6 +1147,7 @@ static void alua_bus_detach(struct scsi_device *sdev)
 	spin_lock(&h->pg_lock);
 	pg = rcu_dereference_protected(h->pg, lockdep_is_held(&h->pg_lock));
 	rcu_assign_pointer(h->pg, NULL);
+	scsi_device_put(h->sdev);
 	h->sdev = NULL;
 	spin_unlock(&h->pg_lock);
 	if (pg) {
-- 
2.12.2

Re: [PATCH v2 4/4] scsi_dh_alua: take sdev reference in alua_bus_attach

[Bart Van Assche]

On Fri, 2017-05-12 at 15:15 +0200, Martin Wilck wrote:
> Modification of the access_state field in struct scsi_device
> in alua_rtpg() may race with alua_bus_detach(). Avoid
> the scsi_device struct to be freed while it's being processed
> in the alua code by taking a reference.

Hello Martin,

The approach of this patch seems weird to me. I don't think that it is a
good idea to let any ALUA work continue while alua_bus_detach() is in progress.
Have you considered to stop the ALUA work from inside alua_bus_detach() by
using cancel_work_sync()?

Bart.