On Thu, 2017-06-22 at 14:21 -0500, Benjamin Marzinski wrote: > On Thu, Jun 22, 2017 at 08:23:44AM +0200, Hannes Reinecke wrote: > > On 06/21/2017 05:06 PM, Martin Wilck wrote: > > > We set the queue_if_no_path feature in assemble_map() already, > > > no need to set it here again. > > > > > > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx> > > > --- > > > libmultipath/configure.c | 15 --------------- > > > 1 file changed, 15 deletions(-) > > > > > > > > > > Watch out. > > 'queue_if_no_path' is set _temporarily_ while 'no_path_retry' is > > active, > > and removed afterwards. > > So there might be valid reasons why it's set here. > > Have you checked? > > IIRC, we used to always set queue_if_no_path before reloading the > map, Why? I've searched for comments explaining that but found nothing. Anyway, we've ceased to do so at least since 7331cf5 "Correctly update feature strings", merged in May 2011. > and then call setup_multipath() afterwards, which would call > set_no_path_retry() to set it to the actual correct value. setup_multipath() is called too, a bit later in the code flow, from configure() after coalesce_paths() and coalesce_maps(). So we're currently setting queue_if_no_path 3 times when creating maps: in domap(), after domap(), and in setup_multipath->set_no_path_retry(). With my patch, we do it only twice :-) The call to dm_queue_if_no_path directly after domap(), which my patch removes, is ancient, it came with 0e6e3113 "[multipath] Extension of the "no_path_retry" scope to the multipath" in 2005. > If we are > correctly setting the feature line when we reload the map, that's a > better solution. Obviously, we can't strip set_no_path_retry() out of > __setup_multipath() since we rely on that to deal with deal with > going > into recovery mode. However, without some more thought and code > reading, > I'm not sure if we do still need the calls to dm_queue_if_no_path() > there for some other reason anymore. At any rate, they won't hurt > anything, except for causing a redundant call to device-mapper. > > Also, if we're yanking these dm_queue_if_no_path() calls from > coalesce_paths, I don't see why we need them in reload_map(), where > they > also seem redundant if we just correctly set the features argument > when > we reloaded the table. > > ACK, but I wouldn't mind seeing the calls pulled from reload_map as > well. Agreed, but I propose to do that in a separate patch. No need to repost the whole series for that. Martin -- Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
