Re: [PATCH v5 09/10] nvme-rdma: use ib_dma_map_sgtable_attrs()

[Logan Gunthorpe <logang@deltatee.com>]

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 <mykola@meshstor.io>

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 <logang@deltatee.com>

> ---
>  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