[PATCH 0/2] nvme-fc/rdma: use blk_rq_nr_phys_segments

[PATCH 0/2] nvme-fc/rdma: use blk_rq_nr_phys_segments

[Chaitanya Kulkarni]

Hi,
Here are two patches to fix the issue reported by Ming :-
(https://marc.info/?l=linux-block&m=155071174416909&w=2).

Ming has tested rdma patch. I've added the code changes for the
host/fc also but I've *not* tested the 2nd patch due to
availability of hardware.

James can you please take a look ? 

Regards,
Chaitanya


Chaitanya Kulkarni (2):
  nvme-rdma: use nr_phys_segments when map rq to sgl
  nvme-fc: use nr_phys_segments when map rq to sgl

 drivers/nvme/host/fc.c   | 4 ++--
 drivers/nvme/host/rdma.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.17.0

[PATCH 1/2] nvme-rdma: use nr_phys_segments when map rq to sgl

[Chaitanya Kulkarni]

Use blk_rq_nr_phys_segments() instead of blk_rq_payload_bytes() to check
if a command contains data to be mapped.  This fixes the case where
a struct request contains LBAs, but it has no payload, such as
Write Zeroes support.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/rdma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index ac365366c2ec..c6a489049fd5 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1150,7 +1150,7 @@ static void nvme_rdma_unmap_data(struct nvme_rdma_queue *queue,
 	struct nvme_rdma_device *dev = queue->device;
 	struct ib_device *ibdev = dev->dev;
 
-	if (!blk_rq_payload_bytes(rq))
+	if (!blk_rq_nr_phys_segments(rq))
 		return;
 
 	if (req->mr) {
@@ -1273,7 +1273,7 @@ static int nvme_rdma_map_data(struct nvme_rdma_queue *queue,
 
 	c->common.flags |= NVME_CMD_SGL_METABUF;
 
-	if (!blk_rq_payload_bytes(rq))
+	if (!blk_rq_nr_phys_segments(rq))
 		return nvme_rdma_set_sg_null(c);
 
 	req->sg_table.sgl = req->first_sgl;
-- 
2.17.0

[COMPILE TEST ONLY PATCH 2/2] nvme-fc: use nr_phys_segments when map rq to sgl

[Chaitanya Kulkarni]

Use blk_rq_nr_phys_segments() instead of blk_rq_payload_bytes() to check
if a command contains data to be mapped.  This fixes the case where
a struct request contains LBAs, but it has no payload, such as
Write Zeroes support.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/fc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 89accc76d71c..fdfaf5e914aa 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2119,7 +2119,7 @@ nvme_fc_map_data(struct nvme_fc_ctrl *ctrl, struct request *rq,
 
 	freq->sg_cnt = 0;
 
-	if (!blk_rq_payload_bytes(rq))
+	if (!blk_rq_nr_phys_segments(req))
 		return 0;
 
 	freq->sg_table.sgl = freq->first_sgl;
@@ -2316,7 +2316,7 @@ nvme_fc_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (ret)
 		return ret;
 
-	data_len = blk_rq_payload_bytes(rq);
+	data_len = blk_rq_nr_phys_segments(rq);
 	if (data_len)
 		io_dir = ((rq_data_dir(rq) == WRITE) ?
 					NVMEFC_FCP_WRITE : NVMEFC_FCP_READ);
-- 
2.17.0

[PATCH 0/2] nvme-fc/rdma: use blk_rq_nr_phys_segments

[Christoph Hellwig]

Thanks,

I'm going to pick up patch 1 for the pull request I'm about to send
to Jens.  2 looks fine to me, but I'd rather wait for James to
review it.

[PATCH 2/2] nvme-fc: use nr_phys_segments when map rq to sgl

[James Smart]

On 2/20/2019 8:13 PM, Chaitanya Kulkarni wrote:
> Use blk_rq_nr_phys_segments() instead of blk_rq_payload_bytes() to check
> if a command contains data to be mapped.  This fixes the case where
> a struct request contains LBAs, but it has no payload, such as
> Write Zeroes support.
>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
> ---
>   drivers/nvme/host/fc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 89accc76d71c..fdfaf5e914aa 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -2119,7 +2119,7 @@ nvme_fc_map_data(struct nvme_fc_ctrl *ctrl, struct request *rq,
>   
>   	freq->sg_cnt = 0;
>   
> -	if (!blk_rq_payload_bytes(rq))
> +	if (!blk_rq_nr_phys_segments(req))

This is broken - needs to be "rq" not "req". Not sure how it passed a 
compile test.

>   		return 0;
>   
>   	freq->sg_table.sgl = freq->first_sgl;
> @@ -2316,7 +2316,7 @@ nvme_fc_queue_rq(struct blk_mq_hw_ctx *hctx,
>   	if (ret)
>   		return ret;
>   
> -	data_len = blk_rq_payload_bytes(rq);
> +	data_len = (rq);
>   	if (data_len)
>   		io_dir = ((rq_data_dir(rq) == WRITE) ?
>   					NVMEFC_FCP_WRITE : NVMEFC_FCP_READ);

This is also broken as data_len is used elsewhere to specify the size of 
the payload (data outside of sqe) that is to be transferred. So although 
it was ok for the local check, you switched things that cared about a 
real length from a bytecount to a segment count. Needless to say it 
killed things.

So what does blk_rq_payload_bytes(rq) on a Write Zeros command returns 
?? Why isn't returning 0 valid for Write Zeros ?

Can you describe what the issue is that prompted this change.

-- james