[PATCH V2 2/2] nvmet-tcp: remove redundant calls to nvmet_tcp_fatal_error()

[Hannes Reinecke]

On 3/13/26 08:51, Maurizio Lombardi wrote:
> On Fri Mar 13, 2026 at 8:20 AM CET, Hannes Reinecke wrote:
>> On 3/12/26 14:40, Maurizio Lombardi wrote:
>>> Executing nvmet_tcp_fatal_error() is generally the responsibility
>>> of the caller (nvmet_tcp_try_recv); all other functions should
>>> just return the error code.
>>>
>>> Signed-off-by: Maurizio Lombardi <mlombard at redhat.com>
>>> ---
>>>    drivers/nvme/target/tcp.c | 22 +++++-----------------
>>>    1 file changed, 5 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>>> index 1fbf12df1183..b250b13854b4 100644
>>> --- a/drivers/nvme/target/tcp.c
>>> +++ b/drivers/nvme/target/tcp.c
>>> @@ -885,7 +885,6 @@ static int nvmet_tcp_handle_icreq(struct nvmet_tcp_queue *queue)
>>>    	if (le32_to_cpu(icreq->hdr.plen) != sizeof(struct nvme_tcp_icreq_pdu)) {
>>>    		pr_err("bad nvme-tcp pdu length (%d)\n",
>>>    			le32_to_cpu(icreq->hdr.plen));
>>> -		nvmet_tcp_fatal_error(queue);
>>>    		return -EPROTO;
>>>    	}
>>>    
>>> @@ -951,7 +950,6 @@ static int nvmet_tcp_handle_req_failure(struct nvmet_tcp_queue *queue,
>>>    	ret = nvmet_tcp_map_data(cmd);
>>>    	if (unlikely(ret)) {
>>>    		pr_err("queue %d: failed to map data\n", queue->idx);
>>> -		nvmet_tcp_fatal_error(queue);
>>>    		return -EPROTO;
>>>    	}
>>>    
>>> @@ -1024,7 +1022,6 @@ static int nvmet_tcp_handle_h2c_data_pdu(struct nvmet_tcp_queue *queue)
>>>    
>>>    err_proto:
>>>    	/* FIXME: use proper transport errors */
>>> -	nvmet_tcp_fatal_error(queue);
>>>    	return -EPROTO;
>>>    }
>>>    
>>> @@ -1039,7 +1036,6 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
>>>    		if (hdr->type != nvme_tcp_icreq) {
>>>    			pr_err("unexpected pdu type (%d) before icreq\n",
>>>    				hdr->type);
>>> -			nvmet_tcp_fatal_error(queue);
>>>    			return -EPROTO;
>>>    		}
>>>    		return nvmet_tcp_handle_icreq(queue);
>>> @@ -1048,7 +1044,6 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
>>>    	if (unlikely(hdr->type == nvme_tcp_icreq)) {
>>>    		pr_err("queue %d: received icreq pdu in state %d\n",
>>>    			queue->idx, queue->state);
>>> -		nvmet_tcp_fatal_error(queue);
>>>    		return -EPROTO;
>>>    	}
>>>    
>>> @@ -1065,7 +1060,6 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
>>>    		pr_err("queue %d: out of commands (%d) send_list_len: %d, opcode: %d",
>>>    			queue->idx, queue->nr_cmds, queue->send_list_len,
>>>    			nvme_cmd->common.opcode);
>>> -		nvmet_tcp_fatal_error(queue);
>>>    		return -ENOMEM;
>>>    	}
>>>    
>>> @@ -1086,9 +1080,9 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
>>>    	if (unlikely(ret)) {
>>>    		pr_err("queue %d: failed to map data\n", queue->idx);
>>>    		if (nvmet_tcp_has_inline_data(queue->cmd))
>>> -			nvmet_tcp_fatal_error(queue);
>>> -		else
>>> -			nvmet_req_complete(req, ret);
>>> +			return -EPROTO;
>>> +
>>> +		nvmet_req_complete(req, ret);
>>>    		ret = -EAGAIN;
>>>    		goto out;
>>>    	}
>>> @@ -1211,7 +1205,6 @@ static int nvmet_tcp_try_recv_pdu(struct nvmet_tcp_queue *queue)
>>>    
>>>    		if (unlikely(!nvmet_tcp_pdu_valid(hdr->type))) {
>>>    			pr_err("unexpected pdu type %d\n", hdr->type);
>>> -			nvmet_tcp_fatal_error(queue);
>>>    			return -EIO;
>>>    		}
>>>    
>>> @@ -1225,16 +1218,12 @@ static int nvmet_tcp_try_recv_pdu(struct nvmet_tcp_queue *queue)
>>>    	}
>>>    
>>>    	if (queue->hdr_digest &&
>>> -	    nvmet_tcp_verify_hdgst(queue, &queue->pdu, hdr->hlen)) {
>>> -		nvmet_tcp_fatal_error(queue); /* fatal */
>>> +	    nvmet_tcp_verify_hdgst(queue, &queue->pdu, hdr->hlen))
>>>    		return -EPROTO;
>>> -	}
>>>    
>>>    	if (queue->data_digest &&
>>> -	    nvmet_tcp_check_ddgst(queue, &queue->pdu)) {
>>> -		nvmet_tcp_fatal_error(queue); /* fatal */
>>> +	    nvmet_tcp_check_ddgst(queue, &queue->pdu))
>>>    		return -EPROTO;
>>> -	}
>>>    
>>>    	return nvmet_tcp_done_recv_pdu(queue);
>>>    }
>>> @@ -1319,7 +1308,6 @@ static int nvmet_tcp_try_recv_ddgst(struct nvmet_tcp_queue *queue)
>>>    			le32_to_cpu(cmd->exp_ddgst));
>>>    		nvmet_req_uninit(&cmd->req);
>>>    		nvmet_tcp_free_cmd_buffers(cmd);
>>> -		nvmet_tcp_fatal_error(queue);
>>>    		ret = -EPROTO;
>>>    		goto out;
>>>    	}
>>
>> I seem to be missing something crucial here. But after applying this
>> patch _all_ invocations to nvmet_tcp_fatal_error() are gone.
>> (Applying the patches on top of linus tree with topmost commit
>> 0257f64bdac7fdca30fa3cae0df8b9ecbec7733a)
>>
>> I must be missing something crucial here ...
> 
> I've just rebased my branch on top of master and I can still
> see nvmet_tcp_socket_error() calling nvmet_tcp_fatal_error()
> 
> nvmet_tcp_socket_error() is called by both nvmet_tcp_try_send()
> and nvmet_tcp_try_recv() in case of errors, so it's still here.
> with us.
> 
Right.
But now nvmet_tcp_fatal_error() has just a single caller, so please
fold it into nvmet_tcp_socket_error() (and remove the forward declaration).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare at 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