All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] fix issue of multipathd not tracking device
@ 2025-03-31 23:17 Benjamin Marzinski
  2025-03-31 23:17 ` [PATCH v2 1/3] multipathd: monitor new multipath dev even if we can't update it Benjamin Marzinski
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Benjamin Marzinski @ 2025-03-31 23:17 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

I ran into an issue where multipathd wasn't tracking a multipath
device that was created by the multipath command. It turns out that if
multipathd fully initialized a path device, and that device later
goes offline and then a multipath device that could use that path is
created by multipath, multipathd will attempt to reload the device
to use the offline path, which will fail. This will cause it to not
track the multipath device at all.

The first patch fixes this. The second patch allows mutipathd to track
these offline paths that should belong to a device, and add them to
device once they come back online. The third patch is just a cleanup.

changes from v2
- 0002: Switch from tracking this state with pp->initialized to a new
        variable, as suggested by Martin Wilck. To make is so that
        multipathd can still show paths in this state without adding
        a new wildcard for it, add [offline] as a possible output for
        the %m (multipath device) path wildcard, for paths that
        couldn't be added to a multipath device because they are
        offline. Also constify a function parameter and add an
        explanitory comment as suggested by Martin.

Benjamin Marzinski (3):
  multipathd: monitor new multipath dev even if we can't update it
  multipathd: re-add paths skipped because they were offline
  multipathd: don't update paths in INIT_MISSING_UDEV

 libmultipath/print.c       |  5 +++-
 libmultipath/structs.h     |  1 +
 libmultipath/structs_vec.c |  5 ++++
 multipathd/main.c          | 60 ++++++++++++++++++++++++++++++++++++--
 multipathd/multipathd.8.in |  5 ++--
 5 files changed, 70 insertions(+), 6 deletions(-)

-- 
2.48.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/3] multipathd: monitor new multipath dev even if we can't update it
  2025-03-31 23:17 [PATCH v2 0/3] fix issue of multipathd not tracking device Benjamin Marzinski
@ 2025-03-31 23:17 ` Benjamin Marzinski
  2025-03-31 23:17 ` [PATCH v2 2/3] multipathd: re-add paths skipped because they were offline Benjamin Marzinski
  2025-03-31 23:17 ` [PATCH v2 3/3] multipathd: don't update paths in INIT_MISSING_UDEV Benjamin Marzinski
  2 siblings, 0 replies; 6+ messages in thread
From: Benjamin Marzinski @ 2025-03-31 23:17 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck, Martin Wilck

If a multipath device was created by the multipath command, multipathd
might not agree with how the device was created. ev_add_map() can reload
the device with a different table by calling add_map_without_path() ->
update_map(). If this reloading of the map failed, multipathd was simply
ignoring the multipath device, even though it still existed.

One way that reloading can fail is if a path that multipathd already has
initialized goes offline. If a multipath device is created by the
multipath command while the path is offline, it will not use the offline
path, since multipath won't be able to get the necessary pathinfo.
However, multipathd will already have the pathinfo for the path, and may
not even know that it's offline, since the path is an orphan. When it
tries to reload the device, it will include the offline path, and the
reload will fail.

Instead of ignoring the device if it can't reload it, multipathd should
just montior it as it is. When the path device is no longer offline, it
can be added back to the multipath device by calling
"multipathd reconfigure" or "multipathd add path <path>".

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck  <mwilck@suse.com>
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index e63b6aa7..7aaae773 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -679,7 +679,7 @@ retry:
 	}
 
 fail:
