[dm-devel] [PATCH 0/2] fix looping when reconfigure is delayed

[dm-devel] [PATCH 0/2] fix looping when reconfigure is delayed

[Benjamin Marzinski]

This patchset fixes Guozhonghua's looping issue for me. It's the first
patch from Martin's patchset, plus a simple patch to make sure that we
really do switch to the idle state when we're delaying the reconfigure,
and that we stop delaying the reconfigure if we schedule a reconfigure.

Benjamin Marzinski (1):
  multipathd: don't keep looping when config is delayed

Martin Wilck (1):
  multipathd: set reload_type in when calling reconfigure()

 multipathd/main.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

-- 
2.17.2

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

[dm-devel] [PATCH 1/2] multipathd: set reload_type in when calling reconfigure()

[Benjamin Marzinski]

From: Martin Wilck <mwilck@suse.com>

Only set reload_type (and reset reconfigure_pending) immediately
before we actually call reconfigure(). This allows us to get rid of
the reload_type global variable, and makes sure that reconfigure()
is called with the reload type that was last requested.

While at it, convert configure() and reconfigure() to static functions.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index f2c0b280..86b1745a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -287,14 +287,10 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 
 /* Don't access this variable without holding config_lock */
 static enum force_reload_types reconfigure_pending = FORCE_RELOAD_NONE;
-/* Only set while changing to DAEMON_CONFIGURE, and only access while
- * reconfiguring or scheduling a delayed reconfig in DAEMON_CONFIGURE */
-static volatile enum force_reload_types reload_type = FORCE_RELOAD_NONE;
 
 static void enable_delayed_reconfig(void)
 {
 	pthread_mutex_lock(&config_lock);
-	reconfigure_pending = reload_type;
 	__delayed_reconfig = true;
 	pthread_mutex_unlock(&config_lock);
 }
@@ -324,11 +320,6 @@ static void __post_config_state(enum daemon_status state)
 			old_state = DAEMON_IDLE;
 			state = DAEMON_CONFIGURE;
 		}
-		if (state == DAEMON_CONFIGURE) {
-			reload_type = (reconfigure_pending == FORCE_RELOAD_YES) ? FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
-			reconfigure_pending = FORCE_RELOAD_NONE;
-			__delayed_reconfig = false;
-		}
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
 		do_sd_notify(old_state, state);
@@ -2714,8 +2705,8 @@ checkerloop (void *ap)
 	return NULL;
 }
 
-int
-configure (struct vectors * vecs)
+static int
+configure (struct vectors * vecs, enum force_reload_types reload_type)
 {
 	struct multipath * mpp;
 	struct path * pp;
@@ -2846,8 +2837,8 @@ void rcu_free_config(struct rcu_head *head)
 	free_config(conf);
 }
 
-int
-reconfigure (struct vectors * vecs)
+static int
+reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
 {
 	struct config * old, *conf;
 	int old_marginal_pathgroups;
@@ -2894,8 +2885,7 @@ reconfigure (struct vectors * vecs)
 #ifdef FPIN_EVENT_HANDLER
 	fpin_clean_marginal_dev_list(NULL);
 #endif
-	configure(vecs);
-
+	configure(vecs, reload_type);
 
 	return 0;
 }
@@ -3406,13 +3396,21 @@ child (__attribute__((unused)) void *param)
 			break;
 		if (state == DAEMON_CONFIGURE) {
 			int rc = 0;
+			enum force_reload_types reload_type;
 
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
 			lock(&vecs->lock);
 			pthread_testcancel();
-			if (!need_to_delay_reconfig(vecs))
-				rc = reconfigure(vecs);
-			else
+			if (!need_to_delay_reconfig(vecs)) {
+				pthread_mutex_lock(&config_lock);
+				reload_type = reconfigure_pending == FORCE_RELOAD_YES ?
+					FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
+				reconfigure_pending = FORCE_RELOAD_NONE;
+				__delayed_reconfig = false;
+				pthread_mutex_unlock(&config_lock);
+
+				rc = reconfigure(vecs, reload_type);
+			} else
 				enable_delayed_reconfig();
 			lock_cleanup_pop(vecs->lock);
 			if (!rc)
-- 
2.17.2

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

[dm-devel] [PATCH 2/2] multipathd: don't keep looping when config is delayed

[Benjamin Marzinski]

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.

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.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 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:
-- 
2.17.2

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

Re: [dm-devel] [PATCH 2/2] multipathd: don't keep looping when config is delayed

[Martin Wilck]

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@redhat.com>
> ---
>  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@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/2] multipathd: don't keep looping when config is delayed

[Benjamin Marzinski]

On Fri, Mar 11, 2022 at 10:21:29AM +0000, Martin Wilck wrote:
> 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()).
> 

Err.. Patch 2 is still borken. The child process will only stop waiting
in the DAEMON_IDLE state and perform the reconfigure if
__delayed_reconfig is false. The only way that __delayed_reconfig can be
set to false is when a reconfigure actually happens. So you can fail to
reconfig if:

- main thread notices that it needs to delay the reconfigure(), and sets
  __delayed_reconfig to true.
- the uevent thread processes a change event on the last device that was
  delaying the reconfigure and calls schedule_reconfigure(), which sets
  reconfigure_pending, but doesn't set __delayed_reconfig to false
- the main thread calls post_config_state(DAEMON_IDLE)

The solution is to set __delayed_reconfig to false in
schedule_reconfigure().

Patch 3 looks fine.

While we're looking at this, does running_state need to be a volatile,
given that we only ever access it while holding the config_lock? 

-Ben
> Regards,
> Martin
> 
> 
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  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@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel