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

[Martin Wilck]

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

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

[Benjamin Marzinski]

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 
> > ---
> >  libmultipath/configure.c | 15 ---
> >  1 file changed, 15 deletions(-)
> > 
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 55dbb261..39912f05 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -1097,21 +1097,6 @@ int coalesce_paths (struct vectors * vecs, vector 
> > newmp, char * refwwid,
> > remove_feature(>features,
> >"queue_if_no_path");
> > }
> > -   else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF) {
> > -   if (mpp->no_path_retry == NO_PATH_RETRY_FAIL) {
> > -   condlog(3, "%s: unset queue_if_no_path feature",
> > -   mpp->alias);
> > -   if (!dm_queue_if_no_path(mpp->alias, 0))
> > -   remove_feature(>features,
> > -  "queue_if_no_path");
> > -   } else {
> > -   condlog(3, "%s: set queue_if_no_path feature",
> > -   mpp->alias);
> > -   if (!dm_queue_if_no_path(mpp->alias, 1))
> > -   add_feature(>features,
> > -   "queue_if_no_path");
> > -   }
> > -   }
> >  
> > if (!is_daemon && mpp->action != ACT_NOTHING) {
> > conf = get_multipath_config();
> > 
> 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,
and then call setup_multipath() afterwards, which would call
set_no_path_retry() to set it to the actual correct value. 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.

-Ben

> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke  Teamlead Storage & Networking
> h...@suse.de +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

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

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

[Martin Wilck]

On Thu, 2017-06-22 at 08:23 +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 
> > ---
> >  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?

Yes, I'm pretty certain that this is correct. We're in coalesce_paths()
here, while we are setting up or reconfiguring maps. The call sequence
is 

   setup_map()
  assemble_map()
   domap()
   ... and then comes the code I'm removing.

We set the feature string in assemble_map() correctly. Thus the removed
code just repeated the same setting that had already been applied in
domap(). This code has been in that place for a very long time, AFAICS
it originates from times where the features string was not correctly
set up before creating or reloading the map.

The logic for disabling queue_if_no_path if the retry count is reached
is implemented elsewhere, mainly in set_no_path_retry() (called e.g.
from ev_remove_path()) and retry_count_tick() (called from checker
loop).

Do you see a case that I have overlooked?

Best,
Martin

-- 
Dr. Martin Wilck , 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

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

[Hannes Reinecke]

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 
> ---
>  libmultipath/configure.c | 15 ---
>  1 file changed, 15 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 55dbb261..39912f05 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1097,21 +1097,6 @@ int coalesce_paths (struct vectors * vecs, vector 
> newmp, char * refwwid,
>   remove_feature(>features,
>  "queue_if_no_path");
>   }
> - else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF) {
> - if (mpp->no_path_retry == NO_PATH_RETRY_FAIL) {
> - condlog(3, "%s: unset queue_if_no_path feature",
> - mpp->alias);
> - if (!dm_queue_if_no_path(mpp->alias, 0))
> - remove_feature(>features,
> -"queue_if_no_path");
> - } else {
> - condlog(3, "%s: set queue_if_no_path feature",
> - mpp->alias);
> - if (!dm_queue_if_no_path(mpp->alias, 1))
> - add_feature(>features,
> - "queue_if_no_path");
> - }
> - }
>  
>   if (!is_daemon && mpp->action != ACT_NOTHING) {
>   conf = get_multipath_config();
> 
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?

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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