* [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
@ 2020-12-05 15:29 Hannes Reinecke
2020-12-07 15:31 ` Keith Busch
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Hannes Reinecke @ 2020-12-05 15:29 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-nvme, Sagi Grimberg, Keith Busch, Hannes Reinecke
If ANA is enabled but no ANA group descriptor is found when creating
a new namespace the ANA log is most likely out of date, so trigger
a re-read. The namespace will be tagged with the NS_ANA_PENDING flag
to exclude it from path selection until the ANA log has been re-read.
Reported-by: Martin George <marting@netapp.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/multipath.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 74896be40c17..ee4a42dc99a0 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -667,6 +667,10 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
if (desc.state) {
/* found the group desc: update */
nvme_update_ns_ana_state(&desc, ns);
+ } else {
+ /* group desc not found: trigger a re-read */
+ set_bit(NVME_NS_ANA_PENDING, &ns->flags);
+ queue_work(nvme_wq, &ns->ctrl->ana_work);
}
} else {
ns->ana_state = NVME_ANA_OPTIMIZED;
--
2.26.2
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-05 15:29 [PATCH] nvme: retrigger ANA log update if group descriptor isn't found Hannes Reinecke
@ 2020-12-07 15:31 ` Keith Busch
2020-12-07 15:34 ` Christoph Hellwig
2021-01-14 3:13 ` Sagi Grimberg
2021-04-02 16:49 ` Christoph Hellwig
2 siblings, 1 reply; 13+ messages in thread
From: Keith Busch @ 2020-12-07 15:31 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Keith Busch, Christoph Hellwig, linux-nvme, Sagi Grimberg
On Sat, Dec 05, 2020 at 04:29:01PM +0100, Hannes Reinecke wrote:
> If ANA is enabled but no ANA group descriptor is found when creating
> a new namespace the ANA log is most likely out of date, so trigger
> a re-read. The namespace will be tagged with the NS_ANA_PENDING flag
> to exclude it from path selection until the ANA log has been re-read.
>
> Reported-by: Martin George <marting@netapp.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
This looks good to me.
Reviewed-by: Keith Busch <kbusch@kernel.org>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-07 15:31 ` Keith Busch
@ 2020-12-07 15:34 ` Christoph Hellwig
2020-12-07 15:46 ` Keith Busch
2020-12-07 15:51 ` Hannes Reinecke
0 siblings, 2 replies; 13+ messages in thread
From: Christoph Hellwig @ 2020-12-07 15:34 UTC (permalink / raw)
To: Keith Busch
Cc: Sagi Grimberg, Keith Busch, Hannes Reinecke, linux-nvme,
Christoph Hellwig
On Mon, Dec 07, 2020 at 07:31:21AM -0800, Keith Busch wrote:
> On Sat, Dec 05, 2020 at 04:29:01PM +0100, Hannes Reinecke wrote:
> > If ANA is enabled but no ANA group descriptor is found when creating
> > a new namespace the ANA log is most likely out of date, so trigger
> > a re-read. The namespace will be tagged with the NS_ANA_PENDING flag
> > to exclude it from path selection until the ANA log has been re-read.
> >
> > Reported-by: Martin George <marting@netapp.com>
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
>
> This looks good to me.
>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
But as I just outlined it just papers over buggy controllers. I really
don't think we should just silently do that.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-07 15:34 ` Christoph Hellwig
@ 2020-12-07 15:46 ` Keith Busch
2020-12-08 14:03 ` Christoph Hellwig
2020-12-07 15:51 ` Hannes Reinecke
1 sibling, 1 reply; 13+ messages in thread
From: Keith Busch @ 2020-12-07 15:46 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, Hannes Reinecke, linux-nvme, Sagi Grimberg
On Mon, Dec 07, 2020 at 04:34:43PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 07, 2020 at 07:31:21AM -0800, Keith Busch wrote:
> > On Sat, Dec 05, 2020 at 04:29:01PM +0100, Hannes Reinecke wrote:
> > > If ANA is enabled but no ANA group descriptor is found when creating
> > > a new namespace the ANA log is most likely out of date, so trigger
> > > a re-read. The namespace will be tagged with the NS_ANA_PENDING flag
> > > to exclude it from path selection until the ANA log has been re-read.
> > >
> > > Reported-by: Martin George <marting@netapp.com>
> > > Signed-off-by: Hannes Reinecke <hare@suse.de>
> >
> > This looks good to me.
> >
> > Reviewed-by: Keith Busch <kbusch@kernel.org>
>
> But as I just outlined it just papers over buggy controllers. I really
> don't think we should just silently do that.
Okay, that's fine with me too. I agree with you on what the correct
event sequence should be, but I just thought this looks like a fairly
harmless work-around.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-07 15:34 ` Christoph Hellwig
2020-12-07 15:46 ` Keith Busch
@ 2020-12-07 15:51 ` Hannes Reinecke
1 sibling, 0 replies; 13+ messages in thread
From: Hannes Reinecke @ 2020-12-07 15:51 UTC (permalink / raw)
To: Christoph Hellwig, Keith Busch; +Cc: Keith Busch, Sagi Grimberg, linux-nvme
On 12/7/20 4:34 PM, Christoph Hellwig wrote:
> On Mon, Dec 07, 2020 at 07:31:21AM -0800, Keith Busch wrote:
>> On Sat, Dec 05, 2020 at 04:29:01PM +0100, Hannes Reinecke wrote:
>>> If ANA is enabled but no ANA group descriptor is found when creating
>>> a new namespace the ANA log is most likely out of date, so trigger
>>> a re-read. The namespace will be tagged with the NS_ANA_PENDING flag
>>> to exclude it from path selection until the ANA log has been re-read.
>>>
>>> Reported-by: Martin George <marting@netapp.com>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>
>> This looks good to me.
>>
>> Reviewed-by: Keith Busch <kbusch@kernel.org>
>
> But as I just outlined it just papers over buggy controllers. I really
> don't think we should just silently do that.
>
What would be the downside of taking this patch?
Personally, I'd rather be lenient and allow to interoperate...
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-07 15:46 ` Keith Busch
@ 2020-12-08 14:03 ` Christoph Hellwig
2020-12-08 19:17 ` Keith Busch
0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2020-12-08 14:03 UTC (permalink / raw)
To: Keith Busch
Cc: Sagi Grimberg, Keith Busch, Christoph Hellwig, linux-nvme,
Hannes Reinecke
On Mon, Dec 07, 2020 at 07:46:50AM -0800, Keith Busch wrote:
> > But as I just outlined it just papers over buggy controllers. I really
> > don't think we should just silently do that.
>
> Okay, that's fine with me too. I agree with you on what the correct
> event sequence should be, but I just thought this looks like a fairly
> harmless work-around.
I see three major downsides:
(1) it papers over our host side bug where we do not process the
different AENs in the order that the controller generated them
(2) the code is black magic as there is no indicator why this condition
would happen
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-08 14:03 ` Christoph Hellwig
@ 2020-12-08 19:17 ` Keith Busch
2020-12-09 7:35 ` Christoph Hellwig
0 siblings, 1 reply; 13+ messages in thread
From: Keith Busch @ 2020-12-08 19:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Keith Busch, Hannes Reinecke, linux-nvme, Sagi Grimberg
On Tue, Dec 08, 2020 at 03:03:52PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 07, 2020 at 07:46:50AM -0800, Keith Busch wrote:
> > > But as I just outlined it just papers over buggy controllers. I really
> > > don't think we should just silently do that.
> >
> > Okay, that's fine with me too. I agree with you on what the correct
> > event sequence should be, but I just thought this looks like a fairly
> > harmless work-around.
>
> I see three major downsides:
You only listed two. :)
> (1) it papers over our host side bug where we do not process the
> different AENs in the order that the controller generated them
We only have one AEN outstanding at a time, though, so we can't process
them in a different order than the controller sent them, right?
> (2) the code is black magic as there is no indicator why this condition
> would happen
Would it help if Hannes adds a dev_warn() that this log refresh is
occuring from a controller's inconsistent reporting?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-08 19:17 ` Keith Busch
@ 2020-12-09 7:35 ` Christoph Hellwig
0 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2020-12-09 7:35 UTC (permalink / raw)
To: Keith Busch
Cc: Sagi Grimberg, Keith Busch, Christoph Hellwig, linux-nvme,
Hannes Reinecke
On Wed, Dec 09, 2020 at 04:17:03AM +0900, Keith Busch wrote:
> On Tue, Dec 08, 2020 at 03:03:52PM +0100, Christoph Hellwig wrote:
> > On Mon, Dec 07, 2020 at 07:46:50AM -0800, Keith Busch wrote:
> > > > But as I just outlined it just papers over buggy controllers. I really
> > > > don't think we should just silently do that.
> > >
> > > Okay, that's fine with me too. I agree with you on what the correct
> > > event sequence should be, but I just thought this looks like a fairly
> > > harmless work-around.
> >
> > I see three major downsides:
>
> You only listed two. :)
Yeah.
>
> > (1) it papers over our host side bug where we do not process the
> > different AENs in the order that the controller generated them
>
> We only have one AEN outstanding at a time, though, so we can't process
> them in a different order than the controller sent them, right?
In a more theoretical than practical race the next AEN could have
completed befoe the workqueue that executes the previous one has run.
> > (2) the code is black magic as there is no indicator why this condition
> > would happen
>
> Would it help if Hannes adds a dev_warn() that this log refresh is
> occuring from a controller's inconsistent reporting?
I really want to settle the discussion first as Fred has an extremely
strange interpretation of the spec.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-05 15:29 [PATCH] nvme: retrigger ANA log update if group descriptor isn't found Hannes Reinecke
2020-12-07 15:31 ` Keith Busch
@ 2021-01-14 3:13 ` Sagi Grimberg
2021-03-25 10:55 ` George, Martin
2021-04-02 16:49 ` Christoph Hellwig
2 siblings, 1 reply; 13+ messages in thread
From: Sagi Grimberg @ 2021-01-14 3:13 UTC (permalink / raw)
To: Hannes Reinecke, Christoph Hellwig; +Cc: linux-nvme, Keith Busch
> If ANA is enabled but no ANA group descriptor is found when creating
> a new namespace the ANA log is most likely out of date, so trigger
> a re-read. The namespace will be tagged with the NS_ANA_PENDING flag
> to exclude it from path selection until the ANA log has been re-read.
>
> Reported-by: Martin George <marting@netapp.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/nvme/host/multipath.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 74896be40c17..ee4a42dc99a0 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -667,6 +667,10 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
> if (desc.state) {
> /* found the group desc: update */
> nvme_update_ns_ana_state(&desc, ns);
> + } else {
> + /* group desc not found: trigger a re-read */
> + set_bit(NVME_NS_ANA_PENDING, &ns->flags);
> + queue_work(nvme_wq, &ns->ctrl->ana_work);
btw, I just commented on the original thread, but I do like this
one better.
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2021-01-14 3:13 ` Sagi Grimberg
@ 2021-03-25 10:55 ` George, Martin
2021-03-25 21:54 ` Keith Busch
0 siblings, 1 reply; 13+ messages in thread
From: George, Martin @ 2021-03-25 10:55 UTC (permalink / raw)
To: hch, kbusch, sagi, hare; +Cc: Meneghini, John, linux-nvme, Knight, Frederick
On Wed, 2021-01-13 at 19:13 -0800, Sagi Grimberg wrote:
>
> > If ANA is enabled but no ANA group descriptor is found when
> > creating
> > a new namespace the ANA log is most likely out of date, so trigger
> > a re-read. The namespace will be tagged with the NS_ANA_PENDING
> > flag
> > to exclude it from path selection until the ANA log has been re-
> > read.
> >
> > Reported-by: Martin George <marting@netapp.com>
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> > drivers/nvme/host/multipath.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/nvme/host/multipath.c
> > b/drivers/nvme/host/multipath.c
> > index 74896be40c17..ee4a42dc99a0 100644
> > --- a/drivers/nvme/host/multipath.c
> > +++ b/drivers/nvme/host/multipath.c
> > @@ -667,6 +667,10 @@ void nvme_mpath_add_disk(struct nvme_ns *ns,
> > struct nvme_id_ns *id)
> > if (desc.state) {
> > /* found the group desc: update */
> > nvme_update_ns_ana_state(&desc, ns);
> > + } else {
> > + /* group desc not found: trigger a re-read */
> > + set_bit(NVME_NS_ANA_PENDING, &ns->flags);
> > + queue_work(nvme_wq, &ns->ctrl->ana_work);
>
> btw, I just commented on the original thread, but I do like this
> one better.
>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
>
We now have a ratified ECN which clarifies that a NVME_AEN_CFG_NS_ATTR
is sufficient for announcing a new namespace along with a new ANA
group, as opposed to additionally raising a NVME_AEN_CFG_ANA_CHANGE as
well for the latter - available at
https://nvmexpress.org/wp-content/uploads/NVM-Express-1.4-Ratified-ECN.zip
. I hope that finally clears the confusion on this topic.
So could we now have this patch pulled in please?
-Martin
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2021-03-25 10:55 ` George, Martin
@ 2021-03-25 21:54 ` Keith Busch
2021-03-25 22:20 ` Sagi Grimberg
0 siblings, 1 reply; 13+ messages in thread
From: Keith Busch @ 2021-03-25 21:54 UTC (permalink / raw)
To: George, Martin
Cc: hch, sagi, hare, Meneghini, John, linux-nvme, Knight, Frederick
On Thu, Mar 25, 2021 at 10:55:51AM +0000, George, Martin wrote:
> On Wed, 2021-01-13 at 19:13 -0800, Sagi Grimberg wrote:
> >
> > > If ANA is enabled but no ANA group descriptor is found when
> > > creating
> > > a new namespace the ANA log is most likely out of date, so trigger
> > > a re-read. The namespace will be tagged with the NS_ANA_PENDING
> > > flag
> > > to exclude it from path selection until the ANA log has been re-
> > > read.
> > >
> > > Reported-by: Martin George <marting@netapp.com>
> > > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > > ---
> > > drivers/nvme/host/multipath.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/nvme/host/multipath.c
> > > b/drivers/nvme/host/multipath.c
> > > index 74896be40c17..ee4a42dc99a0 100644
> > > --- a/drivers/nvme/host/multipath.c
> > > +++ b/drivers/nvme/host/multipath.c
> > > @@ -667,6 +667,10 @@ void nvme_mpath_add_disk(struct nvme_ns *ns,
> > > struct nvme_id_ns *id)
> > > if (desc.state) {
> > > /* found the group desc: update */
> > > nvme_update_ns_ana_state(&desc, ns);
> > > + } else {
> > > + /* group desc not found: trigger a re-read */
> > > + set_bit(NVME_NS_ANA_PENDING, &ns->flags);
> > > + queue_work(nvme_wq, &ns->ctrl->ana_work);
> >
> > btw, I just commented on the original thread, but I do like this
> > one better.
> >
> > Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> >
>
> We now have a ratified ECN which clarifies that a NVME_AEN_CFG_NS_ATTR
> is sufficient for announcing a new namespace along with a new ANA
> group, as opposed to additionally raising a NVME_AEN_CFG_ANA_CHANGE as
> well for the latter - available at
> https://nvmexpress.org/wp-content/uploads/NVM-Express-1.4-Ratified-ECN.zip
> . I hope that finally clears the confusion on this topic.
>
> So could we now have this patch pulled in please?
Yeah, I was fine with the patch initiailly since it is harmless and
provides higher tolerance to unclear behavior. The ECN essentially
requires this patch now, so again:
Reviewed-by: Keith Busch <kbusch@kernel.org>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2021-03-25 21:54 ` Keith Busch
@ 2021-03-25 22:20 ` Sagi Grimberg
0 siblings, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2021-03-25 22:20 UTC (permalink / raw)
To: Keith Busch, George, Martin
Cc: hch, hare, Meneghini, John, linux-nvme, Knight, Frederick
>> So could we now have this patch pulled in please?
>
> Yeah, I was fine with the patch initiailly since it is harmless and
> provides higher tolerance to unclear behavior. The ECN essentially
> requires this patch now, so again:
>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found
2020-12-05 15:29 [PATCH] nvme: retrigger ANA log update if group descriptor isn't found Hannes Reinecke
2020-12-07 15:31 ` Keith Busch
2021-01-14 3:13 ` Sagi Grimberg
@ 2021-04-02 16:49 ` Christoph Hellwig
2 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2021-04-02 16:49 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, linux-nvme
Thanks,
applied to nvme-5.13.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-04-02 16:49 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-05 15:29 [PATCH] nvme: retrigger ANA log update if group descriptor isn't found Hannes Reinecke
2020-12-07 15:31 ` Keith Busch
2020-12-07 15:34 ` Christoph Hellwig
2020-12-07 15:46 ` Keith Busch
2020-12-08 14:03 ` Christoph Hellwig
2020-12-08 19:17 ` Keith Busch
2020-12-09 7:35 ` Christoph Hellwig
2020-12-07 15:51 ` Hannes Reinecke
2021-01-14 3:13 ` Sagi Grimberg
2021-03-25 10:55 ` George, Martin
2021-03-25 21:54 ` Keith Busch
2021-03-25 22:20 ` Sagi Grimberg
2021-04-02 16:49 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).