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

[Martin Wilck <mwilck@suse.com>]

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)