| 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 |
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
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 --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);