Re: [PATCH 0/1] t10-pi bio split fix

Re: [PATCH 0/1] t10-pi bio split fix

[Ivanov, Dmitry (HPC)]

On 14/01/2022, 12:25 +0000, "Martin K. Petersen" <martin.petersen@xxxxxxxxxx> wrote:

> Can you please try the following patch?
> 
> Martin K. Petersen	Oracle Linux Engineering
> 
> diff --git a/block/bio-integrity.c b/block/bio-integrity.c
> index c0eb901315f9..fa5bc5b13c6a 100644
> --- a/block/bio-integrity.c
> +++ b/block/bio-integrity.c
> @@ -387,7 +387,7 @@ void bio_integrity_advance(struct bio *bio, unsigned int bytes_done)
> 	struct blk_integrity *bi = blk_get_integrity(bio->bi_disk);
> 	unsigned bytes = bio_integrity_bytes(bi, bytes_done >> 9);
> 
> -	bip->bip_iter.bi_sector += bytes_done >> 9;
> +	bip->bip_iter.bi_sector += bio_integrity_intervals(bi, bytes_done >> 9);
> 	bvec_iter_advance(bip->bip_vec, &bip->bip_iter, bytes);
> }

I encountered the same issue as Alexey did when writing to DPS Type 2 formatted, 8 bytes metadata NVME device(s) from a stacked driver if bio was split in md layer.
Virtual ref_tags were improperly converted to ref_tags by the block integrity prepare_fn().
The Martin's patch has resolved this issue.

Martin, could you please advance with this patch?

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));
...
}
Not sure if there is another place in drivers where bip iterator is advanced explicitly, i.e. without calling bio_integrity_advance/bio_advance.

--
Dmitry Ivanov
Hewlett Packard Enterprise - HPC AI Labs

Re: [PATCH 0/1] t10-pi bio split fix

[Martin K. Petersen]

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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/1] t10-pi bio split fix

[Milan Broz]

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

Re: [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@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

RE: [PATCH 0/1] t10-pi bio split fix

[Ivanov, Dmitry (HPC)]

On 04/02/2022 08:43 +0100, Milan Broz wrote:

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

1. Let me demonstrate this issue with the following example.

Given a driver which maps a bio to underlying devices with 4096 logical block size and the mapping:
bio: sector:0, size:32, dir:WRITE
Virtual LBA     Physical LBA
0               0
1               1
2               0
3               1

The Type 1 or 2 integrity is generated in bio_integrity_prep() by generate_fn as:
Virtual LBA     Virtual ref_tags
0               0
1               1
2               2
3               3

According to the mapping bio_split() would split it at 16. That would advance bip_iter.bi_sector (aka seed, see bip_get_seed()) by 16.
Virtual LBA     Physical LBA    seed
Split bio
0               0               0
1               1
Updated bio: sector:16, size:16
2               0               16
3               1               +1

Remapped updated bio: sector:0
Submitting it we expect to have ref_tags remapped to the actual physical start sector at the integrity prepare_fn and incremented by one per block of data:
Virtual LBA     Physical LBA    seed    virtual tags    ref_tags
2               0               2       2               16
3               1                +1      3               17

But we get with the current code a wrong seed (0+16) and there is no ref_tages remapping in the block integrity prepare_fn:
Virtual LBA     Physical LBA    seed    virtual tags    ref_tags
2               0               16      2               2
3               1                +1      3               3
This IO would fail by the NVME controller with Reference Tag Check Error (84h) because the first reference tag (2) is not equal to start LBA (16).

Martin's patch resolves this issue advancing the seed (bip_iter.bi_sector) properly by the number of integrity intervals (2) so the tag's remapping takes place in prepare_fn:
                                if (be32_to_cpu(pi->ref_tag) == virt)
                                        pi->ref_tag = cpu_to_be32(ref_tag);

2. Browsing the code I found a snippet of advancing the integrity seed directly, without calling bio_advance->bio_integrity_advance
I hope it does not introduce the abovementioned issue, please advise.

Dmitry

Re: [PATCH 0/1] t10-pi bio split fix

[Damien Le Moal]

On 12/24/21 21:16, Lyashkov, Alexey wrote:

This thread should really be addressed to linux-block@vger.kernel.org
and linux-scsi@vger.kernel.org.

Added to cc here.


> Martin,
> 
> Sorry about delay.
> 
> I don't agree with you about T10 PI reference tag in current code.
> t10_pi_generate works with virtual block numbers and virtual reference tags.
> Virtual tag mapped into real tag later in the 
> static void t10_pi_type1_prepare(struct request *rq)
> ...
>                                 if (be32_to_cpu(pi->ref_tag) == virt)
>                                         pi->ref_tag = cpu_to_be32(ref_tag);
> 
> ...
> So, we need just a pair between these functions to have a good mapping and good real reference tag 
> Once t10_pi_generate have shift a "virtual" ref tag for 4 it make a bio_integrity_advance to be happy.
> And t10_pi_type1_prepare also happy but it need to be shift with 4 as similar to the generate function.
> 
> This patch tested with software raid (raid 1 / raid 6) over over NMVe devices with 4k block size.
> In lustre case it caused a bio integrity prepare called before bio_submit so integrity will be splits before sends to the nvme devices.
> Without patch it caused an T10 write errors for each write over 4k, with patch - no errors.
> 
> Alex
> 
> On 20/12/2021, 19:29, "Martin K. Petersen" <martin.petersen@oracle.com> wrote:
> 
> 
>     Alexey,
> 
>     > t10_pi_generate / t10_pi_type1_prepare have just a increment by “1” for 
>     > the integrity internal which is 4k in my case,
>     > so any bio_integrity_advance call will be move an iterator outside of
>     > generated sequence and t10_pi_type1_prepare can’t be found a good virtual
>     > sector for the mapping.
>     > Changing an increment by “1” to be related to the real integrity size 
>     > solve a problem completely.
> 
>     By definition the T10 PI reference tag is incremented by one per
>     interval (typically the logical block size). If you implement it by a
>     different value than one then it is no longer valid protection
>     information.
> 
>     Seems like the splitting logic is broken somehow although I haven't seen
>     any failures with 4K on SCSI. What does your storage stack look like?
> 
>     -- 
>     Martin K. Petersen	Oracle Linux Engineering
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/1] t10-pi bio split fix

