On Wed, Feb 3, 2021 at 11:12 AM Sagi Grimberg <s...@grimberg.me> wrote:
> On 2/1/21 2:04 AM, Boris Pismenny wrote:
> > @@ -290,12 +341,9 @@ int nvme_tcp_setup_ddp(struct nvme_tcp_queue *queue,
> > }
> >
> > req->ddp.command_id = command_id;
> > - req->ddp.sg_table.sgl = req->ddp.first_sgl;
> > - ret = sg_alloc_table_chained(&req->ddp.sg_table,
> > blk_rq_nr_phys_segments(rq),
> > - req->ddp.sg_table.sgl, SG_CHUNK_SIZE);
> > + ret = nvme_tcp_req_map_sg(req, rq);
>
> Why didn't you introduce nvme_tcp_req_map_sg in the first place?
OK, will do
> > if (ret)
> > return -ENOMEM;
> > - req->ddp.nents = blk_rq_map_sg(rq->q, rq, req->ddp.sg_table.sgl);
> >
> > ret = netdev->tcp_ddp_ops->tcp_ddp_setup(netdev,
> > queue->sock->sk,
> > @@ -1088,17 +1148,29 @@ static int nvme_tcp_recv_ddgst(struct
> > nvme_tcp_queue *queue,
> > if (queue->ddgst_remaining)
> > return 0;
> >
> > - if (queue->recv_ddgst != queue->exp_ddgst) {
> > - dev_err(queue->ctrl->ctrl.device,
> > - "data digest error: recv %#x expected %#x\n",
> > - le32_to_cpu(queue->recv_ddgst),
> > - le32_to_cpu(queue->exp_ddgst));
> > - return -EIO;
> > + offload_fail = !nvme_tcp_ddp_ddgst_ok(queue);
> > + offload_en = test_bit(NVME_TCP_Q_OFF_DDGST_RX, &queue->flags);
> > + if (!offload_en || offload_fail) {
> > + if (offload_en && offload_fail) { // software-fallback
> > + rq = blk_mq_tag_to_rq(nvme_tcp_tagset(queue),
> > + pdu->command_id);
> > + nvme_tcp_ddp_ddgst_recalc(queue->rcv_hash, rq);
> > + }
> > +
> > + nvme_tcp_ddgst_final(queue->rcv_hash, &queue->exp_ddgst);
> > + if (queue->recv_ddgst != queue->exp_ddgst) {
> > + dev_err(queue->ctrl->ctrl.device,
> > + "data digest error: recv %#x expected %#x\n",
> > + le32_to_cpu(queue->recv_ddgst),
> > + le32_to_cpu(queue->exp_ddgst));
> > + return -EIO;
> > + }
>
> I still dislike this hunk. Can you split the recalc login to its
> own ddp function at least? This is just a confusing piece of code.
mmm, we added two boolean predicates (offload_en and
offload_failed) plus a comment (software-fallback)
to clarify this piece... thought it can fly
> > if (pdu->hdr.flags & NVME_TCP_F_DATA_SUCCESS) {
> > - struct request *rq = blk_mq_tag_to_rq(nvme_tcp_tagset(queue),
> > - pdu->command_id);
> > + if (!rq)
> > + rq = blk_mq_tag_to_rq(nvme_tcp_tagset(queue),
> > + pdu->command_id);
>
> Why is this change needed? Maybe just move this assignment up?
OK will move it up
