diff mbox series

multipathd: Added support to handle FPIN-Li events for FC-NVMe

Message ID 20230914215546.2875790-1-muneendra.kumar@broadcom.com (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show
Series multipathd: Added support to handle FPIN-Li events for FC-NVMe | expand

Commit Message

Muneendra Kumar M Sept. 14, 2023, 9:55 p.m. UTC
From: Muneendra <muneendra.kumar@broadcom.com>

This patch adds the support to handle FPIN-Li for FC-NVMe.
On receiving the FPIN-Li events this patch moves the devices paths
which are affected due to link integrity to marginal path groups.
The paths which are set to marginal path group will be unset
on receiving the RSCN events

Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>
---
 multipathd/fpin_handlers.c | 177 ++++++++++++++++++++++++++++---------
 1 file changed, 136 insertions(+), 41 deletions(-)

Comments

Martin Wilck Sept. 15, 2023, 11:43 a.m. UTC | #1
Hello Muneendra,

On Thu, 2023-09-14 at 14:55 -0700, Muneendra Kumar wrote:
> From: Muneendra <muneendra.kumar@broadcom.com>
> 
> This patch adds the support to handle FPIN-Li for FC-NVMe.
> On receiving the FPIN-Li events this patch moves the devices paths
> which are affected due to link integrity to marginal path groups.
> The paths which are set to marginal path group will be unset
> on receiving the RSCN events
> 
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

Thanks! This looks mostly good to me, some comments below.


> ---
>  multipathd/fpin_handlers.c | 177 ++++++++++++++++++++++++++++-------
> --
>  1 file changed, 136 insertions(+), 41 deletions(-)
> 
> diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c
> index aa0f63c9..94f390ec 100644
> --- a/multipathd/fpin_handlers.c
> +++ b/multipathd/fpin_handlers.c
> @@ -203,61 +203,153 @@ fpin_add_marginal_dev_info(uint32_t host_num,
> char *devname)
>  }
>  
>  /*
> - * This function goes through the vecs->pathvec, and for
> - * each path, check that the host  number,
> - * the target WWPN associated with the path matches
> - * with the els wwpn and sets the path and port state to
> + * This function compares Transport Address Controller Port pn,
> + * Host Transport Address Controller Port pn with the els wwpn
> ,attached_wwpn
> + * and return 0 on success
> + */
> +static int  extract_nvme_addresses_chk_path_pwwn(const char
> *address,
> +               uint64_t els_wwpn, uint64_t els_attached_wwpn)
> +
> +{
> +       uint64_t traddr;
> +       uint64_t host_traddr;
> +
> +       /*
> +        *  Find the position of "traddr=" and "host_traddr="
> +        *  and the address will be in the below format
> +        *  "traddr=nn-0x200400110dff9400:pn-0x200400110dff9400,
> +        *  host_traddr=nn-0x200400110dff9400:pn-0x200400110dff9400"
> +        */
> +       const char *traddr_start = strstr(address, "traddr=");
> +       const char *host_traddr_start = strstr(address,
> "host_traddr=");
> +
> +       if (!traddr_start || !host_traddr_start)
> +               return -EINVAL;
> +
> +       /* Extract traddr pn */
> +       if (sscanf(traddr_start, "traddr=nn-%*[^:]:pn-%" SCNx64,
> &traddr) != 1)
> +               return -EINVAL;
> +
> +       /* Extract host_traddr pn*/
> +       if (sscanf(host_traddr_start, "host_traddr=nn-%*[^:]:pn-%"
> SCNx64,
> +                               &host_traddr) != 1)
> +               return -EINVAL;
> +       condlog(4, "traddr 0x%lx hosttraddr 0x%lx els_wwpn 0x%lx
> els_host_traddr 0x%lx",
> +                       traddr, host_traddr,
> +                       els_wwpn, els_attached_wwpn);
> +       if ((host_traddr == els_attached_wwpn) && (traddr ==
> els_wwpn))
> +               return 0;
> +       return -EINVAL;

Please don't return -EINVAL to indicate that the addresses don't match.
The function should return 1 (match) or 0 (no match) or a negative
error code if something is actually wrong.


> +}
> +
> +/*
> + * This function check that the Transport Address Controller Port
> pn,
> + * Host Transport Address Controller Port pn associated with the
> path matches
> + * with the els wwpn ,attached_wwpn and sets the path state to
>   * Marginal
>   */
> -static int  fpin_chk_wwn_setpath_marginal(uint16_t host_num,  struct
> vectors *vecs,
> +static void 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;
> +       const char *address = NULL;
> +       int ret = 0;
> +
> +       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;
> +       }
> +       address = udev_device_get_sysattr_value(ctl, "address");
> +       if (!address) {
> +               condlog(2, "%s: unable to get the address ", pp-
> >dev);
> +               return;
> +       }
> +       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;
> +       ret = fpin_path_setmarginal(pp);
> +       if (ret < 0)
> +               return;
> +       fpin_add_marginal_dev_info(host_num, pp->dev);

