Re: [PATCH] smb: smbdirect: fix MR registration for coalesced SG lists

[Stefan Metzmacher <metze@samba.org>]

Hi Yi,

> ib_dma_map_sg() modifies the provided scatterlist and returns the
> number of mapped entries, which can be fewer than the requested
> mr->sgt.nents if the DMA controller coalesces contiguous memory
> segments. Passing the original, uncoalesced count to ib_map_mr_sg()
> causes memory registration failures if coalescing actually occurs.
> 
> Capture the actual mapped count returned by ib_dma_map_sg() and pass it
> to ib_map_mr_sg() to ensure correct MR registration.
> 
> Also update the ib_dma_map_sg() error logging to drop the error
> pointer formatting, since the return value is an integer count
> rather than an error code.
> 
> Signed-off-by: Yi Kuo <yi@yikuo.dev>
> ---
>   fs/smb/smbdirect/mr.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/smb/smbdirect/mr.c b/fs/smb/smbdirect/mr.c
> index 5228e699cd5d..058dc24bf544 100644
> --- a/fs/smb/smbdirect/mr.c
> +++ b/fs/smb/smbdirect/mr.c
> @@ -269,7 +269,7 @@ smbdirect_connection_register_mr_io(struct smbdirect_socket *sc,
>   {
>   	const struct smbdirect_socket_parameters *sp = &sc->parameters;
>   	struct smbdirect_mr_io *mr;
> -	int ret, num_pages;
> +	int ret, num_pages, num_mapped;
>   	struct ib_reg_wr *reg_wr;
>   
>   	num_pages = iov_iter_npages(iter, sp->max_frmr_depth + 1);
> @@ -300,19 +300,19 @@ smbdirect_connection_register_mr_io(struct smbdirect_socket *sc,
>   		num_pages, iov_iter_count(iter), sp->max_frmr_depth);
>   	smbdirect_iter_to_sgt(iter, &mr->sgt, sp->max_frmr_depth);
>   
> -	ret = ib_dma_map_sg(sc->ib.dev, mr->sgt.sgl, mr->sgt.nents, mr->dir);
> -	if (!ret) {
> +	num_mapped = ib_dma_map_sg(sc->ib.dev, mr->sgt.sgl, mr->sgt.nents, mr->dir);
> +	if (!num_mapped) {
>   		smbdirect_log_rdma_mr(sc, SMBDIRECT_LOG_ERR,
> -			"ib_dma_map_sg num_pages=%u dir=%x ret=%d (%1pe)\n",
> -			num_pages, mr->dir, ret, SMBDIRECT_DEBUG_ERR_PTR(ret));
> +			"ib_dma_map_sg num_pages=%u dir=%x num_mapped=%d\n",
> +			num_pages, mr->dir, num_mapped);

While here we should have ret = -EIO;

>   		goto dma_map_error;
>   	}

Thanks! I didn't notice this and tried to fix it with some other logic:
https://git.samba.org/?p=metze/linux/wip.git;a=commitdiff;h=11fe681cb8a36a8e48366cc5dd6dff37c1de350e

> -	ret = ib_map_mr_sg(mr->mr, mr->sgt.sgl, mr->sgt.nents, NULL, PAGE_SIZE);
> -	if (ret != mr->sgt.nents) {
> +	ret = ib_map_mr_sg(mr->mr, mr->sgt.sgl, num_mapped, NULL, PAGE_SIZE);
> +	if (ret != num_mapped) {
>   		smbdirect_log_rdma_mr(sc, SMBDIRECT_LOG_ERR,
> -			"ib_map_mr_sg failed ret = %d nents = %u\n",
> -			ret, mr->sgt.nents);
> +			"ib_map_mr_sg failed ret = %d num_mapped = %u\n",
> +			ret, num_mapped);

I guess we want something like this:

		if (ret >= 0)
			ret = -EIO;

>   		goto map_mr_error;
>   	}

This should also have

Fixes: de5ef8ec3c46 ("smb: smbdirect: introduce smbdirect_mr.c with client mr code")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221408

Thanks!
metze