[PATCH v3 net-next 08/21] nvme-tcp : Recalculate crc in the end of the capsule

[Or Gerlitz]

On Wed, Feb 3, 2021 at 11:12 AM Sagi Grimberg <sagi at 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