[PATCH 0/2] Handle blacklisted maps on reconfigure

[PATCH 0/2] Handle blacklisted maps on reconfigure

[Benjamin Marzinski]

This patchset is meant to deal with the issue from:
https://github.com/opensvc/multipath-tools/issues/113

If a multipath device that is in-use becomes blacklisted on a multipathd
reconfigure, its paths weren't always getting removed and the device
device could get stuck with queueing enabled, even if there were no
usable paths. This patchset deals with this by marking blacklisted paths
with INIT_REMOVED, making sure they are linked to the multipath device,
and disabling queueing on the invalid multipath device.


Benjamin Marzinski (2):
  libmutipath: handle blacklisted paths on map_discovery
  multipathd: disable queueing on invalid multipath devices

 libmultipath/structs_vec.c | 15 +++++++++++----
 multipathd/main.c          | 16 ++++++++++++++++
 2 files changed, 27 insertions(+), 4 deletions(-)

-- 
2.48.1

[PATCH 1/2] libmutipath: handle blacklisted paths on map_discovery

[Benjamin Marzinski]

If the multipath configuration is changed to blacklist existing devices,
and multipathd is reloaded but the blacklisted multipaths device can't
be removed, multipathd was marking the paths as INIT_PARTIAL, causing
them to stay in the multipath device, at least until the
partial_retrigger_delay timeout elapsed. Instead, mark them as
INIT_REMOVED and set mpp->need_reload, so the device is reloaded and the
paths are removed. To make sure the blacklisted paths are deleted when
the multipath device is removed in coalesce_maps(), set their pp->mpp
to point to map before removing it.

Fixes d9c61332 ("multipathd: trigger uevents for blacklisted paths in reconfigure")

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 15 +++++++++++----
 multipathd/main.c          | 12 ++++++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 663c9053..9232b54b 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -205,10 +205,17 @@ static void update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 						must_reload = true;
 						continue;
 					}
-					condlog(2, "%s: adding new path %s",
-						mpp->alias, pp->dev);
-					pp->initialized = INIT_PARTIAL;
-					pp->partial_retrigger_delay = 180;
+					if (rc == PATHINFO_SKIPPED) {
+						condlog(1, "%s: blacklisted path in %s",
+							pp->dev, mpp->alias);
+						set_path_removed(pp);
+						must_reload = true;
+					} else {
+						condlog(2, "%s: adding new path %s",
+							mpp->alias, pp->dev);
+						pp->initialized = INIT_PARTIAL;
+						pp->partial_retrigger_delay = 180;
+					}
 					store_path(pathvec, pp);
 					pp->tick = 1;
 				}
