On 4/23/25 15:21, Daniel Wagner wrote:
> The abort handling logic expects that the state and the fcpreq are only
> accessed when holding the reqlock lock.
>
> While at it, only handle the aborts in the abort handler.
>
> Signed-off-by: Daniel Wagner <wagi@kernel.org>
> ---
> drivers/nvme/target/fcloop.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 0c0117e03adc81c643e90a7e7832ff087a4c2fd7..9adaee3c7129f7e270842c5d09f78de2e108479a 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -619,12 +619,13 @@ fcloop_fcp_recv_work(struct work_struct *work)
> {
> struct fcloop_fcpreq *tfcp_req =
> container_of(work, struct fcloop_fcpreq, fcp_rcv_work);
> - struct nvmefc_fcp_req *fcpreq = tfcp_req->fcpreq;
> + struct nvmefc_fcp_req *fcpreq;
> unsigned long flags;
> int ret = 0;
> bool aborted = false;
>
> spin_lock_irqsave(&tfcp_req->reqlock, flags);
> + fcpreq = tfcp_req->fcpreq;
> switch (tfcp_req->inistate) {
> case INI_IO_START:
> tfcp_req->inistate = INI_IO_ACTIVE;
> @@ -639,16 +640,19 @@ fcloop_fcp_recv_work(struct work_struct *work)
> }
> spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
>
> - if (unlikely(aborted))
> - ret = -ECANCELED;
> - else {
> - if (likely(!check_for_drop(tfcp_req)))
> - ret = nvmet_fc_rcv_fcp_req(tfcp_req->tport->targetport,
> - &tfcp_req->tgt_fcp_req,
> - fcpreq->cmdaddr, fcpreq->cmdlen);
> - else
> - pr_info("%s: dropped command ********\n", __func__);
> + if (unlikely(aborted)) {
> + /* the abort handler will call fcloop_call_host_done */
> + return;
> + }
> +
> + if (unlikely(check_for_drop(tfcp_req))) {
> + pr_info("%s: dropped command ********\n", __func__);
> + return;
> }
> +
> + ret = nvmet_fc_rcv_fcp_req(tfcp_req->tport->targetport,
> + &tfcp_req->tgt_fcp_req,
> + fcpreq->cmdaddr, fcpreq->cmdlen);
> if (ret)
> fcloop_call_host_done(fcpreq, tfcp_req, ret);
> }
> @@ -663,9 +667,10 @@ fcloop_fcp_abort_recv_work(struct work_struct *work)
> unsigned long flags;
>
> spin_lock_irqsave(&tfcp_req->reqlock, flags);
> - fcpreq = tfcp_req->fcpreq;
> switch (tfcp_req->inistate) {
> case INI_IO_ABORTED:
> + fcpreq = tfcp_req->fcpreq;
> + tfcp_req->fcpreq = NULL;
> break;
> case INI_IO_COMPLETED:
> completed = true;
> @@ -688,10 +693,6 @@ fcloop_fcp_abort_recv_work(struct work_struct *work)
> nvmet_fc_rcv_fcp_abort(tfcp_req->tport->targetport,
> &tfcp_req->tgt_fcp_req);
>
> - spin_lock_irqsave(&tfcp_req->reqlock, flags);
> - tfcp_req->fcpreq = NULL;
> - spin_unlock_irqrestore(&tfcp_req->reqlock, flags);
> -
What happens for INI_IO_COMPLETED?
Don't we need to clear the 'fcpreq' pointer in that case, too?
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