On Thu, 2022-03-10 at 20:02 -0600, Benjamin Marzinski wrote:
> If a reconfigure is delayed because multipathd is waiting on a change
> uevent for a new multipath device, the main thread will not pause,
> but
> will keep looping and rechecking to see if it can reconfigure.
>
> To solve this, when __post_config_state(DAEMON_IDLE) is called, if
> __delayed_reconfig is set we really do want to switch to the
> DAEMON_IDLE state, even if there is a pending reconfigure, since it's
> being delayed. When the last change uevent for a new map arrives (or
> we time out waiting for it), a reconfigure will get triggered.
I had thought about something like this, too. I think there's one good
reason to switch to DAEMON_IDLE even if reconfigure is delayed: if we
don't, and forever reason the uevents we expect arrive with large delay
or not at all, we risk being killed by systemd, which will kill
processes that stay in "RELOADING=1" state for more than
TimeoutStartSec seconds. It's unlikely, but I think we should try to
avoid it if we can, because we have no control about systemd's timeout
configuration.
> However, we need to avoid a race where the main thread calls
> enable_delayed_reconfig() and sets __delayed_reconfig, and then the
> uevent thread processes a change uevent that sets the state to
> DAEMON_CONFIGURE, an then the main thread calls post_config_state().
> In this case, multipathd would move to DAEMON_IDLE, even though
> the reconfigure should no longer be delayed. To avoid this, when
> schedule_reconfigure() is called and the daemon is currently in
> DAEMON_CONFIGURE or DAEMON_RUNNING, __delayed_reconfig should be
> cleared, so switching to DAEMON_IDLE will instead become
> DAEMON_CONFIGURE.
I suppose this would work. The part I don't like so much is that the
DAEMON_CONFIGURE logic remains complex and distributed over different
functions (__post_config_state(), schedule_reconfigure(), child())
which interact in non-obvious ways. I noticed that while looking into
Guozhonghua's problem yesterday - the logic is hard to grok, even
though I wrote a significant part of it myself. In particular, I have
started to dislike the complexity we added in __post_config_state(),
which today doesn't do what a caller would expect it does (which is:
simply setting the state passed to it). I'm aware that this complexity
was created by my own commit 250708c :-)
By adding extra semantics to the DAEMON_RUNNING state (which used to
simply mean "checkers running"), the logic gets even harder to
understand, IMO.
Please have a look at my alternative approach (@dm-devel: only posted
off-list so far). If you think that'd be a viable solution too, I'd
prefer it, because it moves most of the logic into a single place
(child()).
Regards,
Martin
>
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> multipathd/main.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 86b1745a..9bd1f530 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -309,6 +309,7 @@ static void __post_config_state(enum
> daemon_status state)
> * again and start another reconfigure cycle.
> */
> if (reconfigure_pending != FORCE_RELOAD_NONE &&
> + !__delayed_reconfig &&
> state == DAEMON_IDLE &&
> (old_state == DAEMON_CONFIGURE ||
> old_state == DAEMON_RUNNING)) {
> @@ -353,6 +354,7 @@ void schedule_reconfigure(enum force_reload_types
> requested_type)
> break;
> case DAEMON_CONFIGURE:
> case DAEMON_RUNNING:
> + __delayed_reconfig = false;
> reconfigure_pending = type;
> break;
> default:
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
