[dm-devel] [PATCH v2 0/4] multipath: fix multipathd renaming issue

[dm-devel] [PATCH v2 0/4] multipath: fix multipathd renaming issue

[Benjamin Marzinski]

If a multipath device needs to be renamed and reloaded when multipathd
starts, it will only get reloaded. This can happen if the multipath
configuration is different between the initramfs and regular filesystem.
The only thing that can happen along with a rename is a force reload.

This patchset makes libmultipath also do all the other actions that are
compatible with a rename.

changes in v2 (as suggested by Martin Wilck)
0004: If the rename is impossible, multipath now checks for other
      actions, instead of always returning ACT_IMPOSSIBLE

Benjamin Marzinski (4):
  libmultipath: use select_reload_action in select_action
  libmultipath: select resize action even if reload is forced
  libmultipath: cleanup ACT_CREATE code in select_action
  libmultipath: keep renames from stopping other multipath actions

 libmultipath/configure.c | 107 +++++++++++++++++----------------------
 libmultipath/configure.h |   4 +-
 2 files changed, 49 insertions(+), 62 deletions(-)

-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 1/4] libmultipath: use select_reload_action in select_action

[Benjamin Marzinski]

Since we have a function to set the action to reload, use it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index e689f8a7..050b984a 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -729,9 +729,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 
 	if (force_reload) {
 		mpp->force_udev_reload = 1;
-		mpp->action = ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (forced by user)",
-			mpp->alias);
+		select_reload_action(mpp, "forced by user");
 		return;
 	}
 	if (cmpp->size != mpp->size) {
@@ -744,9 +742,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 
 	if (!is_udev_ready(cmpp) && count_active_paths(mpp) > 0) {
 		mpp->force_udev_reload = 1;
-		mpp->action = ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (udev incomplete)",
-			mpp->alias);
+		select_reload_action(mpp, "udev incomplete");
 		return;
 	}
 
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 2/4] libmultipath: select resize action even if reload is forced

[Benjamin Marzinski]

The ACT_RESIZE action is the same as the ACT_RELOAD action, except that
it flushes outstanding IO because the device size is changing and
the new size might be too small for some of the outstanding IO. If we've
detected a size change, and a forced reload is requested, we still need
to flush the IO because the reload will change the device size.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 050b984a..6811e661 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -727,11 +727,6 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 		return;
 	}
 
-	if (force_reload) {
-		mpp->force_udev_reload = 1;
-		select_reload_action(mpp, "forced by user");
-		return;
-	}
 	if (cmpp->size != mpp->size) {
 		mpp->force_udev_reload = 1;
 		mpp->action = ACT_RESIZE;
@@ -740,6 +735,12 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 		return;
 	}
 
+	if (force_reload) {
+		mpp->force_udev_reload = 1;
+		select_reload_action(mpp, "forced by user");
+		return;
+	}
+
 	if (!is_udev_ready(cmpp) && count_active_paths(mpp) > 0) {
 		mpp->force_udev_reload = 1;
 		select_reload_action(mpp, "udev incomplete");
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 3/4] libmultipath: cleanup ACT_CREATE code in select_action

[Benjamin Marzinski]

Combine the two separate blocks that set ACT_CREATE into one.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 6811e661..e870e0f6 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -686,33 +686,29 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 	if (mpp->need_reload || (cmpp && cmpp->need_reload))
 		force_reload = 1;
 