[Lyashkov, Alexey]

Ping for review.

On 30/12/2021, 05:24, "Damien Le Moal" <damien.lemoal@opensource.wdc.com> wrote:

    On 12/24/21 21:16, Lyashkov, Alexey wrote:

    This thread should really be addressed to linux-block@vger.kernel.org
    and linux-scsi@vger.kernel.org.

    Added to cc here.


    > Martin,
    > 
    > Sorry about delay.
    > 
    > I don't agree with you about T10 PI reference tag in current code.
    > t10_pi_generate works with virtual block numbers and virtual reference tags.
    > Virtual tag mapped into real tag later in the 
    > static void t10_pi_type1_prepare(struct request *rq)
    > ...
    >                                 if (be32_to_cpu(pi->ref_tag) == virt)
    >                                         pi->ref_tag = cpu_to_be32(ref_tag);
    > 
    > ...
    > So, we need just a pair between these functions to have a good mapping and good real reference tag 
    > Once t10_pi_generate have shift a "virtual" ref tag for 4 it make a bio_integrity_advance to be happy.
    > And t10_pi_type1_prepare also happy but it need to be shift with 4 as similar to the generate function.
    > 
    > This patch tested with software raid (raid 1 / raid 6) over over NMVe devices with 4k block size.
    > In lustre case it caused a bio integrity prepare called before bio_submit so integrity will be splits before sends to the nvme devices.
    > Without patch it caused an T10 write errors for each write over 4k, with patch - no errors.
    > 
    > Alex
    > 
    > On 20/12/2021, 19:29, "Martin K. Petersen" <martin.petersen@oracle.com> wrote:
    > 
    > 
    >     Alexey,
    > 
    >     > t10_pi_generate / t10_pi_type1_prepare have just a increment by “1” for 
    >     > the integrity internal which is 4k in my case,
    >     > so any bio_integrity_advance call will be move an iterator outside of
    >     > generated sequence and t10_pi_type1_prepare can’t be found a good virtual
    >     > sector for the mapping.
    >     > Changing an increment by “1” to be related to the real integrity size 
    >     > solve a problem completely.
    > 
    >     By definition the T10 PI reference tag is incremented by one per
    >     interval (typically the logical block size). If you implement it by a
    >     different value than one then it is no longer valid protection
    >     information.
    > 
    >     Seems like the splitting logic is broken somehow although I haven't seen
    >     any failures with 4K on SCSI. What does your storage stack look like?
    > 
    >     -- 
    >     Martin K. Petersen	Oracle Linux Engineering
    > 


    -- 
    Damien Le Moal
    Western Digital Research

Re: [PATCH 0/1] t10-pi bio split fix

[Martin K. Petersen]

Alexey,

> Ping for review.

Can you please try the following patch?

-- 
Martin K. Petersen	Oracle Linux Engineering

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index c0eb901315f9..fa5bc5b13c6a 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -387,7 +387,7 @@ void bio_integrity_advance(struct bio *bio, unsigned int bytes_done)
 	struct blk_integrity *bi = blk_get_integrity(bio->bi_disk);
 	unsigned bytes = bio_integrity_bytes(bi, bytes_done >> 9);
 
-	bip->bip_iter.bi_sector += bytes_done >> 9;
+	bip->bip_iter.bi_sector += bio_integrity_intervals(bi, bytes_done >> 9);
 	bvec_iter_advance(bip->bip_vec, &bip->bip_iter, bytes);
 }
 

Re: [PATCH 0/1] t10-pi bio split fix

[Lyashkov, Alexey]

Martin,

I will test shortly. But it mean bi_sector will be in the integrity interval units while 
any sectors expected to be in the SECTOR_SIZE units and it confuse while debug.

Alex

On 13/01/2022, 22:59, "Martin K. Petersen" <martin.petersen@oracle.com> wrote:


    Alexey,

    > Ping for review.

    Can you please try the following patch?

    -- 
    Martin K. Petersen	Oracle Linux Engineering

    diff --git a/block/bio-integrity.c b/block/bio-integrity.c
    index c0eb901315f9..fa5bc5b13c6a 100644
    --- a/block/bio-integrity.c
    +++ b/block/bio-integrity.c
    @@ -387,7 +387,7 @@ void bio_integrity_advance(struct bio *bio, unsigned int bytes_done)
     	struct blk_integrity *bi = blk_get_integrity(bio->bi_disk);
     	unsigned bytes = bio_integrity_bytes(bi, bytes_done >> 9);

    -	bip->bip_iter.bi_sector += bytes_done >> 9;
    +	bip->bip_iter.bi_sector += bio_integrity_intervals(bi, bytes_done >> 9);
     	bvec_iter_advance(bip->bip_vec, &bip->bip_iter, bytes);
     }