diff mbox series

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

Message ID 20230412052443epcms2p836b669a12c4e81368bec2cd340656f73@epcms2p8
State New
Headers
Series Fix NVMe metadata mapping size for integrity |

Commit Message

Jinyoung Choi April 12, 2023, 5:24 a.m. UTC 
  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(-)

Comments

Christoph Hellwig April 12, 2023, 6:57 a.m. UTC | #1 
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.
Jinyoung Choi April 12, 2023, 7:12 a.m. UTC | #2 
>> 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.
Christoph Hellwig April 12, 2023, 11:49 a.m. UTC | #3 
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.
Martin K. Petersen April 13, 2023, 1:34 a.m. UTC | #4 
>> 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 April 13, 2023, 1:37 a.m. UTC | #5 
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.
Jinyoung Choi April 13, 2023, 2:19 a.m. UTC | #6 
> 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.
Martin K. Petersen April 13, 2023, 2:26 a.m. UTC | #7 
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.
diff mbox series

Patch

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