Re: [PATCH v5 07/14] nvmet-fcloop: access fcpreq only when holding reqlock

[Hannes Reinecke]

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