* [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED
@ 2023-06-24 10:35 Irvin Cote
2023-06-27 15:30 ` Keith Busch
2023-06-27 16:02 ` Max Gurtovoy
0 siblings, 2 replies; 6+ messages in thread
From: Irvin Cote @ 2023-06-24 10:35 UTC (permalink / raw)
To: hch; +Cc: kbusch, axboe, sagi, linux-nvme, Irvin Cote
The NVM Command set Identify Namespace Data Structure defines
the metadata capabilities field (mc) that determines support for
metadata. Check for the value of this field before enabling the
NVME_NS_METADATA_SUPPORTED in the nvme_ns data structure.
Signed-off-by: Irvin Cote <irvincoteg@gmail.com>
---
drivers/nvme/host/core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 3ec38e2b9173..465206b5cf6f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1841,7 +1841,8 @@ static void nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
* Note, this check will need to be modified if any drivers
* gain the ability to use other metadata formats.
*/
- if (ctrl->max_integrity_segments && nvme_ns_has_pi(ns))
+ if (ctrl->max_integrity_segments && nvme_ns_has_pi(ns)
+ && (id->mc & NVME_MC_EXTENDED_LBA))
ns->features |= NVME_NS_METADATA_SUPPORTED;
} else {
/*
@@ -1852,7 +1853,7 @@ static void nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
*/
if (id->flbas & NVME_NS_FLBAS_META_EXT)
ns->features |= NVME_NS_EXT_LBAS;
- else
+ else if (id->mc & NVME_MC_METADATA_PTR)
ns->features |= NVME_NS_METADATA_SUPPORTED;
}
}
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED
2023-06-24 10:35 [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED Irvin Cote
@ 2023-06-27 15:30 ` Keith Busch
2023-06-27 16:02 ` Max Gurtovoy
1 sibling, 0 replies; 6+ messages in thread
From: Keith Busch @ 2023-06-27 15:30 UTC (permalink / raw)
To: Irvin Cote; +Cc: hch, axboe, sagi, linux-nvme
On Sat, Jun 24, 2023 at 12:35:02PM +0200, Irvin Cote wrote:
> The NVM Command set Identify Namespace Data Structure defines
> the metadata capabilities field (mc) that determines support for
> metadata. Check for the value of this field before enabling the
> NVME_NS_METADATA_SUPPORTED in the nvme_ns data structure.
The MC field is primarily for use when sending a format command to know
what are valid possibilities. If the reported MS and FLBAS are out of
sync with metadata capabilities, then the controller is broken.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED
2023-06-24 10:35 [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED Irvin Cote
2023-06-27 15:30 ` Keith Busch
@ 2023-06-27 16:02 ` Max Gurtovoy
2023-06-30 17:41 ` irvin cote
1 sibling, 1 reply; 6+ messages in thread
From: Max Gurtovoy @ 2023-06-27 16:02 UTC (permalink / raw)
To: Irvin Cote, hch; +Cc: kbusch, axboe, sagi, linux-nvme
On 24/06/2023 13:35, Irvin Cote wrote:
> The NVM Command set Identify Namespace Data Structure defines
> the metadata capabilities field (mc) that determines support for
> metadata. Check for the value of this field before enabling the
> NVME_NS_METADATA_SUPPORTED in the nvme_ns data structure.
>
> Signed-off-by: Irvin Cote <irvincoteg@gmail.com>
> ---
> drivers/nvme/host/core.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 3ec38e2b9173..465206b5cf6f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1841,7 +1841,8 @@ static void nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
> * Note, this check will need to be modified if any drivers
> * gain the ability to use other metadata formats.
> */
> - if (ctrl->max_integrity_segments && nvme_ns_has_pi(ns))
> + if (ctrl->max_integrity_segments && nvme_ns_has_pi(ns)
> + && (id->mc & NVME_MC_EXTENDED_LBA))
> ns->features |= NVME_NS_METADATA_SUPPORTED;
this probably should go into the WARN_ON_ONCE above since it violate the
spec.
> } else {
> /*
> @@ -1852,7 +1853,7 @@ static void nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
> */
> if (id->flbas & NVME_NS_FLBAS_META_EXT)
> ns->features |= NVME_NS_EXT_LBAS;
we can check the NVME_MC_EXTENDED_LBA here and WARN_ON_ONCE if there is
a spec violation.
> - else
> + else if (id->mc & NVME_MC_METADATA_PTR)
> ns->features |= NVME_NS_METADATA_SUPPORTED;
same here.
> }
> }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED
2023-06-27 16:02 ` Max Gurtovoy
@ 2023-06-30 17:41 ` irvin cote
2023-07-02 14:02 ` irvin cote
2023-07-10 7:01 ` Christoph Hellwig
0 siblings, 2 replies; 6+ messages in thread
From: irvin cote @ 2023-06-30 17:41 UTC (permalink / raw)
To: Max Gurtovoy; +Cc: hch, kbusch, axboe, sagi, linux-nvme
>The MC field is primarily for use when sending a format command to know
>what are valid possibilities. If the reported MS and FLBAS are out of
>sync with metadata capabilities, then the controller is broken.
Beware having MC and FLBAS out of sync does not necessarily mean we
have a broken controller :
MC could be 0 (no metadata support at all) and FLBAS has no way to
express that, it can only indicate which metadata type was selected by
the user.
In such a case we could wrongfully signal metadata as supported when it is not
On Tue, 27 Jun 2023 at 18:02, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
>
>
>
> On 24/06/2023 13:35, Irvin Cote wrote:
> > The NVM Command set Identify Namespace Data Structure defines
> > the metadata capabilities field (mc) that determines support for
> > metadata. Check for the value of this field before enabling the
> > NVME_NS_METADATA_SUPPORTED in the nvme_ns data structure.
> >
> > Signed-off-by: Irvin Cote <irvincoteg@gmail.com>
> > ---
> > drivers/nvme/host/core.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 3ec38e2b9173..465206b5cf6f 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -1841,7 +1841,8 @@ static void nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
> > * Note, this check will need to be modified if any drivers
> > * gain the ability to use other metadata formats.
> > */
> > - if (ctrl->max_integrity_segments && nvme_ns_has_pi(ns))
> > + if (ctrl->max_integrity_segments && nvme_ns_has_pi(ns)
> > + && (id->mc & NVME_MC_EXTENDED_LBA))
> > ns->features |= NVME_NS_METADATA_SUPPORTED;
>
> this probably should go into the WARN_ON_ONCE above since it violate the
> spec.
>
> > } else {
> > /*
> > @@ -1852,7 +1853,7 @@ static void nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
> > */
> > if (id->flbas & NVME_NS_FLBAS_META_EXT)
> > ns->features |= NVME_NS_EXT_LBAS;
>
> we can check the NVME_MC_EXTENDED_LBA here and WARN_ON_ONCE if there is
> a spec violation.
>
> > - else
> > + else if (id->mc & NVME_MC_METADATA_PTR)
> > ns->features |= NVME_NS_METADATA_SUPPORTED;
>
> same here.
>
> > }
> > }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED
2023-06-30 17:41 ` irvin cote
@ 2023-07-02 14:02 ` irvin cote
2023-07-10 7:01 ` Christoph Hellwig
1 sibling, 0 replies; 6+ messages in thread
From: irvin cote @ 2023-07-02 14:02 UTC (permalink / raw)
To: Max Gurtovoy; +Cc: hch, kbusch, axboe, sagi, linux-nvme
Apologies I understand now, no need to check MC if both FLBAS and MS
are checked,
Thanks
On Fri, 30 Jun 2023 at 19:41, irvin cote <irvincoteg@gmail.com> wrote:
>
> >The MC field is primarily for use when sending a format command to know
> >what are valid possibilities. If the reported MS and FLBAS are out of
> >sync with metadata capabilities, then the controller is broken.
>
> Beware having MC and FLBAS out of sync does not necessarily mean we
> have a broken controller :
> MC could be 0 (no metadata support at all) and FLBAS has no way to
> express that, it can only indicate which metadata type was selected by
> the user.
> In such a case we could wrongfully signal metadata as supported when it is not
>
> On Tue, 27 Jun 2023 at 18:02, Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> >
> >
> >
> > On 24/06/2023 13:35, Irvin Cote wrote:
> > > The NVM Command set Identify Namespace Data Structure defines
> > > the metadata capabilities field (mc) that determines support for
> > > metadata. Check for the value of this field before enabling the
> > > NVME_NS_METADATA_SUPPORTED in the nvme_ns data structure.
> > >
> > > Signed-off-by: Irvin Cote <irvincoteg@gmail.com>
> > > ---
> > > drivers/nvme/host/core.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index 3ec38e2b9173..465206b5cf6f 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -1841,7 +1841,8 @@ static void nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
> > > * Note, this check will need to be modified if any drivers
> > > * gain the ability to use other metadata formats.
> > > */
> > > - if (ctrl->max_integrity_segments && nvme_ns_has_pi(ns))
> > > + if (ctrl->max_integrity_segments && nvme_ns_has_pi(ns)
> > > + && (id->mc & NVME_MC_EXTENDED_LBA))
> > > ns->features |= NVME_NS_METADATA_SUPPORTED;
> >
> > this probably should go into the WARN_ON_ONCE above since it violate the
> > spec.
> >
> > > } else {
> > > /*
> > > @@ -1852,7 +1853,7 @@ static void nvme_configure_metadata(struct nvme_ns *ns, struct nvme_id_ns *id)
> > > */
> > > if (id->flbas & NVME_NS_FLBAS_META_EXT)
> > > ns->features |= NVME_NS_EXT_LBAS;
> >
> > we can check the NVME_MC_EXTENDED_LBA here and WARN_ON_ONCE if there is
> > a spec violation.
> >
> > > - else
> > > + else if (id->mc & NVME_MC_METADATA_PTR)
> > > ns->features |= NVME_NS_METADATA_SUPPORTED;
> >
> > same here.
> >
> > > }
> > > }
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED
2023-06-30 17:41 ` irvin cote
2023-07-02 14:02 ` irvin cote
@ 2023-07-10 7:01 ` Christoph Hellwig
1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2023-07-10 7:01 UTC (permalink / raw)
To: irvin cote; +Cc: Max Gurtovoy, hch, kbusch, axboe, sagi, linux-nvme
On Fri, Jun 30, 2023 at 07:41:27PM +0200, irvin cote wrote:
> >The MC field is primarily for use when sending a format command to know
> >what are valid possibilities. If the reported MS and FLBAS are out of
> >sync with metadata capabilities, then the controller is broken.
>
> Beware having MC and FLBAS out of sync does not necessarily mean we
> have a broken controller :
> MC could be 0 (no metadata support at all) and FLBAS has no way to
> express that, it can only indicate which metadata type was selected by
> the user.
> In such a case we could wrongfully signal metadata as supported when it is not
The LBA Format Data Structure indicates the metadata size for any
given format.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-10 7:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-24 10:35 [PATCH] nvme-core: check id->mc before setting NVME_NS_METADATA_SUPPORTED Irvin Cote
2023-06-27 15:30 ` Keith Busch
2023-06-27 16:02 ` Max Gurtovoy
2023-06-30 17:41 ` irvin cote
2023-07-02 14:02 ` irvin cote
2023-07-10 7:01 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox