Re: [PATCH v3 11/11] libmultipath: don't [un]set queue_if_no_path after domap

[Martin Wilck <mwilck@suse.com>]

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@suse.com>
> > > ---
> > >  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@suse.com>, 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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel