Re: [PATCH v5 09/10] nvme-rdma: use ib_dma_map_sgtable_attrs()
From: Logan Gunthorpe <logang@deltatee.com>
Date: 2026-07-24 18:48:44
Also in:
linux-block, linux-nvme, linux-pci, linux-rdma, lkml, stable
On 2026-07-23 2:42 p.m., Mykola Marzhan wrote:
ib_dma_map_sg() folds every mapping error into a zero return, losing the DMA layer's error code. Switch both scatterlist mappings to ib_dma_map_sgtable_attrs(), which preserves it for the next patch to act on. No behavior change: failures are still reported as -EIO. A local sg_table is needed: dma_map_sgtable() maps orig_nents entries, and the embedded table's orig_nents is the allocated count, not the count blk_rq_map_sg() produced. rdma_rw_ctx_init() does the same. Assisted-by: Claude:claude-fable-5 Cc: stable@vger.kernel.org # v7.1 Signed-off-by: Mykola Marzhan <redacted>
I don't know. I think this could still use a bit more cleanup (see thoughts below). But the cleanup is probably too much to ask for this series. See my thoughts below. Reviewd-by: Logan Gunthorpe [off-list ref]
quoted hunk ↗ jump to hunk
--- drivers/nvme/host/rdma.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index 6909e3542794..63830334c73e 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c@@ -1469,6 +1469,7 @@ static int nvme_rdma_dma_map_req(struct ib_device *ibdev, struct request *rq, int *count, int *pi_count) { struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq); + struct sg_table sgt; int ret; req->data_sgl.sg_table.sgl = (struct scatterlist *)(req + 1);@@ -1480,12 +1481,16 @@ static int nvme_rdma_dma_map_req(struct ib_device *ibdev, struct request *rq, req->data_sgl.nents = blk_rq_map_sg(rq, req->data_sgl.sg_table.sgl); - *count = ib_dma_map_sg(ibdev, req->data_sgl.sg_table.sgl, - req->data_sgl.nents, rq_dma_dir(rq)); - if (unlikely(*count <= 0)) { + sgt = (struct sg_table) { + .sgl = req->data_sgl.sg_table.sgl, + .orig_nents = req->data_sgl.nents, + }; + ret = ib_dma_map_sgtable_attrs(ibdev, &sgt, rq_dma_dir(rq), 0); + if (unlikely(ret)) { ret = -EIO; goto out_free_table; } + *count = sgt.nents;
This really makes these calls really complicated and harder to reason about just to get an appropriate return code in the next patch. Though I see the complexity here and I feel like I'm confused by the semantics of nents and orig_nents all the time. It seems like some cases really need to track three counts: the number of valid DMA addresses (nents), the number of valid segments and the number actually allocated. sg_table only has two fields, and orig_nents ends up doing double duty — the DMA API reads it as the valid-segment count, while sg_alloc/free_table_chained() treat it as the allocation size. When those two differ, as they do here, it creates this complexity. One cheap thought that might clean up this patch series is to create a helper that can be used for both the data and the metadata, but that's only slightly better and I don't know if it is worth the effort. A better long term option might be to introduce a dma_map_sgtable like helper that also takes the number of valid segments which would clean this up pretty nicley. But that would be much more work. Thanks, Logan