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

Message

Benjamin Marzinski Nov. 17, 2021, 8:33 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.

Changes from v1 as suggested by Martin Wilck:

0001: update libmultipath.version to handle ABI change in struct config
0003: Clarify commit message

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.

 libmpathpersist/libmpathpersist.version | 12 ++--
 libmultipath/config.h                   |  1 -
 libmultipath/configure.c                |  2 +-
 libmultipath/libmultipath.version       | 22 +++---
 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 +-
 multipathd/multipathd.8                 | 10 ++-
 11 files changed, 91 insertions(+), 67 deletions(-)

Comments

Martin Wilck Nov. 17, 2021, 8:50 p.m. UTC | #1

On Wed, 2021-11-17 at 14:33 -0600, Benjamin Marzinski wrote:
> 
> Changes from v1 as suggested by Martin Wilck:
> 
> 0001: update libmultipath.version to handle ABI change in struct
> config
> 0003: Clarify commit message

Did you overlook the two other comments I had on 0003, or did you
deliberately ignore them? Just asking.

Martin


--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Benjamin Marzinski Nov. 17, 2021, 8:55 p.m. UTC | #2

On Wed, Nov 17, 2021 at 08:50:55PM +0000, Martin Wilck wrote:
> On Wed, 2021-11-17 at 14:33 -0600, Benjamin Marzinski wrote:
> > 
> > Changes from v1 as suggested by Martin Wilck:
> > 
> > 0001: update libmultipath.version to handle ABI change in struct
> > config
> > 0003: Clarify commit message
> 
> Did you overlook the two other comments I had on 0003, or did you
> deliberately ignore them? Just asking.

Ooops. Missed them. I'll resend.

-Ben

> 
> Martin

--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Benjamin Marzinski Nov. 17, 2021, 10:37 p.m. UTC | #3

On Wed, Nov 17, 2021 at 08:50:55PM +0000, Martin Wilck wrote:
> On Wed, 2021-11-17 at 14:33 -0600, Benjamin Marzinski wrote:
> > 
> > Changes from v1 as suggested by Martin Wilck:
> > 
> > 0001: update libmultipath.version to handle ABI change in struct
> > config
> > 0003: Clarify commit message
> 
> Did you overlook the two other comments I had on 0003, or did you
> deliberately ignore them? Just asking.

Now that I look at this, I'd rather not change the reconfigure() and
configure() argument names to reload_type. That's the name of the global
variable which has serialization concerns, and reusing it as an argument
seems like it would lead to more confusion.

Actually, since reload_type can only be changed when switching to the
DAEMON_CONFIGURE state, and you can't switch to that state again until
after do the reconfigure, it's safe to simply have configure() access
the global variable, reload_type.

-Ben

> 
> Martin

--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Martin Wilck Nov. 18, 2021, 3:22 p.m. UTC | #4

On Wed, 2021-11-17 at 16:37 -0600, Benjamin Marzinski wrote:
> On Wed, Nov 17, 2021 at 08:50:55PM +0000, Martin Wilck wrote:
> > On Wed, 2021-11-17 at 14:33 -0600, Benjamin Marzinski wrote:
> > > 
> > > Changes from v1 as suggested by Martin Wilck:
> > > 
> > > 0001: update libmultipath.version to handle ABI change in struct
> > > config
> > > 0003: Clarify commit message
> > 
> > Did you overlook the two other comments I had on 0003, or did you
> > deliberately ignore them? Just asking.
> 
> Now that I look at this, I'd rather not change the reconfigure() and
> configure() argument names to reload_type. That's the name of the
> global
> variable which has serialization concerns, and reusing it as an
> argument
> seems like it would lead to more confusion.

Right. My point was that "type" is too generic. I didn't think about
the name clash.

> Actually, since reload_type can only be changed when switching to the
> DAEMON_CONFIGURE state, and you can't switch to that state again
> until
> after do the reconfigure, it's safe to simply have configure() access
> the global variable, reload_type.

I'm not a big fan of new global variables, but this should be fine.

Regards
Martin


--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel