mbox series

[0/4] Add "reconfigure all" multipath command

Message ID 1632180076-9294-1-git-send-email-bmarzins@redhat.com (mailing list archive)
Headers show
Series Add "reconfigure all" multipath command | expand

Message

Benjamin Marzinski Sept. 20, 2021, 11:21 p.m. UTC 
This patchset is supposed to replace Martin's

multipathd: add "force_reconfigure" option

patch from his uxlsnr overhaul patchset. It also makes the default
reconfigure be a weak reconfigure, but instead of adding a configuration
option to control this, it adds a new multipathd command,
"reconfigure all", to do a full reconfigure. The HUP signal is left
doing only weak reconfigures.
 
In order to keep from having two states that are handled nearly
identically, the code adds an extra variable to track the type of
configuration that was selected, but this could easily be switch to
use a new DAEMON_CONFIGURE_ALL state instead.
 
The final patch, that added the new command, is meant to apply on top of
Martin's changed client handler code. I can send one that works with the
current client handler code, if people would rather review that.

Benjamin Marzinski (4):
  multipathd: move delayed_reconfig out of struct config
  multipathd: remove reconfigure from header file.
  multipathd: pass in the type of reconfigure
  multipathd: add "reconfigure all" command.

 libmultipath/config.h     |  1 -
 libmultipath/configure.c  |  2 +-
 multipath/main.c          |  2 +-
 multipathd/cli.c          |  1 +
 multipathd/cli.h          |  2 +
 multipathd/cli_handlers.c | 12 +++++-
 multipathd/main.c         | 91 +++++++++++++++++++++++----------------
 multipathd/main.h         |  3 +-
 8 files changed, 70 insertions(+), 44 deletions(-)

Comments

Xose Vazquez Perez Sept. 24, 2021, 8:44 p.m. UTC | #1 
On 9/21/21 01:21, Benjamin Marzinski wrote:

> This patchset is supposed to replace Martin's
> 
> multipathd: add "force_reconfigure" option
> 
> patch from his uxlsnr overhaul patchset. It also makes the default
> reconfigure be a weak reconfigure, but instead of adding a configuration
> option to control this, it adds a new multipathd command,
> "reconfigure all", to do a full reconfigure. The HUP signal is left
> doing only weak reconfigures.
>   
> In order to keep from having two states that are handled nearly
> identically, the code adds an extra variable to track the type of
> configuration that was selected, but this could easily be switch to
> use a new DAEMON_CONFIGURE_ALL state instead.
>   
> The final patch, that added the new command, is meant to apply on top of
> Martin's changed client handler code. I can send one that works with the
> current client handler code, if people would rather review that.

This change is going to affect some places, raw search:

$ git grep reconfigure
libdmmp/libdmmp.c:      snprintf(cmd, _IPC_MAX_CMD_LEN, "%s", "reconfigure");
libmultipath/configure.c:        * If we are in this code path (startup or forced reconfigure),
libmultipath/foreign.h:  * don't support it. "multipathd reconfigure" starts foreign device
libmultipath/foreign.h:  * This is called if multipathd reconfigures itself.
multipath/main.c:               p += snprintf(p, n, "reconfigure");
multipathd/cli.c:       r += add_key(keys, "reconfigure", RECONFIGURE, 0);
multipathd/cli_handlers.c:cli_reconfigure(void * v, char ** reply, int * len, void * data)
multipathd/cli_handlers.c:      condlog(2, "reconfigure (operator)");
multipathd/cli_handlers.h:int cli_reconfigure(void * v, char ** reply, int * len, void * data);
multipathd/main.c:                              condlog(2, "reconfigure (delayed)");
multipathd/main.c:      set_unlocked_handler_callback(RECONFIGURE, cli_reconfigure);
multipathd/main.c:              condlog(2, "reconfigure (delayed)");
multipathd/main.c:reconfigure (struct vectors * vecs)
multipathd/main.c:              condlog(2, "reconfigure (signal)");
multipathd/main.c:                              reconfigure(vecs);
multipathd/main.h:int reconfigure (struct vectors *);
multipathd/multipathd.8:happens, it will reconfigure the multipath map the path belongs to, so that this
multipathd/multipathd.8:.B reconfigure
multipathd/multipathd.service:ExecReload=/sbin/multipathd reconfigure
multipathd/uxlsnr.c:     * do it once per reconfigure */

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Benjamin Marzinski Sept. 27, 2021, 3:11 p.m. UTC | #2 
On Fri, Sep 24, 2021 at 10:44:46PM +0200, Xose Vazquez Perez wrote:
> On 9/21/21 01:21, Benjamin Marzinski wrote:
> 
> >This patchset is supposed to replace Martin's
> >
> >multipathd: add "force_reconfigure" option
> >
> >patch from his uxlsnr overhaul patchset. It also makes the default
> >reconfigure be a weak reconfigure, but instead of adding a configuration
> >option to control this, it adds a new multipathd command,
> >"reconfigure all", to do a full reconfigure. The HUP signal is left
> >doing only weak reconfigures.
> >In order to keep from having two states that are handled nearly
> >identically, the code adds an extra variable to track the type of
> >configuration that was selected, but this could easily be switch to
> >use a new DAEMON_CONFIGURE_ALL state instead.
> >The final patch, that added the new command, is meant to apply on top of
> >Martin's changed client handler code. I can send one that works with the
> >current client handler code, if people would rather review that.
> 
> This change is going to affect some places, raw search:

