| Message ID | 1579227500-17196-11-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | Multipath patch dump | expand |
On Thu, 2020-01-16 at 20:18 -0600, Benjamin Marzinski wrote: > The way that the checkers async mode worked in multipathd didn't make > much sense. When multipathd starts up, all checker classes are in the > sync mode, and any pathinfo() calls on devices would run the checker > in > sync mode. However, the First time a checker class was used in > checkerloop, it would set that checker class to async (assuming > force_sync wasn't set). After that, no matter when a checker from > that > class was called, it would always run in async mode. Multipathd > doesn't > need to run checkers in sync mode at all, so don't. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmpathpersist/mpath_persist.c | 2 +- > libmultipath/discovery.c | 10 ++++------ > multipath/main.c | 1 + > 3 files changed, 6 insertions(+), 7 deletions(-) > Reviewed-by: Martin Wilck <mwilck@suse.com> -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, 2020-01-16 at 20:18 -0600, Benjamin Marzinski wrote: > The way that the checkers async mode worked in multipathd didn't make > much sense. When multipathd starts up, all checker classes are in the > sync mode, and any pathinfo() calls on devices would run the checker > in > sync mode. However, the First time a checker class was used in > checkerloop, it would set that checker class to async (assuming > force_sync wasn't set). After that, no matter when a checker from > that > class was called, it would always run in async mode. Multipathd > doesn't > need to run checkers in sync mode at all, so don't. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmpathpersist/mpath_persist.c | 2 +- > libmultipath/discovery.c | 10 ++++------ > multipath/main.c | 1 + > 3 files changed, 6 insertions(+), 7 deletions(-) > Reviewed-by: Martin Wilck <mwilck@suse.com> -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
>>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes:
Benjamin> The way that the checkers async mode worked in multipathd didn't make
Benjamin> much sense. When multipathd starts up, all checker classes are in the
Benjamin> sync mode, and any pathinfo() calls on devices would run the checker in
Benjamin> sync mode. However, the First time a checker class was used in
Benjamin> checkerloop, it would set that checker class to async (assuming
Benjamin> force_sync wasn't set). After that, no matter when a checker from that
Benjamin> class was called, it would always run in async mode. Multipathd doesn't
Benjamin> need to run checkers in sync mode at all, so don't.
Sorry, I had a hard time parsing this description, especially the last
sentence. Do you mean that checkers should default to async then,
instead of sync mode? And from looking at the code, don't you mean
that you force sync mode? I guess the question is whether checker
classes should default sync, or async. And if they move to async,
should they stay there?
Benjamin> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Benjamin> ---
Benjamin> libmpathpersist/mpath_persist.c | 2 +-
Benjamin> libmultipath/discovery.c | 10 ++++------
Benjamin> multipath/main.c | 1 +
Benjamin> 3 files changed, 6 insertions(+), 7 deletions(-)
Benjamin> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
Benjamin> index 603cfc3b..b2238f00 100644
Benjamin> --- a/libmpathpersist/mpath_persist.c
Benjamin> +++ b/libmpathpersist/mpath_persist.c
Benjamin> @@ -47,7 +47,7 @@ mpath_lib_init (void)
Benjamin> condlog(0, "Failed to initialize multipath config.");
Benjamin> return NULL;
Benjamin> }
Benjamin> -
Benjamin> + conf->force_sync = 1;
Benjamin> set_max_fds(conf->max_fds);
Benjamin> return conf;
Benjamin> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
Benjamin> index d2773c3a..1ab093f4 100644
Benjamin> --- a/libmultipath/discovery.c
Benjamin> +++ b/libmultipath/discovery.c
Benjamin> @@ -1643,12 +1643,10 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate)
Benjamin> if (pp->mpp && !c->mpcontext)
Benjamin> checker_mp_init(c, &pp->mpp->mpcontext);
Benjamin> checker_clear_message(c);
Benjamin> - if (daemon) {
Benjamin> - if (conf->force_sync == 0)
Benjamin> - checker_set_async(c);
Benjamin> - else
Benjamin> - checker_set_sync(c);
Benjamin> - }
Benjamin> + if (conf->force_sync == 0)
Benjamin> + checker_set_async(c);
Benjamin> + else
Benjamin> + checker_set_sync(c);
Benjamin> if (!conf->checker_timeout &&
Benjamin> sysfs_get_timeout(pp, &(c->timeout)) <= 0)
c-> timeout = DEF_TIMEOUT;
Benjamin> diff --git a/multipath/main.c b/multipath/main.c
Benjamin> index 4f4d8e89..aebabd9b 100644
Benjamin> --- a/multipath/main.c
Benjamin> +++ b/multipath/main.c
Benjamin> @@ -905,6 +905,7 @@ main (int argc, char *argv[])
Benjamin> exit(RTVL_FAIL);
Benjamin> multipath_conf = conf;
conf-> retrigger_tries = 0;
Benjamin> + conf->force_sync = 1;
Benjamin> while ((arg = getopt(argc, argv, ":adcChl::FfM:v:p:b:BrR:itTquUwW")) != EOF ) {
Benjamin> switch(arg) {
Benjamin> case 1: printf("optarg : %s\n",optarg);
Benjamin> --
Benjamin> 2.17.2
Benjamin> --
Benjamin> dm-devel mailing list
Benjamin> dm-devel@redhat.com
Benjamin> https://www.redhat.com/mailman/listinfo/dm-devel
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
On Sat, Jan 18, 2020 at 12:43:49PM -0500, John Stoffel wrote: > >>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes: > > Benjamin> The way that the checkers async mode worked in multipathd didn't make > Benjamin> much sense. When multipathd starts up, all checker classes are in the > Benjamin> sync mode, and any pathinfo() calls on devices would run the checker in > Benjamin> sync mode. However, the First time a checker class was used in > Benjamin> checkerloop, it would set that checker class to async (assuming > Benjamin> force_sync wasn't set). After that, no matter when a checker from that > Benjamin> class was called, it would always run in async mode. Multipathd doesn't > Benjamin> need to run checkers in sync mode at all, so don't. > > Sorry, I had a hard time parsing this description, especially the last > sentence. Do you mean that checkers should default to async then, > instead of sync mode? And from looking at the code, don't you mean > that you force sync mode? I guess the question is whether checker > classes should default sync, or async. And if they move to async, > should they stay there? > Sorry. I mean that right now multipathd runs the checkers from pathinfo(), wait_for_pending_paths() and check_path(). When multipathd starts, all checkers are in sync mode. The first time a checker is run from check_path(), it is switched to async mode, and stays there for the rest of the time multipathd is runing. There is no need for multipathd to run checkers in sync mode at all, so we shouldn't be doing so for these first checks. -Ben > > > Benjamin> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > Benjamin> --- > Benjamin> libmpathpersist/mpath_persist.c | 2 +- > Benjamin> libmultipath/discovery.c | 10 ++++------ > Benjamin> multipath/main.c | 1 + > Benjamin> 3 files changed, 6 insertions(+), 7 deletions(-) > > Benjamin> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c > Benjamin> index 603cfc3b..b2238f00 100644 > Benjamin> --- a/libmpathpersist/mpath_persist.c > Benjamin> +++ b/libmpathpersist/mpath_persist.c > Benjamin> @@ -47,7 +47,7 @@ mpath_lib_init (void) > Benjamin> condlog(0, "Failed to initialize multipath config."); > Benjamin> return NULL; > Benjamin> } > Benjamin> - > Benjamin> + conf->force_sync = 1; > Benjamin> set_max_fds(conf->max_fds); > > Benjamin> return conf; > Benjamin> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > Benjamin> index d2773c3a..1ab093f4 100644 > Benjamin> --- a/libmultipath/discovery.c > Benjamin> +++ b/libmultipath/discovery.c > Benjamin> @@ -1643,12 +1643,10 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate) > Benjamin> if (pp->mpp && !c->mpcontext) > Benjamin> checker_mp_init(c, &pp->mpp->mpcontext); > Benjamin> checker_clear_message(c); > Benjamin> - if (daemon) { > Benjamin> - if (conf->force_sync == 0) > Benjamin> - checker_set_async(c); > Benjamin> - else > Benjamin> - checker_set_sync(c); > Benjamin> - } > Benjamin> + if (conf->force_sync == 0) > Benjamin> + checker_set_async(c); > Benjamin> + else > Benjamin> + checker_set_sync(c); > Benjamin> if (!conf->checker_timeout && > Benjamin> sysfs_get_timeout(pp, &(c->timeout)) <= 0) > c-> timeout = DEF_TIMEOUT; > Benjamin> diff --git a/multipath/main.c b/multipath/main.c > Benjamin> index 4f4d8e89..aebabd9b 100644 > Benjamin> --- a/multipath/main.c > Benjamin> +++ b/multipath/main.c > Benjamin> @@ -905,6 +905,7 @@ main (int argc, char *argv[]) > Benjamin> exit(RTVL_FAIL); > Benjamin> multipath_conf = conf; > conf-> retrigger_tries = 0; > Benjamin> + conf->force_sync = 1; > Benjamin> while ((arg = getopt(argc, argv, ":adcChl::FfM:v:p:b:BrR:itTquUwW")) != EOF ) { > Benjamin> switch(arg) { > Benjamin> case 1: printf("optarg : %s\n",optarg); > Benjamin> -- > Benjamin> 2.17.2 > > Benjamin> -- > Benjamin> dm-devel mailing list > Benjamin> dm-devel@redhat.com > Benjamin> https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
>>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes: Benjamin> On Sat, Jan 18, 2020 at 12:43:49PM -0500, John Stoffel wrote: >> >>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes: >> Benjamin> The way that the checkers async mode worked in multipathd didn't make Benjamin> much sense. When multipathd starts up, all checker classes are in the Benjamin> sync mode, and any pathinfo() calls on devices would run the checker in Benjamin> sync mode. However, the First time a checker class was used in Benjamin> checkerloop, it would set that checker class to async (assuming Benjamin> force_sync wasn't set). After that, no matter when a checker from that Benjamin> class was called, it would always run in async mode. Multipathd doesn't Benjamin> need to run checkers in sync mode at all, so don't. >> >> Sorry, I had a hard time parsing this description, especially the last >> sentence. Do you mean that checkers should default to async then, >> instead of sync mode? And from looking at the code, don't you mean >> that you force sync mode? I guess the question is whether checker >> classes should default sync, or async. And if they move to async, >> should they stay there? >> Benjamin> Sorry. I mean that right now multipathd runs the checkers from pathinfo(), Benjamin> wait_for_pending_paths() and check_path(). When multipathd starts, all Benjamin> checkers are in sync mode. The first time a checker is run from Benjamin> check_path(), it is switched to async mode, and stays there for the rest Benjamin> of the time multipathd is runing. Benjamin> There is no need for multipathd to run checkers in sync mode at all, so Benjamin> we shouldn't be doing so for these first checks. Thanks, that makes the entire issue much more clear. This raises the question of why there is a sync mode at all then? In any case, the above makes the issue more understandable. John -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, Jan 20, 2020 at 03:20:14PM -0500, John Stoffel wrote: > >>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes: > > Benjamin> On Sat, Jan 18, 2020 at 12:43:49PM -0500, John Stoffel wrote: > >> >>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes: > >> > Benjamin> The way that the checkers async mode worked in multipathd didn't make > Benjamin> much sense. When multipathd starts up, all checker classes are in the > Benjamin> sync mode, and any pathinfo() calls on devices would run the checker in > Benjamin> sync mode. However, the First time a checker class was used in > Benjamin> checkerloop, it would set that checker class to async (assuming > Benjamin> force_sync wasn't set). After that, no matter when a checker from that > Benjamin> class was called, it would always run in async mode. Multipathd doesn't > Benjamin> need to run checkers in sync mode at all, so don't. > >> > >> Sorry, I had a hard time parsing this description, especially the last > >> sentence. Do you mean that checkers should default to async then, > >> instead of sync mode? And from looking at the code, don't you mean > >> that you force sync mode? I guess the question is whether checker > >> classes should default sync, or async. And if they move to async, > >> should they stay there? > >> > > Benjamin> Sorry. I mean that right now multipathd runs the checkers from pathinfo(), > Benjamin> wait_for_pending_paths() and check_path(). When multipathd starts, all > Benjamin> checkers are in sync mode. The first time a checker is run from > Benjamin> check_path(), it is switched to async mode, and stays there for the rest > Benjamin> of the time multipathd is runing. > > Benjamin> There is no need for multipathd to run checkers in sync mode at all, so > Benjamin> we shouldn't be doing so for these first checks. > > Thanks, that makes the entire issue much more clear. This raises the > question of why there is a sync mode at all then? In any case, the > above makes the issue more understandable. The multipath command, which uses the same checkers, runs in sync mode. -Ben > > John -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
>>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes: Benjamin> On Mon, Jan 20, 2020 at 03:20:14PM -0500, John Stoffel wrote: >> >>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes: >> Benjamin> On Sat, Jan 18, 2020 at 12:43:49PM -0500, John Stoffel wrote: >> >> >>>>> "Benjamin" == Benjamin Marzinski <bmarzins@redhat.com> writes: >> >> Benjamin> The way that the checkers async mode worked in multipathd didn't make Benjamin> much sense. When multipathd starts up, all checker classes are in the Benjamin> sync mode, and any pathinfo() calls on devices would run the checker in Benjamin> sync mode. However, the First time a checker class was used in Benjamin> checkerloop, it would set that checker class to async (assuming Benjamin> force_sync wasn't set). After that, no matter when a checker from that Benjamin> class was called, it would always run in async mode. Multipathd doesn't Benjamin> need to run checkers in sync mode at all, so don't. >> >> >> >> Sorry, I had a hard time parsing this description, especially the last >> >> sentence. Do you mean that checkers should default to async then, >> >> instead of sync mode? And from looking at the code, don't you mean >> >> that you force sync mode? I guess the question is whether checker >> >> classes should default sync, or async. And if they move to async, >> >> should they stay there? >> >> >> Benjamin> Sorry. I mean that right now multipathd runs the checkers from pathinfo(), Benjamin> wait_for_pending_paths() and check_path(). When multipathd starts, all Benjamin> checkers are in sync mode. The first time a checker is run from Benjamin> check_path(), it is switched to async mode, and stays there for the rest Benjamin> of the time multipathd is runing. >> Benjamin> There is no need for multipathd to run checkers in sync mode at all, so Benjamin> we shouldn't be doing so for these first checks. >> >> Thanks, that makes the entire issue much more clear. This raises the >> question of why there is a sync mode at all then? In any case, the >> above makes the issue more understandable. Benjamin> The multipath command, which uses the same checkers, runs in sync mode. Crystal clean now. Thanks for your patience! -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c index 603cfc3b..b2238f00 100644 --- a/libmpathpersist/mpath_persist.c +++ b/libmpathpersist/mpath_persist.c @@ -47,7 +47,7 @@ mpath_lib_init (void) condlog(0, "Failed to initialize multipath config."); return NULL; } - + conf->force_sync = 1; set_max_fds(conf->max_fds); return conf; diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index d2773c3a..1ab093f4 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -1643,12 +1643,10 @@ get_state (struct path * pp, struct config *conf, int daemon, int oldstate) if (pp->mpp && !c->mpcontext) checker_mp_init(c, &pp->mpp->mpcontext); checker_clear_message(c); - if (daemon) { - if (conf->force_sync == 0) - checker_set_async(c); - else - checker_set_sync(c); - } + if (conf->force_sync == 0) + checker_set_async(c); + else + checker_set_sync(c); if (!conf->checker_timeout && sysfs_get_timeout(pp, &(c->timeout)) <= 0) c->timeout = DEF_TIMEOUT; diff --git a/multipath/main.c b/multipath/main.c index 4f4d8e89..aebabd9b 100644 --- a/multipath/main.c +++ b/multipath/main.c @@ -905,6 +905,7 @@ main (int argc, char *argv[]) exit(RTVL_FAIL); multipath_conf = conf; conf->retrigger_tries = 0; + conf->force_sync = 1; while ((arg = getopt(argc, argv, ":adcChl::FfM:v:p:b:BrR:itTquUwW")) != EOF ) { switch(arg) { case 1: printf("optarg : %s\n",optarg);
The way that the checkers async mode worked in multipathd didn't make much sense. When multipathd starts up, all checker classes are in the sync mode, and any pathinfo() calls on devices would run the checker in sync mode. However, the First time a checker class was used in checkerloop, it would set that checker class to async (assuming force_sync wasn't set). After that, no matter when a checker from that class was called, it would always run in async mode. Multipathd doesn't need to run checkers in sync mode at all, so don't. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmpathpersist/mpath_persist.c | 2 +- libmultipath/discovery.c | 10 ++++------ multipath/main.c | 1 + 3 files changed, 6 insertions(+), 7 deletions(-)