Re: [PATCH v5 10/14] nvmet-fcloop: don't wait for lport cleanup

[Hannes Reinecke]

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