Re: [PATCH v2 7/7] nvme: fcloop: Add set_marginal_rport to sysfs

From: Jesse Taube

Date: Wed Sep 23 2026 - 13:54:02 EST

On Wed, Sep 23, 2026 at 11:34 AM Hannes Reinecke <hare@xxxxxxx> wrote:
>
> On 9/23/26 4:56 PM, Jesse Taube wrote:
> > On Mon, Sep 21, 2026 at 10:03 AM Hannes Reinecke <hare@xxxxxxx> wrote:
> >>
> >> On 9/2/26 10:05 PM, Jesse Taube wrote:
> >>> To allow testing of multipath failover, add a sysfs attribute to set a
> >>> remote port as marginal. This will allow the fcloop LLDD to set the
> >>> marginal flag on a remote port, simulating a marginal link.
> >>>
> >>> Example:
> >>> Turn on marginal for a remote port matching wwnn and wwpn:
> >>> `echo 'wwnn=0x200000109b5f2956,wwpn=0x100000109b5f2956,marginal=1' >
> >>> /sys/class/fcloop/ctl/set_marginal_rport`
> >>>
> >>> Turn off marginal for a remote port matching wwnn and wwpn:
> >>> `echo 'wwnn=0x200000109b5f2956,wwpn=0x100000109b5f2956,marginal=0' >
> >>> /sys/class/fcloop/ctl/set_marginal_rport`
> >>>
> >>> Suggested-by: John Meneghini <jmeneghi@xxxxxxxxxx>
> >>> Signed-off-by: Jesse Taube <jtaubepe@xxxxxxxxxx>
> >>> V10 -> V1:
> >>> - New patch
> >>> V1 -> V2:
> >>> - Fix reference count leak in error path
> >>> ---
> >>> drivers/nvme/target/fcloop.c | 50 ++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 50 insertions(+)
> >>>
> >>> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> >>> index b63af3b643a6..9977c2a71bd5 100644
> >>> --- a/drivers/nvme/target/fcloop.c
> >>> +++ b/drivers/nvme/target/fcloop.c
> >>> @@ -6,6 +6,7 @@
> >>> #include <linux/module.h>
> >>> #include <linux/parser.h>
> >>> #include <uapi/scsi/fc/fc_fs.h>
> >>> +#include <uapi/scsi/fc/fc_els.h>
> >>>
> >>> #include "../host/nvme.h"
> >>> #include "../target/nvmet.h"
> >>> @@ -21,6 +22,7 @@ enum {
> >>> NVMF_OPT_FCADDR = 1 << 3,
> >>> NVMF_OPT_LPWWNN = 1 << 4,
> >>> NVMF_OPT_LPWWPN = 1 << 5,
> >>> + NVMF_OPT_MARGINAL = 1 << 6,
> >>> };
> >>>
> >>> struct fcloop_ctrl_options {
> >>> @@ -31,6 +33,7 @@ struct fcloop_ctrl_options {
> >>> u32 fcaddr;
> >>> u64 lpwwnn;
> >>> u64 lpwwpn;
> >>> + u32 marginal;
> >>> };
> >>>
> >>> static const match_table_t opt_tokens = {
> >>> @@ -40,6 +43,7 @@ static const match_table_t opt_tokens = {
> >>> { NVMF_OPT_FCADDR, "fcaddr=%x" },
> >>> { NVMF_OPT_LPWWNN, "lpwwnn=%s" },
> >>> { NVMF_OPT_LPWWPN, "lpwwpn=%s" },
> >>> + { NVMF_OPT_MARGINAL, "marginal=%d" },
> >>> { NVMF_OPT_ERR, NULL }
> >>> };
> >>>
> >>> @@ -120,6 +124,13 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts,
> >>> }
> >>> opts->lpwwpn = token64;
> >>> break;
> >>> + case NVMF_OPT_MARGINAL:
> >>> + if (match_int(args, &token)) {
> >>> + ret = -EINVAL;
> >>> + goto out_free_options;
> >>> + }
> >>> + opts->marginal = token;
> >>> + break;
> >>> default:
> >>> pr_warn("unknown parameter or missing value '%s'\n", p);
> >>> ret = -EINVAL;
> >>> @@ -199,6 +210,9 @@ fcloop_parse_nm_options(struct device *dev, u64 *nname, u64 *pname,
> >>>
> >>> #define TGTPORT_OPTS (NVMF_OPT_WWNN | NVMF_OPT_WWPN)
> >>>
> >>> +#define MARGINAL_OPTS (NVMF_OPT_WWNN | NVMF_OPT_WWPN | \
> >>> + NVMF_OPT_MARGINAL)
> >>> +
> >>>
> >>> static DEFINE_SPINLOCK(fcloop_lock);
> >>> static LIST_HEAD(fcloop_lports);
> >>> @@ -1663,6 +1677,40 @@ fcloop_set_cmd_drop(struct device *dev, struct device_attribute *attr,
> >>> return count;
> >>> }
> >>>
> >>> +static ssize_t
> >>> +fcloop_set_marginal_rport(struct device *dev, struct device_attribute *attr,
> >>> + const char *buf, size_t count)
> >>> +{
> >>> + struct fcloop_nport *nport;
> >>> + struct fcloop_ctrl_options opts = {};
> >>> + unsigned long flags;
> >>> + int ret;
> >>> +
> >>> + ret = fcloop_parse_options(&opts, buf);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + /* everything there ? */
> >>> + if ((opts.mask & MARGINAL_OPTS) != MARGINAL_OPTS)
> >>> + return -EINVAL;
> >>> +
> >>> + nport = fcloop_nport_lookup(opts.wwnn, opts.wwpn);
> >>> + if (!nport)
> >>> + return -ENOENT;
> >>> +
> >>> + spin_lock_irqsave(&fcloop_lock, flags);
> >>> + if (!nport->rport || !nport->rport->remoteport) {
> >>> + spin_unlock_irqrestore(&fcloop_lock, flags);
> >>> + fcloop_nport_put(nport);
> >>> + return -ENOENT;
> >>> + }
> >>> +
> >>> + nvme_fc_set_remoteport_fpin(nport->rport->remoteport, opts.marginal);
> >>> + spin_unlock_irqrestore(&fcloop_lock, flags);
> >>> + fcloop_nport_put(nport);
> >>> +
> >>> + return count;
> >>> +}
> >>>
> >>> static DEVICE_ATTR(add_local_port, 0200, NULL, fcloop_create_local_port);
> >>> static DEVICE_ATTR(del_local_port, 0200, NULL, fcloop_delete_local_port);
> >>> @@ -1671,6 +1719,7 @@ static DEVICE_ATTR(del_remote_port, 0200, NULL, fcloop_delete_remote_port);
> >>> static DEVICE_ATTR(add_target_port, 0200, NULL, fcloop_create_target_port);
> >>> static DEVICE_ATTR(del_target_port, 0200, NULL, fcloop_delete_target_port);
> >>> static DEVICE_ATTR(set_cmd_drop, 0200, NULL, fcloop_set_cmd_drop);
> >>> +static DEVICE_ATTR(set_marginal_rport, 0200, NULL, fcloop_set_marginal_rport);
> >>>
> >>> static struct attribute *fcloop_dev_attrs[] = {
> >>> &dev_attr_add_local_port.attr,
> >>> @@ -1680,6 +1729,7 @@ static struct attribute *fcloop_dev_attrs[] = {
> >>> &dev_attr_add_target_port.attr,
> >>> &dev_attr_del_target_port.attr,
> >>> &dev_attr_set_cmd_drop.attr,
> >>> + &dev_attr_set_marginal_rport.attr,
> >>> NULL
> >>> };
> >>>
> >> Please don't make it a fabrics option.
> >
> > What do you mean by fabrics option? How should I fix this?
> >
> Your patch does two things; one is to modify the parser in
> fcloop_parse_options, and the other is to implement a new
> sysfs rport attribute 'set_marginal_rport()'.
> My suggestion is to drop the first part;

You want to drop the parser?

> we really should not start off
> with a marginal port, rather only set ports to marginal during runtime.

Yes it starts as not marginal and will only be set or unset
with a write to `set_marginal_rport` during runtime.

Thanks,
Jesse Taube

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke Kernel Storage Architect
> hare@xxxxxxx +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
>