-	if (new_map && (retries < 0 || wait_for_events(mpp, vecs))) {
+	if (new_map && wait_for_events(mpp, vecs)) {
 		condlog(0, "%s: failed to create new map", mpp->alias);
 		remove_map(mpp, vecs->pathvec, vecs->mpvec);
 		return 1;
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] multipathd: re-add paths skipped because they were offline
  2025-03-31 23:17 [PATCH v2 0/3] fix issue of multipathd not tracking device Benjamin Marzinski
  2025-03-31 23:17 ` [PATCH v2 1/3] multipathd: monitor new multipath dev even if we can't update it Benjamin Marzinski
@ 2025-03-31 23:17 ` Benjamin Marzinski
  2025-04-02 16:42   ` Martin Wilck
  2025-03-31 23:17 ` [PATCH v2 3/3] multipathd: don't update paths in INIT_MISSING_UDEV Benjamin Marzinski
  2 siblings, 1 reply; 6+ messages in thread
From: Benjamin Marzinski @ 2025-03-31 23:17 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

When a new device is added by the multipath command, multipathd may know
of other paths that cannot be added to the device because they are
currently offline. Instead of ignoring these paths, multipathd will now
re-add them when they come back online. To do this, it multipathd needs
a new path variable add_when_online, to track devices that could not be
added to an existing multipath device because they were offline. These
paths are handled along with the other uninitialized paths.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/print.c       |  5 +++-
 libmultipath/structs.h     |  1 +
 libmultipath/structs_vec.c |  5 ++++
 multipathd/main.c          | 59 ++++++++++++++++++++++++++++++++++++--
 multipathd/multipathd.8.in |  5 ++--
 5 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 00c03ace..019ae56f 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -658,8 +658,11 @@ snprint_path_serial (struct strbuf *buff, const struct path * pp)
 static int
 snprint_path_mpp (struct strbuf *buff, const struct path * pp)
 {
-	if (!pp->mpp)
+	if (!pp->mpp) {
+		if (pp->add_when_online)
+			return append_strbuf_str(buff, "[offline]");
 		return append_strbuf_str(buff, "[orphan]");
+	}
 	if (!pp->mpp->alias)
 		return append_strbuf_str(buff, "[unknown]");
 	return snprint_str(buff, pp->mpp->alias);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 28de9a7f..39d1c71c 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -413,6 +413,7 @@ struct path {
 	int eh_deadline;
 	enum check_path_states is_checked;
 	bool can_use_env_uid;
+	bool add_when_online;
 	unsigned int checker_timeout;
 	/* configlet pointers */
 	vector hwe;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index f6407e12..663c9053 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -389,6 +389,9 @@ static void orphan_paths(vector pathvec, struct multipath *mpp, const char *reas
 				free_path(pp);
 			} else
 				orphan_path(pp, reason);
+		} else if (pp->add_when_online &&
+			   strncmp(mpp->wwid, pp->wwid, WWID_SIZE) == 0) {
+			pp->add_when_online = false;
 		}
 	}
 }
@@ -595,6 +598,8 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 		found = 0;
 		vector_foreach_slot(mpp->pg, pgp, j) {
 			if (find_slot(pgp->paths, (void *)pp) != -1) {
+				if (pp->add_when_online)
+					pp->add_when_online = false;
 				found = 1;
 				break;
 			}
diff --git a/multipathd/main.c b/multipathd/main.c
index 7aaae773..9aa5a2fa 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -644,11 +644,45 @@ pr_register_active_paths(struct multipath *mpp)
 	}
 }
 
+static void
+save_offline_paths(const struct multipath *mpp, vector offline_paths)
+{
+	unsigned int i, j;
+	struct path *pp;
+	struct pathgroup *pgp;
+
+	vector_foreach_slot (mpp->pg, pgp, i)
+		vector_foreach_slot (pgp->paths, pp, j)
+			if (pp->initialized == INIT_OK &&
+			    pp->sysfs_state == PATH_DOWN)
+				/* ignore failures storing the paths. */
+				store_path(offline_paths, pp);
+}
+
+static void
+handle_orphaned_offline_paths(vector offline_paths)
+{
+	unsigned int i;
+	struct path *pp;
+
+	vector_foreach_slot (offline_paths, pp, i)
+		if (pp->mpp == NULL)
+			pp->add_when_online = true;
+}
+
+static void
+cleanup_reset_vec(struct vector_s **v)
+{
+	vector_reset(*v);
+}
+
 static int
 update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
 	int retries = 3;
 	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
