On Wed, Feb 01, 2023 at 08:00:25AM +0000, Martin Wilck wrote:
> On Tue, 2023-01-31 at 13:34 -0600, Benjamin Marzinski wrote:
> > If select_action() is called and a multipath device needs to be
> > renamed,
> > the code currently checks if force_reload is set, and if so, does the
> > reload after the rename. But if force_reload isn't set, only the
> > rename
> > happens, regardless of what other actions are needed. This can happen
> > if
> > multipathd starts up and a device needs both a reload and a rename.
> >
> > Make multipath check for resize, reload, and switch pathgroup along
> > with
> > rename, and do both if necessary.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
>
> Looks good, but I have some questions below.
>
> > ---
> > libmultipath/configure.c | 60 +++++++++++++++++---------------------
> > --
> > libmultipath/configure.h | 4 ++-
> > 2 files changed, 29 insertions(+), 35 deletions(-)
> >
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index e870e0f6..2228176d 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -670,7 +670,8 @@ static bool is_udev_ready(struct multipath *cmpp)
> > static void
> > select_reload_action(struct multipath *mpp, const char *reason)
> > {
> > - mpp->action = ACT_RELOAD;
> > + mpp->action = mpp->action == ACT_RENAME ? ACT_RELOAD_RENAME :
> > + ACT_RELOAD;
> > condlog(3, "%s: set ACT_RELOAD (%s)", mpp->alias, reason);
> > }
> >
> > @@ -681,6 +682,7 @@ void select_action (struct multipath *mpp, const
> > struct _vector *curmp,
> > struct multipath * cmpp_by_name;
> > char * mpp_feat, * cmpp_feat;
> >
> > + mpp->action = ACT_NOTHING;
> > cmpp = find_mp_by_wwid(curmp, mpp->wwid);
> > cmpp_by_name = find_mp_by_alias(curmp, mpp->alias);
> > if (mpp->need_reload || (cmpp && cmpp->need_reload))
> > @@ -705,14 +707,8 @@ void select_action (struct multipath *mpp, const
> > struct _vector *curmp,
> > mpp->alias);
> > strlcpy(mpp->alias_old, cmpp->alias, WWID_SIZE);
> > mpp->action = ACT_RENAME;
> > - if (force_reload) {
> > - mpp->force_udev_reload = 1;
> > - mpp->action = ACT_FORCERENAME;
> > - }
> > - return;
> > - }
> > -
> > - if (cmpp != cmpp_by_name) {
> > + /* don't return here. Check for other needed actions
> > */
> > + } else if (cmpp != cmpp_by_name) {
>
> Why does your "check for other needed actions" logic not apply for this
> case? AFAICS, even if we can't rename the map, we might need to resize
> or reload.
Oops. Good catch. I'll fix that.
>
>
> > condlog(2, "%s: unable to rename %s to %s (%s is used
> > by %s)",
> > mpp->wwid, cmpp->alias, mpp->alias,
> > mpp->alias, cmpp_by_name->wwid);
> > @@ -725,7 +721,8 @@ void select_action (struct multipath *mpp, const
> > struct _vector *curmp,
> >
> > if (cmpp->size != mpp->size) {
> > mpp->force_udev_reload = 1;
> > - mpp->action = ACT_RESIZE;
> > + mpp->action = mpp->action == ACT_RENAME ?
> > ACT_RESIZE_RENAME :
> > + ACT_RESIZE;
>
>
> This code makes we wonder if we should transform the ACT_... enum into
> a bitmap of required actions that would be ORed together.
> At least ACT_RENAME is now orthogonal to the rest of the actions.
I thought about that too, as well as breaking rename out from the other
actions, so it could be checked separately. But since, AFAICS, there
are only these three special cases, it didn't seem like there'd be much
benefit, and it could give the false impression that something like
ACT_RELOAD | ACT_SWITCHPG makes sense. If you feel strongly about this,
I can do it, but I did consider it and I didn't see a reason to change
it.
-Ben
> > condlog(3, "%s: set ACT_RESIZE (size change)",
> > mpp->alias);
> > return;
> > @@ -801,14 +798,14 @@ void select_action (struct multipath *mpp,
> > const struct _vector *curmp,
> > return;
> > }
> > if (cmpp->nextpg != mpp->bestpg) {
> > - mpp->action = ACT_SWITCHPG;
> > + mpp->action = mpp->action == ACT_RENAME ?
> > ACT_SWITCHPG_RENAME :
> > + ACT_SWITCHPG;
>
> See above.
>
> > condlog(3, "%s: set ACT_SWITCHPG (next path group
> > change)",
> > mpp->alias);
> > return;
> > }
> > - mpp->action = ACT_NOTHING;
> > - condlog(3, "%s: set ACT_NOTHING (map unchanged)",
> > - mpp->alias);
> > + if (mpp->action == ACT_NOTHING)
> > + condlog(3, "%s: set ACT_NOTHING (map unchanged)",
> > mpp->alias);
> > return;
> > }
> >
> > @@ -909,6 +906,17 @@ int domap(struct multipath *mpp, char *params,
> > int is_daemon)
> > }
> > }
> >
> > + if (mpp->action == ACT_RENAME || mpp->action ==
> > ACT_SWITCHPG_RENAME ||
> > + mpp->action == ACT_RELOAD_RENAME ||
> > + mpp->action == ACT_RESIZE_RENAME) {
> > + conf = get_multipath_config();
> > + pthread_cleanup_push(put_multipath_config, conf);
> > + r = dm_rename(mpp->alias_old, mpp->alias,
> > + conf->partition_delim, mpp-
> > >skip_kpartx);
> > + pthread_cleanup_pop(1);
> > + if (r == DOMAP_FAIL)
> > + return r;
> > + }
> > switch (mpp->action) {
> > case ACT_REJECT:
> > case ACT_NOTHING:
> > @@ -916,6 +924,7 @@ int domap(struct multipath *mpp, char *params,
> > int is_daemon)
> > return DOMAP_EXIST;
> >
> > case ACT_SWITCHPG:
> > + case ACT_SWITCHPG_RENAME:
> > dm_switchgroup(mpp->alias, mpp->bestpg);
> > /*
> > * we may have avoided reinstating paths because
> > there where in
> > @@ -942,6 +951,7 @@ int domap(struct multipath *mpp, char *params,
> > int is_daemon)
> > break;
> >
> > case ACT_RELOAD:
> > + case ACT_RELOAD_RENAME:
> > sysfs_set_max_sectors_kb(mpp, 1);
> > if (mpp->ghost_delay_tick > 0 && pathcount(mpp,
> > PATH_UP))
> > mpp->ghost_delay_tick = 0;
> > @@ -949,6 +959,7 @@ int domap(struct multipath *mpp, char *params,
> > int is_daemon)
> > break;
> >
> > case ACT_RESIZE:
> > + case ACT_RESIZE_RENAME:
> > sysfs_set_max_sectors_kb(mpp, 1);
> > if (mpp->ghost_delay_tick > 0 && pathcount(mpp,
> > PATH_UP))
> > mpp->ghost_delay_tick = 0;
> > @@ -956,29 +967,10 @@ int domap(struct multipath *mpp, char *params,
> > int is_daemon)
> > break;
> >
> > case ACT_RENAME:
> > - conf = get_multipath_config();
> > - pthread_cleanup_push(put_multipath_config, conf);
> > - r = dm_rename(mpp->alias_old, mpp->alias,
> > - conf->partition_delim, mpp-
> > >skip_kpartx);
> > - pthread_cleanup_pop(1);
> > - break;
> > -
> > - case ACT_FORCERENAME:
> > - conf = get_multipath_config();
> > - pthread_cleanup_push(put_multipath_config, conf);
> > - r = dm_rename(mpp->alias_old, mpp->alias,
> > - conf->partition_delim, mpp-
> > >skip_kpartx);
> > - pthread_cleanup_pop(1);
> > - if (r) {
> > - sysfs_set_max_sectors_kb(mpp, 1);
> > - if (mpp->ghost_delay_tick > 0 &&
> > - pathcount(mpp, PATH_UP))
> > - mpp->ghost_delay_tick = 0;
> > - r = dm_addmap_reload(mpp, params, 0);
> > - }
> > break;
> >
> > default:
> > + r = DOMAP_FAIL;
> > break;
> > }
> >
> > diff --git a/libmultipath/configure.h b/libmultipath/configure.h
> > index 2bf73e65..9d935db3 100644
> > --- a/libmultipath/configure.h
> > +++ b/libmultipath/configure.h
> > @@ -18,9 +18,11 @@ enum actions {
> > ACT_RENAME,
> > ACT_CREATE,
> > ACT_RESIZE,
> > - ACT_FORCERENAME,
> > + ACT_RELOAD_RENAME,
> > ACT_DRY_RUN,
> > ACT_IMPOSSIBLE,
> > + ACT_RESIZE_RENAME,
> > + ACT_SWITCHPG_RENAME,
> > };
> >
> > /*
>
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
