[PATCH] nvme: fix write zeroes pi

[PATCH] nvme: fix write zeroes pi

[Klaus Jensen]

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>
---
 drivers/nvme/host/core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3825b596ca16..532bad4de025 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -914,10 +914,15 @@ static inline blk_status_t nvme_setup_write_zeroes(struct nvme_ns *ns,
 		cpu_to_le64(nvme_sect_to_lba(ns, blk_rq_pos(req)));
 	cmnd->write_zeroes.length =
 		cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
-	if (nvme_ns_has_pi(ns))
+
+	switch (ns->pi_type) {
+	case NVME_NS_DPS_PI_TYPE1:
+	case NVME_NS_DPS_PI_TYPE2:
 		cmnd->write_zeroes.control = cpu_to_le16(NVME_RW_PRINFO_PRACT);
-	else
-		cmnd->write_zeroes.control = 0;
+		cmnd->write_zeroes.reftag = cpu_to_le32(t10_pi_ref_tag(req));
+		break;
+	}
+
 	return BLK_STS_OK;
 }
 
-- 
2.33.1

Re: [PATCH] nvme: fix write zeroes pi

[Martin K. Petersen]

Klaus,

> 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).

The NVMe spec now clarifies that ILBRT is not inferred for Type 1 so
that looks good to me.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] nvme: fix write zeroes pi

[Chaitanya Kulkarni]

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 ?

Re: [PATCH] nvme: fix write zeroes pi

[Klaus Jensen]

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

On Nov  4 19:16, 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>
> ---
>  drivers/nvme/host/core.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 3825b596ca16..532bad4de025 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -914,10 +914,15 @@ static inline blk_status_t nvme_setup_write_zeroes(struct nvme_ns *ns,
>  		cpu_to_le64(nvme_sect_to_lba(ns, blk_rq_pos(req)));
>  	cmnd->write_zeroes.length =
>  		cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
> -	if (nvme_ns_has_pi(ns))
> +
> +	switch (ns->pi_type) {
> +	case NVME_NS_DPS_PI_TYPE1:
> +	case NVME_NS_DPS_PI_TYPE2:
>  		cmnd->write_zeroes.control = cpu_to_le16(NVME_RW_PRINFO_PRACT);
> -	else
> -		cmnd->write_zeroes.control = 0;
> +		cmnd->write_zeroes.reftag = cpu_to_le32(t10_pi_ref_tag(req));
> +		break;
> +	}
> +
>  	return BLK_STS_OK;
>  }
>  

Ugh. I goofed up. This fails to set PRACT for type 3.

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

Re: [PATCH] nvme: fix write zeroes pi

[Klaus Jensen]

[-- 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 --]

Re: [PATCH] nvme: fix write zeroes pi

[Chaitanya Kulkarni]

On 11/10/2021 1:16 AM, Klaus Jensen wrote:


I think we should wait until you test this on real H/W.

Please do update us with next version once you do so,
as this needs to be tested on fabrics controller too
since we now support PI there.