Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 10 Nov 2021 10:16:55 +0100
From:   Klaus Jensen <its@irrelevant.dk>
To:     Chaitanya Kulkarni <chaitanyak@nvidia.com>
Cc:     Keith Busch <kbusch@kernel.org>, Christoph Hellwig <hch@lst.de>,
        Jens Axboe <axboe@fb.com>, Sagi Grimberg <sagi@grimberg.me>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        Klaus Jensen <k.jensen@samsung.com>
Subject: Re: [PATCH] nvme: fix write zeroes pi
Message-ID: <YYuOB9VytsFiDox2@apples.localdomain>
References: <20211104181618.864157-1-its@irrelevant.dk>
 <db739cda-2dd7-015a-5ad9-d2d6e44131c6@nvidia.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="Ye8ibXikhyU4UpIe"
Content-Disposition: inline
In-Reply-To: <db739cda-2dd7-015a-5ad9-d2d6e44131c6@nvidia.com>
Precedence: bulk


--Ye8ibXikhyU4UpIe
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Nov 10 06:10, Chaitanya Kulkarni wrote:
> Klaus,
>=20
> On 11/4/2021 11:16 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> >=20
> > 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.
> >=20
> > Fix this by setting the reftag for type 1 and 2 (for type 3, reads will
> > not check the reftag).
> >=20
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>=20
> Patch looks good to me but did you get a chance to test on
> multiple non-QEMU controllers ?
>=20

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.

--Ye8ibXikhyU4UpIe
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEUigzqnXi3OaiR2bATeGvMW1PDekFAmGLjgYACgkQTeGvMW1P
DemRCAgAhY6rLFxCImjQJF4u0l7EQP2gO0+MNOkHKPjljy54QzBOMaOEErk9ycdp
Bx/km3rIt9hMY7AlrdcTAkO6EivAjdke0muHlDqovvxKTEs3Fp7GGPzvGcaL+n/O
H/fEtjYXv2I0VhhR29bi8h9hQFsU4QFCpvof2lT1Tftym3Xix4NPPOS5bO+JhWJb
2DBoW/0L2Nougm2QKaGhAhmHeGHJGAL486P1Bv9m/XmsVmuV/jcoAQd613+yK/NC
2/PgFXqTeWFDwpR1o8yd9Gq768cW76jz4ptgl4NCc3tvfzlKpHMH00frmSBZef/p
Om0gu1ilaHyAaNj4UwhR9qbQI1Q01A==
=EaqI
-----END PGP SIGNATURE-----

--Ye8ibXikhyU4UpIe--