Yes. I specifically broke the code that actually changes how multipathd
operates from a user' point of view into a seperate patch (4/4) because
distributions might need to revert in, if they want to pull in recent
upstream changes, but don't what this kind of change in multipathd's
behavior.

I admit, this patchset needs to include documentation to mention the
changed behavior. I'll add that.  But I'm not sure what to make of the
list below.  I don't see any code in it that I didn't think about.  We
can disagree as to whether, for instance, dmmp_reconfig() should do a
full or a weak reconfig. But without some more information, I'm not sure
what you are asking of me.

-Ben

> $ git grep reconfigure
> libdmmp/libdmmp.c:      snprintf(cmd, _IPC_MAX_CMD_LEN, "%s", "reconfigure");
> libmultipath/configure.c:        * If we are in this code path (startup or forced reconfigure),
> libmultipath/foreign.h:  * don't support it. "multipathd reconfigure" starts foreign device
> libmultipath/foreign.h:  * This is called if multipathd reconfigures itself.
> multipath/main.c:               p += snprintf(p, n, "reconfigure");
> multipathd/cli.c:       r += add_key(keys, "reconfigure", RECONFIGURE, 0);
> multipathd/cli_handlers.c:cli_reconfigure(void * v, char ** reply, int * len, void * data)
> multipathd/cli_handlers.c:      condlog(2, "reconfigure (operator)");
> multipathd/cli_handlers.h:int cli_reconfigure(void * v, char ** reply, int * len, void * data);
> multipathd/main.c:                              condlog(2, "reconfigure (delayed)");
> multipathd/main.c:      set_unlocked_handler_callback(RECONFIGURE, cli_reconfigure);
> multipathd/main.c:              condlog(2, "reconfigure (delayed)");
> multipathd/main.c:reconfigure (struct vectors * vecs)
> multipathd/main.c:              condlog(2, "reconfigure (signal)");
> multipathd/main.c:                              reconfigure(vecs);
> multipathd/main.h:int reconfigure (struct vectors *);
> multipathd/multipathd.8:happens, it will reconfigure the multipath map the path belongs to, so that this
> multipathd/multipathd.8:.B reconfigure
> multipathd/multipathd.service:ExecReload=/sbin/multipathd reconfigure
> multipathd/uxlsnr.c:     * do it once per reconfigure */

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Martin Wilck Sept. 28, 2021, 8:25 a.m. UTC | #3 
On Mon, 2021-09-27 at 10:11 -0500, Benjamin Marzinski wrote:
> On Fri, Sep 24, 2021 at 10:44:46PM +0200, Xose Vazquez Perez wrote:
> > On 9/21/21 01:21, Benjamin Marzinski wrote:
> > 
> > > This patchset is supposed to replace Martin's
> > > 
> > > multipathd: add "force_reconfigure" option
> > > 
> > > patch from his uxlsnr overhaul patchset. It also makes the
> > > default
> > > reconfigure be a weak reconfigure, but instead of adding a
> > > configuration
> > > option to control this, it adds a new multipathd command,
> > > "reconfigure all", to do a full reconfigure. The HUP signal is
> > > left
> > > doing only weak reconfigures.
> > > In order to keep from having two states that are handled nearly
> > > identically, the code adds an extra variable to track the type of
> > > configuration that was selected, but this could easily be switch
> > > to
> > > use a new DAEMON_CONFIGURE_ALL state instead.
> > > The final patch, that added the new command, is meant to apply on
> > > top of
> > > Martin's changed client handler code. I can send one that works
> > > with the
> > > current client handler code, if people would rather review that.
> > 
> > This change is going to affect some places, raw search:
> 
> Yes. I specifically broke the code that actually changes how
> multipathd
> operates from a user' point of view into a seperate patch (4/4)
> because
> distributions might need to revert in, if they want to pull in recent
> upstream changes, but don't what this kind of change in multipathd's
> behavior.
> 
> I admit, this patchset needs to include documentation to mention the
> changed behavior. I'll add that. 

Well, the idea is that there is actually no difference between "weak"
and "hard" reconfigure in terms of the end result. If a change must be
applied to reconcile kernel state and user settings, "weak" reconfigure
will do it. The documentation should express that and avoid stipulating
doubt among users. The main difference is that "hard" reconfigure
always executes a reload operation, which comes down to a
suspend/reload/resume, and thus a) is slow and b) unnecessarily
interrupts IO on the map for a few fractions of a second.

My personal PoV is that we should consider it a bug if a user reports a
situation where a "hard" reconfigure has a different outcome than a
"weak" one. Of course distros need to think twice when any defaults
change, and therefore the way Ben split the patch set makes a lot of
sense. Yet if we had serious doubts that "weak" reconfigure works, we
shouldn't switch to it upstream, either. I personally don't have such
doubts any more. Xose, if I'm missing something, let me know.

Cheers,
Martin



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