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.
The simplest solution is to decouple the lifetime of the fcloop_lsreq
object by managing them directly within fcloop. Since this is not a
performance-critical path and only a small number of LS objects are used
during setup and cleanup, it does not significantly impact performance
to allocate them during normal operation.
Signed-off-by: Daniel Wagner <wagi@kernel.org>
---
drivers/nvme/target/fcloop.c | 53 +++++++++++++++++++++++++++++---------------
1 file changed, 35 insertions(+), 18 deletions(-)
diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index 06f42da6a0335c53ae319133119d057aab12e07e..537fc6533a4cf5d39855cf850b82af739eeb3056 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -342,6 +342,7 @@ fcloop_rport_lsrqst_work(struct work_struct *work)
* callee may free memory containing tls_req.
* do not reference lsreq after this.
*/
+ kfree(tls_req);
spin_lock(&rport->lock);
}
@@ -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;
+ }
+
memcpy(lsreq->rspaddr, lsrsp->rspbuf,
((lsreq->rsplen < lsrsp->rsplen) ?
lsreq->rsplen : lsrsp->rsplen));
lsrsp->done(lsrsp);
- if (remoteport) {
- rport = remoteport->private;
- spin_lock(&rport->lock);
- list_add_tail(&tls_req->ls_list, &rport->ls_list);
- spin_unlock(&rport->lock);
- queue_work(nvmet_wq, &rport->ls_work);
- }
+ rport = remoteport->private;
+ spin_lock(&rport->lock);
+ list_add_tail(&tls_req->ls_list, &rport->ls_list);
+ spin_unlock(&rport->lock);
+ queue_work(nvmet_wq, &rport->ls_work);
return 0;
}
@@ -426,6 +434,7 @@ fcloop_tport_lsrqst_work(struct work_struct *work)
* callee may free memory containing tls_req.
* do not reference lsreq after this.
*/
+ kfree(tls_req);
spin_lock(&tport->lock);
}
@@ -436,8 +445,8 @@ static int
fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
struct nvmefc_ls_req *lsreq)
{
- struct fcloop_lsreq *tls_req = lsreq->private;
struct fcloop_tport *tport = targetport->private;
+ struct fcloop_lsreq *tls_req;
int ret = 0;
/*
@@ -445,6 +454,10 @@ fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
* hosthandle ignored as fcloop currently is
* 1:1 tgtport vs remoteport
*/
+
+ tls_req = kmalloc(sizeof(*tls_req), GFP_KERNEL);
+ if (!tls_req)
+ return -ENOMEM;
tls_req->lsreq = lsreq;
INIT_LIST_HEAD(&tls_req->ls_list);
@@ -461,6 +474,9 @@ fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
ret = nvme_fc_rcv_ls_req(tport->remoteport, &tls_req->ls_rsp,
lsreq->rqstaddr, lsreq->rqstlen);
+ if (ret)
+ kfree(tls_req);
+
return ret;
}
@@ -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;
+ }
+
memcpy(lsreq->rspaddr, lsrsp->rspbuf,
((lsreq->rsplen < lsrsp->rsplen) ?
lsreq->rsplen : lsrsp->rsplen));
lsrsp->done(lsrsp);
- if (targetport) {
- tport = targetport->private;
- spin_lock(&tport->lock);
- list_add_tail(&tport->ls_list, &tls_req->ls_list);
- spin_unlock(&tport->lock);
- queue_work(nvmet_wq, &tport->ls_work);
- }
+ tport = targetport->private;
+ spin_lock(&tport->lock);
+ list_add_tail(&tport->ls_list, &tls_req->ls_list);
+ spin_unlock(&tport->lock);
+ queue_work(nvmet_wq, &tport->ls_work);
return 0;
}
@@ -1129,7 +1148,6 @@ static struct nvme_fc_port_template fctemplate = {
/* sizes of additional private data for data structures */
.local_priv_sz = sizeof(struct fcloop_lport_priv),
.remote_priv_sz = sizeof(struct fcloop_rport),
- .lsrqst_priv_sz = sizeof(struct fcloop_lsreq),
.fcprqst_priv_sz = sizeof(struct fcloop_ini_fcpreq),
};
@@ -1152,7 +1170,6 @@ static struct nvmet_fc_target_template tgttemplate = {
.target_features = 0,
/* sizes of additional private data for data structures */
.target_priv_sz = sizeof(struct fcloop_tport),
- .lsrqst_priv_sz = sizeof(struct fcloop_lsreq),
};
static ssize_t
--
2.48.1On 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.
>
> The simplest solution is to decouple the lifetime of the fcloop_lsreq
> object by managing them directly within fcloop. Since this is not a
> performance-critical path and only a small number of LS objects are used
> during setup and cleanup, it does not significantly impact performance
> to allocate them during normal operation.
ok
>
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
> drivers/nvme/target/fcloop.c | 53 +++++++++++++++++++++++++++++---------------
> 1 file changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 06f42da6a0335c53ae319133119d057aab12e07e..537fc6533a4cf5d39855cf850b82af739eeb3056 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -342,6 +342,7 @@ fcloop_rport_lsrqst_work(struct work_struct *work)
> * callee may free memory containing tls_req.
> * do not reference lsreq after this.
> */
> + kfree(tls_req);
>
> spin_lock(&rport->lock);
> }
> @@ -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.
> 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;
}
>
> - if (remoteport) {
> - rport = remoteport->private;
> - spin_lock(&rport->lock);
> - list_add_tail(&tls_req->ls_list, &rport->ls_list);
> - spin_unlock(&rport->lock);
> - queue_work(nvmet_wq, &rport->ls_work);
> - }
> + rport = remoteport->private;
> + spin_lock(&rport->lock);
> + list_add_tail(&tls_req->ls_list, &rport->ls_list);
> + spin_unlock(&rport->lock);
> + queue_work(nvmet_wq, &rport->ls_work);
this is just an indentation style - whichever way works.
>
> return 0;
> }
> @@ -426,6 +434,7 @@ fcloop_tport_lsrqst_work(struct work_struct *work)
> * callee may free memory containing tls_req.
> * do not reference lsreq after this.
> */
> + kfree(tls_req);
>
> spin_lock(&tport->lock);
> }
> @@ -436,8 +445,8 @@ static int
> fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
> struct nvmefc_ls_req *lsreq)
> {
> - struct fcloop_lsreq *tls_req = lsreq->private;
> struct fcloop_tport *tport = targetport->private;
> + struct fcloop_lsreq *tls_req;
> int ret = 0;
>
> /*
> @@ -445,6 +454,10 @@ fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
> * hosthandle ignored as fcloop currently is
> * 1:1 tgtport vs remoteport
> */
> +
> + tls_req = kmalloc(sizeof(*tls_req), GFP_KERNEL);
> + if (!tls_req)
> + return -ENOMEM;
> tls_req->lsreq = lsreq;
> INIT_LIST_HEAD(&tls_req->ls_list);
>
> @@ -461,6 +474,9 @@ fcloop_t2h_ls_req(struct nvmet_fc_target_port *targetport, void *hosthandle,
> ret = nvme_fc_rcv_ls_req(tport->remoteport, &tls_req->ls_rsp,
> lsreq->rqstaddr, lsreq->rqstlen);
>
> + if (ret)
> + kfree(tls_req);
> +
> return ret;
> }
>
> @@ -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.
> memcpy(lsreq->rspaddr, lsrsp->rspbuf,
> ((lsreq->rsplen < lsrsp->rsplen) ?
> lsreq->rsplen : lsrsp->rsplen));
> lsrsp->done(lsrsp);
>
Same for this done().
instead, put the check here
if (!targetport) {
kfree(tls_req);
return 0;
}
> - if (targetport) {
> - tport = targetport->private;
> - spin_lock(&tport->lock);
> - list_add_tail(&tport->ls_list, &tls_req->ls_list);
> - spin_unlock(&tport->lock);
> - queue_work(nvmet_wq, &tport->ls_work);
> - }
> + tport = targetport->private;
> + spin_lock(&tport->lock);
> + list_add_tail(&tport->ls_list, &tls_req->ls_list);
> + spin_unlock(&tport->lock);
> + queue_work(nvmet_wq, &tport->ls_work);
>
> return 0;
> }
> @@ -1129,7 +1148,6 @@ static struct nvme_fc_port_template fctemplate = {
> /* sizes of additional private data for data structures */
> .local_priv_sz = sizeof(struct fcloop_lport_priv),
> .remote_priv_sz = sizeof(struct fcloop_rport),
> - .lsrqst_priv_sz = sizeof(struct fcloop_lsreq),
> .fcprqst_priv_sz = sizeof(struct fcloop_ini_fcpreq),
> };
>
> @@ -1152,7 +1170,6 @@ static struct nvmet_fc_target_template tgttemplate = {
> .target_features = 0,
> /* sizes of additional private data for data structures */
> .target_priv_sz = sizeof(struct fcloop_tport),
> - .lsrqst_priv_sz = sizeof(struct fcloop_lsreq),
> };
>
> static ssize_t
>
-- james
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
On 3/18/25 11:40, 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.
>
> The simplest solution is to decouple the lifetime of the fcloop_lsreq
> object by managing them directly within fcloop. Since this is not a
> performance-critical path and only a small number of LS objects are used
> during setup and cleanup, it does not significantly impact performance
> to allocate them during normal operation.
>
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
> drivers/nvme/target/fcloop.c | 53 +++++++++++++++++++++++++++++---------------
> 1 file changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target
fcloop.c> index
06f42da6a0335c53ae319133119d057aab12e07e..537fc6533a4cf5d39855cf850b82af739eeb3056
100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -342,6 +342,7 @@ fcloop_rport_lsrqst_work(struct work_struct *work)
> * callee may free memory containing tls_req.
> * do not reference lsreq after this.
> */
> + kfree(tls_req);
>
> spin_lock(&rport->lock);
> }
> @@ -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;
This cries out for kmem_cache_alloc() ...
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 Tue, Mar 18, 2025 at 12:17:11PM +0100, Hannes Reinecke wrote: > > + tls_req = kmalloc(sizeof(*tls_req), GFP_KERNEL); > > + if (!tls_req) > > + return -ENOMEM; > > This cries out for kmem_cache_alloc() ... Okay, will switch to this API. FWIW, in the same call path there are more kmallocs.
On Tue, Mar 18, 2025 at 02:58:35PM +0100, Daniel Wagner wrote: > On Tue, Mar 18, 2025 at 12:17:11PM +0100, Hannes Reinecke wrote: > > > + tls_req = kmalloc(sizeof(*tls_req), GFP_KERNEL); > > > + if (!tls_req) > > > + return -ENOMEM; > > > > This cries out for kmem_cache_alloc() ... > > Okay, will switch to this API. FWIW, in the same call path there are > more kmallocs. This change send me down yet another rabbit hole because when deallocatin the cache, it reported a leak caused by a list_add_tail(x, y) vs list_add_tail(y,x) bug which I missed the last time...
© 2016 - 2025 Red Hat, Inc.