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 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: Sets 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
The bio_integrity_add_page() returns the set length if the execution
result is successful. Otherwise, return 0.
Unnecessary if statement was removed. And when the result value was less
than the set value, it was changed to failed.
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
On Mon, Jul 31, 2023 at 09:54:59PM +0900, Jinyoung Choi wrote:
> The bio_integrity_add_page() returns the set length if the execution
> result is successful. Otherwise, return 0.
>
> Unnecessary if statement was removed. And when the result value was less
> than the set value, it was changed to failed.
Maybe word this as
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.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
> On Mon, Jul 31, 2023 at 09:54:59PM +0900, Jinyoung Choi wrote:
> > The bio_integrity_add_page() returns the set length if the execution
> > result is successful. Otherwise, return 0.
> >
> > Unnecessary if statement was removed. And when the result value was less
> > than the set value, it was changed to failed.
>
> Maybe word this as
>
> 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.
>
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <[email protected]>
Hi, Christoph.
Thank you for your review. I will update comment soon!
Best Regards,
Jinyoung.