[v3,0/4] multi-page bvec configuration for integrity payload

Message

Jinyoung Choi Aug. 3, 2023, 2:46 a.m. UTC

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/[e-mail]/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(-)

Comments

Martin K. Petersen Aug. 9, 2023, 1:36 a.m. UTC | #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 <[e-mail]>

Martin K. Petersen Aug. 9, 2023, 10:04 p.m. UTC | #2

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 <[e-mail]>

Jinyoung Choi Aug. 10, 2023, 2:06 a.m. UTC | #3

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 <[e-mail]>
> 
> -- 
> 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.

Jens Axboe Aug. 10, 2023, 1:20 p.m. UTC | #4

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,