Re: [PATCH 4/7] dm mpath: remove process_queued_ios()

[Hannes Reinecke <hare@suse.de>]

On 02/04/2014 10:27 AM, Junichi Nomura wrote:
> On 02/04/14 18:08, Hannes Reinecke wrote:
>> On 02/04/2014 09:55 AM, Junichi Nomura wrote:
>>> On 02/04/14 17:18, Hannes Reinecke wrote:
>>>>>> @@ -1591,8 +1563,17 @@ static int multipath_ioctl(struct dm_target *ti, unsigned int cmd,
>>>>> ...
>>>>>> +		if (m->current_pg && m->pg_init_required)
>>>>>> +			__pg_init_all_paths(m, 0);
>>>>>> +		spin_unlock_irqrestore(&m->lock, flags);
>>>>>> +		dm_table_run_md_queue_async(m->ti->table);
>>>>>> +	}
>>>>>
>>>>> What happens if "!m->current_pg && m->pg_init_required"?
>>>>>
>>>> >From the current logic it means that no valid pg was found, so
>>>> calling pg_init would be pointless.
>>>> We're calling __choose_pgpath() before that, so if that returns
>>>> with current_pg == NULL all paths are down, and calling
>>>> pg_init would be pointless.
>>>>
>>>> But I think I see to have pg_init_required handling cleared up;
>>>> I'll be doing a patch to unset it at the start of __choose_pgpath(),
>>>> this we we can be sure that it'll be set correctly.
>>>
>>> I think it is possible that __choose_pgpath() being called twice
>>> before pg_init_required is checked.
>>>
>>> For example,
>>>
>>>   multipath_ioctl()
>>>     __choose_pgpath()
>>>       clear pg_init_required
>>>       select a new pg
>>>       __switch_pg()
>>>         set pg_init_required
>>>
>>>   map_io()
>>>     __choose_pgpath()
>>>       clear pg_init_required
>>>       select the same pg
>>>       (pg_init_required is not set)
>>>       ...
>>>
>> But why should 'map_io' calling __choose_pgpath()?
>>
>> Either __choose_path() from ioctl was able to set ->current_pg
>> (in which case __choose_pgpath() wouldn't be called in map_io)
>> or it was not, in which case pg_init_required would need to be reset
>> during __choose_pgpath() when called from map_io().
> 
> __choose_pgpath() is a function to select "path".
> Even if current_pg is set, map_io() may call the function
> to select a path. (Please look at the repeat_count check)
> 
... But only if 'queue_io' is not set, ie no pg_init is running.
At which point 'pg_init_required' should be set to '0' anyway.

What I'm arguing here is an inconsistency in __choose_pg():
__choose_pg() might or might not set 'current_pg', but if
'current_pg' is not set the status of 'pg_init_required'
is undefined upon return.

This makes it (in my view) unnecessarily complicated to
check if we need to initialize the paths, as we have to
check for both, current_pg _and_ pg_init_required.
If it were _that_ easy we wouldn't have this discussion :-)

> So, I suggested the followings:
>   Call __pg_init_all_paths() regardless of current_pg.
>   __pg_init_all_paths() returns whether pg_init has started.
>   If !current_pg, it returns 0.
>   (https://www.redhat.com/archives/dm-devel/2014-February/msg00013.html)
> 
> The semantics is clear:
>   - if pg_init_required, call __pg_init_all_paths()
>   - __pg_init_all_paths() might fail to start pg_init (= returns 0).
>     Then caller should take some action or give up.
> 
All right, will be modifying the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@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)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel