Re: [PATCH 1/3] block: bio-integrity: add PI iovec to bio

From: "Alexander V. Buev" <a.buev@yadro.com>
To: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: <linux-block@vger.kernel.org>, Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@lst.de>,
	Mikhail Malygin <m.malygin@yadro.com>, <linux@yadro.com>
Subject: Re: [PATCH 1/3] block: bio-integrity: add PI iovec to bio
Date: Fri, 29 Oct 2021 13:59:16 +0300	[thread overview]
Message-ID: <20211029105916.6eedcdpd2jho2c5q@yadro.com> (raw)
In-Reply-To: <yq1cznov6q7.fsf@ca-mkp.ca.oracle.com>

> 
> Alexander,
> 
> Thanks for working on this!
> 
> > +		if (bip->bip_flags & BIP_RELEASE_PAGES) {
> > +			bio_integrity_payload_release_pages(bip);
> > +		}
> 
> In my parallel attempt the flag was called BIP_USER_MAPPED to mirror
> BIO_USER_MAPPED. But with the latter now gone it doesn't really
> matter. BIP_RELEASE_PAGES is fine.
ok 

> I find bio_integrity_payload_release_pages() a bit long. All the other
> functions just use a bio_integrity_ prefix and take a bio. But no
> biggie.
> 
> > +int bio_integrity_add_pi_iovec(struct bio *bio, struct iovec *pi_iov)
> 
> bio_integrity_add_iovec(), please. _pi is redundant.

I definitely agree with this (As with other similar notes)
It's just my habit of naming functions by struct name.

> 
> > +	bip_set_seed(bip, bio->bi_iter.bi_sector);
> > +
> > +	if (bi->flags & BLK_INTEGRITY_IP_CHECKSUM)
> > +		bip->bip_flags |= BIP_IP_CHECKSUM;
> 
> The last couple of months I have been working on a version of our DIX/PI
> qualification tooling that does not depend on the DB I/O stack.
> 
> For the test tooling to work I need to be able to pass the seed and the
> BIP_* flags as part of the command. The tooling needs to be able to
> select the type of checksum and to be able to disable checking for
> initiator and target on a per-I/O basis. So these would need to be
> passed in.
> 
> Note that extended PI formats have been defined in NVMe. These allow for
> larger CRCs and reference tags to be specified in addition to a storage
> tag. So we'll need to be careful when defining the SQE fields here.

After discussion, I plan to start by adding a pointer to the PI data 
in SQE struct.
This struct now has some padding that can be used for this.
Size of PI data can be determined according to integrity profile of
device and size of normal data.

Also, as recommended, the new version will use opcodes instead of the flag.

Thanks everyone for the detailed feedback. It's important for me.
I will try to correct all the issue's in the next version.

But some later )

-- 
Alexander Buev