+	struct vector_s offline_paths_vec = { .allocated = 0 };
+	vector offline_paths __attribute__((cleanup(cleanup_reset_vec))) = &offline_paths_vec;
 
 retry:
 	condlog(4, "%s: updating new map", mpp->alias);
@@ -685,6 +719,9 @@ fail:
 		return 1;
 	}
 
+	if (new_map && retries < 0)
+		save_offline_paths(mpp, offline_paths);
+
 	if (setup_multipath(vecs, mpp))
 		return 1;
 
@@ -695,6 +732,9 @@ fail:
 	if (mpp->prflag == PRFLAG_SET)
 		pr_register_active_paths(mpp);
 
+	if (VECTOR_SIZE(offline_paths) != 0)
+		handle_orphaned_offline_paths(offline_paths);
+
 	if (retries < 0)
 		condlog(0, "%s: failed reload in new map update", mpp->alias);
 	return 0;
@@ -2793,7 +2833,8 @@ check_uninitialized_path(struct path * pp, unsigned int ticks)
 	struct config *conf;
 
 	if (pp->initialized != INIT_NEW && pp->initialized != INIT_FAILED &&
-	    pp->initialized != INIT_MISSING_UDEV)
+	    pp->initialized != INIT_MISSING_UDEV &&
+	    !(pp->initialized == INIT_OK && pp->add_when_online))
 		return CHECK_PATH_SKIPPED;
 
 	if (pp->tick)
@@ -2849,7 +2890,8 @@ update_uninitialized_path(struct vectors * vecs, struct path * pp)
 	struct config *conf;
 
 	if (pp->initialized != INIT_NEW && pp->initialized != INIT_FAILED &&
-	    pp->initialized != INIT_MISSING_UDEV)
+	    pp->initialized != INIT_MISSING_UDEV &&
+	    !(pp->initialized == INIT_OK && pp->add_when_online))
 		return CHECK_PATH_SKIPPED;
 
 	newstate = get_new_state(pp);
@@ -2875,6 +2917,19 @@ update_uninitialized_path(struct vectors * vecs, struct path * pp)
 			free_path(pp);
 			return CHECK_PATH_REMOVED;
 		}
+	} else if (pp->initialized == INIT_OK && pp->add_when_online &&
+		   (newstate == PATH_UP || newstate == PATH_GHOST)) {
+		pp->initialized = INIT_OK;
+		if (pp->recheck_wwid == RECHECK_WWID_ON &&
+		    check_path_wwid_change(pp)) {
+			condlog(0, "%s: path wwid change detected. Removing",
+				pp->dev);
+			return handle_path_wwid_change(pp, vecs)?
+					CHECK_PATH_REMOVED :
+					CHECK_PATH_SKIPPED;
+		}
+		ev_add_path(pp, vecs, 1);
+		pp->tick = 1;
 	}
 	return CHECK_PATH_CHECKED;
 }
diff --git a/multipathd/multipathd.8.in b/multipathd/multipathd.8.in
index 43f87bf8..a91acff1 100644
--- a/multipathd/multipathd.8.in
+++ b/multipathd/multipathd.8.in
@@ -595,8 +595,9 @@ The device serial number.
 The device marginal state, either \fImarginal\fR or \fInormal\fR.
 .TP
 .B %m
-The multipath device that this device is a path of, or \fI[orphan]\fR if
-it is not part of any multipath device.
+The multipath device that this device is a path of, or \fI[offline]\fR
+if this device could not be added to a device because is is offline or
+\fI[orphan]\fR if it is not part of any multipath device.
 .TP
 .B %N
 The host World Wide Node Name (WWNN) of the device, if any.
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] multipathd: don't update paths in INIT_MISSING_UDEV
  2025-03-31 23:17 [PATCH v2 0/3] fix issue of multipathd not tracking device Benjamin Marzinski
  2025-03-31 23:17 ` [PATCH v2 1/3] multipathd: monitor new multipath dev even if we can't update it Benjamin Marzinski
  2025-03-31 23:17 ` [PATCH v2 2/3] multipathd: re-add paths skipped because they were offline Benjamin Marzinski