-	if (!cmpp_by_name) {
-		if (cmpp) {
-			condlog(2, "%s: rename %s to %s", mpp->wwid,
-				cmpp->alias, 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) {
+		if (cmpp_by_name) {
+			condlog(1, "%s: can't use alias \"%s\" used by %s, falling back to WWID",
+				mpp->wwid, mpp->alias, cmpp_by_name->wwid);
+			/* We can do this because wwid wasn't found */
+			free(mpp->alias);
+			mpp->alias = strdup(mpp->wwid);
 		}
 		mpp->action = ACT_CREATE;
-		condlog(3, "%s: set ACT_CREATE (map does not exist)",
-			mpp->alias);
+		condlog(3, "%s: set ACT_CREATE (map does not exist%s)",
+			mpp->alias, cmpp_by_name ? ", name changed" : "");
 		return;
 	}
 
-	if (!cmpp) {
-		condlog(1, "%s: can't use alias \"%s\" used by %s, falling back to WWID",
-			mpp->wwid, mpp->alias, cmpp_by_name->wwid);
-		/* We can do this because wwid wasn't found */
-		free(mpp->alias);
-		mpp->alias = strdup(mpp->wwid);
-		mpp->action = ACT_CREATE;
-		condlog(3, "%s: set ACT_CREATE (map does not exist, name changed)",
+	if (!cmpp_by_name) {
+		condlog(2, "%s: rename %s to %s", mpp->wwid, cmpp->alias,
 			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;
 	}
 
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 4/4] libmultipath: keep renames from stopping other multipath actions

[Benjamin Marzinski]

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 | 62 +++++++++++++++++-----------------------
 libmultipath/configure.h |  4 ++-
 2 files changed, 30 insertions(+), 36 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index e870e0f6..4a1c28bb 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);
@@ -720,12 +716,13 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 		free(mpp->alias);
 		mpp->alias = strdup(cmpp->alias);
 		mpp->action = ACT_IMPOSSIBLE;
-		return;
+		/* don't return here. Check for other needed actions */
 	}
 
 	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,
 };
 
 /*
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 0/4] multipath: fix multipathd renaming issue

[Martin Wilck]

On Wed, 2023-02-01 at 09:56 -0600, Benjamin Marzinski wrote:
> If a multipath device needs to be renamed and reloaded when
> multipathd
> starts, it will only get reloaded. This can happen if the multipath
> configuration is different between the initramfs and regular
> filesystem.
> The only thing that can happen along with a rename is a force reload.
> 
> This patchset makes libmultipath also do all the other actions that
> are
> compatible with a rename.
> 
> changes in v2 (as suggested by Martin Wilck)
> 0004: If the rename is impossible, multipath now checks for other
>       actions, instead of always returning ACT_IMPOSSIBLE
> 
> Benjamin Marzinski (4):
>   libmultipath: use select_reload_action in select_action
>   libmultipath: select resize action even if reload is forced
>   libmultipath: cleanup ACT_CREATE code in select_action
>   libmultipath: keep renames from stopping other multipath actions
> 
>  libmultipath/configure.c | 107 +++++++++++++++++--------------------
> --
>  libmultipath/configure.h |   4 +-
>  2 files changed, 49 insertions(+), 62 deletions(-)
> 


For the series:

Reviewed-by: Martin Wilck <mwilck@suse.com>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 0/4] multipath: fix multipathd renaming issue

[Benjamin Marzinski]

On Thu, Feb 02, 2023 at 08:44:53AM +0000, Martin Wilck wrote:
> On Wed, 2023-02-01 at 09:56 -0600, Benjamin Marzinski wrote:
> > If a multipath device needs to be renamed and reloaded when
> > multipathd
> > starts, it will only get reloaded. This can happen if the multipath
> > configuration is different between the initramfs and regular
> > filesystem.
> > The only thing that can happen along with a rename is a force reload.
> > 
> > This patchset makes libmultipath also do all the other actions that
> > are
> > compatible with a rename.
> > 
> > changes in v2 (as suggested by Martin Wilck)
> > 0004: If the rename is impossible, multipath now checks for other
> >       actions, instead of always returning ACT_IMPOSSIBLE
> > 
> > Benjamin Marzinski (4):
> >   libmultipath: use select_reload_action in select_action
> >   libmultipath: select resize action even if reload is forced
> >   libmultipath: cleanup ACT_CREATE code in select_action
> >   libmultipath: keep renames from stopping other multipath actions
> > 
> >  libmultipath/configure.c | 107 +++++++++++++++++--------------------
> > --
> >  libmultipath/configure.h |   4 +-
> >  2 files changed, 49 insertions(+), 62 deletions(-)
> > 
> 
> 
> For the series:
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>

It looks like you pulled the first version of my patches, without the
fix you suggested for impossible renames, into your queue branch. 

-Ben
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 0/4] multipath: fix multipathd renaming issue

[Martin Wilck]

On Thu, 2023-02-02 at 09:14 -0600, Benjamin Marzinski wrote:
> 
> It looks like you pulled the first version of my patches, without the
> fix you suggested for impossible renames, into your queue branch. 

Oops, thanks for noticing and notifying me. Fixed.

Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel