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

[Hannes Reinecke]

On 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@xxxxxxxxxx>
> ---
>   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@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