[PATCH v3] nvme: fix nvme_ns_has_pi() to check PI size if metadata size or below

[PATCH v3] nvme: fix nvme_ns_has_pi() to check PI size if metadata size or below

[Tokunori Ikegami]

The PI is the first bytes or last bytes of the metadata.
So its size is not equal to the metadata size only but below also.

Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
---
Changes since v2 to v3:
- Delete the nvme_submit_io() changes as only change the nvme_ns_has_pi().

Changes since v1:
- Fix the commit message spelling miss hte to the.

 drivers/nvme/host/nvme.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 093cb423f536..fbcb2243ba84 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -542,7 +542,7 @@ struct nvme_ns {
 /* NVMe ns supports metadata actions by the controller (generate/strip) */
 static inline bool nvme_ns_has_pi(struct nvme_ns_head *head)
 {
-	return head->pi_type && head->ms == head->pi_size;
+	return head->pi_type && head->ms >= head->pi_size;
 }
 
 struct nvme_ctrl_ops {
-- 
2.45.2

Re: [PATCH v3] nvme: fix nvme_ns_has_pi() to check PI size if metadata size or below

[Keith Busch]

On Wed, Oct 23, 2024 at 03:10:54AM +0900, Tokunori Ikegami wrote:
> The PI is the first bytes or last bytes of the metadata.
> So its size is not equal to the metadata size only but below also.

This is still wrong because the only users of this function are with
respect to PRACT. If anything, you could change the name from "has_pi"
to "supports_pract", or something like that.

>  static inline bool nvme_ns_has_pi(struct nvme_ns_head *head)
>  {
> -	return head->pi_type && head->ms == head->pi_size;
> +	return head->pi_type && head->ms >= head->pi_size;
>  }

Re: [PATCH v3] nvme: fix nvme_ns_has_pi() to check PI size if metadata size or below

[Tokunori Ikegami]

Just fixed the function name as mentioned by the v4 patch. Thank you.

On 2024/10/23 4:20, Keith Busch wrote:
> On Wed, Oct 23, 2024 at 03:10:54AM +0900, Tokunori Ikegami wrote:
>> The PI is the first bytes or last bytes of the metadata.
>> So its size is not equal to the metadata size only but below also.
> This is still wrong because the only users of this function are with
> respect to PRACT. If anything, you could change the name from "has_pi"
> to "supports_pract", or something like that.
>
>>   static inline bool nvme_ns_has_pi(struct nvme_ns_head *head)
>>   {
>> -	return head->pi_type && head->ms == head->pi_size;
>> +	return head->pi_type && head->ms >= head->pi_size;
>>   }

Re: [PATCH v3] nvme: fix nvme_ns_has_pi() to check PI size if metadata size or below

[Christoph Hellwig]

On Tue, Oct 22, 2024 at 01:20:44PM -0600, Keith Busch wrote:
> On Wed, Oct 23, 2024 at 03:10:54AM +0900, Tokunori Ikegami wrote:
> > The PI is the first bytes or last bytes of the metadata.
> > So its size is not equal to the metadata size only but below also.
> 
> This is still wrong because the only users of this function are with
> respect to PRACT. If anything, you could change the name from "has_pi"
> to "supports_pract", or something like that.

It's also use in the NVME_NS_METADATA_SUPPORTED for fabrics.
Where fabrics really means RDMA with the Mellanox PI offload.  I don't
really know if that supports PI smaller than the metadata size offhand.