diff --git a/multipathd/main.c b/multipathd/main.c
index 99d603f9..bdd97178 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -804,6 +804,18 @@ coalesce_maps(struct vectors *vecs, vector nmpv)
 	vector_foreach_slot (ompv, ompp, i) {
 		condlog(3, "%s: coalesce map", ompp->alias);
 		if (!find_mp_by_wwid(nmpv, ompp->wwid)) {
+			struct pathgroup *pgp;
+			struct path *pp;
+			int j, k;
+
+			/*
+			 * set pp->mpp for all the old map's paths,
+			 * so that they can be properly removed
+			 */
+			vector_foreach_slot (ompp->pg, pgp, j)
+				vector_foreach_slot(pgp->paths, pp, k)
+					if (!pp->mpp)
+						pp->mpp = ompp;
 			/*
 			 * remove all current maps not allowed by the
 			 * current configuration
-- 
2.48.1

[PATCH 2/2] multipathd: disable queueing on invalid multipath devices

[Benjamin Marzinski]

If the multipath configuration has changed and a multipath device is no
longer valid after a reconfigure, disable queueing on it before
attempting to delete it. This makes it more likely to be deleted and if
the device isn't valid, then it there is no defined no_path_retry value
for it.

Fixes: https://github.com/opensvc/multipath-tools/issues/113
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index bdd97178..83a0b055 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -820,6 +820,10 @@ coalesce_maps(struct vectors *vecs, vector nmpv)
 			 * remove all current maps not allowed by the
 			 * current configuration
 			 */
+			ompp->retry_tick = 0;
+			ompp->no_path_retry = NO_PATH_RETRY_FAIL;
+			ompp->disable_queueing = 1;
+			dm_queue_if_no_path(ompp, 0);
 			if (dm_flush_map(ompp->alias) != DM_FLUSH_OK) {
 				condlog(0, "%s: unable to flush devmap",
 					ompp->alias);
-- 
2.48.1

Re: [PATCH 1/2] libmutipath: handle blacklisted paths on map_discovery

[Martin Wilck]

On Mon, 2025-04-28 at 17:45 -0400, Benjamin Marzinski wrote:
> If the multipath configuration is changed to blacklist existing
> devices,
> and multipathd is reloaded but the blacklisted multipaths device
> can't
> be removed, multipathd was marking the paths as INIT_PARTIAL, causing
> them to stay in the multipath device, at least until the
> partial_retrigger_delay timeout elapsed. Instead, mark them as
> INIT_REMOVED and set mpp->need_reload, so the device is reloaded and
> the
> paths are removed. To make sure the blacklisted paths are deleted
> when
> the multipath device is removed in coalesce_maps(), set their pp->mpp
> to point to map before removing it.
> 
> Fixes d9c61332 ("multipathd: trigger uevents for blacklisted paths in
> reconfigure")

The patch looks good to me, but I only vaguely understand why the bug
is introduced in d9c61332. Are you positive that before d9c61332, the
hang didn't occur?

> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

Re: [PATCH 2/2] multipathd: disable queueing on invalid multipath devices

[Martin Wilck]

On Mon, 2025-04-28 at 17:45 -0400, Benjamin Marzinski wrote:
> If the multipath configuration has changed and a multipath device is
> no
> longer valid after a reconfigure, disable queueing on it before
> attempting to delete it. This makes it more likely to be deleted and
> if
> the device isn't valid, then it there is no defined no_path_retry
> value
> for it.
> 
> Fixes: https://github.com/opensvc/multipath-tools/issues/113
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

Re: [PATCH 1/2] libmutipath: handle blacklisted paths on map_discovery

[Benjamin Marzinski]

On Tue, Apr 29, 2025 at 09:59:40PM +0200, Martin Wilck wrote:
> On Mon, 2025-04-28 at 17:45 -0400, Benjamin Marzinski wrote:
> > If the multipath configuration is changed to blacklist existing
> > devices,
> > and multipathd is reloaded but the blacklisted multipaths device
> > can't
> > be removed, multipathd was marking the paths as INIT_PARTIAL, causing
> > them to stay in the multipath device, at least until the
> > partial_retrigger_delay timeout elapsed. Instead, mark them as
> > INIT_REMOVED and set mpp->need_reload, so the device is reloaded and
> > the
> > paths are removed. To make sure the blacklisted paths are deleted
> > when
> > the multipath device is removed in coalesce_maps(), set their pp->mpp
> > to point to map before removing it.
> > 
> > Fixes d9c61332 ("multipathd: trigger uevents for blacklisted paths in
> > reconfigure")
> 
> The patch looks good to me, but I only vaguely understand why the bug
> is introduced in d9c61332. Are you positive that before d9c61332, the
> hang didn't occur?

Well, I'm pretty sure these blacklisted paths stopped getting deleted
during reconfigure by d9c61332. Before d9c61332, blacklisted paths were
immediately deleted in update_pathvec_from_dm(). After this change

@@ -193,7 +196,8 @@ static void update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 		rc = pathinfo(pp, conf,
 		              DI_SYSFS|DI_WWID|DI_BLACKLIST|DI_NOFALLBACK|pathinfo_flags);
 		pthread_cleanup_pop(1);
-		if (rc != PATHINFO_OK) {
+		if (rc == PATHINFO_FAILED ||
+		    (rc == PATHINFO_SKIPPED && !map_discovery)) {
 		        condlog(1, "%s: error %d in pathinfo, discarding path",
 		                pp->dev, rc);
 		        vector_del_slot(pgp->paths, j--);

they started hanging around, so that uevents could be generated for them
by trigger_paths_udev_change(). However, since coalesce_paths() will
usually clear pp->mpp, they won't get removed when orphan_paths() is
later called by remove_maps() to remove the old maps. I admit I didn't
test if the paths got removed before that commit, but the commit message
says that they did.

-Ben

> 
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>

Coding stye (was Re: [PATCH 1/2] libmutipath: handle blacklisted paths on map_discovery)

[Martin Wilck]

On Tue, 2025-04-29 at 16:27 -0400, Benjamin Marzinski wrote:
> On Tue, Apr 29, 2025 at 09:59:40PM +0200, Martin Wilck wrote:
> > On Mon, 2025-04-28 at 17:45 -0400, Benjamin Marzinski wrote:
> > > If the multipath configuration is changed to blacklist existing
> > > devices,
> > > and multipathd is reloaded but the blacklisted multipaths device
> > > can't
> > > be removed, multipathd was marking the paths as INIT_PARTIAL,
> > > causing
> > > them to stay in the multipath device, at least until the
> > > partial_retrigger_delay timeout elapsed. Instead, mark them as
> > > INIT_REMOVED and set mpp->need_reload, so the device is reloaded
> > > and
> > > the
> > > paths are removed. To make sure the blacklisted paths are deleted
> > > when
> > > the multipath device is removed in coalesce_maps(), set their pp-
> > > >mpp
> > > to point to map before removing it.
> > > 
> > > Fixes d9c61332 ("multipathd: trigger uevents for blacklisted
> > > paths in
> > > reconfigure")
> > 
> > The patch looks good to me, but I only vaguely understand why the
> > bug
> > is introduced in d9c61332. Are you positive that before d9c61332,
> > the
> > hang didn't occur?
> 
> Well, I'm pretty sure these blacklisted paths stopped getting deleted
> during reconfigure by d9c61332. Before d9c61332, blacklisted paths
> were
> immediately deleted in update_pathvec_from_dm().

Right, thanks.

I've taken the liberty to fix the typo ("libmutipath") and make some
minor coding style adjustments in my queue branch. You can inspect the
result there.

The coding style stuff is work in progress, I've added a clang-format
based workflow on my tip branch.

I'd like to avoid major reformatting of past code while enforcing
consistent formatting for present and future submissions which is more-
or-less in line with what we used to do "sub-conciously" during the
last years. clang-format has a lot of options [1]. I'm still struggling
to find style settings that match ours [2].

Let me know if you dislike this, or if you have suggestions for
improving the settings.

Regards
Martin

[1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html
[2] https://github.com/openSUSE/multipath-tools/blob/refs/heads/tip/.clang-format

>  After this change
> 
> @@ -193,7 +196,8 @@ static void update_pathvec_from_dm(vector
> pathvec, struct multipath *mpp,
>  		rc = pathinfo(pp, conf,
>  		             
> DI_SYSFS|DI_WWID|DI_BLACKLIST|DI_NOFALLBACK|pathinfo_flags);
>  		pthread_cleanup_pop(1);
> -		if (rc != PATHINFO_OK) {
> +		if (rc == PATHINFO_FAILED ||
> +		    (rc == PATHINFO_SKIPPED && !map_discovery)) {
>  		        condlog(1, "%s: error %d in pathinfo,
> discarding path",
>  		                pp->dev, rc);
>  		        vector_del_slot(pgp->paths, j--);
> 
> they started hanging around, so that uevents could be generated for
> them
> by trigger_paths_udev_change(). However, since coalesce_paths() will
> usually clear pp->mpp, they won't get removed when orphan_paths() is
> later called by remove_maps() to remove the old maps. I admit I
> didn't
> test if the paths got removed before that commit, but the commit
> message
> says that they did.
> 
> -Ben
> 
> > 
> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > 
> > Reviewed-by: Martin Wilck <mwilck@suse.com>

Re: Coding stye (was Re: [PATCH 1/2] libmutipath: handle blacklisted paths on map_discovery)

[Benjamin Marzinski]

On Wed, Apr 30, 2025 at 09:55:43AM +0200, Martin Wilck wrote:
> On Tue, 2025-04-29 at 16:27 -0400, Benjamin Marzinski wrote:
> > On Tue, Apr 29, 2025 at 09:59:40PM +0200, Martin Wilck wrote:
> > > On Mon, 2025-04-28 at 17:45 -0400, Benjamin Marzinski wrote:
> > > > If the multipath configuration is changed to blacklist existing
> > > > devices,
> > > > and multipathd is reloaded but the blacklisted multipaths device
> > > > can't
> > > > be removed, multipathd was marking the paths as INIT_PARTIAL,
> > > > causing
> > > > them to stay in the multipath device, at least until the
> > > > partial_retrigger_delay timeout elapsed. Instead, mark them as
> > > > INIT_REMOVED and set mpp->need_reload, so the device is reloaded
> > > > and
> > > > the
> > > > paths are removed. To make sure the blacklisted paths are deleted
> > > > when
> > > > the multipath device is removed in coalesce_maps(), set their pp-
> > > > >mpp
> > > > to point to map before removing it.
> > > > 
> > > > Fixes d9c61332 ("multipathd: trigger uevents for blacklisted
> > > > paths in
> > > > reconfigure")
> > > 
> > > The patch looks good to me, but I only vaguely understand why the
> > > bug
> > > is introduced in d9c61332. Are you positive that before d9c61332,
> > > the
> > > hang didn't occur?
> > 
> > Well, I'm pretty sure these blacklisted paths stopped getting deleted
> > during reconfigure by d9c61332. Before d9c61332, blacklisted paths
> > were
> > immediately deleted in update_pathvec_from_dm().
> 
> Right, thanks.
> 
> I've taken the liberty to fix the typo ("libmutipath") and make some
> minor coding style adjustments in my queue branch. You can inspect the
> result there.
> 
> The coding style stuff is work in progress, I've added a clang-format
> based workflow on my tip branch.
> 
> I'd like to avoid major reformatting of past code while enforcing
> consistent formatting for present and future submissions which is more-
> or-less in line with what we used to do "sub-conciously" during the
> last years. clang-format has a lot of options [1]. I'm still struggling
> to find style settings that match ours [2].
> 
> Let me know if you dislike this, or if you have suggestions for
> improving the settings.

I'm fine with your changes and with using clang-format in general. And I
do prefer just formatting our new changes, like you mentioned in your
tip commit, but if someone wants to make the case for a mass reformat,
I'm willing to consider it. 

-Ben

> 
> Regards
> Martin
> 
> [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html
> [2] https://github.com/openSUSE/multipath-tools/blob/refs/heads/tip/.clang-format
> 
> >  After this change
> > 
> > @@ -193,7 +196,8 @@ static void update_pathvec_from_dm(vector
> > pathvec, struct multipath *mpp,
> >  		rc = pathinfo(pp, conf,
> >  		             
> > DI_SYSFS|DI_WWID|DI_BLACKLIST|DI_NOFALLBACK|pathinfo_flags);
> >  		pthread_cleanup_pop(1);
> > -		if (rc != PATHINFO_OK) {
> > +		if (rc == PATHINFO_FAILED ||
> > +		    (rc == PATHINFO_SKIPPED && !map_discovery)) {
> >  		        condlog(1, "%s: error %d in pathinfo,
> > discarding path",
> >  		                pp->dev, rc);
> >  		        vector_del_slot(pgp->paths, j--);
> > 
> > they started hanging around, so that uevents could be generated for
> > them
> > by trigger_paths_udev_change(). However, since coalesce_paths() will
> > usually clear pp->mpp, they won't get removed when orphan_paths() is
> > later called by remove_maps() to remove the old maps. I admit I
> > didn't
> > test if the paths got removed before that commit, but the commit
> > message
> > says that they did.
> > 
> > -Ben
> > 
> > > 
> > > > 
> > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > 
> > > Reviewed-by: Martin Wilck <mwilck@suse.com>

Re: Coding stye (was Re: [PATCH 1/2] libmutipath: handle blacklisted paths on map_discovery)

[Martin Wilck]

On Wed, 2025-04-30 at 11:22 -0400, Benjamin Marzinski wrote:
> On Wed, Apr 30, 2025 at 09:55:43AM +0200, Martin Wilck wrote:
> > On Tue, 2025-04-29 at 16:27 -0400, Benjamin Marzinski wrote:
> > > On Tue, Apr 29, 2025 at 09:59:40PM +0200, Martin Wilck wrote:
> > > > On Mon, 2025-04-28 at 17:45 -0400, Benjamin Marzinski wrote:
> > > > > If the multipath configuration is changed to blacklist
> > > > > existing
> > > > > devices,
> > > > > and multipathd is reloaded but the blacklisted multipaths
> > > > > device
> > > > > can't
> > > > > be removed, multipathd was marking the paths as INIT_PARTIAL,
> > > > > causing
> > > > > them to stay in the multipath device, at least until the
> > > > > partial_retrigger_delay timeout elapsed. Instead, mark them
> > > > > as
> > > > > INIT_REMOVED and set mpp->need_reload, so the device is
> > > > > reloaded
> > > > > and
> > > > > the
> > > > > paths are removed. To make sure the blacklisted paths are
> > > > > deleted
> > > > > when
> > > > > the multipath device is removed in coalesce_maps(), set their
> > > > > pp-
> > > > > > mpp
> > > > > to point to map before removing it.
> > > > > 
> > > > > Fixes d9c61332 ("multipathd: trigger uevents for blacklisted
> > > > > paths in
> > > > > reconfigure")
> > > > 
> > > > The patch looks good to me, but I only vaguely understand why
> > > > the
> > > > bug
> > > > is introduced in d9c61332. Are you positive that before
> > > > d9c61332,
> > > > the
> > > > hang didn't occur?
> > > 
> > > Well, I'm pretty sure these blacklisted paths stopped getting
> > > deleted
> > > during reconfigure by d9c61332. Before d9c61332, blacklisted
> > > paths
> > > were
> > > immediately deleted in update_pathvec_from_dm().
> > 
> > Right, thanks.
> > 
> > I've taken the liberty to fix the typo ("libmutipath") and make
> > some
> > minor coding style adjustments in my queue branch. You can inspect
> > the
> > result there.
> > 
> > The coding style stuff is work in progress, I've added a clang-
> > format
> > based workflow on my tip branch.
> > 
> > I'd like to avoid major reformatting of past code while enforcing
> > consistent formatting for present and future submissions which is
> > more-
> > or-less in line with what we used to do "sub-conciously" during the
> > last years. clang-format has a lot of options [1]. I'm still
> > struggling
> > to find style settings that match ours [2].
> > 
> > Let me know if you dislike this, or if you have suggestions for
> > improving the settings.
> 
> I'm fine with your changes and with using clang-format in general.
> And I
> do prefer just formatting our new changes, like you mentioned in your
> tip commit, but if someone wants to make the case for a mass
> reformat,
> I'm willing to consider it. 

Ok, let's try it then. If it becomes too annoying we can disable the
workflow any time.

Martin