[dm-devel] [PATCH 4/5] multipathd: reset __delayed_reconfig from ev_add_map()

[Martin Wilck]

On Wed, 2022-03-16 at 15:13 -0500, Benjamin Marzinski wrote:
> On Mon, Mar 14, 2022 at 10:30:35PM +0100, mwilck at suse.com wrote:
> > From: Martin Wilck <mwilck at suse.com>
> > 
> > With the previous patch, one race condition between child() and
> > the uevent handler (ev_add_map()) remains: 1. child() sets
> > __delayed_reconfig, 2. ev_add_map() calls schedule_reconfigure()
> > and
> > thus DAEMON_CONFIGURE, 3. child() sets DAEMON_IDLE. This would
> > cause
> > the pending reconfigure request to be missed.
> > 
> > To fix this, reset __delayed_reconfig immediately from ev_add_map()
> > (and likewise, missing_uev_wait_tick()). This way the wait loop in
> > child() terminates on the reconfigure_pending condition.
> > 
> > Also, these schedule_reconfigure() callers don't really request a
> > reconfigure() call; they just unblock processing previously
> > requested
> > reconfigure() calls. Add a new helper, unblock_reconfigure(), that
> > does just that.
> > 
> 
> Doesn't unblock_reconfigure() allow us to set the state to
> DAEMON_CONFIGURE, right after the checkerloop() has set it to
> DAEMON_RUNNING.

Right. I added the __post_config_state() there in order to be sure that
child() would be woken up. But unblock_reconfigure() should only call
__post_config_state if running_state is DAEMON_IDLE, like
schedule_reconfigure(). Otherwise, it can be sure that some other
process will wake up child().

>  schedule_reconfigure() didn't update the state if it was
> DAEMON_RUNNING. Instead, it would wait till checkerloop() set the
> state
> back to DAEMON_IDLE for the reconfigure to start.

> I have also come up with a different bug that effects these fixes,
> including mine. But it will effect this fix worse. This fix assumes
> that
> when a reconfigure is delayed, it should remain delayed until either
> a
> change event happens on the multipath device (ev_add_map) or
> multipathd
> times out waiting for one (missing_uev_wait_tick). However the map
> could
> be removed before either of those things happen.  It's possible that
> multipathd could get a remove uevent after the add event but before
> the
> change event. Either that or a multipathd client command could remove
> it, or a dm event could happen either removing the device, or
> updating
> it, but with __setup_multipath() removing it. Those are just the
> examples I came up with off the top of my head.


> So there's a specific problem where we don't handle clearing
> __delayed_reconfig if a map was removed. That can be handled in a
> different patch.  But this, or any other oversight we might have here
> can be mitigated by being more proactive in removing
> __delayed_reconfig.
> For instance, when then main thread tries to reconfigure, it checks
> need_to_delay_reconfig(), and only does the reconfigure if this
> returns
> that it's ok.  __delayed_reconfig mostly exists so that the main
> thread
> doesn't need to grab the vecs lock and loop through the the multipath
> devices to know if it need to delay, but need_to_delay_reconfig() is
> the definitive answer. When the main thread checks this, we should be
> updating __delayed_reconfig to match the result. This patch removes
> the
> "__delayed_reconfig = false;" line, which I think is a mistake.

Agreed. My thinking was that this is done by the uevent handler when
the state of need_to_delay_reconfig() changes. But you're right with
the "remove" special case. We should re-add this, it definitely doesn't
hurt.

> Imagine if, because a map got removed before ev_add_map() was called,
> __delayed_reconfig was true, even though there were no maps with
> mpp->wait_for_udev set. Any existing config request would still be
> delayed, but if another reconfigure was requested not only would it
> happen, but if the "__delayed_reconfig = false;" line was back,
> __delayed_reconfig would go back to the correct value.

So we should call unblock_reconfigure() from remove_map().
Unfortunately that requires another callback from libmultipath into
multipathd. Trying to figure it out... 

Thanks,
Martin