[dm-devel] [PATCH] multipathd: Added support to handle FPIN-Li events for FC-NVMe

[Muneendra Kumar M]

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 at suse.com>
Sent: Friday, September 15, 2023 5:14 PM
To: Muneendra Kumar <muneendra.kumar at broadcom.com>; dm-devel at redhat.com;
bmarzins at redhat.com
Cc: mkumar at 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 at 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 at 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

-- 
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4220 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20230920/0fa465b5/attachment.p7s>