I think that you should call fpin_add_marginal_dev_info() first, and
only set the path to marginal state if it has succeeded.
I realize the same applies to the SCSI code flow.

Regards,
Martin

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Muneendra Kumar M Sept. 20, 2023, 5:07 a.m. UTC | #2
Hi Martin,
Thanks for the review.
I will incorporate your review comments and will post the v2 soon.

Regards,
Muneendra.

-----Original Message-----
From: Martin Wilck <mwilck@suse.com>
Sent: Friday, September 15, 2023 5:14 PM
To: Muneendra Kumar <muneendra.kumar@broadcom.com>; dm-devel@redhat.com;
bmarzins@redhat.com
Cc: mkumar@redhat.com
Subject: Re: [PATCH] multipathd: Added support to handle FPIN-Li events
for FC-NVMe

Hello Muneendra,

On Thu, 2023-09-14 at 14:55 -0700, Muneendra Kumar wrote:
> From: Muneendra <muneendra.kumar@broadcom.com>
>
> This patch adds the support to handle FPIN-Li for FC-NVMe.
> On receiving the FPIN-Li events this patch moves the devices paths
> which are affected due to link integrity to marginal path groups.
> The paths which are set to marginal path group will be unset on
> receiving the RSCN events
>
> Signed-off-by: Muneendra <muneendra.kumar@broadcom.com>

Thanks! This looks mostly good to me, some comments below.


> ---
>  multipathd/fpin_handlers.c | 177 ++++++++++++++++++++++++++++-------
> --
>  1 file changed, 136 insertions(+), 41 deletions(-)
>
> diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c
> index aa0f63c9..94f390ec 100644
> --- a/multipathd/fpin_handlers.c
> +++ b/multipathd/fpin_handlers.c
> @@ -203,61 +203,153 @@ fpin_add_marginal_dev_info(uint32_t host_num,
> char *devname)
>  }
>
>  /*
> - * This function goes through the vecs->pathvec, and for
> - * each path, check that the host  number,
> - * the target WWPN associated with the path matches
> - * with the els wwpn and sets the path and port state to
> + * This function compares Transport Address Controller Port pn,
> + * Host Transport Address Controller Port pn with the els wwpn
> ,attached_wwpn
> + * and return 0 on success
> + */
> +static int  extract_nvme_addresses_chk_path_pwwn(const char
> *address,
> +               uint64_t els_wwpn, uint64_t els_attached_wwpn)
> +
> +{
> +       uint64_t traddr;
> +       uint64_t host_traddr;
> +
> +       /*
> +        *  Find the position of "traddr=" and "host_traddr="
> +        *  and the address will be in the below format
> +        *  "traddr=nn-0x200400110dff9400:pn-0x200400110dff9400,
> +        *  host_traddr=nn-0x200400110dff9400:pn-0x200400110dff9400"
> +        */
> +       const char *traddr_start = strstr(address, "traddr=");
> +       const char *host_traddr_start = strstr(address,
> "host_traddr=");
> +
> +       if (!traddr_start || !host_traddr_start)
> +               return -EINVAL;
> +
> +       /* Extract traddr pn */
> +       if (sscanf(traddr_start, "traddr=nn-%*[^:]:pn-%" SCNx64,
> &traddr) != 1)
> +               return -EINVAL;
> +
> +       /* Extract host_traddr pn*/
> +       if (sscanf(host_traddr_start, "host_traddr=nn-%*[^:]:pn-%"
> SCNx64,
> +                               &host_traddr) != 1)
> +               return -EINVAL;
> +       condlog(4, "traddr 0x%lx hosttraddr 0x%lx els_wwpn 0x%lx
> els_host_traddr 0x%lx",
> +                       traddr, host_traddr,
> +                       els_wwpn, els_attached_wwpn);
> +       if ((host_traddr == els_attached_wwpn) && (traddr ==
> els_wwpn))
> +               return 0;
> +       return -EINVAL;

Please don't return -EINVAL to indicate that the addresses don't match.
The function should return 1 (match) or 0 (no match) or a negative error
code if something is actually wrong.


> +}
> +
> +/*
> + * This function check that the Transport Address Controller Port
> pn,
> + * Host Transport Address Controller Port pn associated with the
> path matches
> + * with the els wwpn ,attached_wwpn and sets the path state to
>   * Marginal
>   */
> -static int  fpin_chk_wwn_setpath_marginal(uint16_t host_num,  struct
> vectors *vecs,
> +static void 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;
> +       const char *address = NULL;
> +       int ret = 0;
> +
> +       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;
> +       }
> +       address = udev_device_get_sysattr_value(ctl, "address");
> +       if (!address) {
> +               condlog(2, "%s: unable to get the address ", pp-
> >dev);
> +               return;
> +       }
> +       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;
> +       ret = fpin_path_setmarginal(pp);
> +       if (ret < 0)
> +               return;
> +       fpin_add_marginal_dev_info(host_num, pp->dev);

