Re: [PATCH v3 10/18] nvmet-fcloop: allocate/free fcloop_lsreq directly

[Daniel Wagner <dwagner@suse.de>]

Hi James,

On Wed, Mar 19, 2025 at 03:47:20PM -0700, James Smart wrote:
> On 3/18/2025 3:40 AM, Daniel Wagner wrote:
> > fcloop depends on the host or the target to allocate the fcloop_lsreq
> > object. This means that the lifetime of the fcloop_lsreq is tied to
> > either the host or the target. Consequently, the host or the target must
> > cooperate during shutdown.
> > 
> > Unfortunately, this approach does not work well when the target forces a
> > shutdown, as there are dependencies that are difficult to resolve in a
> > clean way.
> 
> ok - although I'm guessing you'll trading one set of problems for
> another.

Yes, there is no free lunch :)

So far I was able to deal with new problems. I've run the updated series
over night with the nasty blktest case where the target is constantly
removed and added back without any problems.

> > @@ -353,10 +354,13 @@ fcloop_h2t_ls_req(struct nvme_fc_local_port *localport,
> >   			struct nvme_fc_remote_port *remoteport,
> >   			struct nvmefc_ls_req *lsreq)
> >   {
> > -	struct fcloop_lsreq *tls_req = lsreq->private;
> >   	struct fcloop_rport *rport = remoteport->private;
> > +	struct fcloop_lsreq *tls_req;
> >   	int ret = 0;
> > +	tls_req = kmalloc(sizeof(*tls_req), GFP_KERNEL);
> > +	if (!tls_req)
> > +		return -ENOMEM;
> >   	tls_req->lsreq = lsreq;
> >   	INIT_LIST_HEAD(&tls_req->ls_list);
> > @@ -387,19 +391,23 @@ fcloop_h2t_xmt_ls_rsp(struct nvmet_fc_target_port *targetport,
> >   	struct nvme_fc_remote_port *remoteport = tport->remoteport;
> >   	struct fcloop_rport *rport;
> > +
> > +	if (!remoteport) {
> > +		kfree(tls_req);
> > +		return -ECONNREFUSED;
> > +	}
> > +
> don't do this - this is not a path the lldd would generate.

Ok, after looking at it again, I think I don't need it.

> 
> >   	memcpy(lsreq->rspaddr, lsrsp->rspbuf,
> >   		((lsreq->rsplen < lsrsp->rsplen) ?
> >   				lsreq->rsplen : lsrsp->rsplen));
> >   	lsrsp->done(lsrsp);
> 
> This done() call should always be made regardless of the remoteport
> presence.
> 
> instead, put the check here
> 
>         if (!remoteport) {
>             kfree(tls_req);
>             return 0;
>         }

Will do

> > @@ -475,18 +491,21 @@ fcloop_t2h_xmt_ls_rsp(struct nvme_fc_local_port *localport,
> >   	struct nvmet_fc_target_port *targetport = rport->targetport;
> >   	struct fcloop_tport *tport;
> > +	if (!targetport) {
> > +		kfree(tls_req);
> > +		return -ECONNREFUSED;
> > +	}
> > +
> 
> same here - don't do this - this is not a path the lldd would
> generate.

The early return and freeing tls_req is necessary. If the host doesn't
expect the error code, then fine. I'll remove it. The reason why we need
it is:

  The target port is gone. The target doesn't expect any response
  anymore and the ->done call is not valid because the resources have
  been freed by nvmet_fc_free_pending_reqs.

  We end up here from delete association exchange:
  nvmet_fc_xmt_disconnect_assoc sends a async request.

I am adding this as comment here.

Thanks for the review. Highly appreciated!
Daniel