[dm-devel] [PATCH 0/1] t10-pi bio split fix

[Lyashkov, Alexey]

Milan,

I and Dmitriy tries to fix a problem with BIO split with integrity data attached.
This is case, integrity generated/attached before bio_submit over raid device (md stack in my case) and bio is subject of bio_integrity_advance.
But bio_integrity_advance incorrectly split an integrity data, as it's assume integrity iterator in the 512b blocks.
( bip->bip_iter.bi_sector += bytes_done >> 9 )
But t10 generate function (t10_pi_generate) increase a iterator by 1 for each integrity interval in the t10_pi_generate function.
(        for (i = 0 ; i < iter->data_size ; i += iter->interval) {
                iter->seed++;
        }
)
It's good for the scsi with 512b blocks, but bad for the nvme with 4k block size.

So two solutions exist 
1) my solution - lets fix an t10_pi_generate / t10_pi_verify /  t10_pi_type1_prepare to increase iterator by value related to real integrity block in the 512b blocks.
2) Martin solution, just change an bio_integrity_advance function to make iterator shit in this integrity data blocks units instead of 512b blocks.

Alex

On 04/02/2022, 10:44, "Milan Broz" <gmazyland at gmail.com> wrote:

    On 04/02/2022 04:44, Martin K. Petersen wrote:
    > 
    > Dmitry,
    > 
    >> My only concern is dm_crypt target which operates on bip_iter directly
    >> with the code copy-pasted from bio_integrity_advance:
    >>
    >> static int dm_crypt_integrity_io_alloc(struct dm_crypt_io *io, struct bio *bio)
    >> {
    >> 	struct bio_integrity_payload *bip;
    >> 	unsigned int tag_len;
    >> 	int ret;
    >>
    >> 	if (!bio_sectors(bio) || !io->cc->on_disk_tag_size)
    >> 		return 0;
    >>
    >> 	bip = bio_integrity_alloc(bio, GFP_NOIO, 1);
    >> 	if (IS_ERR(bip))
    >> 		return PTR_ERR(bip);
    >>
    >> 	tag_len = io->cc->on_disk_tag_size * (bio_sectors(bio) >> io->cc->sector_shift);
    >>
    >> 	bip->bip_iter.bi_size = tag_len;
    >> 	bip->bip_iter.bi_sector = io->cc->start + io->sector;
    >>                 ^^^
    >>
    >> 	ret = bio_integrity_add_page(bio, virt_to_page(io->integrity_metadata),
    >> 				     tag_len, offset_in_page(io->integrity_metadata));
    >> ...
    >> }
    > 
    > I copied Milan and Mike who are more familiar with the dm-drypt internals.

    Hi,

    What's the problem here you are trying to fix?
    Even if I read linux-block posts, I do not understand the context...

    Anyway, cc to Mikulas and dm-devel, as dm-integrity/dm-crypt is
    the major user of bio_integrity here.

    If you touch the code, please be sure you run cryptsetup testsuite
    with the integrity tests.
    (IOW integritysetup tests and LUKS2 with authenticated encryption
    that uses dm-crypt over dm-integrity.)

    All we need is that dm-integrity can process bio integrity data
    directly. (I know some people do not like it, but this was
    the most "elegant" solution here.)

    Here dm-crypt uses the data for authenticated encryption (additional
    auth tag in bio field), so because dm-crypt owns bio, integrity data
    must be allocated in dm-crypt (stacked over dm-integrity).

    Milan