I think that you should call fpin_add_marginal_dev_info() first, and only
set the path to marginal state if it has succeeded.
I realize the same applies to the SCSI code flow.

Regards,
Martin
diff mbox series

Patch

diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c
index aa0f63c9..94f390ec 100644
--- a/multipathd/fpin_handlers.c
+++ b/multipathd/fpin_handlers.c
@@ -203,61 +203,153 @@  fpin_add_marginal_dev_info(uint32_t host_num, char *devname)
 }
 
 /*
- * This function goes through the vecs->pathvec, and for
- * each path, check that the host  number,
- * the target WWPN associated with the path matches
- * with the els wwpn and sets the path and port state to
+ * This function compares Transport Address Controller Port pn,
+ * Host Transport Address Controller Port pn with the els wwpn ,attached_wwpn
+ * and return 0 on success
+ */
+static int  extract_nvme_addresses_chk_path_pwwn(const char *address,
+		uint64_t els_wwpn, uint64_t els_attached_wwpn)
+
+{
+	uint64_t traddr;
+	uint64_t host_traddr;
+
+	/*
+	 *  Find the position of "traddr=" and "host_traddr="
+	 *  and the address will be in the below format
+	 *  "traddr=nn-0x200400110dff9400:pn-0x200400110dff9400,
+	 *  host_traddr=nn-0x200400110dff9400:pn-0x200400110dff9400"
+	 */
+	const char *traddr_start = strstr(address, "traddr=");
+	const char *host_traddr_start = strstr(address, "host_traddr=");
+
+	if (!traddr_start || !host_traddr_start)
+		return -EINVAL;
+
+	/* Extract traddr pn */
+	if (sscanf(traddr_start, "traddr=nn-%*[^:]:pn-%" SCNx64, &traddr) != 1)
+		return -EINVAL;
+
+	/* Extract host_traddr pn*/
+	if (sscanf(host_traddr_start, "host_traddr=nn-%*[^:]:pn-%" SCNx64,
+				&host_traddr) != 1)
+		return -EINVAL;
+	condlog(4, "traddr 0x%lx hosttraddr 0x%lx els_wwpn 0x%lx els_host_traddr 0x%lx",
+			traddr, host_traddr,
+			els_wwpn, els_attached_wwpn);
+	if ((host_traddr == els_attached_wwpn) && (traddr == els_wwpn))
+		return 0;
+	return -EINVAL;
+}
+
+/*
+ * This function check that the Transport Address Controller Port pn,
+ * Host Transport Address Controller Port pn associated with the path matches
+ * with the els wwpn ,attached_wwpn and sets the path state to
  * Marginal
  */
