| Message ID | 20240713060506.2015463-10-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | path checker refactor and misc fixes | expand |
On Sat, 2024-07-13 at 02:04 -0400, Benjamin Marzinski wrote: > When handle_uninitialized_path() checks a path, it will either: > 1. trigger a new uevent to initalize the path, in which case it won't > check the path again until that uevent is processed and updates > pp->tick > 2. blacklist the path, in which case the path gets removed > 3. intialize the path correctly, in which case it sets pp->tick = 1 > 4. fail to initialize the path, in which case it was supposed to set > pp->tick to max_checkint, but instead it set pp->checkint to > max_checkint, which never worked correctly. > > By setting pp->tick to max_checkint at the start, > handle_uninitialized_path() will continue to work as it previously > did, > except in case 4, where in now works correctly. There's no point in > messing with pp->checkint for paths that haven't been initialized > yet. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com> Q: I suppose it makes sense to backport this to earlier releases? Martin
On Mon, Jul 15, 2024 at 06:00:33PM +0200, Martin Wilck wrote: > On Sat, 2024-07-13 at 02:04 -0400, Benjamin Marzinski wrote: > > When handle_uninitialized_path() checks a path, it will either: > > 1. trigger a new uevent to initalize the path, in which case it won't > > check the path again until that uevent is processed and updates > > pp->tick > > 2. blacklist the path, in which case the path gets removed > > 3. intialize the path correctly, in which case it sets pp->tick = 1 > > 4. fail to initialize the path, in which case it was supposed to set > > pp->tick to max_checkint, but instead it set pp->checkint to > > max_checkint, which never worked correctly. > > > > By setting pp->tick to max_checkint at the start, > > handle_uninitialized_path() will continue to work as it previously > > did, > > except in case 4, where in now works correctly. There's no point in > > messing with pp->checkint for paths that haven't been initialized > > yet. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > Reviewed-by: Martin Wilck <mwilck@suse.com> > > Q: I suppose it makes sense to backport this to earlier releases? It's not a horrible bug. Previously, we set pp->tick to checkint early in the function. This just means that these problematic paths will be checked every checkint instead of every max_checkint. Also, do you mean distros backporting this change? I'm not sure how much value there is in maintaining upstream stable branches. -Ben > > Martin
On Tue, 2024-07-16 at 10:52 -0400, Benjamin Marzinski wrote: > On Mon, Jul 15, 2024 at 06:00:33PM +0200, Martin Wilck wrote: > > On Sat, 2024-07-13 at 02:04 -0400, Benjamin Marzinski wrote: > > > When handle_uninitialized_path() checks a path, it will either: > > > 1. trigger a new uevent to initalize the path, in which case it > > > won't > > > check the path again until that uevent is processed and > > > updates > > > pp->tick > > > 2. blacklist the path, in which case the path gets removed > > > 3. intialize the path correctly, in which case it sets pp->tick = > > > 1 > > > 4. fail to initialize the path, in which case it was supposed to > > > set > > > pp->tick to max_checkint, but instead it set pp->checkint to > > > max_checkint, which never worked correctly. > > > > > > By setting pp->tick to max_checkint at the start, > > > handle_uninitialized_path() will continue to work as it > > > previously > > > did, > > > except in case 4, where in now works correctly. There's no point > > > in > > > messing with pp->checkint for paths that haven't been initialized > > > yet. > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > > Reviewed-by: Martin Wilck <mwilck@suse.com> > > > > Q: I suppose it makes sense to backport this to earlier releases? > > It's not a horrible bug. Previously, we set pp->tick to checkint > early > in the function. This just means that these problematic paths will > be checked every checkint instead of every max_checkint. > > Also, do you mean distros backporting this change? I'm not sure how > much value there is in maintaining upstream stable branches. Yes, I was referring to distro backporting. I don't want to start stable branches upstream, although we've had several cases where I thought it might be worth it. Martin
diff --git a/multipathd/main.c b/multipathd/main.c index 234fbf7d..5e89fae5 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2625,7 +2625,6 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp, { int newstate; int retrigger_tries; - unsigned int checkint, max_checkint; struct config *conf; int ret; @@ -2640,8 +2639,7 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp, conf = get_multipath_config(); retrigger_tries = conf->retrigger_tries; - checkint = conf->checkint; - max_checkint = conf->max_checkint; + pp->tick = conf->max_checkint; put_multipath_config(conf); if (pp->initialized == INIT_MISSING_UDEV) { @@ -2675,12 +2673,6 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp, } } - /* - * provision a next check soonest, - * in case we exit abnormally from here - */ - pp->tick = checkint; - newstate = check_path_state(pp); if (!strlen(pp->wwid) && @@ -2695,14 +2687,8 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp, if (pp->initialized == INIT_OK) { ev_add_path(pp, vecs, 1); pp->tick = 1; - } else { - if (ret == PATHINFO_SKIPPED) - return -1; - /* - * We failed multiple times to initialize this - * path properly. Don't re-check too often. - */ - pp->checkint = max_checkint; + } else if (ret == PATHINFO_SKIPPED) { + return -1; } } return 0;
When handle_uninitialized_path() checks a path, it will either: 1. trigger a new uevent to initalize the path, in which case it won't check the path again until that uevent is processed and updates pp->tick 2. blacklist the path, in which case the path gets removed 3. intialize the path correctly, in which case it sets pp->tick = 1 4. fail to initialize the path, in which case it was supposed to set pp->tick to max_checkint, but instead it set pp->checkint to max_checkint, which never worked correctly. By setting pp->tick to max_checkint at the start, handle_uninitialized_path() will continue to work as it previously did, except in case 4, where in now works correctly. There's no point in messing with pp->checkint for paths that haven't been initialized yet. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- multipathd/main.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-)