[dm-devel] [PATCH 3/5] dm-multipath: remove process_queued_ios()

[Hannes Reinecke]

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 at suse.de			      +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)