diff mbox

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

Message ID 20170428130626.32162-2-mwilck@suse.com (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Martin Wilck April 28, 2017, 1:06 p.m. UTC 
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(-)

Comments

Bart Van Assche April 28, 2017, 6:35 p.m. UTC | #1 
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.
Martin Wilck April 28, 2017, 7:49 p.m. UTC | #2 
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
Hannes Reinecke May 2, 2017, 6:17 a.m. UTC | #3 
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
diff mbox

Patch

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;