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.
> 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.
> 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
