[PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Keith Busch]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Keith Busch]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Christoph Hellwig]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Keith Busch]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Christoph Hellwig]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Hannes Reinecke]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Sagi Grimberg]

> 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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[George, Martin]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Keith Busch]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Sagi Grimberg]

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

Re: [PATCH] nvme: retrigger ANA log update if group descriptor isn't found

[Christoph Hellwig]

Thanks,

applied to nvme-5.13.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme