The lifetime of the fcloop_lsreq is not tight to the lifetime of the
host or target port, thus there is no need anymore to synchronize the
cleanup path anymore.

Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
 drivers/nvme/target/fcloop.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 47ce51183a66b4b37fc850cced2ddf70be2cdb42..ca11230bffedb0f2313a99e24e54e892daf0d644 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -207,7 +207,6 @@ static LIST_HEAD(fcloop_nports);
 struct fcloop_lport {
 	struct nvme_fc_local_port *localport;
 	struct list_head lport_list;
-	struct completion unreg_done;
 	refcount_t ref;
 };
 
@@ -1083,9 +1082,6 @@ fcloop_localport_delete(struct nvme_fc_local_port *localport)
 	struct fcloop_lport_priv *lport_priv = localport->private;
 	struct fcloop_lport *lport = lport_priv->lport;
 
-	/* release any threads waiting for the unreg to complete */
-	complete(&lport->unreg_done);
-
 	fcloop_lport_put(lport);
 }
 
@@ -1238,18 +1234,9 @@ fcloop_create_local_port(struct device *dev, struct device_attribute *attr,
 }
 
 static int
-__wait_localport_unreg(struct fcloop_lport *lport)
+__localport_unreg(struct fcloop_lport *lport)
 {
-	int ret;
-
-	init_completion(&lport->unreg_done);
-
-	ret = nvme_fc_unregister_localport(lport->localport);
-
-	if (!ret)
-		wait_for_completion(&lport->unreg_done);
-
-	return ret;
+	return nvme_fc_unregister_localport(lport->localport);
 }
 
 static struct fcloop_nport *
@@ -1332,7 +1319,7 @@ fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
 	if (!lport)
 		return -ENOENT;
 
-	ret = __wait_localport_unreg(lport);
+	ret = __localport_unreg(lport);
 	fcloop_lport_put(lport);
 
 	return ret ? ret : count;
@@ -1776,7 +1763,7 @@ static void __exit fcloop_exit(void)
 
 		spin_unlock_irqrestore(&fcloop_lock, flags);
 
-		ret = __wait_localport_unreg(lport);
+		ret = __localport_unreg(lport);
 		if (ret)
 			pr_warn("%s: Failed deleting local port\n", __func__);
 

-- 
2.49.0

On 4/23/25 15:21, Daniel Wagner wrote:
> The lifetime of the fcloop_lsreq is not tight to the lifetime of the
> host or target port, thus there is no need anymore to synchronize the
> cleanup path anymore.
> 
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
>   drivers/nvme/target/fcloop.c | 21 ++++-----------------
>   1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 47ce51183a66b4b37fc850cced2ddf70be2cdb42..ca11230bffedb0f2313a99e24e54e892daf0d644 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -207,7 +207,6 @@ static LIST_HEAD(fcloop_nports);
>   struct fcloop_lport {
>   	struct nvme_fc_local_port *localport;
>   	struct list_head lport_list;
> -	struct completion unreg_done;
>   	refcount_t ref;
>   };
>   
> @@ -1083,9 +1082,6 @@ fcloop_localport_delete(struct nvme_fc_local_port *localport)
>   	struct fcloop_lport_priv *lport_priv = localport->private;
>   	struct fcloop_lport *lport = lport_priv->lport;
>   
> -	/* release any threads waiting for the unreg to complete */
> -	complete(&lport->unreg_done);
> -
>   	fcloop_lport_put(lport);
>   }
>   
> @@ -1238,18 +1234,9 @@ fcloop_create_local_port(struct device *dev, struct device_attribute *attr,
>   }
>   
>   static int
> -__wait_localport_unreg(struct fcloop_lport *lport)
> +__localport_unreg(struct fcloop_lport *lport)
>   {
> -	int ret;
> -
> -	init_completion(&lport->unreg_done);
> -
> -	ret = nvme_fc_unregister_localport(lport->localport);
> -
> -	if (!ret)
> -		wait_for_completion(&lport->unreg_done);
> -
> -	return ret;
> +	return nvme_fc_unregister_localport(lport->localport);
>   }
>   
>   static struct fcloop_nport *
> @@ -1332,7 +1319,7 @@ fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
>   	if (!lport)
>   		return -ENOENT;
>   
> -	ret = __wait_localport_unreg(lport);
> +	ret = __localport_unreg(lport);
>   	fcloop_lport_put(lport);
>   
>   	return ret ? ret : count;
> @@ -1776,7 +1763,7 @@ static void __exit fcloop_exit(void)
>   
>   		spin_unlock_irqrestore(&fcloop_lock, flags);
>   
> -		ret = __wait_localport_unreg(lport);
> +		ret = __localport_unreg(lport);
>   		if (ret)
>   			pr_warn("%s: Failed deleting local port\n", __func__);
>   
> 
And who is freeing the allocated fcloop_lsreq structures?

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 Thu, Apr 24, 2025 at 12:21:09PM +0200, Hannes Reinecke wrote:
> > @@ -1776,7 +1763,7 @@ static void __exit fcloop_exit(void)
> >   		spin_unlock_irqrestore(&fcloop_lock, flags);
> > -		ret = __wait_localport_unreg(lport);
> > +		ret = __localport_unreg(lport);
> >   		if (ret)
> >   			pr_warn("%s: Failed deleting local port\n", __func__);
> > 
> And who is freeing the allocated fcloop_lsreq structures?

