[PATCH 0/2] multipath: set rport port_state on NVMe FPIN events

[PATCH 0/2] multipath: set rport port_state on NVMe FPIN events

[Benjamin Marzinski]

When multipathd handles link integrity FPIN events, it sets the rport
associated with the effected SCSI paths to Marginal. These patches make
it do the same thing for NVMe paths.

I actually have a stupid Fibre Channel question. Does the host number
and the target wwpn uniquely identfy an rport? I never see the channel
set to anything other than 0, but I don't think that it MUST be 0. But
if we knew that there was only one rport for a given host number and
target WWPN, we could stop checking as soon as we found one. These
patches don't do that. They check all the rports.

Benjamin Marzinski (2):
  libmultipath: export udev pthread cleanup functions
  multipathd: set rport port_state to marginal for NVMe devices

 libmultipath/discovery.c          |  4 +-
 libmultipath/discovery.h          |  2 +
 libmultipath/libmultipath.version |  6 +++
 multipathd/fpin_handlers.c        | 74 ++++++++++++++++++++++++++++---
 4 files changed, 78 insertions(+), 8 deletions(-)

-- 
2.46.2

[PATCH 1/2] libmultipath: export udev pthread cleanup functions

[Benjamin Marzinski]

A future patch will make use of cleanup_udev_enumerate_ptr() and
cleanup_udev_device_ptr().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c          | 4 ++--
 libmultipath/discovery.h          | 2 ++
 libmultipath/libmultipath.version | 6 ++++++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index b5851561..72ea0c98 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -146,7 +146,7 @@ path_discover (vector pathvec, struct config * conf,
 		return pathinfo(pp, conf, flag);
 }
 
-static void cleanup_udev_enumerate_ptr(void *arg)
+void cleanup_udev_enumerate_ptr(void *arg)
 {
 	struct udev_enumerate *ue;
 
@@ -157,7 +157,7 @@ static void cleanup_udev_enumerate_ptr(void *arg)
 		(void)udev_enumerate_unref(ue);
 }
 
