| Message ID | 20241122211133.718861-7-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | multipath-tools: Handle tableless DM devices | expand |
On Fri, 2024-11-22 at 16:11 -0500, Benjamin Marzinski wrote: > Multipath already notices if there is any empty device with the same > alias and wwid as a device it is trying to create, and switches to a > reload. However, if the empty device has a different alias but the > same > wwid, instead of reloading and renaming the device like it would for > a device with a table. multipath will attempt to create a new device > and fail. > > Fix this by checking for these devices, and doing a reload and > rename. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/configure.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > index a7257981..d0e9c958 100644 > --- a/libmultipath/configure.c > +++ b/libmultipath/configure.c > @@ -835,7 +835,6 @@ int domap(struct multipath *mpp, char *params, > int is_daemon) > { > int r = DOMAP_FAIL; > struct config *conf; > - char wwid[WWID_SIZE]; > > /* > * last chance to quit before touching the devmaps > @@ -846,6 +845,7 @@ int domap(struct multipath *mpp, char *params, > int is_daemon) > } > > if (mpp->action == ACT_CREATE) { > + char wwid[WWID_SIZE]; > int rc = dm_get_wwid(mpp->alias, wwid, > sizeof(wwid)); > > if (rc == DMP_OK && !strncmp(mpp->wwid, wwid, > sizeof(wwid))) { > @@ -863,7 +863,26 @@ int domap(struct multipath *mpp, char *params, > int is_daemon) > mpp->action = ACT_REJECT; > } > } > + if (mpp->action == ACT_CREATE) { > + char alias[WWID_SIZE]; > + int rc = dm_find_map_by_wwid(mpp->wwid, alias, > NULL); > > + if (rc == DMP_NO_MATCH) { > + condlog(1, "%s: wwid \"%s\" already in use > by non-multipath map \"%s\"", > + mpp->alias, mpp->wwid, alias); Nitpick: This is almost the same message that dm_find_map_by_wwid() has printed immediately before. Similar messages are also printed in all other callers of dm_find_map_by_wwid(). I suggest that we either remove the log message in the callers or in dm_find_map_by_wwid() itself. Probably the latter, then we can decide on a case by case basis in the caller whether the situation needs to be logged. Martin
On Fri, Nov 22, 2024 at 10:54:31PM +0100, Martin Wilck wrote: > On Fri, 2024-11-22 at 16:11 -0500, Benjamin Marzinski wrote: > > Multipath already notices if there is any empty device with the same > > alias and wwid as a device it is trying to create, and switches to a > > reload. However, if the empty device has a different alias but the > > same > > wwid, instead of reloading and renaming the device like it would for > > a device with a table. multipath will attempt to create a new device > > and fail. > > > > Fix this by checking for these devices, and doing a reload and > > rename. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > libmultipath/configure.c | 21 ++++++++++++++++++++- > > 1 file changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > > index a7257981..d0e9c958 100644 > > --- a/libmultipath/configure.c > > +++ b/libmultipath/configure.c > > @@ -835,7 +835,6 @@ int domap(struct multipath *mpp, char *params, > > int is_daemon) > > { > > int r = DOMAP_FAIL; > > struct config *conf; > > - char wwid[WWID_SIZE]; > > > > /* > > * last chance to quit before touching the devmaps > > @@ -846,6 +845,7 @@ int domap(struct multipath *mpp, char *params, > > int is_daemon) > > } > > > > if (mpp->action == ACT_CREATE) { > > + char wwid[WWID_SIZE]; > > int rc = dm_get_wwid(mpp->alias, wwid, > > sizeof(wwid)); > > > > if (rc == DMP_OK && !strncmp(mpp->wwid, wwid, > > sizeof(wwid))) { > > @@ -863,7 +863,26 @@ int domap(struct multipath *mpp, char *params, > > int is_daemon) > > mpp->action = ACT_REJECT; > > } > > } > > + if (mpp->action == ACT_CREATE) { > > + char alias[WWID_SIZE]; > > + int rc = dm_find_map_by_wwid(mpp->wwid, alias, > > NULL); > > > > + if (rc == DMP_NO_MATCH) { > > + condlog(1, "%s: wwid \"%s\" already in use > > by non-multipath map \"%s\"", > > + mpp->alias, mpp->wwid, alias); > > Nitpick: This is almost the same message that dm_find_map_by_wwid() has > printed immediately before. Similar messages are also printed in all > other callers of dm_find_map_by_wwid(). I suggest that we either remove > the log message in the callers or in dm_find_map_by_wwid() itself. > Probably the latter, then we can decide on a case by case basis in the > caller whether the situation needs to be logged. I'm confused here. dm_find_map_by_wwid() doesn't print any message itself. It calls libmp_mapinfo__(), which could print either: condlog(lvl, "%s: map %s has multiple targets", fname__, map_id); or: condlog(lvl, "%s: target type mismatch: \"%s\" != \"%s\"", fname__, tgt_type, target_type); in this case, but at level 4 verbosity, and these are used to distinguish between the various reasons why DMP_NO_MATCH could be returned, which seems like a reasonable thing at that verbosity. -Ben > > Martin
On Mon, 2024-11-25 at 13:05 -0500, Benjamin Marzinski wrote: > On Fri, Nov 22, 2024 at 10:54:31PM +0100, Martin Wilck wrote: > > On Fri, 2024-11-22 at 16:11 -0500, Benjamin Marzinski wrote: > > > > > > + if (mpp->action == ACT_CREATE) { > > > + char alias[WWID_SIZE]; > > > + int rc = dm_find_map_by_wwid(mpp->wwid, alias, > > > NULL); > > > > > > + if (rc == DMP_NO_MATCH) { > > > + condlog(1, "%s: wwid \"%s\" already in > > > use > > > by non-multipath map \"%s\"", > > > + mpp->alias, mpp->wwid, alias); > > > > Nitpick: This is almost the same message that dm_find_map_by_wwid() > > has > > printed immediately before. Similar messages are also printed in > > all > > other callers of dm_find_map_by_wwid(). I suggest that we either > > remove > > the log message in the callers or in dm_find_map_by_wwid() itself. > > Probably the latter, then we can decide on a case by case basis in > > the > > caller whether the situation needs to be logged. > > I'm confused here. dm_find_map_by_wwid() doesn't print any message > itself. It calls libmp_mapinfo__(), which could print either: > > condlog(lvl, "%s: map %s has multiple targets", fname__, map_id); > > or: > > condlog(lvl, "%s: target type mismatch: \"%s\" != \"%s\"", > fname__, tgt_type, target_type); > > in this case, but at level 4 verbosity, and these are used to > distinguish between the various reasons why DMP_NO_MATCH could be > returned, which seems like a reasonable thing at that verbosity. You are right. Not sure what I was looking at. Probably I was just reviewing your patch too late at night :-/ Forget this. So your series LGTM except for the typo in 08 and the suggestion to squash patch 9 and 10. Thanks Martin
diff --git a/libmultipath/configure.c b/libmultipath/configure.c index a7257981..d0e9c958 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -835,7 +835,6 @@ int domap(struct multipath *mpp, char *params, int is_daemon) { int r = DOMAP_FAIL; struct config *conf; - char wwid[WWID_SIZE]; /* * last chance to quit before touching the devmaps @@ -846,6 +845,7 @@ int domap(struct multipath *mpp, char *params, int is_daemon) } if (mpp->action == ACT_CREATE) { + char wwid[WWID_SIZE]; int rc = dm_get_wwid(mpp->alias, wwid, sizeof(wwid)); if (rc == DMP_OK && !strncmp(mpp->wwid, wwid, sizeof(wwid))) { @@ -863,7 +863,26 @@ int domap(struct multipath *mpp, char *params, int is_daemon) mpp->action = ACT_REJECT; } } + if (mpp->action == ACT_CREATE) { + char alias[WWID_SIZE]; + int rc = dm_find_map_by_wwid(mpp->wwid, alias, NULL); + if (rc == DMP_NO_MATCH) { + condlog(1, "%s: wwid \"%s\" already in use by non-multipath map \"%s\"", + mpp->alias, mpp->wwid, alias); + mpp->action = ACT_REJECT; + } else if (rc == DMP_OK || rc == DMP_EMPTY) { + /* + * we already handled the case were rc == DMO_OK and + * the alias == mpp->alias above. So the alias must be + * different here. + */ + condlog(3, "%s: map already present with a different name \"%s\". reloading", + mpp->alias, alias); + strlcpy(mpp->alias_old, alias, WWID_SIZE); + mpp->action = ACT_RELOAD_RENAME; + } + } if (mpp->action == ACT_RENAME || mpp->action == ACT_SWITCHPG_RENAME || mpp->action == ACT_RELOAD_RENAME || mpp->action == ACT_RESIZE_RENAME) {
Multipath already notices if there is any empty device with the same alias and wwid as a device it is trying to create, and switches to a reload. However, if the empty device has a different alias but the same wwid, instead of reloading and renaming the device like it would for a device with a table. multipath will attempt to create a new device and fail. Fix this by checking for these devices, and doing a reload and rename. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/configure.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)