A nport object is always used in association with targerport,
remoteport, tport and rport objects. Add explicit references for any of
the associated object. This makes the unreliable reference updates in
the two callback fcloop_targetport_delete and fcloop_remoteport_delete
obsolete.
Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
drivers/nvme/target/fcloop.c | 57 +++++++++++++++++++++++++++++++++++---------
1 file changed, 46 insertions(+), 11 deletions(-)
diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index de1963c34bd88d0335f70de569565740fd395a0a..80693705c069dd114b2d4f15d0482dd2d713a273 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -1071,16 +1071,11 @@ fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
struct fcloop_rport *rport = remoteport->private;
flush_work(&rport->ls_work);
- fcloop_nport_put(rport->nport);
}
static void
fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
{
- struct fcloop_tport *tport = targetport->private;
-
- flush_work(&tport->ls_work);
- fcloop_nport_put(tport->nport);
}
#define FCLOOP_HW_QUEUES 4
@@ -1358,6 +1353,7 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
struct nvme_fc_port_info pinfo;
int ret;
+ /* nport ref get: rport */
nport = fcloop_alloc_nport(buf, count, true);
if (!nport)
return -EIO;
@@ -1375,6 +1371,9 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
return ret;
}
+ /* nport ref get: remoteport */
+ fcloop_nport_get(nport);
+
/* success */
rport = remoteport->private;
rport->remoteport = remoteport;
@@ -1403,16 +1402,27 @@ __unlink_remote_port(struct fcloop_nport *nport)
nport->tport->remoteport = NULL;
nport->rport = NULL;
+ /* nport ref put: rport */
+ fcloop_nport_put(nport);
+
return rport;
}
static int
__remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
{
- if (!rport)
- return -EALREADY;
+ int ret;
- return nvme_fc_unregister_remoteport(rport->remoteport);
+ if (!rport) {
+ ret = -EALREADY;
+ goto out;
+ }
+
+ ret = nvme_fc_unregister_remoteport(rport->remoteport);
+out:
+ /* nport ref put: remoteport */
+ fcloop_nport_put(nport);
+ return ret;
}
static ssize_t
@@ -1434,6 +1444,9 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
if (tmpport->node_name == nodename &&
tmpport->port_name == portname && tmpport->rport) {
+
+ if (!fcloop_nport_get(tmpport))
+ break;
nport = tmpport;
rport = __unlink_remote_port(nport);
break;
@@ -1447,6 +1460,8 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
ret = __remoteport_unreg(nport, rport);
+ fcloop_nport_put(nport);
+
return ret ? ret : count;
}
@@ -1460,6 +1475,7 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
struct nvmet_fc_port_info tinfo;
int ret;
+ /* nport ref get: tport */
nport = fcloop_alloc_nport(buf, count, false);
if (!nport)
return -EIO;
@@ -1475,6 +1491,9 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
return ret;
}
+ /* nport ref get: targetport */
+ fcloop_nport_get(nport);
+
/* success */
tport = targetport->private;
tport->targetport = targetport;
@@ -1501,16 +1520,27 @@ __unlink_target_port(struct fcloop_nport *nport)
nport->rport->targetport = NULL;
nport->tport = NULL;
+ /* nport ref put: tport */
+ fcloop_nport_put(nport);
+
return tport;
}
static int
__targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
{
- if (!tport)
- return -EALREADY;
+ int ret;
- return nvmet_fc_unregister_targetport(tport->targetport);
+ if (!tport) {
+ ret = -EALREADY;
+ goto out;
+ }
+
+ ret = nvmet_fc_unregister_targetport(tport->targetport);
+out:
+ /* nport ref put: targetport */
+ fcloop_nport_put(nport);
+ return ret;
}
static ssize_t
@@ -1532,6 +1562,9 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
if (tmpport->node_name == nodename &&
tmpport->port_name == portname && tmpport->tport) {
+
+ if (!fcloop_nport_get(tmpport))
+ break;
nport = tmpport;
tport = __unlink_target_port(nport);
break;
@@ -1545,6 +1578,8 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
ret = __targetport_unreg(nport, tport);
+ fcloop_nport_put(nport);
+
return ret ? ret : count;
}
--
2.48.1On 2/26/25 19:45, Daniel Wagner wrote:
> A nport object is always used in association with targerport,
> remoteport, tport and rport objects. Add explicit references for any of
> the associated object. This makes the unreliable reference updates in
> the two callback fcloop_targetport_delete and fcloop_remoteport_delete
> obsolete.
>
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
> drivers/nvme/target/fcloop.c | 57 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index de1963c34bd88d0335f70de569565740fd395a0a..80693705c069dd114b2d4f15d0482dd2d713a273 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -1071,16 +1071,11 @@ fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
> struct fcloop_rport *rport = remoteport->private;
>
> flush_work(&rport->ls_work);
> - fcloop_nport_put(rport->nport);
> }
>
> static void
> fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
> {
> - struct fcloop_tport *tport = targetport->private;
> -
> - flush_work(&tport->ls_work);
> - fcloop_nport_put(tport->nport);
> }
>
Errm. Isn't this function empty now? Can't it be remove altogether?
> #define FCLOOP_HW_QUEUES 4
> @@ -1358,6 +1353,7 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
> struct nvme_fc_port_info pinfo;
> int ret;
>
> + /* nport ref get: rport */
> nport = fcloop_alloc_nport(buf, count, true);
> if (!nport)
> return -EIO;
> @@ -1375,6 +1371,9 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
> return ret;
> }
>
> + /* nport ref get: remoteport */
> + fcloop_nport_get(nport);
> +
> /* success */
> rport = remoteport->private;
> rport->remoteport = remoteport;
> @@ -1403,16 +1402,27 @@ __unlink_remote_port(struct fcloop_nport *nport)
> nport->tport->remoteport = NULL;
> nport->rport = NULL;
>
> + /* nport ref put: rport */
> + fcloop_nport_put(nport);
> +
> return rport;
> }
>
> static int
> __remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
> {
> - if (!rport)
> - return -EALREADY;
> + int ret;
>
> - return nvme_fc_unregister_remoteport(rport->remoteport);
> + if (!rport) {
> + ret = -EALREADY;
> + goto out;
> + }
> +
> + ret = nvme_fc_unregister_remoteport(rport->remoteport);
> +out:
> + /* nport ref put: remoteport */
> + fcloop_nport_put(nport);
> + return ret;
> }
>
> static ssize_t
> @@ -1434,6 +1444,9 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
> list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
> if (tmpport->node_name == nodename &&
> tmpport->port_name == portname && tmpport->rport) {
> +
> + if (!fcloop_nport_get(tmpport))
> + break;
> nport = tmpport;
> rport = __unlink_remote_port(nport);
> break;
> @@ -1447,6 +1460,8 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
>
> ret = __remoteport_unreg(nport, rport);
>
> + fcloop_nport_put(nport);
> +
> return ret ? ret : count;
> }
>
> @@ -1460,6 +1475,7 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
> struct nvmet_fc_port_info tinfo;
> int ret;
>
> + /* nport ref get: tport */
> nport = fcloop_alloc_nport(buf, count, false);
> if (!nport)
> return -EIO;
> @@ -1475,6 +1491,9 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
> return ret;
> }
>
> + /* nport ref get: targetport */
> + fcloop_nport_get(nport);
> +
I would rather move it to the end of the function, after we set all the
references. But that's probably personal style...
> /* success */
> tport = targetport->private;
> tport->targetport = targetport;
> @@ -1501,16 +1520,27 @@ __unlink_target_port(struct fcloop_nport *nport)
> nport->rport->targetport = NULL;
> nport->tport = NULL;
>
> + /* nport ref put: tport */
> + fcloop_nport_put(nport);
> +
> return tport;
> }
>
> static int
> __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
> {
> - if (!tport)
> - return -EALREADY;
> + int ret;
That's iffy.
Q1: How can you end up with a NULL tport?
Q2: Why do we have _two_ arguments? Can't we use 'nport->tport'?
Q3: What do you do when tport is valid and tport != nport->tport?
>
> - return nvmet_fc_unregister_targetport(tport->targetport);
> + if (!tport) {
> + ret = -EALREADY;
> + goto out;
> + }
> +
> + ret = nvmet_fc_unregister_targetport(tport->targetport);
> +out:
> + /* nport ref put: targetport */
> + fcloop_nport_put(nport);
> + return ret;
> }
>
> static ssize_t
> @@ -1532,6 +1562,9 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
> list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
> if (tmpport->node_name == nodename &&
> tmpport->port_name == portname && tmpport->tport) {
> +
> + if (!fcloop_nport_get(tmpport))
> + break;
> nport = tmpport;
> tport = __unlink_target_port(nport);
> break;
> @@ -1545,6 +1578,8 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
>
> ret = __targetport_unreg(nport, tport);
>
> + fcloop_nport_put(nport);
> +
> return ret ? ret : count;
> }
>
>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +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
On Fri, Feb 28, 2025 at 08:19:19AM +0100, Hannes Reinecke wrote:
> > static int
> > __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
> > {
> > - if (!tport)
> > - return -EALREADY;
> > + int ret;
> That's iffy.
> Q1: How can you end up with a NULL tport?
The existing code doesn't get the life time right. I don't think it's
necessary anymore. This gets removed in the next two patches.
> Q2: Why do we have _two_ arguments? Can't we use 'nport->tport'?
This is addressed when the rport and tport get their own ref counting
(next two patches).
> Q3: What do you do when tport is valid and tport != nport->tport?
That is not going to happen after the full series.
I've tried to keep the changes logically separated. This patch here is
only updating the nport ref counters. It took a long time to get these
changes into a readable state. I really would like to avoid doing to
many things in one patch.
On Fri, Feb 28, 2025 at 08:19:19AM +0100, Hannes Reinecke wrote:
> > static void
> > fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
> > {
> > - struct fcloop_tport *tport = targetport->private;
> > -
> > - flush_work(&tport->ls_work);
> > - fcloop_nport_put(tport->nport);
> > }
> Errm. Isn't this function empty now? Can't it be remove altogether?
Sure, I'll get rid of it.
> > nport = fcloop_alloc_nport(buf, count, false);
> > if (!nport)
> > return -EIO;
> > @@ -1475,6 +1491,9 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
> > return ret;
> > }
> > + /* nport ref get: targetport */
> > + fcloop_nport_get(nport);
> > +
>
> I would rather move it to the end of the function, after we set all the
> references. But that's probably personal style...
I don't mind, During debugging I started to move the get/put function
where the pointers are updated, to highlight why we do the get/put. But
that was still pretty hard to find the leaks. Eventually I added some
debugging patches which annotated the call. The left over of these debug
patches are the 'nport ref get: transport' comments (*). Now that I am
confident that all is in balance, there is no reason not to make the
code more streamlined.
(*) Would something like this here be a no go? It really helps when
trying to debug the inbalance:
+static void __nvmet_fc_tgtport_put(struct nvmet_fc_tgtport *tgtport);
+static int __nvmet_fc_tgtport_get(struct nvmet_fc_tgtport *tgtport);
+
+#if 1
+#define nvmet_fc_tgtport_put(p) \
+({ \
+ pr_info("nvmet_fc_tgtport_put: %px %d %s:%d\n", \
+ p, atomic_read(&p->ref.refcount.refs), \
+ __func__, __LINE__); \
+ __nvmet_fc_tgtport_put(p); \
+})
+
+#define nvmet_fc_tgtport_get(p) \
+({ \
+ int ___r = __nvmet_fc_tgtport_get(p); \
+ \
+ pr_info("nvmet_fc_tgtport_get: %px %d %s:%d\n", \
+ p, atomic_read(&p->ref.refcount.refs), \
+ __func__, __LINE__); \
+ ___r; \
+})
+#else
+#define nvmet_fc_tgtport_put(p) __nvmet_fc_tgtport_put(p)
+#define nvmet_fc_tgtport_get(p) __nvmet_fc_tgtport_get(p)
+#endif
© 2016 - 2026 Red Hat, Inc.