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 <[email protected]>
---
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
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.
>> 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.
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.
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
>> 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
> 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.
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