Re: [PATCH v20 06/20] nvme-tcp: Add DDP data-path

Sagi Grimberg
2023-11-28T10:40:21Z

> +static void nvme_tcp_complete_request(struct request *rq,
> + __le16 status,
> + union nvme_result result,
> + __u16 command_id)
> +{
> +#ifdef CONFIG_ULP_DDP
> + struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> +
> + if (req->offloaded) {
> + req->ddp_status = status;

unless this is really a ddp_status, don't name it as such. afiact
it is the nvme status, so lets stay consistent with the naming.

btw, for making the code simpler we can promote the request
status/result capture out of CONFIG_ULP_DDP to the general logic
and then I think the code will look slightly simpler.

This will be consistent with what we do in nvme-rdma and PI.

> + req->result = result;
> + nvme_tcp_teardown_ddp(req->queue, rq);
> + return;
> + }
> +#endif
> +
> + if (!nvme_try_complete_req(rq, status, result))
> + nvme_complete_rq(rq);
> +}
> +
> static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
> struct nvme_completion *cqe)
> {
> @@ -772,10 +865,9 @@ static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
> if (req->status == cpu_to_le16(NVME_SC_SUCCESS))
> req->status = cqe->status;
>
> - if (!nvme_try_complete_req(rq, req->status, cqe->result))
> - nvme_complete_rq(rq);
> + nvme_tcp_complete_request(rq, req->status, cqe->result,
> + cqe->command_id);
> queue->nr_cqe++;
> -
> return 0;
> }
>
> @@ -973,10 +1065,13 @@ static int nvme_tcp_recv_pdu(struct nvme_tcp_queue *queue, struct sk_buff *skb,
>
> static inline void nvme_tcp_end_request(struct request *rq, u16 status)
> {
> + struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> + struct nvme_tcp_queue *queue = req->queue;
> + struct nvme_tcp_data_pdu *pdu = (void *)queue->pdu;
> union nvme_result res = {};
>
> - if (!nvme_try_complete_req(rq, cpu_to_le16(status << 1), res))
> - nvme_complete_rq(rq);
> + nvme_tcp_complete_request(rq, cpu_to_le16(status << 1), res,
> + pdu->command_id);
> }
>
> static int nvme_tcp_recv_data(struct nvme_tcp_queue *queue, struct sk_buff *skb,
> @@ -1283,6 +1378,9 @@ static int nvme_tcp_try_send_cmd_pdu(struct nvme_tcp_request *req)
> else
> msg.msg_flags |= MSG_EOR;
>
> + if (test_bit(NVME_TCP_Q_OFF_DDP, &queue->flags))
> + nvme_tcp_setup_ddp(queue, blk_mq_rq_from_pdu(req));
> +

We keep coming back to this. Why isn't setup done at setup time?