Re: [PATCH 04/11] nvmet-fcloop: track ref counts for nports

[Daniel Wagner <dwagner@suse.de>]

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