@ 2025-03-31 23:17 ` Benjamin Marzinski
  2 siblings, 0 replies; 6+ messages in thread
From: Benjamin Marzinski @ 2025-03-31 23:17 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck, Martin Wilck

There is nothing for update_uninitialized_path() to do for paths in the
INIT_MISSING_UDEV state. In fact, there shouldn't be any paths in this
state when update_uninitialized_path() is called, since they will have
switched to a different state in check_uninitialized_path().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck  <mwilck@suse.com>
---
 multipathd/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 9aa5a2fa..40ca83c7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2890,7 +2890,6 @@ update_uninitialized_path(struct vectors * vecs, struct path * pp)
 	struct config *conf;
 
 	if (pp->initialized != INIT_NEW && pp->initialized != INIT_FAILED &&
-	    pp->initialized != INIT_MISSING_UDEV &&
 	    !(pp->initialized == INIT_OK && pp->add_when_online))
 		return CHECK_PATH_SKIPPED;
 
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] multipathd: re-add paths skipped because they were offline
  2025-03-31 23:17 ` [PATCH v2 2/3] multipathd: re-add paths skipped because they were offline Benjamin Marzinski
@ 2025-04-02 16:42   ` Martin Wilck
  2025-04-02 18:31     ` Benjamin Marzinski
  0 siblings, 1 reply; 6+ messages in thread
From: Martin Wilck @ 2025-04-02 16:42 UTC (permalink / raw)
  To: Benjamin Marzinski, Christophe Varoqui; +Cc: device-mapper development

