| Message ID | 1675193661-1147-5-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath: fix multipathd renaming issue | expand |
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@redhat.com> 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@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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@redhat.com> > > 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@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, 2023-02-01 at 08:39 -0600, Benjamin Marzinski wrote: > On Wed, Feb 01, 2023 at 08:00:25AM +0000, Martin Wilck wrote: > > > > > 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. Ok, leave it as you had it. Fine with me. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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) { 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; 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; 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, }; /*
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@redhat.com> --- libmultipath/configure.c | 60 +++++++++++++++++----------------------- libmultipath/configure.h | 4 ++- 2 files changed, 29 insertions(+), 35 deletions(-)