| Message ID | 20170621150630.25773-12-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
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(-) > > 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(&mpp->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(&mpp->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(&mpp->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
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 <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? 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
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(-) > > > > 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(&mpp->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(&mpp->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(&mpp->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 > hare@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
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
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(&mpp->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(&mpp->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(&mpp->features, - "queue_if_no_path"); - } - } if (!is_daemon && mpp->action != ACT_NOTHING) { conf = get_multipath_config();
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(-)