-static int  fpin_chk_wwn_setpath_marginal(uint16_t host_num,  struct vectors *vecs,
+static void 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;
+	const char *address = NULL;
+	int ret = 0;
+
+	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;
+	}
+	address = udev_device_get_sysattr_value(ctl, "address");
+	if (!address) {
+		condlog(2, "%s: unable to get the address ", pp->dev);
+		return;
+	}
+	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;
+	ret = fpin_path_setmarginal(pp);
+	if (ret < 0)
+		return;
+	fpin_add_marginal_dev_info(host_num, pp->dev);
+	return;
+
+}
+
+/*
+ * This function check the host  number, the target WWPN
+ * associated with the path matches with the els wwpn and
+ * sets the path and port state to Marginal
+ */
+static void fpin_check_set_scsi_path_marginal(uint16_t host_num, struct path *pp,
 		uint64_t els_wwpn)
 {
-	struct path *pp;
-	struct multipath *mpp;
-	int i, k;
 	char rport_id[42];
 	const char *value = NULL;
 	struct udev_device *rport_dev = NULL;
 	uint64_t wwpn;
 	int ret = 0;
+	sprintf(rport_id, "rport-%d:%d-%d",
+			pp->sg_id.host_no, pp->sg_id.channel, pp->sg_id.transport_id);
+	rport_dev = udev_device_new_from_subsystem_sysname(udev,
+			"fc_remote_ports", rport_id);
+	if (!rport_dev) {
+		condlog(2, "%s: No fc_remote_port device for '%s'", pp->dev,
+				rport_id);
+		return;
+	}
+	pthread_cleanup_push(_udev_device_unref, rport_dev);
+	value = udev_device_get_sysattr_value(rport_dev, "port_name");
+	if (!value)
+		goto unref;
+
+	wwpn =  strtol(value, NULL, 16);
+	/*
+	 * If the port wwpn matches sets the path and port state
+	 * to marginal
+	 */
+	if (wwpn == els_wwpn) {
+		ret = fpin_path_setmarginal(pp);
+		if (ret < 0)
+			goto unref;
+		fpin_set_rport_marginal(rport_dev);
+		fpin_add_marginal_dev_info(host_num, pp->dev);
+	}
+unref:
+	pthread_cleanup_pop(1);
+	return;
+
+}
+
+/*
+ * This function goes through the vecs->pathvec, and for
+ * each path, it checks and sets the path state to marginal
+ * if the path's associated port wwpn ,hostnum  matches with
+ * els wwnpn ,attached_wwpn
+ */
+static int  fpin_chk_wwn_setpath_marginal(uint16_t host_num,  struct vectors *vecs,
+		uint64_t els_wwpn, uint64_t attached_wwpn)
+{
+	struct path *pp;
+	struct multipath *mpp;
+	int i, k;
+	int ret = 0;
 
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
 	lock(&vecs->lock);
 	pthread_testcancel();
 
 	vector_foreach_slot(vecs->pathvec, pp, k) {
-		/* Checks the host number and also for the SCSI FCP */
-		if (pp->bus != SYSFS_BUS_SCSI || pp->sg_id.proto_id != SCSI_PROTOCOL_FCP || host_num !=  pp->sg_id.host_no)
-			continue;
-		sprintf(rport_id, "rport-%d:%d-%d",
-				pp->sg_id.host_no, pp->sg_id.channel, pp->sg_id.transport_id);
-		rport_dev = udev_device_new_from_subsystem_sysname(udev,
-				"fc_remote_ports", rport_id);
-		if (!rport_dev) {
-			condlog(2, "%s: No fc_remote_port device for '%s'", pp->dev,
-					rport_id);
-			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);
+		} else if ((pp->bus == SYSFS_BUS_SCSI) &&
+			(pp->sg_id.proto_id == SCSI_PROTOCOL_FCP) &&
+			(host_num ==  pp->sg_id.host_no)) {
+			/* Checks the host number and also for the SCSI FCP */
+			fpin_check_set_scsi_path_marginal(host_num, pp, els_wwpn);
 		}
-		pthread_cleanup_push(_udev_device_unref, rport_dev);
-		value = udev_device_get_sysattr_value(rport_dev, "port_name");
-		if (!value)
-			goto unref;
-
-		if (value)
-			wwpn =  strtol(value, NULL, 16);
-		/*
-		 * If the port wwpn matches sets the path and port state
-		 * to marginal
-		 */
-		if (wwpn == els_wwpn) {
-			ret = fpin_path_setmarginal(pp);
-			if (ret < 0)
-				goto unref;
-			fpin_set_rport_marginal(rport_dev);
-			fpin_add_marginal_dev_info(host_num, pp->dev);
-		}
-unref:
-		pthread_cleanup_pop(1);
 	}
 	/* walk backwards because reload_and_sync_map() can remove mpp */
 	vector_foreach_slot_backwards(vecs->mpvec, mpp, i) {
@@ -286,14 +378,17 @@  fpin_parse_li_els_setpath_marginal(uint16_t host_num, struct fc_tlv_desc *tlv,
 	struct fc_fn_li_desc *li_desc = (struct fc_fn_li_desc *)tlv;
 	int count = 0;
 	int ret = 0;
+	uint64_t attached_wwpn;
 
 	/* Update the wwn to list */
 	wwn_count = be32_to_cpu(li_desc->pname_count);
-	condlog(4, "Got wwn count as %d\n", wwn_count);
+	attached_wwpn = be64_to_cpu(li_desc->attached_wwpn);
+	condlog(4, "Got wwn count as %d detecting wwn 0x%lx attached_wwpn 0x%lx\n",
+			wwn_count, be64_to_cpu(li_desc->detecting_wwpn), attached_wwpn);
 
 	for (iter = 0; iter < wwn_count; iter++) {
 		wwpn = be64_to_cpu(li_desc->pname_list[iter]);
-		ret = fpin_chk_wwn_setpath_marginal(host_num, vecs, wwpn);
+		ret = fpin_chk_wwn_setpath_marginal(host_num, vecs, wwpn, attached_wwpn);
 		if (ret < 0)
 			condlog(2, "failed to set the path marginal associated with wwpn: 0x%" PRIx64 "\n", wwpn);