Re: [PATCH v3 16/18] nvmet-fc: take tgtport refs for portentry

[Hannes Reinecke <hare@suse.de>]

On 3/18/25 11:40, Daniel Wagner wrote:
> Ensure that the tgtport is not going away as long portentry has a
> pointer on it.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fc.c | 45 +++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index d10ddcb57c1b09d871152f0d9a48f93ec6dc8685..649afce908bbade0a843efc4b8b76105c164b035 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -1265,6 +1265,7 @@ nvmet_fc_portentry_bind(struct nvmet_fc_tgtport *tgtport,
>   {
>   	lockdep_assert_held(&nvmet_fc_tgtlock);
>   
> +	nvmet_fc_tgtport_get(tgtport);
>   	pe->tgtport = tgtport;
>   	tgtport->pe = pe;
>   
> @@ -1284,8 +1285,10 @@ nvmet_fc_portentry_unbind(struct nvmet_fc_port_entry *pe)
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&nvmet_fc_tgtlock, flags);
> -	if (pe->tgtport)
> +	if (pe->tgtport) {
> +		nvmet_fc_tgtport_put(pe->tgtport);
>   		pe->tgtport->pe = NULL;
> +	}
>   	list_del(&pe->pe_list);
>   	spin_unlock_irqrestore(&nvmet_fc_tgtlock, flags);
>   }
> @@ -1303,8 +1306,10 @@ nvmet_fc_portentry_unbind_tgt(struct nvmet_fc_tgtport *tgtport)
>   
>   	spin_lock_irqsave(&nvmet_fc_tgtlock, flags);
>   	pe = tgtport->pe;
> -	if (pe)
> +	if (pe) {
> +		nvmet_fc_tgtport_put(pe->tgtport);
>   		pe->tgtport = NULL;
> +	}
>   	tgtport->pe = NULL;
>   	spin_unlock_irqrestore(&nvmet_fc_tgtlock, flags);
>   }
> @@ -1327,6 +1332,9 @@ nvmet_fc_portentry_rebind_tgt(struct nvmet_fc_tgtport *tgtport)
>   	list_for_each_entry(pe, &nvmet_fc_portentry_list, pe_list) {
>   		if (tgtport->fc_target_port.node_name == pe->node_name &&
>   		    tgtport->fc_target_port.port_name == pe->port_name) {
> +			if (!nvmet_fc_tgtport_get(tgtport))
> +				continue;
> +
>   			WARN_ON(pe->tgtport);
>   			tgtport->pe = pe;
>   			pe->tgtport = tgtport;
> @@ -1664,7 +1672,6 @@ nvmet_fc_unregister_targetport(struct nvmet_fc_target_port *target_port)
>   }
>   EXPORT_SYMBOL_GPL(nvmet_fc_unregister_targetport);
>   
> -
>   /* ********************** FC-NVME LS RCV Handling ************************* */
>   
>   

Empty line ...

> @@ -2901,12 +2908,17 @@ nvmet_fc_add_port(struct nvmet_port *port)
>   	list_for_each_entry(tgtport, &nvmet_fc_target_list, tgt_list) {
>   		if ((tgtport->fc_target_port.node_name == traddr.nn) &&
>   		    (tgtport->fc_target_port.port_name == traddr.pn)) {
> +			if (!nvmet_fc_tgtport_get(tgtport))
> +				continue;
> +
>   			/* a FC port can only be 1 nvmet port id */
>   			if (!tgtport->pe) {
>   				nvmet_fc_portentry_bind(tgtport, pe, port);
>   				ret = 0;
>   			} else
>   				ret = -EALREADY;
> +
> +			nvmet_fc_tgtport_put(tgtport);
>   			break;
>   		}
>   	}
> @@ -2922,11 +2934,21 @@ static void
>   nvmet_fc_remove_port(struct nvmet_port *port)
>   {
>   	struct nvmet_fc_port_entry *pe = port->priv;
> +	struct nvmet_fc_tgtport *tgtport = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&nvmet_fc_tgtlock, flags);
> +	if (pe->tgtport && nvmet_fc_tgtport_get(pe->tgtport))
> +		tgtport = pe->tgtport;
> +	spin_unlock_irqrestore(&nvmet_fc_tgtlock, flags);
>   
>   	nvmet_fc_portentry_unbind(pe);
>   
> -	/* terminate any outstanding associations */
> -	__nvmet_fc_free_assocs(pe->tgtport);
> +	if (tgtport) {
> +		/* terminate any outstanding associations */
> +		__nvmet_fc_free_assocs(tgtport);
> +		nvmet_fc_tgtport_put(tgtport);
> +	}
>   
>   	kfree(pe);
>   }
> @@ -2935,10 +2957,21 @@ static void
>   nvmet_fc_discovery_chg(struct nvmet_port *port)
>   {
>   	struct nvmet_fc_port_entry *pe = port->priv;
> -	struct nvmet_fc_tgtport *tgtport = pe->tgtport;
> +	struct nvmet_fc_tgtport *tgtport = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&nvmet_fc_tgtlock, flags);
> +	if (pe->tgtport && nvmet_fc_tgtport_get(pe->tgtport))
> +		tgtport = pe->tgtport;
> +	spin_unlock_irqrestore(&nvmet_fc_tgtlock, flags);
> +
> +	if (!tgtport)
> +		return;
>   
>   	if (tgtport && tgtport->ops->discovery_event)
>   		tgtport->ops->discovery_event(&tgtport->fc_target_port);
> +
> +	nvmet_fc_tgtport_put(tgtport);
>   }
>   
>   static ssize_t
> 
Otherwise:

Reviewed-by: Hannes Reinecke <hare@suse.de>

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