In the case of NVMe, it has an integrity payload consisting of one segment.
So, rather than configuring SG_LIST, it was changed by direct DMA mapping.
The page-merge is not performed for the struct bio_vec when creating
a integrity payload in block.
As a result, when creating an integrity paylaod beyond one page, each
struct bio_vec is generated, and its bv_len does not exceed the PAGESIZE.
To solve it, bio_integrity_add_page() should just add to the existing
bvec, similar to bio_add_page() and friends.
(ref: https://lore.kernel.org/linux-nvme/[email protected]/T/#t)
Tested like this:
- Format (support pi)
$ sudo nvme format /dev/nvme2n1 --force -n 1 -i 1 -p 0 -m 0 -l 1 -r
- Run FIO
[global]
ioengine=libaio
group_reporting
[job]
bs=512k
iodepth=256
rw=write
numjobs=8
direct=1
runtime=10s
filename=/dev/nvme2n1
- Result
...
[ 93.496218] nvme2n1: I/O Cmd(0x1) @ LBA 62464, 1024 blocks, I/O Error (sct 0x2 / sc 0x82) MORE
[ 93.496227] protection error, dev nvme2n1, sector 62464 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
[ 93.538788] nvme2n1: I/O Cmd(0x1) @ LBA 6144, 1024 blocks, I/O Error (sct 0x2 / sc 0x82) MORE
[ 93.538798] protection error, dev nvme2n1, sector 6144 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
[ 93.566231] nvme2n1: I/O Cmd(0x1) @ LBA 124928, 1024 blocks, I/O Error (sct 0x0 / sc 0x4)
[ 93.566241] I/O error, dev nvme2n1, sector 124928 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
[ 93.694147] nvme2n1: I/O Cmd(0x1) @ LBA 64512, 1024 blocks, I/O Error (sct 0x2 / sc 0x82) MORE
[ 93.694155] protection error, dev nvme2n1, sector 64512 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
[ 93.694299] nvme2n1: I/O Cmd(0x1) @ LBA 5120, 1024 blocks, I/O Error (sct 0x2 / sc 0x82) MORE
[ 93.694305] protection error, dev nvme2n1, sector 5120 op 0x1:(WRITE) flags 0x18800 phys_seg 3 prio class 2
...
Changes to v2:
- apply review related to comment (by Christoph)
Changes to v1:
- add a patch to modify the location of the bi_size update code.
- split a patch (create multi-page bvecs in bio_integirty_add_page()).
Jinyoung Choi (4):
block: make bvec_try_merge_hw_page() non-static
bio-integrity: update the payload size in bio_integrity_add_page()
bio-integrity: cleanup adding integrity pages to bip's bvec.
bio-integrity: create multi-page bvecs in bio_integrity_add_page()
block/bio-integrity.c | 49 ++++++++++++++++-------------
block/bio.c | 2 +-
block/blk.h | 4 +++
drivers/md/dm-crypt.c | 1 -
drivers/nvme/host/ioctl.c | 1 -
drivers/nvme/target/io-cmd-bdev.c | 3 +-
drivers/target/target_core_iblock.c | 3 +-
7 files changed, 35 insertions(+), 28 deletions(-)
--
2.34.1
bio_integrity_add_page() returns the add length if successful, else 0,
just as bio_add_page. Simply check return value checking in
bio_integrity_prep to not deal with a > 0 but < len case that can't
happen.
Cc: Christoph Hellwig <[email protected]>
Cc: Martin K. Petersen <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Jinyoung Choi <[email protected]>
---
block/bio-integrity.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 6220a99977a4..c6b3bc86e1f9 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -252,27 +252,18 @@ bool bio_integrity_prep(struct bio *bio)
/* Map it */
offset = offset_in_page(buf);
- for (i = 0 ; i < nr_pages ; i++) {
- int ret;
+ for (i = 0; i < nr_pages && len > 0; i++) {
bytes = PAGE_SIZE - offset;
- if (len <= 0)
- break;
-
if (bytes > len)
bytes = len;
- ret = bio_integrity_add_page(bio, virt_to_page(buf),
- bytes, offset);
-
- if (ret == 0) {
+ if (bio_integrity_add_page(bio, virt_to_page(buf),
+ bytes, offset) < bytes) {
printk(KERN_ERR "could not attach integrity payload\n");
goto err_end_io;
}
- if (ret < bytes)
- break;
-
buf += bytes;
len -= bytes;
offset = 0;
@@ -291,7 +282,6 @@ bool bio_integrity_prep(struct bio *bio)
bio->bi_status = BLK_STS_RESOURCE;
bio_endio(bio);
return false;
-
}
EXPORT_SYMBOL(bio_integrity_prep);
--
2.34.1
Jinyoung,
> In the case of NVMe, it has an integrity payload consisting of one
> segment. So, rather than configuring SG_LIST, it was changed by direct
> DMA mapping.
>
> The page-merge is not performed for the struct bio_vec when creating a
> integrity payload in block. As a result, when creating an integrity
> paylaod beyond one page, each struct bio_vec is generated, and its
> bv_len does not exceed the PAGESIZE.
>
> To solve it, bio_integrity_add_page() should just add to the existing
> bvec, similar to bio_add_page() and friends.
This looks OK to me. I'll test on physical SCSI hardware tomorrow to
make sure there are no regressions.
Reviewed-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
Jinyoung,
> This looks OK to me. I'll test on physical SCSI hardware tomorrow to
> make sure there are no regressions.
Lab test complete, no regressions seen.
Tested-by: Martin K. Petersen <[email protected]>
--
Martin K. Petersen Oracle Linux Engineering
Hi, Martin.
> Jinyoung,
>
> > This looks OK to me. I'll test on physical SCSI hardware tomorrow to
> > make sure there are no regressions.
>
> Lab test complete, no regressions seen.
>
> Tested-by: Martin K. Petersen <[email protected]>
>
> --
> Martin K. Petersen Oracle Linux Engineering
I uploaded only the parts that would not be a problem in the patch-set
I prepared for the first time.
I will prepare to make good use of the nr_integrity_segments in the
request structure.
Thank you for your time.
Have a nice day!
Kind Regards,
Jinyoung.
On Thu, 03 Aug 2023 11:46:56 +0900, Jinyoung Choi wrote:
> In the case of NVMe, it has an integrity payload consisting of one segment.
> So, rather than configuring SG_LIST, it was changed by direct DMA mapping.
>
> The page-merge is not performed for the struct bio_vec when creating
> a integrity payload in block.
> As a result, when creating an integrity paylaod beyond one page, each
> struct bio_vec is generated, and its bv_len does not exceed the PAGESIZE.
>
> [...]
Applied, thanks!
[1/4] block: make bvec_try_merge_hw_page() non-static
commit: 7c8998f75d2d42ddefb172239b0f689392958309
[2/4] bio-integrity: update the payload size in bio_integrity_add_page()
commit: 80814b8e359f7207595f52702aea432a7bd61200
[3/4] bio-integrity: cleanup adding integrity pages to bip's bvec.
commit: d1f04c2e23c99258049c6081c3147bae69e5bcb8
[4/4] bio-integrity: create multi-page bvecs in bio_integrity_add_page()
commit: 0ece1d649b6dd615925a72bc1824d6b9fa5b998a
Best regards,
--
Jens Axboe