On Mon, 2025-03-31 at 19:17 -0400, Benjamin Marzinski wrote:
> When a new device is added by the multipath command, multipathd may
> know
> of other paths that cannot be added to the device because they are
> currently offline. Instead of ignoring these paths, multipathd will
> now
> re-add them when they come back online. To do this, it multipathd
> needs
> a new path variable add_when_online, to track devices that could not
> be
> added to an existing multipath device because they were offline.
> These
> paths are handled along with the other uninitialized paths.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/print.c       |  5 +++-
>  libmultipath/structs.h     |  1 +
>  libmultipath/structs_vec.c |  5 ++++
>  multipathd/main.c          | 59
> ++++++++++++++++++++++++++++++++++++--
>  multipathd/multipathd.8.in |  5 ++--
>  5 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/libmultipath/print.c b/libmultipath/print.c
> index 00c03ace..019ae56f 100644
> --- a/libmultipath/print.c
> +++ b/libmultipath/print.c
> @@ -658,8 +658,11 @@ snprint_path_serial (struct strbuf *buff, const
> struct path * pp)
>  static int
>  snprint_path_mpp (struct strbuf *buff, const struct path * pp)
>  {
> -	if (!pp->mpp)
> +	if (!pp->mpp) {
> +		if (pp->add_when_online)
> +			return append_strbuf_str(buff, "[offline]");
>  		return append_strbuf_str(buff, "[orphan]");
> +	}
>  	if (!pp->mpp->alias)
>  		return append_strbuf_str(buff, "[unknown]");
>  	return snprint_str(buff, pp->mpp->alias);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 28de9a7f..39d1c71c 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -413,6 +413,7 @@ struct path {
>  	int eh_deadline;
>  	enum check_path_states is_checked;
>  	bool can_use_env_uid;
> +	bool add_when_online;
>  	unsigned int checker_timeout;
>  	/* configlet pointers */
>  	vector hwe;
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index f6407e12..663c9053 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -389,6 +389,9 @@ static void orphan_paths(vector pathvec, struct
> multipath *mpp, const char *reas
>  				free_path(pp);
>  			} else
>  				orphan_path(pp, reason);
> +		} else if (pp->add_when_online &&
> +			   strncmp(mpp->wwid, pp->wwid, WWID_SIZE)
> == 0) {
> +			pp->add_when_online = false;
>  		}
>  	}
>  }
> @@ -595,6 +598,8 @@ void sync_paths(struct multipath *mpp, vector
> pathvec)
>  		found = 0;
>  		vector_foreach_slot(mpp->pg, pgp, j) {
>  			if (find_slot(pgp->paths, (void *)pp) != -1)
> {
> +				if (pp->add_when_online)
> +					pp->add_when_online = false;
>  				found = 1;
>  				break;
>  			}
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 7aaae773..9aa5a2fa 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -644,11 +644,45 @@ pr_register_active_paths(struct multipath *mpp)
>  	}
>  }
>  
> +static void
> +save_offline_paths(const struct multipath *mpp, vector
> offline_paths)
> +{
> +	unsigned int i, j;
> +	struct path *pp;
> +	struct pathgroup *pgp;
> +
> +	vector_foreach_slot (mpp->pg, pgp, i)
> +		vector_foreach_slot (pgp->paths, pp, j)
> +			if (pp->initialized == INIT_OK &&
> +			    pp->sysfs_state == PATH_DOWN)
> +				/* ignore failures storing the
> paths. */
> +				store_path(offline_paths, pp);
> +}
> +
> +static void
> +handle_orphaned_offline_paths(vector offline_paths)
> +{
> +	unsigned int i;
> +	struct path *pp;
> +
> +	vector_foreach_slot (offline_paths, pp, i)
> +		if (pp->mpp == NULL)
> +			pp->add_when_online = true;
> +}
> +
> +static void
> +cleanup_reset_vec(struct vector_s **v)
> +{
> +	vector_reset(*v);
> +}
> +
>  static int
>  update_map (struct multipath *mpp, struct vectors *vecs, int
> new_map)
>  {
>  	int retries = 3;
>  	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> +	struct vector_s offline_paths_vec = { .allocated = 0 };
> +	vector offline_paths
> __attribute__((cleanup(cleanup_reset_vec))) = &offline_paths_vec;
>  
>  retry:
>  	condlog(4, "%s: updating new map", mpp->alias);
> @@ -685,6 +719,9 @@ fail:
>  		return 1;
>  	}
>  
> +	if (new_map && retries < 0)
> +		save_offline_paths(mpp, offline_paths);
> +
>  	if (setup_multipath(vecs, mpp))
>  		return 1;
>  
> @@ -695,6 +732,9 @@ fail:
>  	if (mpp->prflag == PRFLAG_SET)
>  		pr_register_active_paths(mpp);
>  
> +	if (VECTOR_SIZE(offline_paths) != 0)
> +		handle_orphaned_offline_paths(offline_paths);
> +
>  	if (retries < 0)
>  		condlog(0, "%s: failed reload in new map update",
> mpp->alias);
>  	return 0;
> @@ -2793,7 +2833,8 @@ check_uninitialized_path(struct path * pp,
> unsigned int ticks)
>  	struct config *conf;
>  
>  	if (pp->initialized != INIT_NEW && pp->initialized !=
> INIT_FAILED &&
> -	    pp->initialized != INIT_MISSING_UDEV)
> +	    pp->initialized != INIT_MISSING_UDEV &&
> +	    !(pp->initialized == INIT_OK && pp->add_when_online))
>  		return CHECK_PATH_SKIPPED;
>  
>  	if (pp->tick)
> @@ -2849,7 +2890,8 @@ update_uninitialized_path(struct vectors *
> vecs, struct path * pp)
>  	struct config *conf;
>  
>  	if (pp->initialized != INIT_NEW && pp->initialized !=
> INIT_FAILED &&
> -	    pp->initialized != INIT_MISSING_UDEV)
> +	    pp->initialized != INIT_MISSING_UDEV &&
> +	    !(pp->initialized == INIT_OK && pp->add_when_online))
>  		return CHECK_PATH_SKIPPED;
>  
>  	newstate = get_new_state(pp);
> @@ -2875,6 +2917,19 @@ update_uninitialized_path(struct vectors *
> vecs, struct path * pp)
>  			free_path(pp);
>  			return CHECK_PATH_REMOVED;
>  		}
> +	} else if (pp->initialized == INIT_OK && pp->add_when_online
> &&
> +		   (newstate == PATH_UP || newstate == PATH_GHOST))
> {
> +		pp->initialized = INIT_OK;
> +		if (pp->recheck_wwid == RECHECK_WWID_ON &&

I wonder if we should always check the WWID here. After all, this path
has never been part of the map, and it was offline when the map was
created (IOW, the WWID couldn't be checked at that point in time). 
Can we rely on the stored WWID?

Other than that, LGTM.

Regards,
Martin

> +		    check_path_wwid_change(pp)) {
> +			condlog(0, "%s: path wwid change detected.
> Removing",
> +				pp->dev);
> +			return handle_path_wwid_change(pp, vecs)?
> +					CHECK_PATH_REMOVED :
> +					CHECK_PATH_SKIPPED;
> +		}
> +		ev_add_path(pp, vecs, 1);
> +		pp->tick = 1;
>  	}
>  	return CHECK_PATH_CHECKED;
>  }
> diff --git a/multipathd/multipathd.8.in b/multipathd/multipathd.8.in
> index 43f87bf8..a91acff1 100644
> --- a/multipathd/multipathd.8.in
> +++ b/multipathd/multipathd.8.in
> @@ -595,8 +595,9 @@ The device serial number.
>  The device marginal state, either \fImarginal\fR or \fInormal\fR.
>  .TP
>  .B %m
> -The multipath device that this device is a path of, or
> \fI[orphan]\fR if
> -it is not part of any multipath device.
> +The multipath device that this device is a path of, or
> \fI[offline]\fR
> +if this device could not be added to a device because is is offline
> or
> +\fI[orphan]\fR if it is not part of any multipath device.
>  .TP
>  .B %N
>  The host World Wide Node Name (WWNN) of the device, if any.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] multipathd: re-add paths skipped because they were offline
  2025-04-02 16:42   ` Martin Wilck
@ 2025-04-02 18:31     ` Benjamin Marzinski
  0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Marzinski @ 2025-04-02 18:31 UTC (permalink / raw)
  To: Martin Wilck; +Cc: Christophe Varoqui, device-mapper development

