[PATCH 2/2] nvme-pci: fix metadata mapping length

[PATCH 2/2] nvme-pci: fix metadata mapping length

[Jinyoung CHOI]

Even if the memory allocated for integrity is physically continuous,
struct bio_vec is composed separately for each page size.
In order not to use the blk_rq_map_integrity_sg(), the length of the
DMA mapping should be the total size of integrity payload.

Fixes: 783b94bd9250 ("nvme-pci: do not build a scatterlist to map metadata")
Signed-off-by: Jinyoung Choi <j-young.choi@samsung.com>
---
 drivers/nvme/host/pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 593f86323e25..611c677add67 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -827,8 +827,10 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
 
-	iod->meta_dma = dma_map_bvec(dev->dev, rq_integrity_vec(req),
-			rq_dma_dir(req), 0);
+	iod->meta_dma = dma_map_page(dev->dev, rq_integrity_vec(req)->bv_page,
+				     rq_integrity_vec(req)->bv_offset,
+				     rq_integrity_payload_size(req),
+				     rq_dma_dir(req));
 	if (dma_mapping_error(dev->dev, iod->meta_dma))
 		return BLK_STS_IOERR;
 	cmnd->rw.metadata = cpu_to_le64(iod->meta_dma);
@@ -968,7 +970,8 @@ static __always_inline void nvme_pci_unmap_rq(struct request *req)
 	        struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
 
 		dma_unmap_page(dev->dev, iod->meta_dma,
-			       rq_integrity_vec(req)->bv_len, rq_data_dir(req));
+			       rq_integrity_payload_size(req),
+			       rq_data_dir(req));
 	}
 
 	if (blk_rq_nr_phys_segments(req))
-- 
2.34.1

Re: [PATCH 2/2] nvme-pci: fix metadata mapping length

[hch]

On Wed, Apr 12, 2023 at 02:24:43PM +0900, Jinyoung CHOI wrote:
> Even if the memory allocated for integrity is physically continuous,
> struct bio_vec is composed separately for each page size.
> In order not to use the blk_rq_map_integrity_sg(), the length of the
> DMA mapping should be the total size of integrity payload.

Hmm, looking outside the bio_vec is pretty nasty.

I think the problem here is that bio_integrity_add_page should
just add to the existing bvec, similar to bio_add_page and friends.

RE:(2) [PATCH 2/2] nvme-pci: fix metadata mapping length

[Jinyoung CHOI]

>> Even if the memory allocated for integrity is physically continuous,
>> struct bio_vec is composed separately for each page size.
>> In order not to use the blk_rq_map_integrity_sg(), the length of the
>> DMA mapping should be the total size of integrity payload.
> 
> Hmm, looking outside the bio_vec is pretty nasty.
> 
> I think the problem here is that bio_integrity_add_page should
> just add to the existing bvec, similar to bio_add_page and friends.

I agree with you.
I think the problem is bio_integrity_add_page().
If it is modified, sg functions for blk-integrity should also 
be modified.

If you think the blk-integrity modification is better, 
I will send an mail to block mailing after modifying it.

Best Regards.
Jinyoung.

Re: (2) [PATCH 2/2] nvme-pci: fix metadata mapping length

[hch]

On Wed, Apr 12, 2023 at 04:12:30PM +0900, Jinyoung CHOI wrote:
> I agree with you.
> I think the problem is bio_integrity_add_page().
> If it is modified, sg functions for blk-integrity should also 
> be modified.
> 
> If you think the blk-integrity modification is better, 
> I will send an mail to block mailing after modifying it.

Please wait for feedback from Martin.

Re: [PATCH 2/2] nvme-pci: fix metadata mapping length

[Martin K. Petersen]

>> Even if the memory allocated for integrity is physically continuous,
>> struct bio_vec is composed separately for each page size.
>> In order not to use the blk_rq_map_integrity_sg(), the length of the
>> DMA mapping should be the total size of integrity payload.
>
> Hmm, looking outside the bio_vec is pretty nasty.
>
> I think the problem here is that bio_integrity_add_page should
> just add to the existing bvec, similar to bio_add_page and friends.

Yep, that seems like a better approach. We should try to merge.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: (2) [PATCH 2/2] nvme-pci: fix metadata mapping length

[Martin K. Petersen]

Jinyoung,

> I think the problem is bio_integrity_add_page().  If it is modified,
> sg functions for blk-integrity should also be modified.

The bio integrity scatterlist handling predates NVMe and wasn't written
with the single segment use case in mind. For SCSI we required the
hardware to support an integrity segment per data segment.

-- 
Martin K. Petersen	Oracle Linux Engineering

RE:(2) (2) [PATCH 2/2] nvme-pci: fix metadata mapping length

[Jinyoung CHOI]

> Jinyoung,
> 
>> I think the problem is bio_integrity_add_page().  If it is modified,
>> sg functions for blk-integrity should also be modified.
>
> The bio integrity scatterlist handling predates NVMe and wasn't written
> with the single segment use case in mind. For SCSI we required the
> hardware to support an integrity segment per data segment.
> 
> -- 
> Martin K. Petersen        Oracle Linux Engineering

Hi Martin.
When merging is performed in bio_integrity_add_page(),
I think SG functions for integrity will be able to be modified more 
concisely. It was just the reason. :)
If you are okay,
can I try to modify it to solve the problem with add_page?

Best Regards,
Jinyoung.

Re: (2) (2) [PATCH 2/2] nvme-pci: fix metadata mapping length

[Martin K. Petersen]

Jinyoung,

> When merging is performed in bio_integrity_add_page(), I think SG
> functions for integrity will be able to be modified more concisely. It
> was just the reason. :) If you are okay, can I try to modify it to
> solve the problem with add_page?

Yes, go ahead.

-- 
Martin K. Petersen	Oracle Linux Engineering