On Thu, 2024-06-27 at 14:39 -0400, Benjamin Marzinski wrote:
> On Thu, Jun 27, 2024 at 02:33:00PM -0400, Benjamin Marzinski wrote:
> > On Thu, Jun 27, 2024 at 09:47:33AM +0200, Martin Wilck wrote:
> > > On Wed, 2024-06-05 at 19:22 -0400, Benjamin Marzinski wrote:
> > > > Make sure that all callers of set_no_path_retry() update the
> > > > multipath->features string from the kernel before calling it,
> > > > so that
> > > > they have the current queueing state to check.
> > > >
> > > > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> > >
> > > I am not sure about this one.
> > >
> > > refresh_multipath() is not a cheap operation, requiring at least
> > > 3
> > > device-mapper ioctls, and we hold the multipath lock while we
> > > call it,
> > > meaning that multipathd can't take care of other important things
> > > while
> > > it's handling this CLI request.
> > >
> > > OTOH, the kernel only changes the features string when the map is
> > > reloaded. Nobody should be reloading the map under normal
> > > conditions
> > > anyway, and if they do, we would see an uevent shortly
> > > afterwards. IOW,
> > > I think it's safe to assume that multipathd has the correct
> > > features
> > > string cached, except in pathological situations.
> > >
> > > Am I overlooking something? Do you have evidence of errors
> > > occuring
> > > because multipathd has a wrong features string cached?
> >
> > Well, I did run into a situation where I couldn't disable queueing
> > because of mpp->features being out of date. I fixed that specific
> > issue
> > in patch 1/7. But in a broader context, the kernel doesn't send out
> > any
> > events when it gets a message to change the queueing mode in the
> > features string, so there is always a possibility that something
> > outside
> > of multipathd changed it, and multipathd doesn't find out about it.
Right, and that's a general problem that can't be solved in multipath-
tools. All we can do is make sure that our own tool - multipath -
doesn't mess with the kernel state at all while multipathd is running.
If users use "dmsetup message" or the like while multipathd is running,
they're obviously on their own. Perhaps we should really disable all
code in multipath that falls back to local actions, or at least run the
fallback code only when the user adds an option like "--force".
> > Normally, check_path() will find out and resolve the issue within
> > seconds. However, if the multipath device lost all its path
> > devices,
> > then check_path() won't ever call set_no_path_retry() on it
>
> Err... It won't ever call update_multipath_strings(), which is
> what will sync the mpp->features string with the kernel and make
> set_no_path_retry() do the right thing.
Yeah, check_path() is not the right place for handling maps with no
paths.
I have thought for a long time that the periodic checker, instead of
blindly going through the paths vec, should follow the multipath vec,
handle each path of the mpp in turn, and when that's done, act on the
map. If we did it this way, we could also handle the special case of an
empty map in the checker loop.
> > and nothing
> > will resync it with the kernel state until an event comes in.
> >
> > Here's a (admittedly fairly improbable) sequence of events that
> > would
> > get you into problems.
> >
> > - There is a multipath device with no paths that is currently in
> > recovery mode waiting to timeout. It's opened by something with
> > outstanding IO.
> >
> > - A user runs "multipath -f" to remove the device, and deligation
> > to
> > multipathd times out so multipath attempts to locally remove the
> > device. It disables queueing and attempts the remove, which fails
> > because the device is still in use.
> >
> > - multipathd hits the retries timeout and disables queueing.
> >
> > - multipath restores queueing because the remove failed.
> >
> > - more IO comes into the multipath device.
> >
> > In this case, the multipath device will be stuck open due to queued
> > IO. The user could reasonably try to disable queueing to resolve
> > this
> > issue. This will report success, but not actually do anything.
> because multipathd never syncs the features string with the kernel
> before calling set_no_path_retry().
Are you suggesting that we call refresh_multipath() (or
update_multipath_table()) from set_no_path_retry()?
Before 0f850db ("multipathd: clean up set_no_path_retry"), multipathd
would always set queue_if_no_path to the value it deemed correct, and I
think this was ok, and generally race-free. IIUC, the main purpose of
0f850db was to avoid unnecessary message ioctls (which would cause
locking and flag manipulation in the kernel).
0f850db introduced the check of the current features string as cached
by multipathd. Later we added some patches to be more confident that
this cached string correctly reflects the kernel state, but we can
never by 100% sure, of course.
But if we now call refresh_multipath() every time in
set_no_path_retry(), we'll be replacing one DM ioctl (the original
dm_message()) with multiple ioctls, while still maintaining a race
window. That wouldn't make sense to me.
> >
> > Granted, there is an unavoidable race here. It's always possible
> > that
> > after multipathd updates the features string, it gets changed
> > outside of
> > its control. But this cuts the window down to something small
> > instead
> > of anytime after you lost all your paths. Any if you get unlucky
> > enough
> > to hit that window, you can just rerun the command and it will
> > work.
> >
> > My main reason for wanting this is that cli_disable_queueing() (and
> > friends) is not a command you run all the time. It's a command to
> > run
> > when you are cleaning up a mess. It doesn't have to be fast. It has
> > to
> > work when things are stuck.
> >
> > Thoughts?
I remain somewhat confused how this should be handled in general; see
above. But I won't insist on rejecting your patch.
Martin