After a fcloop_lsreq is allocated it is put on either rport->ls_list or
tport->ls_list and later freed in fcloop_tport_lsrqst_work, resp.
fcloop_rport_lsrqst_work for the non-error case (or in the callbacks).

That means when the localport is unregistered successfully there are no
fcloop_lsreq around. The issue is what to do when __localport_unreq
returns an error.

This code could loop forever when the port_state is not in
FC_OBJSTATE_ONLINE when calling nvme_fc_unregister_localport(). This
should not happen (it did before this series) but now it shouldn't be
the case anymore. I suppose a pr_warn_once() and a sleep might be a good
idea to avoid a busy loop?

On 4/24/25 13:48, Daniel Wagner wrote:
> On Thu, Apr 24, 2025 at 12:21:09PM +0200, Hannes Reinecke wrote:
>>> @@ -1776,7 +1763,7 @@ static void __exit fcloop_exit(void)
>>>    		spin_unlock_irqrestore(&fcloop_lock, flags);
>>> -		ret = __wait_localport_unreg(lport);
>>> +		ret = __localport_unreg(lport);
>>>    		if (ret)
>>>    			pr_warn("%s: Failed deleting local port\n", __func__);
>>>
>> And who is freeing the allocated fcloop_lsreq structures?
> 
> After a fcloop_lsreq is allocated it is put on either rport->ls_list or
> tport->ls_list and later freed in fcloop_tport_lsrqst_work, resp.
> fcloop_rport_lsrqst_work for the non-error case (or in the callbacks).
> 
> That means when the localport is unregistered successfully there are no
> fcloop_lsreq around. The issue is what to do when __localport_unreq
> returns an error.
> 
> This code could loop forever when the port_state is not in
> FC_OBJSTATE_ONLINE when calling nvme_fc_unregister_localport(). This
> should not happen (it did before this series) but now it shouldn't be
> the case anymore. I suppose a pr_warn_once() and a sleep might be a good
> idea to avoid a busy loop?

My point was more: you call kmem_cache_destroy() unconditionally upon
exit. But the boilerplate says that you have to free all allocations
from the kmem_cache before that call.
Yet the exit function doesn't do that.
Question is: what are the guarantees that the cache is empty upon exit?

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 Thu, Apr 24, 2025 at 02:10:20PM +0200, Hannes Reinecke wrote:
> My point was more: you call kmem_cache_destroy() unconditionally upon
> exit. But the boilerplate says that you have to free all allocations
> from the kmem_cache before that call.
> Yet the exit function doesn't do that.
> Question is: what are the guarantees that the cache is empty upon exit?

The first loop will only terminate when all request have been freed,
thus it is safe to destroy the cache afterwards.