On Wed, Apr 02, 2025 at 06:42:34PM +0200, Martin Wilck wrote:
> On Mon, 2025-03-31 at 19:17 -0400, Benjamin Marzinski wrote:
> > When a new device is added by the multipath command, multipathd may
> > know
> > of other paths that cannot be added to the device because they are
> > currently offline. Instead of ignoring these paths, multipathd will
> > now
> > re-add them when they come back online. To do this, it multipathd
> > needs
> > a new path variable add_when_online, to track devices that could not
> > be
> > added to an existing multipath device because they were offline.
> > These
> > paths are handled along with the other uninitialized paths.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/print.c       |  5 +++-
> >  libmultipath/structs.h     |  1 +
> >  libmultipath/structs_vec.c |  5 ++++
> >  multipathd/main.c          | 59
> > ++++++++++++++++++++++++++++++++++++--
> >  multipathd/multipathd.8.in |  5 ++--
> >  5 files changed, 70 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libmultipath/print.c b/libmultipath/print.c
> > index 00c03ace..019ae56f 100644
> > --- a/libmultipath/print.c
> > +++ b/libmultipath/print.c
> > @@ -658,8 +658,11 @@ snprint_path_serial (struct strbuf *buff, const
> > struct path * pp)
> >  static int
> >  snprint_path_mpp (struct strbuf *buff, const struct path * pp)
> >  {
> > -	if (!pp->mpp)
> > +	if (!pp->mpp) {
> > +		if (pp->add_when_online)
> > +			return append_strbuf_str(buff, "[offline]");
> >  		return append_strbuf_str(buff, "[orphan]");
> > +	}
> >  	if (!pp->mpp->alias)
> >  		return append_strbuf_str(buff, "[unknown]");
> >  	return snprint_str(buff, pp->mpp->alias);
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index 28de9a7f..39d1c71c 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -413,6 +413,7 @@ struct path {
> >  	int eh_deadline;
> >  	enum check_path_states is_checked;
> >  	bool can_use_env_uid;
> > +	bool add_when_online;
> >  	unsigned int checker_timeout;
> >  	/* configlet pointers */
> >  	vector hwe;
> > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> > index f6407e12..663c9053 100644
> > --- a/libmultipath/structs_vec.c
> > +++ b/libmultipath/structs_vec.c
> > @@ -389,6 +389,9 @@ static void orphan_paths(vector pathvec, struct
> > multipath *mpp, const char *reas
> >  				free_path(pp);
> >  			} else
> >  				orphan_path(pp, reason);
> > +		} else if (pp->add_when_online &&
> > +			   strncmp(mpp->wwid, pp->wwid, WWID_SIZE)
> > == 0) {
> > +			pp->add_when_online = false;
> >  		}
> >  	}
> >  }
> > @@ -595,6 +598,8 @@ void sync_paths(struct multipath *mpp, vector
> > pathvec)
> >  		found = 0;
> >  		vector_foreach_slot(mpp->pg, pgp, j) {
> >  			if (find_slot(pgp->paths, (void *)pp) != -1)
> > {
> > +				if (pp->add_when_online)
> > +					pp->add_when_online = false;
> >  				found = 1;
> >  				break;
> >  			}
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 7aaae773..9aa5a2fa 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -644,11 +644,45 @@ pr_register_active_paths(struct multipath *mpp)
> >  	}
> >  }
> >  
> > +static void
> > +save_offline_paths(const struct multipath *mpp, vector
> > offline_paths)
> > +{
> > +	unsigned int i, j;
> > +	struct path *pp;
> > +	struct pathgroup *pgp;
> > +
> > +	vector_foreach_slot (mpp->pg, pgp, i)
> > +		vector_foreach_slot (pgp->paths, pp, j)
> > +			if (pp->initialized == INIT_OK &&
> > +			    pp->sysfs_state == PATH_DOWN)
> > +				/* ignore failures storing the
> > paths. */
> > +				store_path(offline_paths, pp);
> > +}
> > +
> > +static void
> > +handle_orphaned_offline_paths(vector offline_paths)
> > +{
> > +	unsigned int i;
> > +	struct path *pp;
> > +
> > +	vector_foreach_slot (offline_paths, pp, i)
> > +		if (pp->mpp == NULL)
> > +			pp->add_when_online = true;
> > +}
> > +
> > +static void
> > +cleanup_reset_vec(struct vector_s **v)
> > +{
> > +	vector_reset(*v);
> > +}
> > +
> >  static int
> >  update_map (struct multipath *mpp, struct vectors *vecs, int
> > new_map)
> >  {
> >  	int retries = 3;
> >  	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> > +	struct vector_s offline_paths_vec = { .allocated = 0 };
> > +	vector offline_paths
> > __attribute__((cleanup(cleanup_reset_vec))) = &offline_paths_vec;
> >  
> >  retry:
> >  	condlog(4, "%s: updating new map", mpp->alias);
> > @@ -685,6 +719,9 @@ fail:
> >  		return 1;
> >  	}
> >  
> > +	if (new_map && retries < 0)
> > +		save_offline_paths(mpp, offline_paths);
> > +
> >  	if (setup_multipath(vecs, mpp))
> >  		return 1;
> >  
> > @@ -695,6 +732,9 @@ fail:
> >  	if (mpp->prflag == PRFLAG_SET)
> >  		pr_register_active_paths(mpp);
> >  
> > +	if (VECTOR_SIZE(offline_paths) != 0)
> > +		handle_orphaned_offline_paths(offline_paths);
> > +
> >  	if (retries < 0)
> >  		condlog(0, "%s: failed reload in new map update",
> > mpp->alias);
> >  	return 0;
> > @@ -2793,7 +2833,8 @@ check_uninitialized_path(struct path * pp,
> > unsigned int ticks)
> >  	struct config *conf;
> >  
> >  	if (pp->initialized != INIT_NEW && pp->initialized !=
> > INIT_FAILED &&
> > -	    pp->initialized != INIT_MISSING_UDEV)
> > +	    pp->initialized != INIT_MISSING_UDEV &&
> > +	    !(pp->initialized == INIT_OK && pp->add_when_online))
> >  		return CHECK_PATH_SKIPPED;
> >  
> >  	if (pp->tick)
> > @@ -2849,7 +2890,8 @@ update_uninitialized_path(struct vectors *
> > vecs, struct path * pp)
> >  	struct config *conf;
> >  
> >  	if (pp->initialized != INIT_NEW && pp->initialized !=
> > INIT_FAILED &&
> > -	    pp->initialized != INIT_MISSING_UDEV)
> > +	    pp->initialized != INIT_MISSING_UDEV &&
> > +	    !(pp->initialized == INIT_OK && pp->add_when_online))
> >  		return CHECK_PATH_SKIPPED;
> >  
> >  	newstate = get_new_state(pp);
> > @@ -2875,6 +2917,19 @@ update_uninitialized_path(struct vectors *
> > vecs, struct path * pp)
> >  			free_path(pp);
> >  			return CHECK_PATH_REMOVED;
> >  		}
> > +	} else if (pp->initialized == INIT_OK && pp->add_when_online
> > &&
> > +		   (newstate == PATH_UP || newstate == PATH_GHOST))
> > {
> > +		pp->initialized = INIT_OK;
> > +		if (pp->recheck_wwid == RECHECK_WWID_ON &&
> 
> I wonder if we should always check the WWID here. After all, this path
> has never been part of the map, and it was offline when the map was
> created (IOW, the WWID couldn't be checked at that point in time). 
> Can we rely on the stored WWID?

That make sense. I also noticed a typo in my documentation changes. I'll
send a new patch.

-Ben

> Other than that, LGTM.
> 
> Regards,
> Martin
> 
> > +		    check_path_wwid_change(pp)) {
> > +			condlog(0, "%s: path wwid change detected.
> > Removing",
> > +				pp->dev);
> > +			return handle_path_wwid_change(pp, vecs)?
> > +					CHECK_PATH_REMOVED :
> > +					CHECK_PATH_SKIPPED;
> > +		}
> > +		ev_add_path(pp, vecs, 1);
> > +		pp->tick = 1;
> >  	}
> >  	return CHECK_PATH_CHECKED;
> >  }
> > diff --git a/multipathd/multipathd.8.in b/multipathd/multipathd.8.in
> > index 43f87bf8..a91acff1 100644
> > --- a/multipathd/multipathd.8.in
> > +++ b/multipathd/multipathd.8.in
> > @@ -595,8 +595,9 @@ The device serial number.
> >  The device marginal state, either \fImarginal\fR or \fInormal\fR.
> >  .TP
> >  .B %m
> > -The multipath device that this device is a path of, or
> > \fI[orphan]\fR if
> > -it is not part of any multipath device.
> > +The multipath device that this device is a path of, or
> > \fI[offline]\fR
> > +if this device could not be added to a device because is is offline
> > or
> > +\fI[orphan]\fR if it is not part of any multipath device.
> >  .TP
> >  .B %N
> >  The host World Wide Node Name (WWNN) of the device, if any.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-04-02 18:31 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-31 23:17 [PATCH v2 0/3] fix issue of multipathd not tracking device Benjamin Marzinski
2025-03-31 23:17 ` [PATCH v2 1/3] multipathd: monitor new multipath dev even if we can't update it Benjamin Marzinski
2025-03-31 23:17 ` [PATCH v2 2/3] multipathd: re-add paths skipped because they were offline Benjamin Marzinski
2025-04-02 16:42   ` Martin Wilck
2025-04-02 18:31     ` Benjamin Marzinski
2025-03-31 23:17 ` [PATCH v2 3/3] multipathd: don't update paths in INIT_MISSING_UDEV Benjamin Marzinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.