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@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