-static void cleanup_udev_device_ptr(void *arg)
+void cleanup_udev_device_ptr(void *arg)
 {
 	struct udev_device *ud;
 
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 7d42eae5..1f7a6e20 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -59,6 +59,8 @@ bool has_uid_fallback(struct path *pp);
 int get_uid(struct path * pp, int path_state, struct udev_device *udev,
 	    int allow_fallback);
 bool is_vpd_page_supported(int fd, int pg);
+void cleanup_udev_enumerate_ptr(void *arg);
+void cleanup_udev_device_ptr(void *arg);
 
 /*
  * discovery bitmask
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 6bdf6944..63f970b7 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -244,3 +244,9 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_27.1.0 {
+global:
+	cleanup_udev_enumerate_ptr;
+	cleanup_udev_device_ptr;
+} LIBMULTIPATH_27.0.0;
-- 
2.46.2

RE: [PATCH 1/2] libmultipath: export udev pthread cleanup functions

[Muneendra Kumar M]

[-- Attachment #1: Type: text/plain, Size: 3077 bytes --]

Hi Benjamin,
Change's looks good.

Reviewed-by: Muneendra Kumar <muneendra.kumar@broadcom.com>



-----Original Message-----
From: Benjamin Marzinski <bmarzins@redhat.com>
Sent: Friday, December 20, 2024 7:33 AM
To: Christophe Varoqui <christophe.varoqui@opensvc.com>
Cc: device-mapper development <dm-devel@lists.linux.dev>; Martin Wilck
<Martin.Wilck@suse.com>; Muneendra Kumar <muneendra.kumar@broadcom.com>
Subject: [PATCH 1/2] libmultipath: export udev pthread cleanup functions

A future patch will make use of cleanup_udev_enumerate_ptr() and
cleanup_udev_device_ptr().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c          | 4 ++--
 libmultipath/discovery.h          | 2 ++
 libmultipath/libmultipath.version | 6 ++++++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index
b5851561..72ea0c98 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -146,7 +146,7 @@ path_discover (vector pathvec, struct config * conf,
 		return pathinfo(pp, conf, flag);
 }

-static void cleanup_udev_enumerate_ptr(void *arg)
+void cleanup_udev_enumerate_ptr(void *arg)
 {
 	struct udev_enumerate *ue;

@@ -157,7 +157,7 @@ static void cleanup_udev_enumerate_ptr(void *arg)
 		(void)udev_enumerate_unref(ue);
 }

-static void cleanup_udev_device_ptr(void *arg)
+void cleanup_udev_device_ptr(void *arg)
 {
 	struct udev_device *ud;

diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h index
7d42eae5..1f7a6e20 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -59,6 +59,8 @@ bool has_uid_fallback(struct path *pp);  int
get_uid(struct path * pp, int path_state, struct udev_device *udev,
 	    int allow_fallback);
 bool is_vpd_page_supported(int fd, int pg);
+void cleanup_udev_enumerate_ptr(void *arg); void
+cleanup_udev_device_ptr(void *arg);

 /*
  * discovery bitmask
diff --git a/libmultipath/libmultipath.version
b/libmultipath/libmultipath.version
index 6bdf6944..63f970b7 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -244,3 +244,9 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_27.1.0 {
+global:
+	cleanup_udev_enumerate_ptr;
+	cleanup_udev_device_ptr;
+} LIBMULTIPATH_27.0.0;
--
2.46.2

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4220 bytes --]

[PATCH 2/2] multipathd: set rport port_state to marginal for NVMe devices

[Benjamin Marzinski]

When a scsi path device is set to marginal, it updates the rport state.
Do this for NVMe devices as well.

Fixes: 1cada778 ("multipathd: Added support to handle FPIN-Li events for FC-NVMe")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/fpin_handlers.c | 74 ++++++++++++++++++++++++++++++++++----
 1 file changed, 68 insertions(+), 6 deletions(-)

diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c
index 6b56f9b7..8b436067 100644
--- a/multipathd/fpin_handlers.c
+++ b/multipathd/fpin_handlers.c
@@ -15,6 +15,7 @@
 #include "debug.h"
 #include "util.h"
 #include "sysfs.h"
+#include "discovery.h"
 
 #include "fpin.h"
 #include "devmapper.h"
@@ -253,7 +254,7 @@ static int  extract_nvme_addresses_chk_path_pwwn(const char *address,
  * with the els wwpn ,attached_wwpn and sets the path state to
  * Marginal
  */
-static void fpin_check_set_nvme_path_marginal(uint16_t host_num, struct path *pp,
+static bool fpin_check_set_nvme_path_marginal(uint16_t host_num, struct path *pp,
 		uint64_t els_wwpn, uint64_t attached_wwpn)
 {
 	struct udev_device *ctl = NULL;
@@ -263,21 +264,79 @@ static void fpin_check_set_nvme_path_marginal(uint16_t host_num, struct path *pp
 	ctl = udev_device_get_parent_with_subsystem_devtype(pp->udev, "nvme", NULL);
 	if (ctl == NULL) {
 		condlog(2, "%s: No parent device for ", pp->dev);
-		return;
+		return false;
 	}
 	address = udev_device_get_sysattr_value(ctl, "address");
 	if (!address) {
 		condlog(2, "%s: unable to get the address ", pp->dev);
-		return;
+		return false;
 	}
 	condlog(4, "\n address %s: dev :%s\n", address, pp->dev);
 	ret = extract_nvme_addresses_chk_path_pwwn(address, els_wwpn, attached_wwpn);
 	if (ret <= 0)
-		return;
+		return false;
 	ret = fpin_add_marginal_dev_info(host_num, pp->dev);
 	if (ret < 0)
-		return;
+		return false;
 	fpin_path_setmarginal(pp);
+	return true;
+}
+
+static void fpin_nvme_set_rport_marginal(uint16_t host_num, uint64_t els_wwpn)
+{
+	struct udev_enumerate *udev_enum = NULL;
+	struct udev_list_entry *entry;
+
+	pthread_cleanup_push(cleanup_udev_enumerate_ptr, &udev_enum);
+	udev_enum = udev_enumerate_new(udev);
+	if (!udev_enum) {
+		condlog(0, "fpin: rport udev_enumerate_new() failed: %m");
+		goto out;
+	}
+	if (udev_enumerate_add_match_subsystem(udev_enum, "fc_remote_ports") < 0 ||
+	    udev_enumerate_add_match_is_initialized(udev_enum) < 0 ||
+	    udev_enumerate_scan_devices(udev_enum) < 0) {
+		condlog(0, "fpin: error setting up rport enumeration: %m");
+		goto out;
+	}
+	udev_list_entry_foreach(entry,
+				udev_enumerate_get_list_entry(udev_enum)) {
+		const char *devpath;
+		const char *rport_id, *value;
+		struct udev_device *rport_dev = NULL;
+		uint16_t rport_hostnum;
+		uint64_t rport_wwpn;
+		unsigned int unused;
+
+		pthread_cleanup_push(cleanup_udev_device_ptr, &rport_dev);
+		devpath = udev_list_entry_get_name(entry);
+		if (!devpath)
+			goto next;
+		rport_id = libmp_basename(devpath);
+		if (sscanf(rport_id, "rport-%hu:%u-%u", &rport_hostnum, &unused,
+			   &unused) != 3 || rport_hostnum != host_num)
+			goto next;
+
+		rport_dev = udev_device_new_from_syspath(udev, devpath);
+		if (!rport_dev) {
+			condlog(0, "%s: error getting rport dev: %m", rport_id);
+			goto next;
+		}
+		value = udev_device_get_sysattr_value(rport_dev, "port_name");
+		if (!value) {
+			condlog(0, "%s: error getting port_name: %m", rport_id);
+			goto next;
+		}
+
+		rport_wwpn = strtol(value, NULL, 16);
+		/* If the rport wwpn matches, set the port state to marginal */
+		if (rport_wwpn == els_wwpn)
+			fpin_set_rport_marginal(rport_dev);
+next:
+		pthread_cleanup_pop(1);
+	}
+out:
+	pthread_cleanup_pop(1);
 }
 
 /*
@@ -338,6 +397,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t host_num,  struct vectors *ve
 	struct multipath *mpp;
 	int i, k;
 	int ret = 0;
+	bool found_nvme = false;
 
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
 	lock(&vecs->lock);
@@ -348,7 +408,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t host_num,  struct vectors *ve
 			continue;
 		/*checks if the bus type is nvme  and the protocol is FC-NVMe*/
 		if ((pp->bus == SYSFS_BUS_NVME) && (pp->sg_id.proto_id == NVME_PROTOCOL_FC)) {
-			fpin_check_set_nvme_path_marginal(host_num, pp, els_wwpn, attached_wwpn);
+			found_nvme = fpin_check_set_nvme_path_marginal(host_num, pp, els_wwpn, attached_wwpn) || found_nvme;
 		} else if ((pp->bus == SYSFS_BUS_SCSI) &&
 			(pp->sg_id.proto_id == SCSI_PROTOCOL_FCP) &&
 			(host_num ==  pp->sg_id.host_no)) {
@@ -356,6 +416,8 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t host_num,  struct vectors *ve
 			fpin_check_set_scsi_path_marginal(host_num, pp, els_wwpn);
 		}
 	}
+	if (found_nvme)
+		fpin_nvme_set_rport_marginal(host_num, els_wwpn);
 	/* walk backwards because reload_and_sync_map() can remove mpp */
 	vector_foreach_slot_backwards(vecs->mpvec, mpp, i) {
 		if (mpp->fpin_must_reload) {
-- 
2.46.2

RE: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe devices

[Muneendra Kumar M]

[-- Attachment #1: Type: text/plain, Size: 7471 bytes --]

Hi Benjamin,
Thanks for the changes.
But below is the reason why we didn't add support to set the rport
port_state to marginal for NVMe devices

In the case of SCSI  once rport-state is set to Marginal ,
If any pending I/O's on the marginal path hit's the scsi-timeout (after
abort success) the scsi-layer checks the rport-state  and If the
rport-state is set to Marginal it will not do any retries on the Marginal
path instead the I/O
Will be retried on the other Active paths.


This particular functionality(checking the rport-state and acting
accordingly) we didn't add( as far I know) in the case of NVME and we need
to check how we can handle this in the case of NVME .
That's the reason we didn't set the port state to Marginal in the case of
NVMe.


In a brief SCSI layer handles the case when the rport-state is set to
Marginal whereas in NVMe it doesn't(AFIK).

Atleast with the below changes we make sure that once we get the FPIN-LI
notification we will set the affected path , as well as the rport-state to
Marginal irrespective of SCSI and NVMe.

I am just thinking that until we have some changes in the NVME driver to
handle (the rport-state to marginal) this changes doesn't show any impact
in the case of NVMe
 other then keeping it  on par with SCSI implementation.

Please let me know your opinion on the same.


Regards,
Muneendra.



-----Original Message-----
From: Benjamin Marzinski <bmarzins@redhat.com>
Sent: Friday, December 20, 2024 7:33 AM
To: Christophe Varoqui <christophe.varoqui@opensvc.com>
Cc: device-mapper development <dm-devel@lists.linux.dev>; Martin Wilck
<Martin.Wilck@suse.com>; Muneendra Kumar <muneendra.kumar@broadcom.com>
Subject: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe
devices

When a scsi path device is set to marginal, it updates the rport state.
Do this for NVMe devices as well.

Fixes: 1cada778 ("multipathd: Added support to handle FPIN-Li events for
FC-NVMe")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/fpin_handlers.c | 74 ++++++++++++++++++++++++++++++++++----
 1 file changed, 68 insertions(+), 6 deletions(-)

diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c index
6b56f9b7..8b436067 100644
--- a/multipathd/fpin_handlers.c
+++ b/multipathd/fpin_handlers.c
@@ -15,6 +15,7 @@
 #include "debug.h"
 #include "util.h"
 #include "sysfs.h"
+#include "discovery.h"

 #include "fpin.h"
 #include "devmapper.h"
@@ -253,7 +254,7 @@ static int  extract_nvme_addresses_chk_path_pwwn(const
char *address,
  * with the els wwpn ,attached_wwpn and sets the path state to
  * Marginal
  */
-static void fpin_check_set_nvme_path_marginal(uint16_t host_num, struct
path *pp,
+static bool fpin_check_set_nvme_path_marginal(uint16_t host_num, struct
+path *pp,
 		uint64_t els_wwpn, uint64_t attached_wwpn)  {
 	struct udev_device *ctl = NULL;
@@ -263,21 +264,79 @@ static void
fpin_check_set_nvme_path_marginal(uint16_t host_num, struct path *pp
 	ctl = udev_device_get_parent_with_subsystem_devtype(pp->udev,
"nvme", NULL);
 	if (ctl == NULL) {
 		condlog(2, "%s: No parent device for ", pp->dev);
-		return;
+		return false;
 	}
 	address = udev_device_get_sysattr_value(ctl, "address");
 	if (!address) {
 		condlog(2, "%s: unable to get the address ", pp->dev);
-		return;
+		return false;
 	}
 	condlog(4, "\n address %s: dev :%s\n", address, pp->dev);
 	ret = extract_nvme_addresses_chk_path_pwwn(address, els_wwpn,
attached_wwpn);
 	if (ret <= 0)
-		return;
+		return false;
 	ret = fpin_add_marginal_dev_info(host_num, pp->dev);
 	if (ret < 0)
-		return;
+		return false;
 	fpin_path_setmarginal(pp);
+	return true;
+}
+
+static void fpin_nvme_set_rport_marginal(uint16_t host_num, uint64_t
+els_wwpn) {
+	struct udev_enumerate *udev_enum = NULL;
+	struct udev_list_entry *entry;
+
+	pthread_cleanup_push(cleanup_udev_enumerate_ptr, &udev_enum);
+	udev_enum = udev_enumerate_new(udev);
+	if (!udev_enum) {
+		condlog(0, "fpin: rport udev_enumerate_new() failed: %m");
+		goto out;
+	}
+	if (udev_enumerate_add_match_subsystem(udev_enum,
"fc_remote_ports") < 0 ||
+	    udev_enumerate_add_match_is_initialized(udev_enum) < 0 ||
+	    udev_enumerate_scan_devices(udev_enum) < 0) {
+		condlog(0, "fpin: error setting up rport enumeration:
%m");
+		goto out;
+	}
+	udev_list_entry_foreach(entry,
+				udev_enumerate_get_list_entry(udev_enum))
{
+		const char *devpath;
+		const char *rport_id, *value;
+		struct udev_device *rport_dev = NULL;
+		uint16_t rport_hostnum;
+		uint64_t rport_wwpn;
+		unsigned int unused;
+
+		pthread_cleanup_push(cleanup_udev_device_ptr, &rport_dev);
+		devpath = udev_list_entry_get_name(entry);
+		if (!devpath)
+			goto next;
+		rport_id = libmp_basename(devpath);
+		if (sscanf(rport_id, "rport-%hu:%u-%u", &rport_hostnum,
&unused,
+			   &unused) != 3 || rport_hostnum != host_num)
+			goto next;
+
+		rport_dev = udev_device_new_from_syspath(udev, devpath);
+		if (!rport_dev) {
+			condlog(0, "%s: error getting rport dev: %m",
rport_id);
+			goto next;
+		}
+		value = udev_device_get_sysattr_value(rport_dev,
"port_name");
+		if (!value) {
+			condlog(0, "%s: error getting port_name: %m",
rport_id);
+			goto next;
+		}
+
+		rport_wwpn = strtol(value, NULL, 16);
+		/* If the rport wwpn matches, set the port state to
marginal */
+		if (rport_wwpn == els_wwpn)
+			fpin_set_rport_marginal(rport_dev);
+next:
+		pthread_cleanup_pop(1);
+	}
+out:
+	pthread_cleanup_pop(1);
 }

 /*
@@ -338,6 +397,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
host_num,  struct vectors *ve
 	struct multipath *mpp;
 	int i, k;
 	int ret = 0;
+	bool found_nvme = false;

 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
 	lock(&vecs->lock);
@@ -348,7 +408,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
host_num,  struct vectors *ve
 			continue;
 		/*checks if the bus type is nvme  and the protocol is
FC-NVMe*/
 		if ((pp->bus == SYSFS_BUS_NVME) && (pp->sg_id.proto_id ==
NVME_PROTOCOL_FC)) {
-			fpin_check_set_nvme_path_marginal(host_num, pp,
els_wwpn, attached_wwpn);
+			found_nvme =
fpin_check_set_nvme_path_marginal(host_num, pp,
+els_wwpn, attached_wwpn) || found_nvme;
 		} else if ((pp->bus == SYSFS_BUS_SCSI) &&
 			(pp->sg_id.proto_id == SCSI_PROTOCOL_FCP) &&
 			(host_num ==  pp->sg_id.host_no)) {
@@ -356,6 +416,8 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
host_num,  struct vectors *ve
 			fpin_check_set_scsi_path_marginal(host_num, pp,
els_wwpn);
 		}
 	}
+	if (found_nvme)
+		fpin_nvme_set_rport_marginal(host_num, els_wwpn);
 	/* walk backwards because reload_and_sync_map() can remove mpp */
 	vector_foreach_slot_backwards(vecs->mpvec, mpp, i) {
 		if (mpp->fpin_must_reload) {
--
2.46.2

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4220 bytes --]

Re: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe devices

[Benjamin Marzinski]

On Fri, Dec 20, 2024 at 04:25:22PM +0530, Muneendra Kumar M wrote:
> Hi Benjamin,
> Thanks for the changes.
> But below is the reason why we didn't add support to set the rport
> port_state to marginal for NVMe devices
> 
> In the case of SCSI  once rport-state is set to Marginal ,
> If any pending I/O's on the marginal path hit's the scsi-timeout (after
> abort success) the scsi-layer checks the rport-state  and If the
> rport-state is set to Marginal it will not do any retries on the Marginal
> path instead the I/O
> Will be retried on the other Active paths.
> 
> 
> This particular functionality(checking the rport-state and acting
> accordingly) we didn't add( as far I know) in the case of NVME and we need
> to check how we can handle this in the case of NVME .
> That's the reason we didn't set the port state to Marginal in the case of
> NVMe.
> 
> 
> In a brief SCSI layer handles the case when the rport-state is set to
> Marginal whereas in NVMe it doesn't(AFIK).
> 
> Atleast with the below changes we make sure that once we get the FPIN-LI
> notification we will set the affected path , as well as the rport-state to
> Marginal irrespective of SCSI and NVMe.
> 
> I am just thinking that until we have some changes in the NVME driver to
> handle (the rport-state to marginal) this changes doesn't show any impact
> in the case of NVMe
>  other then keeping it  on par with SCSI implementation.
> 
> Please let me know your opinion on the same.

The request to do this came because the user wanted to see which target
ports were effected, but when they looked, none of them we in the
Marginal state. So there is some value in this for users, even if the
systems behavior doesn't change because of it.

-Ben

> 
> 
> Regards,
> Muneendra.
> 
> 
> 
> -----Original Message-----
> From: Benjamin Marzinski <bmarzins@redhat.com>
> Sent: Friday, December 20, 2024 7:33 AM
> To: Christophe Varoqui <christophe.varoqui@opensvc.com>
> Cc: device-mapper development <dm-devel@lists.linux.dev>; Martin Wilck
> <Martin.Wilck@suse.com>; Muneendra Kumar <muneendra.kumar@broadcom.com>
> Subject: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe
> devices
> 
> When a scsi path device is set to marginal, it updates the rport state.
> Do this for NVMe devices as well.
> 
> Fixes: 1cada778 ("multipathd: Added support to handle FPIN-Li events for
> FC-NVMe")
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/fpin_handlers.c | 74 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 68 insertions(+), 6 deletions(-)
> 
> diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c index
> 6b56f9b7..8b436067 100644
> --- a/multipathd/fpin_handlers.c
> +++ b/multipathd/fpin_handlers.c
> @@ -15,6 +15,7 @@
>  #include "debug.h"
>  #include "util.h"
>  #include "sysfs.h"
> +#include "discovery.h"
> 
>  #include "fpin.h"
>  #include "devmapper.h"
> @@ -253,7 +254,7 @@ static int  extract_nvme_addresses_chk_path_pwwn(const
> char *address,
>   * with the els wwpn ,attached_wwpn and sets the path state to
>   * Marginal
>   */
> -static void fpin_check_set_nvme_path_marginal(uint16_t host_num, struct
> path *pp,
> +static bool fpin_check_set_nvme_path_marginal(uint16_t host_num, struct
> +path *pp,
>  		uint64_t els_wwpn, uint64_t attached_wwpn)  {
>  	struct udev_device *ctl = NULL;
> @@ -263,21 +264,79 @@ static void
> fpin_check_set_nvme_path_marginal(uint16_t host_num, struct path *pp
>  	ctl = udev_device_get_parent_with_subsystem_devtype(pp->udev,
> "nvme", NULL);
>  	if (ctl == NULL) {
>  		condlog(2, "%s: No parent device for ", pp->dev);
> -		return;
> +		return false;
>  	}
>  	address = udev_device_get_sysattr_value(ctl, "address");
>  	if (!address) {
>  		condlog(2, "%s: unable to get the address ", pp->dev);
> -		return;
> +		return false;
>  	}
>  	condlog(4, "\n address %s: dev :%s\n", address, pp->dev);
>  	ret = extract_nvme_addresses_chk_path_pwwn(address, els_wwpn,
> attached_wwpn);
>  	if (ret <= 0)
> -		return;
> +		return false;
>  	ret = fpin_add_marginal_dev_info(host_num, pp->dev);
>  	if (ret < 0)
> -		return;
> +		return false;
>  	fpin_path_setmarginal(pp);
> +	return true;
> +}
> +
> +static void fpin_nvme_set_rport_marginal(uint16_t host_num, uint64_t
> +els_wwpn) {
> +	struct udev_enumerate *udev_enum = NULL;
> +	struct udev_list_entry *entry;
> +
> +	pthread_cleanup_push(cleanup_udev_enumerate_ptr, &udev_enum);
> +	udev_enum = udev_enumerate_new(udev);
> +	if (!udev_enum) {
> +		condlog(0, "fpin: rport udev_enumerate_new() failed: %m");
> +		goto out;
> +	}
> +	if (udev_enumerate_add_match_subsystem(udev_enum,
> "fc_remote_ports") < 0 ||
> +	    udev_enumerate_add_match_is_initialized(udev_enum) < 0 ||
> +	    udev_enumerate_scan_devices(udev_enum) < 0) {
> +		condlog(0, "fpin: error setting up rport enumeration:
> %m");
> +		goto out;
> +	}
> +	udev_list_entry_foreach(entry,
> +				udev_enumerate_get_list_entry(udev_enum))
> {
> +		const char *devpath;
> +		const char *rport_id, *value;
> +		struct udev_device *rport_dev = NULL;
> +		uint16_t rport_hostnum;
> +		uint64_t rport_wwpn;
> +		unsigned int unused;
> +
> +		pthread_cleanup_push(cleanup_udev_device_ptr, &rport_dev);
> +		devpath = udev_list_entry_get_name(entry);
> +		if (!devpath)
> +			goto next;
> +		rport_id = libmp_basename(devpath);
> +		if (sscanf(rport_id, "rport-%hu:%u-%u", &rport_hostnum,
> &unused,
> +			   &unused) != 3 || rport_hostnum != host_num)
> +			goto next;
> +
> +		rport_dev = udev_device_new_from_syspath(udev, devpath);
> +		if (!rport_dev) {
> +			condlog(0, "%s: error getting rport dev: %m",
> rport_id);
> +			goto next;
> +		}
> +		value = udev_device_get_sysattr_value(rport_dev,
> "port_name");
> +		if (!value) {
> +			condlog(0, "%s: error getting port_name: %m",
> rport_id);
> +			goto next;
> +		}
> +
> +		rport_wwpn = strtol(value, NULL, 16);
> +		/* If the rport wwpn matches, set the port state to
> marginal */
> +		if (rport_wwpn == els_wwpn)
> +			fpin_set_rport_marginal(rport_dev);
> +next:
> +		pthread_cleanup_pop(1);
> +	}
> +out:
> +	pthread_cleanup_pop(1);
>  }
> 
>  /*
> @@ -338,6 +397,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  	struct multipath *mpp;
>  	int i, k;
>  	int ret = 0;
> +	bool found_nvme = false;
> 
>  	pthread_cleanup_push(cleanup_lock, &vecs->lock);
>  	lock(&vecs->lock);
> @@ -348,7 +408,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  			continue;
>  		/*checks if the bus type is nvme  and the protocol is
> FC-NVMe*/
>  		if ((pp->bus == SYSFS_BUS_NVME) && (pp->sg_id.proto_id ==
> NVME_PROTOCOL_FC)) {
> -			fpin_check_set_nvme_path_marginal(host_num, pp,
> els_wwpn, attached_wwpn);
> +			found_nvme =
> fpin_check_set_nvme_path_marginal(host_num, pp,
> +els_wwpn, attached_wwpn) || found_nvme;
>  		} else if ((pp->bus == SYSFS_BUS_SCSI) &&
>  			(pp->sg_id.proto_id == SCSI_PROTOCOL_FCP) &&
>  			(host_num ==  pp->sg_id.host_no)) {
> @@ -356,6 +416,8 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  			fpin_check_set_scsi_path_marginal(host_num, pp,
> els_wwpn);
>  		}
>  	}
> +	if (found_nvme)
> +		fpin_nvme_set_rport_marginal(host_num, els_wwpn);
>  	/* walk backwards because reload_and_sync_map() can remove mpp */
>  	vector_foreach_slot_backwards(vecs->mpvec, mpp, i) {
>  		if (mpp->fpin_must_reload) {
> --
> 2.46.2
> 
> -- 
> This electronic communication and the information and any files transmitted 
> with it, or attached to it, are confidential and are intended solely for 
> the use of the individual or entity to whom it is addressed and may contain 
> information that is confidential, legally privileged, protected by privacy 
> laws, or otherwise restricted from disclosure to anyone else. If you are 
> not the intended recipient or the person responsible for delivering the 
> e-mail to the intended recipient, you are hereby notified that any use, 
> copying, distributing, dissemination, forwarding, printing, or copying of 
> this e-mail is strictly prohibited. If you received this e-mail in error, 
> please return the e-mail to the sender, delete it from your computer, and 
> destroy any printed copy of it.

RE: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe devices

[Muneendra Kumar M]

[-- Attachment #1: Type: text/plain, Size: 9762 bytes --]

Hi Benjamin,

>>The request to do this came because the user wanted to see which target
ports were effected, but when they looked, none of them we in the Marginal
state. So there is some value in this for users, even if the systems
behavior doesn't change because of it.

Thanks for the clarification.
The changes looks good.

Regards,
Muneendra.

-----Original Message-----
From: Benjamin Marzinski <bmarzins@redhat.com>
Sent: Monday, January 6, 2025 11:10 PM
To: Muneendra Kumar M <muneendra.kumar@broadcom.com>
Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>; device-mapper
development <dm-devel@lists.linux.dev>; Martin Wilck
<Martin.Wilck@suse.com>
Subject: Re: [PATCH 2/2] multipathd: set rport port_state to marginal for
NVMe devices

On Fri, Dec 20, 2024 at 04:25:22PM +0530, Muneendra Kumar M wrote:
> Hi Benjamin,
> Thanks for the changes.
> But below is the reason why we didn't add support to set the rport
> port_state to marginal for NVMe devices
>
> In the case of SCSI  once rport-state is set to Marginal , If any
> pending I/O's on the marginal path hit's the scsi-timeout (after abort
> success) the scsi-layer checks the rport-state  and If the rport-state
> is set to Marginal it will not do any retries on the Marginal path
> instead the I/O Will be retried on the other Active paths.
>
>
> This particular functionality(checking the rport-state and acting
> accordingly) we didn't add( as far I know) in the case of NVME and we
> need to check how we can handle this in the case of NVME .
> That's the reason we didn't set the port state to Marginal in the case
> of NVMe.
>
>
> In a brief SCSI layer handles the case when the rport-state is set to
> Marginal whereas in NVMe it doesn't(AFIK).
>
> Atleast with the below changes we make sure that once we get the
> FPIN-LI notification we will set the affected path , as well as the
> rport-state to Marginal irrespective of SCSI and NVMe.
>
> I am just thinking that until we have some changes in the NVME driver
> to handle (the rport-state to marginal) this changes doesn't show any
> impact in the case of NVMe  other then keeping it  on par with SCSI
> implementation.
>
> Please let me know your opinion on the same.

The request to do this came because the user wanted to see which target
ports were effected, but when they looked, none of them we in the Marginal
state. So there is some value in this for users, even if the systems
behavior doesn't change because of it.

-Ben

>
>
> Regards,
> Muneendra.
>
>
>
> -----Original Message-----
> From: Benjamin Marzinski <bmarzins@redhat.com>
> Sent: Friday, December 20, 2024 7:33 AM
> To: Christophe Varoqui <christophe.varoqui@opensvc.com>
> Cc: device-mapper development <dm-devel@lists.linux.dev>; Martin Wilck
> <Martin.Wilck@suse.com>; Muneendra Kumar
> <muneendra.kumar@broadcom.com>
> Subject: [PATCH 2/2] multipathd: set rport port_state to marginal for
> NVMe devices
>
> When a scsi path device is set to marginal, it updates the rport state.
> Do this for NVMe devices as well.
>
> Fixes: 1cada778 ("multipathd: Added support to handle FPIN-Li events
> for
> FC-NVMe")
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/fpin_handlers.c | 74
> ++++++++++++++++++++++++++++++++++----
>  1 file changed, 68 insertions(+), 6 deletions(-)
>
> diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c
> index
> 6b56f9b7..8b436067 100644
> --- a/multipathd/fpin_handlers.c
> +++ b/multipathd/fpin_handlers.c
> @@ -15,6 +15,7 @@
>  #include "debug.h"
>  #include "util.h"
>  #include "sysfs.h"
> +#include "discovery.h"
>
>  #include "fpin.h"
>  #include "devmapper.h"
> @@ -253,7 +254,7 @@ static int
> extract_nvme_addresses_chk_path_pwwn(const
> char *address,
>   * with the els wwpn ,attached_wwpn and sets the path state to
>   * Marginal
>   */
> -static void fpin_check_set_nvme_path_marginal(uint16_t host_num,
> struct path *pp,
> +static bool fpin_check_set_nvme_path_marginal(uint16_t host_num,
> +struct path *pp,
>  		uint64_t els_wwpn, uint64_t attached_wwpn)  {
>  	struct udev_device *ctl = NULL;
> @@ -263,21 +264,79 @@ static void
> fpin_check_set_nvme_path_marginal(uint16_t host_num, struct path *pp
>  	ctl = udev_device_get_parent_with_subsystem_devtype(pp->udev,
> "nvme", NULL);
>  	if (ctl == NULL) {
>  		condlog(2, "%s: No parent device for ", pp->dev);
> -		return;
> +		return false;
>  	}
>  	address = udev_device_get_sysattr_value(ctl, "address");
>  	if (!address) {
>  		condlog(2, "%s: unable to get the address ", pp->dev);
> -		return;
> +		return false;
>  	}
>  	condlog(4, "\n address %s: dev :%s\n", address, pp->dev);
>  	ret = extract_nvme_addresses_chk_path_pwwn(address, els_wwpn,
> attached_wwpn);
>  	if (ret <= 0)
> -		return;
> +		return false;
>  	ret = fpin_add_marginal_dev_info(host_num, pp->dev);
>  	if (ret < 0)
> -		return;
> +		return false;
>  	fpin_path_setmarginal(pp);
> +	return true;
> +}
> +
> +static void fpin_nvme_set_rport_marginal(uint16_t host_num, uint64_t
> +els_wwpn) {
> +	struct udev_enumerate *udev_enum = NULL;
> +	struct udev_list_entry *entry;
> +
> +	pthread_cleanup_push(cleanup_udev_enumerate_ptr, &udev_enum);
> +	udev_enum = udev_enumerate_new(udev);
> +	if (!udev_enum) {
> +		condlog(0, "fpin: rport udev_enumerate_new() failed: %m");
> +		goto out;
> +	}
> +	if (udev_enumerate_add_match_subsystem(udev_enum,
> "fc_remote_ports") < 0 ||
> +	    udev_enumerate_add_match_is_initialized(udev_enum) < 0 ||
> +	    udev_enumerate_scan_devices(udev_enum) < 0) {
> +		condlog(0, "fpin: error setting up rport enumeration:
> %m");
> +		goto out;
> +	}
> +	udev_list_entry_foreach(entry,
> +				udev_enumerate_get_list_entry(udev_enum))
> {
> +		const char *devpath;
> +		const char *rport_id, *value;
> +		struct udev_device *rport_dev = NULL;
> +		uint16_t rport_hostnum;
> +		uint64_t rport_wwpn;
> +		unsigned int unused;
> +
> +		pthread_cleanup_push(cleanup_udev_device_ptr, &rport_dev);
> +		devpath = udev_list_entry_get_name(entry);
> +		if (!devpath)
> +			goto next;
> +		rport_id = libmp_basename(devpath);
> +		if (sscanf(rport_id, "rport-%hu:%u-%u", &rport_hostnum,
> &unused,
> +			   &unused) != 3 || rport_hostnum != host_num)
> +			goto next;
> +
> +		rport_dev = udev_device_new_from_syspath(udev, devpath);
> +		if (!rport_dev) {
> +			condlog(0, "%s: error getting rport dev: %m",
> rport_id);
> +			goto next;
> +		}
> +		value = udev_device_get_sysattr_value(rport_dev,
> "port_name");
> +		if (!value) {
> +			condlog(0, "%s: error getting port_name: %m",
> rport_id);
> +			goto next;
> +		}
> +
> +		rport_wwpn = strtol(value, NULL, 16);
> +		/* If the rport wwpn matches, set the port state to
> marginal */
> +		if (rport_wwpn == els_wwpn)
> +			fpin_set_rport_marginal(rport_dev);
> +next:
> +		pthread_cleanup_pop(1);
> +	}
> +out:
> +	pthread_cleanup_pop(1);
>  }
>
>  /*
> @@ -338,6 +397,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  	struct multipath *mpp;
>  	int i, k;
>  	int ret = 0;
> +	bool found_nvme = false;
>
>  	pthread_cleanup_push(cleanup_lock, &vecs->lock);
>  	lock(&vecs->lock);
> @@ -348,7 +408,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  			continue;
>  		/*checks if the bus type is nvme  and the protocol is
FC-NVMe*/
>  		if ((pp->bus == SYSFS_BUS_NVME) && (pp->sg_id.proto_id ==
> NVME_PROTOCOL_FC)) {
> -			fpin_check_set_nvme_path_marginal(host_num, pp,
> els_wwpn, attached_wwpn);
> +			found_nvme =
> fpin_check_set_nvme_path_marginal(host_num, pp,
> +els_wwpn, attached_wwpn) || found_nvme;
>  		} else if ((pp->bus == SYSFS_BUS_SCSI) &&
>  			(pp->sg_id.proto_id == SCSI_PROTOCOL_FCP) &&
>  			(host_num ==  pp->sg_id.host_no)) { @@ -356,6
+416,8 @@ static int
> fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  			fpin_check_set_scsi_path_marginal(host_num, pp,
els_wwpn);
>  		}
>  	}
> +	if (found_nvme)
> +		fpin_nvme_set_rport_marginal(host_num, els_wwpn);
>  	/* walk backwards because reload_and_sync_map() can remove mpp */
>  	vector_foreach_slot_backwards(vecs->mpvec, mpp, i) {
>  		if (mpp->fpin_must_reload) {
> --
> 2.46.2
>
> --
> This electronic communication and the information and any files
> transmitted with it, or attached to it, are confidential and are
> intended solely for the use of the individual or entity to whom it is
> addressed and may contain information that is confidential, legally
> privileged, protected by privacy laws, or otherwise restricted from
> disclosure to anyone else. If you are not the intended recipient or
> the person responsible for delivering the e-mail to the intended
> recipient, you are hereby notified that any use, copying,
> distributing, dissemination, forwarding, printing, or copying of this
> e-mail is strictly prohibited. If you received this e-mail in error,
> please return the e-mail to the sender, delete it from your computer,
and destroy any printed copy of it.

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4220 bytes --]

RE: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe devices

[Muneendra Kumar M]

[-- Attachment #1: Type: text/plain, Size: 10235 bytes --]

Reviewed-by: Muneendra Kumar <muneendra.kumar@broadcom.com>

-----Original Message-----
From: Muneendra Kumar M <muneendra.kumar@broadcom.com>
Sent: Thursday, January 9, 2025 11:34 AM
To: 'Benjamin Marzinski' <bmarzins@redhat.com>
Cc: 'Christophe Varoqui' <christophe.varoqui@opensvc.com>; 'device-mapper
development' <dm-devel@lists.linux.dev>; 'Martin Wilck'
<Martin.Wilck@suse.com>
Subject: RE: [PATCH 2/2] multipathd: set rport port_state to marginal for
NVMe devices

Hi Benjamin,

>>The request to do this came because the user wanted to see which target
ports were effected, but when they looked, none of them we in the Marginal
state. So there is some value in this for users, even if the systems
behavior doesn't change because of it.

Thanks for the clarification.
The changes looks good.

Regards,
Muneendra.

-----Original Message-----
From: Benjamin Marzinski <bmarzins@redhat.com>
Sent: Monday, January 6, 2025 11:10 PM
To: Muneendra Kumar M <muneendra.kumar@broadcom.com>
Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>; device-mapper
development <dm-devel@lists.linux.dev>; Martin Wilck
<Martin.Wilck@suse.com>
Subject: Re: [PATCH 2/2] multipathd: set rport port_state to marginal for
NVMe devices

On Fri, Dec 20, 2024 at 04:25:22PM +0530, Muneendra Kumar M wrote:
> Hi Benjamin,
> Thanks for the changes.
> But below is the reason why we didn't add support to set the rport
> port_state to marginal for NVMe devices
>
> In the case of SCSI  once rport-state is set to Marginal , If any
> pending I/O's on the marginal path hit's the scsi-timeout (after abort
> success) the scsi-layer checks the rport-state  and If the rport-state
> is set to Marginal it will not do any retries on the Marginal path
> instead the I/O Will be retried on the other Active paths.
>
>
> This particular functionality(checking the rport-state and acting
> accordingly) we didn't add( as far I know) in the case of NVME and we
> need to check how we can handle this in the case of NVME .
> That's the reason we didn't set the port state to Marginal in the case
> of NVMe.
>
>
> In a brief SCSI layer handles the case when the rport-state is set to
> Marginal whereas in NVMe it doesn't(AFIK).
>
> Atleast with the below changes we make sure that once we get the
> FPIN-LI notification we will set the affected path , as well as the
> rport-state to Marginal irrespective of SCSI and NVMe.
>
> I am just thinking that until we have some changes in the NVME driver
> to handle (the rport-state to marginal) this changes doesn't show any
> impact in the case of NVMe  other then keeping it  on par with SCSI
> implementation.
>
> Please let me know your opinion on the same.

The request to do this came because the user wanted to see which target
ports were effected, but when they looked, none of them we in the Marginal
state. So there is some value in this for users, even if the systems
behavior doesn't change because of it.

-Ben

>
>
> Regards,
> Muneendra.
>
>
>
> -----Original Message-----
> From: Benjamin Marzinski <bmarzins@redhat.com>
> Sent: Friday, December 20, 2024 7:33 AM
> To: Christophe Varoqui <christophe.varoqui@opensvc.com>
> Cc: device-mapper development <dm-devel@lists.linux.dev>; Martin Wilck
> <Martin.Wilck@suse.com>; Muneendra Kumar
> <muneendra.kumar@broadcom.com>
> Subject: [PATCH 2/2] multipathd: set rport port_state to marginal for
> NVMe devices
>
> When a scsi path device is set to marginal, it updates the rport state.
> Do this for NVMe devices as well.
>
> Fixes: 1cada778 ("multipathd: Added support to handle FPIN-Li events
> for
> FC-NVMe")
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/fpin_handlers.c | 74
> ++++++++++++++++++++++++++++++++++----
>  1 file changed, 68 insertions(+), 6 deletions(-)
>
> diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c
> index
> 6b56f9b7..8b436067 100644
> --- a/multipathd/fpin_handlers.c
> +++ b/multipathd/fpin_handlers.c
> @@ -15,6 +15,7 @@
>  #include "debug.h"
>  #include "util.h"
>  #include "sysfs.h"
> +#include "discovery.h"
>
>  #include "fpin.h"
>  #include "devmapper.h"
> @@ -253,7 +254,7 @@ static int
> extract_nvme_addresses_chk_path_pwwn(const
> char *address,
>   * with the els wwpn ,attached_wwpn and sets the path state to
>   * Marginal
>   */
> -static void fpin_check_set_nvme_path_marginal(uint16_t host_num,
> struct path *pp,
> +static bool fpin_check_set_nvme_path_marginal(uint16_t host_num,
> +struct path *pp,
>  		uint64_t els_wwpn, uint64_t attached_wwpn)  {
>  	struct udev_device *ctl = NULL;
> @@ -263,21 +264,79 @@ static void
> fpin_check_set_nvme_path_marginal(uint16_t host_num, struct path *pp
>  	ctl = udev_device_get_parent_with_subsystem_devtype(pp->udev,
> "nvme", NULL);
>  	if (ctl == NULL) {
>  		condlog(2, "%s: No parent device for ", pp->dev);
> -		return;
> +		return false;
>  	}
>  	address = udev_device_get_sysattr_value(ctl, "address");
>  	if (!address) {
>  		condlog(2, "%s: unable to get the address ", pp->dev);
> -		return;
> +		return false;
>  	}
>  	condlog(4, "\n address %s: dev :%s\n", address, pp->dev);
>  	ret = extract_nvme_addresses_chk_path_pwwn(address, els_wwpn,
> attached_wwpn);
>  	if (ret <= 0)
> -		return;
> +		return false;
>  	ret = fpin_add_marginal_dev_info(host_num, pp->dev);
>  	if (ret < 0)
> -		return;
> +		return false;
>  	fpin_path_setmarginal(pp);
> +	return true;
> +}
> +
> +static void fpin_nvme_set_rport_marginal(uint16_t host_num, uint64_t
> +els_wwpn) {
> +	struct udev_enumerate *udev_enum = NULL;
> +	struct udev_list_entry *entry;
> +
> +	pthread_cleanup_push(cleanup_udev_enumerate_ptr, &udev_enum);
> +	udev_enum = udev_enumerate_new(udev);
> +	if (!udev_enum) {
> +		condlog(0, "fpin: rport udev_enumerate_new() failed: %m");
> +		goto out;
> +	}
> +	if (udev_enumerate_add_match_subsystem(udev_enum,
> "fc_remote_ports") < 0 ||
> +	    udev_enumerate_add_match_is_initialized(udev_enum) < 0 ||
> +	    udev_enumerate_scan_devices(udev_enum) < 0) {
> +		condlog(0, "fpin: error setting up rport enumeration:
> %m");
> +		goto out;
> +	}
> +	udev_list_entry_foreach(entry,
> +				udev_enumerate_get_list_entry(udev_enum))
> {
> +		const char *devpath;
> +		const char *rport_id, *value;
> +		struct udev_device *rport_dev = NULL;
> +		uint16_t rport_hostnum;
> +		uint64_t rport_wwpn;
> +		unsigned int unused;
> +
> +		pthread_cleanup_push(cleanup_udev_device_ptr, &rport_dev);
> +		devpath = udev_list_entry_get_name(entry);
> +		if (!devpath)
> +			goto next;
> +		rport_id = libmp_basename(devpath);
> +		if (sscanf(rport_id, "rport-%hu:%u-%u", &rport_hostnum,
> &unused,
> +			   &unused) != 3 || rport_hostnum != host_num)
> +			goto next;
> +
> +		rport_dev = udev_device_new_from_syspath(udev, devpath);
> +		if (!rport_dev) {
> +			condlog(0, "%s: error getting rport dev: %m",
> rport_id);
> +			goto next;
> +		}
> +		value = udev_device_get_sysattr_value(rport_dev,
> "port_name");
> +		if (!value) {
> +			condlog(0, "%s: error getting port_name: %m",
> rport_id);
> +			goto next;
> +		}
> +
> +		rport_wwpn = strtol(value, NULL, 16);
> +		/* If the rport wwpn matches, set the port state to
> marginal */
> +		if (rport_wwpn == els_wwpn)
> +			fpin_set_rport_marginal(rport_dev);
> +next:
> +		pthread_cleanup_pop(1);
> +	}
> +out:
> +	pthread_cleanup_pop(1);
>  }
>
>  /*
> @@ -338,6 +397,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  	struct multipath *mpp;
>  	int i, k;
>  	int ret = 0;
> +	bool found_nvme = false;
>
>  	pthread_cleanup_push(cleanup_lock, &vecs->lock);
>  	lock(&vecs->lock);
> @@ -348,7 +408,7 @@ static int  fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  			continue;
>  		/*checks if the bus type is nvme  and the protocol is
FC-NVMe*/
>  		if ((pp->bus == SYSFS_BUS_NVME) && (pp->sg_id.proto_id ==
> NVME_PROTOCOL_FC)) {
> -			fpin_check_set_nvme_path_marginal(host_num, pp,
> els_wwpn, attached_wwpn);
> +			found_nvme =
> fpin_check_set_nvme_path_marginal(host_num, pp,
> +els_wwpn, attached_wwpn) || found_nvme;
>  		} else if ((pp->bus == SYSFS_BUS_SCSI) &&
>  			(pp->sg_id.proto_id == SCSI_PROTOCOL_FCP) &&
>  			(host_num ==  pp->sg_id.host_no)) { @@ -356,6
+416,8 @@ static int
> fpin_chk_wwn_setpath_marginal(uint16_t
> host_num,  struct vectors *ve
>  			fpin_check_set_scsi_path_marginal(host_num, pp,
els_wwpn);
>  		}
>  	}
> +	if (found_nvme)
> +		fpin_nvme_set_rport_marginal(host_num, els_wwpn);
>  	/* walk backwards because reload_and_sync_map() can remove mpp */
>  	vector_foreach_slot_backwards(vecs->mpvec, mpp, i) {
>  		if (mpp->fpin_must_reload) {
> --
> 2.46.2
>
> --
> This electronic communication and the information and any files
> transmitted with it, or attached to it, are confidential and are
> intended solely for the use of the individual or entity to whom it is
> addressed and may contain information that is confidential, legally
> privileged, protected by privacy laws, or otherwise restricted from
> disclosure to anyone else. If you are not the intended recipient or
> the person responsible for delivering the e-mail to the intended
> recipient, you are hereby notified that any use, copying,
> distributing, dissemination, forwarding, printing, or copying of this
> e-mail is strictly prohibited. If you received this e-mail in error,
> please return the e-mail to the sender, delete it from your computer,
and destroy any printed copy of it.

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4220 bytes --]

Re: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe devices

[Martin Wilck]

On Mon, 2025-01-06 at 12:39 -0500, Benjamin Marzinski wrote:
> On Fri, Dec 20, 2024 at 04:25:22PM +0530, Muneendra Kumar M wrote:
> > Hi Benjamin,
> > Thanks for the changes.
> > But below is the reason why we didn't add support to set the rport
> > port_state to marginal for NVMe devices
> > 
> > In the case of SCSI  once rport-state is set to Marginal ,
> > If any pending I/O's on the marginal path hit's the scsi-timeout
> > (after
> > abort success) the scsi-layer checks the rport-state  and If the
> > rport-state is set to Marginal it will not do any retries on the
> > Marginal
> > path instead the I/O
> > Will be retried on the other Active paths.
> > 
> > 
> > This particular functionality(checking the rport-state and acting
> > accordingly) we didn't add( as far I know) in the case of NVME and
> > we need
> > to check how we can handle this in the case of NVME .
> > That's the reason we didn't set the port state to Marginal in the
> > case of
> > NVMe.
> > 
> > 
> > In a brief SCSI layer handles the case when the rport-state is set
> > to
> > Marginal whereas in NVMe it doesn't(AFIK).
> > 
> > Atleast with the below changes we make sure that once we get the
> > FPIN-LI
> > notification we will set the affected path , as well as the rport-
> > state to
> > Marginal irrespective of SCSI and NVMe.
> > 
> > I am just thinking that until we have some changes in the NVME
> > driver to
> > handle (the rport-state to marginal) this changes doesn't show any
> > impact
> > in the case of NVMe
> >  other then keeping it  on par with SCSI implementation.
> > 
> > Please let me know your opinion on the same.
> 
> The request to do this came because the user wanted to see which
> target
> ports were effected, but when they looked, none of them we in the
> Marginal state. So there is some value in this for users, even if the
> systems behavior doesn't change because of it.

Please add a note to the commit message explaining this.

I'm actually a bit concerned about it ... won't other users be even
more confused when they see the path devices as marginal but don't
observe any change in the system's usage of these paths?

Regards,
Martin

Re: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe devices

[Benjamin Marzinski]

On Tue, Jan 14, 2025 at 11:31:00PM +0100, Martin Wilck wrote:
> On Mon, 2025-01-06 at 12:39 -0500, Benjamin Marzinski wrote:
> > On Fri, Dec 20, 2024 at 04:25:22PM +0530, Muneendra Kumar M wrote:
> > > Hi Benjamin,
> > > Thanks for the changes.
> > > But below is the reason why we didn't add support to set the rport
> > > port_state to marginal for NVMe devices
> > > 
> > > In the case of SCSI  once rport-state is set to Marginal ,
> > > If any pending I/O's on the marginal path hit's the scsi-timeout
> > > (after
> > > abort success) the scsi-layer checks the rport-state  and If the
> > > rport-state is set to Marginal it will not do any retries on the
> > > Marginal
> > > path instead the I/O
> > > Will be retried on the other Active paths.
> > > 
> > > 
> > > This particular functionality(checking the rport-state and acting
> > > accordingly) we didn't add( as far I know) in the case of NVME and
> > > we need
> > > to check how we can handle this in the case of NVME .
> > > That's the reason we didn't set the port state to Marginal in the
> > > case of
> > > NVMe.
> > > 
> > > 
> > > In a brief SCSI layer handles the case when the rport-state is set
> > > to
> > > Marginal whereas in NVMe it doesn't(AFIK).
> > > 
> > > Atleast with the below changes we make sure that once we get the
> > > FPIN-LI
> > > notification we will set the affected path , as well as the rport-
> > > state to
> > > Marginal irrespective of SCSI and NVMe.
> > > 
> > > I am just thinking that until we have some changes in the NVME
> > > driver to
> > > handle (the rport-state to marginal) this changes doesn't show any
> > > impact
> > > in the case of NVMe
> > >  other then keeping it  on par with SCSI implementation.
> > > 
> > > Please let me know your opinion on the same.
> > 
> > The request to do this came because the user wanted to see which
> > target
> > ports were effected, but when they looked, none of them we in the
> > Marginal state. So there is some value in this for users, even if the
> > systems behavior doesn't change because of it.
> 
> Please add a note to the commit message explaining this.
> 
> I'm actually a bit concerned about it ... won't other users be even
> more confused when they see the path devices as marginal but don't
> observe any change in the system's usage of these paths?

Sure, I can add a commit message. But while this isn't implemented in
the NVMe layer, multipath will still route future IO differently. So,
any IO that multipath has already dispatched to the now marginal path
won't get treated differently, users will notice a change in IO patterns
going forward. Right?

-Ben

> 
> Regards,
> Martin

Re: [PATCH 2/2] multipathd: set rport port_state to marginal for NVMe devices

[Martin Wilck]

On Wed, 2025-01-15 at 13:59 -0500, Benjamin Marzinski wrote:
> On Tue, Jan 14, 2025 at 11:31:00PM +0100, Martin Wilck wrote:
> > On Mon, 2025-01-06 at 12:39 -0500, Benjamin Marzinski wrote:
> > > On Fri, Dec 20, 2024 at 04:25:22PM +0530, Muneendra Kumar M
> > > wrote:
> > > > Hi Benjamin,
> > > > Thanks for the changes.
> > > > But below is the reason why we didn't add support to set the
> > > > rport
> > > > port_state to marginal for NVMe devices
> > > > 
> > > > In the case of SCSI  once rport-state is set to Marginal ,
> > > > If any pending I/O's on the marginal path hit's the scsi-
> > > > timeout
> > > > (after
> > > > abort success) the scsi-layer checks the rport-state  and If
> > > > the
> > > > rport-state is set to Marginal it will not do any retries on
> > > > the
> > > > Marginal
> > > > path instead the I/O
> > > > Will be retried on the other Active paths.
> > > > 
> > > > 
> > > > This particular functionality(checking the rport-state and
> > > > acting
> > > > accordingly) we didn't add( as far I know) in the case of NVME
> > > > and
> > > > we need
> > > > to check how we can handle this in the case of NVME .
> > > > That's the reason we didn't set the port state to Marginal in
> > > > the
> > > > case of
> > > > NVMe.
> > > > 
> > > > 
> > > > In a brief SCSI layer handles the case when the rport-state is
> > > > set
> > > > to
> > > > Marginal whereas in NVMe it doesn't(AFIK).
> > > > 
> > > > Atleast with the below changes we make sure that once we get
> > > > the
> > > > FPIN-LI
> > > > notification we will set the affected path , as well as the
> > > > rport-
> > > > state to
> > > > Marginal irrespective of SCSI and NVMe.
> > > > 
> > > > I am just thinking that until we have some changes in the NVME
> > > > driver to
> > > > handle (the rport-state to marginal) this changes doesn't show
> > > > any
> > > > impact
> > > > in the case of NVMe
> > > >  other then keeping it  on par with SCSI implementation.
> > > > 
> > > > Please let me know your opinion on the same.
> > > 
> > > The request to do this came because the user wanted to see which
> > > target
> > > ports were effected, but when they looked, none of them we in the
> > > Marginal state. So there is some value in this for users, even if
> > > the
> > > systems behavior doesn't change because of it.
> > 
> > Please add a note to the commit message explaining this.
> > 
> > I'm actually a bit concerned about it ... won't other users be even
> > more confused when they see the path devices as marginal but don't
> > observe any change in the system's usage of these paths?
> 
> Sure, I can add a commit message. But while this isn't implemented in
> the NVMe layer, multipath will still route future IO differently. So,
> any IO that multipath has already dispatched to the now marginal path
> won't get treated differently, users will notice a change in IO
> patterns
> going forward. Right?

Ok. I may have misunderstood Muneendra's reply. I admit I didn't double
check in depth. I may also have mixed up native and dm multipath
behavior.

Regards
Martin

Re: [PATCH 0/2] multipath: set rport port_state on NVMe FPIN events

[Martin Wilck]

On Thu, 2024-12-19 at 21:02 -0500, Benjamin Marzinski wrote:
> When multipathd handles link integrity FPIN events, it sets the rport
> associated with the effected SCSI paths to Marginal. These patches
> make
> it do the same thing for NVMe paths.
> 
> I actually have a stupid Fibre Channel question. Does the host number
> and the target wwpn uniquely identfy an rport? I never see the
> channel
> set to anything other than 0, but I don't think that it MUST be 0.

IMO, yes. As the WWPN is unique, it's by itself sufficient to identify
the physical remote port. By adding the host number,
which corresponds to a unique WWPN on the host, you uniquely identify
the Linux fc_remote_port.  

This also fits the fc symlinks that udev creates under by-path.

> But
> if we knew that there was only one rport for a given host number and
> target WWPN, we could stop checking as soon as we found one. These
> patches don't do that. They check all the rports.

I think we could stop if the WWPN is found.

Martin

> 
> Benjamin Marzinski (2):
>   libmultipath: export udev pthread cleanup functions
>   multipathd: set rport port_state to marginal for NVMe devices
> 
>  libmultipath/discovery.c          |  4 +-
>  libmultipath/discovery.h          |  2 +
>  libmultipath/libmultipath.version |  6 +++
>  multipathd/fpin_handlers.c        | 74 ++++++++++++++++++++++++++++-
> --
>  4 files changed, 78 insertions(+), 8 deletions(-)
>