Re: [PATCH v6 10/12] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] DMA API violation causing stale PI metadata (Data Corruption).

Pre-existing issues:
- [High] Incomplete fix: multipath I/O accounting leak on nvme_rdma_post_send() failure.
--

--- Patch [10]: [PATCH v6 10/12] nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers ---
commit d3d8febf3662843e41333a6adc3c3ea1d88ff373
Author: Mykola Marzhan <[email protected]>

nvme-rdma: return BLK_STS_P2PDMA for unsupported P2P transfers

This commit propagates -EREMOTEIO from ib_dma_map_sgtable_attrs() as BLK_STS_P2PDMA
to prevent infinite multipath requeues. It also delays nvme_start_request() until
after data mapping succeeds to avoid multipath accounting leaks on mapping errors.

Cc: [email protected] # v7.1: requires BLK_STS_P2PDMA and the ib_dma_map_sgtable_attrs() conversion

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 63830334c73e2..c38d949502b7c 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -2033,8 +2029,6 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	if (ret)
>  		goto unmap_qe;
>  
> -	nvme_start_request(rq);
> -
>  	if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) &&
>  	    queue->pi_support &&
>  	    (c->common.opcode == nvme_cmd_write ||
> @@ -2051,6 +2045,8 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		goto err;
>  	}
>  
> +	nvme_start_request(rq);
> +

[Severity: High]
Does moving nvme_start_request() after nvme_rdma_map_data() violate the DMA API?

nvme_start_request() calls blk_mq_start_request(), which invokes
blk_integrity_prepare(). This modifies the PI metadata buffers in CPU memory
to update reference tags.

Because nvme_rdma_map_data() has already mapped these buffers for DMA
(DMA_TO_DEVICE), could CPU writes happening after the map operation remain
unsynced on non-coherent architectures or when bounce buffers are used?

This might cause the device to read stale PI metadata, leading to PI
verification failures or data corruption.

>  	sqe->cqe.done = nvme_rdma_send_done;
>  
>  	ib_dma_sync_single_for_device(dev, sqe->dma,

[ ... ]

> @@ -2070,6 +2066,9 @@ static blk_status_t nvme_rdma_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		ret = nvme_host_path_error(rq);
>  	else if (err == -ENOMEM || err == -EAGAIN)
>  		ret = BLK_STS_RESOURCE;
> +	/* Peer memory unreachable from this device: don't retry. */
> +	else if (err == -EREMOTEIO)
> +		ret = BLK_STS_P2PDMA;
>  	else
>  		ret = BLK_STS_IOERR;

[Severity: High]
This is a pre-existing issue, but I noticed a potential multipath I/O
accounting leak if nvme_rdma_post_send() fails.

The commit message notes that starting the request before a failure causes
multipath accounting leaks when blk-mq hard-completes the request directly.

If nvme_rdma_post_send() fails (e.g., returns -EINVAL), execution jumps to
err_unmap and falls through to the err: block, returning a hard error status
like BLK_STS_IOERR:

	err = nvme_rdma_post_send(queue, sqe, ...);
	if (unlikely(err))
		goto err_unmap;
	...
err:
	...
	else
		ret = BLK_STS_IOERR;

Since nvme_start_request() was already called, will blk-mq hard-complete the
request directly, bypassing the driver's nvme_mpath_end_request() and leaking
multipath accounting like bd_stats->in_flight and nr_active?

This could permanently block block device teardown.

>  	nvme_cleanup_cmd(rq);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10