Re: [PATCH] nvme: fix write zeroes pi

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]

On Nov 10 06:10, Chaitanya Kulkarni wrote:
> Klaus,
> 
> On 11/4/2021 11:16 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Write Zeroes sets PRACT when block integrity is enabled (as it should),
> > but neglects to also set the reftag which is expected by reads. This
> > causes protection errors on reads.
> > 
> > Fix this by setting the reftag for type 1 and 2 (for type 3, reads will
> > not check the reftag).
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> 
> Patch looks good to me but did you get a chance to test on
> multiple non-QEMU controllers ?
> 

Hi Chaitanya,

No, I should of course have mentioned that, my apologies. I hit this on
QEMU, so just tested it with that. I didn't have a pi-capable device on
hand.

Feel free to leave this hanging until it can be tested with real
hardware, I think I can dig something up :) However, the spec is pretty
clear that the reftag must be set when using PRACT. It is, however,
kinda strange that the spec also says that PRCHK must be cleared to
zeroes - in my opinion, the device should check the reftag in type 1 and
2 if requested to do so.

A reason this doesnt bug out on real hardware is probably that the
blocks end up deallocated, which bypasses the pi check. However,
currently, in QEMU, if PRACT is set, QEMU will not deallocate the block
in write zeroes. Honestly, I'm not sure why it doesnt. I dont see any
reason for it to not request the QEMU block layer to deallocate... Yours
truly is the idiot that wrote it so I'll consider fixing that :)

The commit that introduced PRACT for write zeroes (commit aaeb7bb061be5
"nvme: set the PRACT bit when using Write Zeroes with T10 PI") also
mentions that without PRACT reads may have validation errors if the
write zeroes didnt deallocate, and QEMU is one of those devices.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]