On 01/31/2014 10:43 AM, Junichi Nomura wrote:
> On 01/31/14 18:10, Hannes Reinecke wrote:
>> @@ -1217,9 +1185,21 @@ static void pg_init_done(void *data, int errors)
>>
>> if (!m->pg_init_required)
>> m->queue_io = 0;
>> -
>> - m->pg_init_delay_retry = delay_retry;
>> - queue_work(kmultipathd, &m->process_queued_ios);
>> + else {
>> + if (!pg_init_delay) {
>> + m->pg_init_delay_retry = 0;
>> + /*
>> + * Add a small delay to move the
>> + * activation to a workqueue
>> + */
>> + pg_init_delay = HZ / 50;
>> + } else
>> + m->pg_init_delay_retry = 1;
>> + if (queue_delayed_work(kmpath_handlerd, &pgpath->activate_path,
>> + pg_init_delay))
>> + m->pg_init_in_progress++;
>> + goto out;
>> + }
>
> This code is called when pg_init_progress becomes 0,
> that means it is possible that multiple pgpaths have requested retries.
> So you have to activate_path for all non-active pgpath in this pg,
> like __pg_init_all_paths() does in the current code.
>
Not necessarily. SCSI_DH_RETRY (initially) is per-path, so it's well
feasible that pg_init on the first path returns SCSI_DH_OK, and
pg_init on the other path returns SCSI_DH_RETRY.
> Also, you aren't clearing pg_init_required here.
> I think it causes extra pg_init unnecessarily.
>
Indeed. I'll need to fix this up.
> What do you think if we just call __pg_init_all_paths() here
> instead of open coding it?
>
That depends on the intended policy.
Problem here is that pg_init_done() is per path, so at face value
SCSI_DH_RETRY is per path, too.
So from that we should be retrying this path (and this path only).
Hence it would be correct to call queue_delayed_work directly.
However, typically any pg_init affects _every_ path in the multipath
device (active paths become passive and vice versa).
Which seems to be the intended usage, as we're checking for
pg_init_in_progress prior to invoking queue_delayed_work().
But _if_ we assume that, then we only need to send a _single_
pg_init, as this will switch all paths. So again, a call to
__pg_init_all_paths will not do the correct thing as it'll
send activations to _every_ active path.
(And, in fact, we're trying really hard in scsi_dh_rdac
and scsi_dh_alua to bunch together all the various pg_init
requests precisely for the cited reason).
So my inclination here would be to treat SCSI_DH_RETRY
as _per path_, and retry only this specific path.
IE removing the check to pg_init_in_progress and call
queue_delayed_work() directly.
IMHO this would impose the least restriction on
the internal workings of the various device handlers.
What do you think?
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@xxxxxxx +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
