Re: [PATCHv4] blk-integrity: support arbitrary buffer alignment

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Nov 13, 2025 at 02:48:43PM -0500, Keith Busch wrote:
> On Thu, Nov 13, 2025 at 07:20:22PM +0000, Eric Biggers wrote:
> > On Thu, Nov 13, 2025 at 01:14:13PM -0500, Keith Busch wrote:
> > > On Thu, Nov 13, 2025 at 09:31:35AM -0800, Eric Biggers wrote:
> > > > On Thu, Nov 13, 2025 at 07:26:21AM -0800, Keith Busch wrote:
> > > > > +static void blk_set_ip_pi(struct t10_pi_tuple *pi,
> > > > > +			  struct blk_integrity_iter *iter)
> > > > >  {
> > > > > -	u8 offset = bi->pi_offset;
> > > > > -	unsigned int i;
> > > > > -
> > > > > -	for (i = 0 ; i < iter->data_size ; i += iter->interval) {
> > > > > -		struct crc64_pi_tuple *pi = iter->prot_buf + offset;
> > > > > +	__be16 csum = (__force __be16)~(lower_16_bits(iter->crc));
> > > > 
> > > > This just throws away half of the checksum instead of properly combining
> > > > the two halves.  How is this being tested?
> > > 
> > > Yeah, this is the only guard type I've never seen a device subscribe to,
> > > so not particularly easily tested on my side. I just forced the code
> > > path down here anyway and checked if the result matches the result from
> > > the existing code calling "ip_compute_csum()". Maybe I can just continue
> > > using that as I suspect devices using that can't handle split data
> > > intervals that I'm trying to enable.
> > 
> > Wouldn't csum_fold() combine the halves correctly?
> 
> It doesn't look like that would be correct if we assume the existing
> code is correct. The current result from ip_compute_csum() just
> downcasts the result without folding, just like I'm doing here. My new
> code is produces the same result as the existing code, so worst case
> scenario, this isn't introducing a regression.
> 
> > Anyway, it needs to be tested.
> 
> Like on real hardware? I'm a bit at a loss as to how, I've never seen
> anything subscribe to this format, not even in emulation. The only thing
> I can readily do to test this is run random data through the old code,
> print the result, then run the same data through the new code and see if
> they're the same. That test is successful. Not good enough?

ip_compute_csum() returns a folded 16-bit checksum, whereas
csum_partial() returns an unfolded 32-bit checksum.  I don't see how the
checksums can be the same as before.  Hence my concern about whether
this was tested.  It could be with hardware, an emulator, or a unit
test.

- Eric