Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Christoph Hellwig]

On Wed, Nov 18, 2020 at 05:18:59PM +0530, Martin George wrote:
> From: Martin George <marting@netapp.com>
> 
> The current nvme rescan triggered in response to a NS Attr Changed
> AEN, fails to add a namespace if belonging to a new ANA group. This
> is because ana_log_buf->ngrps is not updated in nvme_parse_ana_log(),
> due to which it is unable to locate the new ANA group's descriptor,
> eventually resulting in the failure to add the namespace during the
> rescan. Fix this by reading the ANA log page as part of ana_work so
> that ngrps gets updated, prior to invoking nvme_queue_scan().

How can the namespace reference an ANA group that we haven't been
notified about using the NVME_AEN_CFG_ANA_CHANGE AEN before?

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Sagi Grimberg]

> Fix this by reading the ANA log page as part of ana_work so
> that ngrps gets updated, prior to invoking nvme_queue_scan().

Nothing guarantees ordering here..

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index fff90200497c..749d45c207f7 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4315,6 +4315,10 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
>   	switch (aer_notice_type) {
>   	case NVME_AER_NOTICE_NS_CHANGED:
>   		set_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events);
> +#ifdef CONFIG_NVME_MULTIPATH
> +		if (ctrl->ana_log_buf)
> +			queue_work(nvme_wq, &ctrl->ana_work);
> +#endif
>   		nvme_queue_scan(ctrl);
>   		break;
>   	case NVME_AER_NOTICE_FW_ACT_STARTING:
> 

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[George, Martin]

On Wed, 2020-11-18 at 17:24 +0100, Christoph Hellwig wrote:
> On Wed, Nov 18, 2020 at 05:18:59PM +0530, Martin George wrote:
> > From: Martin George <marting@netapp.com>
> > 
> > The current nvme rescan triggered in response to a NS Attr Changed
> > AEN, fails to add a namespace if belonging to a new ANA group. This
> > is because ana_log_buf->ngrps is not updated in
> > nvme_parse_ana_log(),
> > due to which it is unable to locate the new ANA group's descriptor,
> > eventually resulting in the failure to add the namespace during the
> > rescan. Fix this by reading the ANA log page as part of ana_work so
> > that ngrps gets updated, prior to invoking nvme_queue_scan().
> 
> How can the namespace reference an ANA group that we haven't been
> notified about using the NVME_AEN_CFG_ANA_CHANGE AEN before?

Well, I think that could be possible for a newly created namespace,
which happens to belong to a new ANA group. To quote the 1.4 spec in
context of Asymmetric Namespace Access Change under the Asynchronous
Event Information - Notice (Figure 147):

"A controller shall not send this event if:
a) the change is due to the creation of a namespace (refer to section
5.20); or
b) the change is due to the deletion of a namespace (refer to section
5.20),
as the Namespace Attribute Changed event is sent for these changes."

So it looks the NVME_AEN_CFG_ANA_CHANGE AEN is not required here for
such cases, but instead the NVME_AEN_CFG_NS_ATTR AEN would do.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[George, Martin]

On Wed, 2020-11-18 at 08:51 -0800, Sagi Grimberg wrote:
> Fix this by reading the ANA log page as part of ana_work so
> > that ngrps gets updated, prior to invoking nvme_queue_scan().
> 
> Nothing guarantees ordering here..
> 

Ok. I suppose we may have to explicitly read the ANA log page then to
get this updated, in say nvme_mpath_add_disk() or somewhere else?

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[hch]

On Wed, Nov 18, 2020 at 08:09:43PM +0000, George, Martin wrote:
> > How can the namespace reference an ANA group that we haven't been
> > notified about using the NVME_AEN_CFG_ANA_CHANGE AEN before?
> 
> Well, I think that could be possible for a newly created namespace,
> which happens to belong to a new ANA group. To quote the 1.4 spec in
> context of Asymmetric Namespace Access Change under the Asynchronous
> Event Information - Notice (Figure 147):
> 
> "A controller shall not send this event if:
> a) the change is due to the creation of a namespace (refer to section
> 5.20); or
> b) the change is due to the deletion of a namespace (refer to section
> 5.20),
> as the Namespace Attribute Changed event is sent for these changes."
> 
> So it looks the NVME_AEN_CFG_ANA_CHANGE AEN is not required here for
> such cases, but instead the NVME_AEN_CFG_NS_ATTR AEN would do.

For the newly created namespace to beong to a newly created ANA group
the controller first needs to send NVME_AEN_CFG_ANA_CHANGE to announce
the ANA group, it then send NVME_AEN_CFG_NS_ATTR to announce the
new namespaces.  But the new namespace can't reference an ANA group
that didn't exist, that is a separate event.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Christoph Hellwig]

On Wed, Nov 18, 2020 at 08:51:30AM -0800, Sagi Grimberg wrote:
>
>> Fix this by reading the ANA log page as part of ana_work so
>> that ngrps gets updated, prior to invoking nvme_queue_scan().
>
> Nothing guarantees ordering here..

Yeah.  My memory is fading, but if we processed all events on the
same workqueue we'd be able to order processing of the ANA AEN that
needs to be sent first with the ns changed one that needs to come
later.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Meneghini, John]

On 11/20/20, 4:49 AM, "Linux-nvme on behalf of hch@lst.de" <linux-nvme-bounces@lists.infradead.org on behalf of hch@lst.de> wrote:

    On Wed, Nov 18, 2020 at 08:09:43PM +0000, George, Martin wrote:
    > > How can the namespace reference an ANA group that we haven't been
    > > notified about using the NVME_AEN_CFG_ANA_CHANGE AEN before?
    >
    > Well, I think that could be possible for a newly created namespace,
    > which happens to belong to a new ANA group. To quote the 1.4 spec in
    > context of Asymmetric Namespace Access Change under the Asynchronous
    > Event Information - Notice (Figure 147):
    >
    > "A controller shall not send this event if:
    > a) the change is due to the creation of a namespace (refer to section
    > 5.20); or
    > b) the change is due to the deletion of a namespace (refer to section
    > 5.20),
    > as the Namespace Attribute Changed event is sent for these changes."
    >
    > So it looks the NVME_AEN_CFG_ANA_CHANGE AEN is not required here for
    > such cases, but instead the NVME_AEN_CFG_NS_ATTR AEN would do.

    For the newly created namespace to beong to a newly created ANA group
    the controller first needs to send NVME_AEN_CFG_ANA_CHANGE to announce
    the ANA group, it then send NVME_AEN_CFG_NS_ATTR to announce the
    new namespaces.  But the new namespace can't reference an ANA group
    that didn't exist, that is a separate event.

I remember discussing this at length when we wrote this text for TP-4004.   We explicitly restricted  NVME_AEN_CFG_ANA_CHANGE AENs to ANA state change events, and ANAGRPID change events.  And we restricted NVME_AEN_CFG_NS_ATTR AENs so that ANA state/ANAGRPID change events would not generate an NVME_AEN_CFG_NS_ATTR.  We did this so as to avoid duplicate AENs. The discovery of new ANAGRP IDs was always supposed to take place as a part of namespace discovery.  

/John

Figure 49: Asynchronous Event Information - Notice

Namespace Attribute Changed:
...
A controller shall not send this event if:
   a) Namespace Utilization (refer to Figure 114) has changed, as this is a frequent event that does not require action by the host;
   b) the ANAGRPID field (refer to Figure 114) has changed; or
   c) capacity information (i.e., the NUSE field and the NVMCAP field) returned in the Identify Namespace Data Structure (refer to Figure 114) changed as a result of an ANA state change.
...

Asymmetric Namespace Access Change: 

The Asymmetric Namespace Access information (refer to section 5.14.1.12) related to an ANA Group that contains namespaces attached to this controller has changed (e.g., an ANA state has changed, an ANAGRPID has changed). The current Asymmetric Namespace Access information for attached namespaces is indicated in the Asymmetric Namespace Access log page (refer to section 5.14.1.12). To clear this event, the host issues a Get Log Page with Retain Asynchronous Event cleared to ‘0’ for the Asymmetric Namespace Access Log.

A controller shall not send this event if:
   a) the change is due to the creation of a namespace (refer to section 5.20); or
   b) the change is due to the deletion of a namespace (refer to section 5.20), as the Namespace Attribute Changed event is sent for these changes.

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

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

Here is what I would expect to:
A) create a namespace in existing ANA group:
	1) NS Attribute Changed AEN (because the NS List returned has changed)
	2) Host gets the list - discovers the NEW NS
		Including: Determine which ANAGRPID that NS belongs to (for which the host already knows the state).

B) create a namespace in a NEW ANA group
	1) NS Attribute Changed AEN (because the NS List returned has changed)
	2) Host gets the list - discovers the new NS
		Including: Determine there is a BRAND NEW ANAGRPID that the host knows NOTHING about.
		Since the host knows NOTHING about that ANAGRPID, it seems the host should go look.

It seems that the existing text says there is NO ANA AEN for a "new" namespace.  John quoted the text:

A controller shall not send this event if:
a)	the change is due to the creation of a namespace (refer to section 5.20); or
b)	the change is due to the deletion of a namespace (refer to section 5.20),
as the Namespace Attribute Changed event is sent for these changes.

Christoph claims that the ANA AEN should be send before the NS Attribute Changed AEN.  And, that could be true for an ANAGRPID that changed (although, I don't see text describing any precedence or order requirements for delivery of AENs), but the NS creation case is explicitly excluded.  FWIW – Christoph was one of the people that requested this exclusion to prevent spaming the host with multiple AENs for the same event (a NS create).

	Fred

-----Original Message-----
From: Meneghini, John <John.Meneghini@netapp.com> 
Sent: Monday, November 23, 2020 9:36 AM
To: hch@lst.de; George, Martin <Martin.George@netapp.com>
Cc: kbusch@kernel.org; linux-nvme@lists.infradead.org; sagi@grimberg.me; Meneghini, John <John.Meneghini@netapp.com>; Knight, Frederick <Frederick.Knight@netapp.com>; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

On 11/20/20, 4:49 AM, "Linux-nvme on behalf of mailto:hch@lst.de" <mailto:linux-nvme-bounces@lists.infradead.org on behalf of hch@lst.de> wrote:

    On Wed, Nov 18, 2020 at 08:09:43PM +0000, George, Martin wrote:
    > > How can the namespace reference an ANA group that we haven't been
    > > notified about using the NVME_AEN_CFG_ANA_CHANGE AEN before?
    >
    > Well, I think that could be possible for a newly created namespace,
    > which happens to belong to a new ANA group. To quote the 1.4 spec in
    > context of Asymmetric Namespace Access Change under the Asynchronous
    > Event Information - Notice (Figure 147):
    >
    > "A controller shall not send this event if:
    > a) the change is due to the creation of a namespace (refer to section
    > 5.20); or
    > b) the change is due to the deletion of a namespace (refer to section
    > 5.20),
    > as the Namespace Attribute Changed event is sent for these changes."
    >
    > So it looks the NVME_AEN_CFG_ANA_CHANGE AEN is not required here for
    > such cases, but instead the NVME_AEN_CFG_NS_ATTR AEN would do.

    For the newly created namespace to beong to a newly created ANA group
    the controller first needs to send NVME_AEN_CFG_ANA_CHANGE to announce
    the ANA group, it then send NVME_AEN_CFG_NS_ATTR to announce the
    new namespaces.  But the new namespace can't reference an ANA group
    that didn't exist, that is a separate event.

I remember discussing this at length when we wrote this text for TP-4004.   We explicitly restricted  NVME_AEN_CFG_ANA_CHANGE AENs to ANA state change events, and ANAGRPID change events.  And we restricted NVME_AEN_CFG_NS_ATTR AENs so that ANA state/ANAGRPID change events would not generate an NVME_AEN_CFG_NS_ATTR.  We did this so as to avoid duplicate AENs. The discovery of new ANAGRP IDs was always supposed to take place as a part of namespace discovery.  

/John

Figure 49: Asynchronous Event Information - Notice

Namespace Attribute Changed:
...
A controller shall not send this event if:
   a) Namespace Utilization (refer to Figure 114) has changed, as this is a frequent event that does not require action by the host;
   b) the ANAGRPID field (refer to Figure 114) has changed; or
   c) capacity information (i.e., the NUSE field and the NVMCAP field) returned in the Identify Namespace Data Structure (refer to Figure 114) changed as a result of an ANA state change.
...

Asymmetric Namespace Access Change: 

The Asymmetric Namespace Access information (refer to section 5.14.1.12) related to an ANA Group that contains namespaces attached to this controller has changed (e.g., an ANA state has changed, an ANAGRPID has changed). The current Asymmetric Namespace Access information for attached namespaces is indicated in the Asymmetric Namespace Access log page (refer to section 5.14.1.12). To clear this event, the host issues a Get Log Page with Retain Asynchronous Event cleared to ‘0’ for the Asymmetric Namespace Access Log.

A controller shall not send this event if:
   a) the change is due to the creation of a namespace (refer to section 5.20); or
   b) the change is due to the deletion of a namespace (refer to section 5.20), as the Namespace Attribute Changed event is sent for these changes.

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


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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Meneghini, John]

   On 11/23/20, 10:27 AM, "Knight, Frederick" <Frederick.Knight@netapp.com> wrote:

    Christoph claims that the ANA AEN should be send before the NS Attribute Changed AEN.  And, that could be true for an ANAGRPID 
    that changed (although, I don't see text describing any precedence or order requirements for delivery of AENs), but the NS creation
    case is explicitly excluded.  FWIW – Christoph was one of the people that requested this exclusion to prevent spaming the host with
    multiple AENs for the same event (a NS create).

An existing ANAGRPID which changes on an attached namespace should not produce an NVME_AEN_CFG_NS_ATTR AEN.  It produces an 
NVME_AEN_CFG_ANA_CHANGE AEN.  This is covered in: 

>  Namespace Attribute Changed: A controller shall not send this event if: b) the ANAGRPID field (refer to Figure 114) has changed;

  - and -

>  The Asymmetric Namespace Access information (refer to section 5.14.1.12) related to an ANA Group that contains namespaces attached to this controller
> has changed (e.g., an ANA state has changed, an ANAGRPID has changed). 

Although, I remember agreeing with Christoph that Linux would not support the "ANAGRPID has changed" AEN.

/John

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Fri, Nov 20, 2020 at 10:44:32AM +0100, hch@lst.de wrote:
> On Wed, Nov 18, 2020 at 08:09:43PM +0000, George, Martin wrote:
> > > How can the namespace reference an ANA group that we haven't been
> > > notified about using the NVME_AEN_CFG_ANA_CHANGE AEN before?
> > 
> > Well, I think that could be possible for a newly created namespace,
> > which happens to belong to a new ANA group. To quote the 1.4 spec in
> > context of Asymmetric Namespace Access Change under the Asynchronous
> > Event Information - Notice (Figure 147):
> > 
> > "A controller shall not send this event if:
> > a) the change is due to the creation of a namespace (refer to section
> > 5.20); or
> > b) the change is due to the deletion of a namespace (refer to section
> > 5.20),
> > as the Namespace Attribute Changed event is sent for these changes."
> > 
> > So it looks the NVME_AEN_CFG_ANA_CHANGE AEN is not required here for
> > such cases, but instead the NVME_AEN_CFG_NS_ATTR AEN would do.
> 
> For the newly created namespace to beong to a newly created ANA group
> the controller first needs to send NVME_AEN_CFG_ANA_CHANGE to announce
> the ANA group, it then send NVME_AEN_CFG_NS_ATTR to announce the
> new namespaces.  But the new namespace can't reference an ANA group
> that didn't exist, that is a separate event.

That sequence would have a group with no namespaces, which I is okay,
but we can tolerant targets that don't use that sequence without much
difficulty. This ought to do it:

---
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 74896be40c17..300cff8c616d 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -667,6 +667,8 @@ 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 {
+			nvme_read_ana_log(ctrl);
 		}
 	} else {
 		ns->ana_state = NVME_ANA_OPTIMIZED; 
--

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Sagi Grimberg]

>>>> How can the namespace reference an ANA group that we haven't been
>>>> notified about using the NVME_AEN_CFG_ANA_CHANGE AEN before?
>>>
>>> Well, I think that could be possible for a newly created namespace,
>>> which happens to belong to a new ANA group. To quote the 1.4 spec in
>>> context of Asymmetric Namespace Access Change under the Asynchronous
>>> Event Information - Notice (Figure 147):
>>>
>>> "A controller shall not send this event if:
>>> a) the change is due to the creation of a namespace (refer to section
>>> 5.20); or
>>> b) the change is due to the deletion of a namespace (refer to section
>>> 5.20),
>>> as the Namespace Attribute Changed event is sent for these changes."
>>>
>>> So it looks the NVME_AEN_CFG_ANA_CHANGE AEN is not required here for
>>> such cases, but instead the NVME_AEN_CFG_NS_ATTR AEN would do.
>>
>> For the newly created namespace to beong to a newly created ANA group
>> the controller first needs to send NVME_AEN_CFG_ANA_CHANGE to announce
>> the ANA group, it then send NVME_AEN_CFG_NS_ATTR to announce the
>> new namespaces.  But the new namespace can't reference an ANA group
>> that didn't exist, that is a separate event.
> 
> That sequence would have a group with no namespaces, which I is okay,
> but we can tolerant targets that don't use that sequence without much
> difficulty. This ought to do it:

This looks like a good simple solution to me...

> ---
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 74896be40c17..300cff8c616d 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -667,6 +667,8 @@ 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 {
> +			nvme_read_ana_log(ctrl);
>   		}
>   	} else {
>   		ns->ana_state = NVME_ANA_OPTIMIZED;
> --
> 

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[George, Martin]

On Mon, 2020-11-23 at 11:16 -0800, Sagi Grimberg wrote:
> How can the namespace reference an ANA group that we haven't
> > > > > been
> > > > > notified about using the NVME_AEN_CFG_ANA_CHANGE AEN before?
> > > > 
> > > > Well, I think that could be possible for a newly created
> > > > namespace,
> > > > which happens to belong to a new ANA group. To quote the 1.4
> > > > spec in
> > > > context of Asymmetric Namespace Access Change under the
> > > > Asynchronous
> > > > Event Information - Notice (Figure 147):
> > > > 
> > > > "A controller shall not send this event if:
> > > > a) the change is due to the creation of a namespace (refer to
> > > > section
> > > > 5.20); or
> > > > b) the change is due to the deletion of a namespace (refer to
> > > > section
> > > > 5.20),
> > > > as the Namespace Attribute Changed event is sent for these
> > > > changes."
> > > > 
> > > > So it looks the NVME_AEN_CFG_ANA_CHANGE AEN is not required
> > > > here for
> > > > such cases, but instead the NVME_AEN_CFG_NS_ATTR AEN would do.
> > > 
> > > For the newly created namespace to beong to a newly created ANA
> > > group
> > > the controller first needs to send NVME_AEN_CFG_ANA_CHANGE to
> > > announce
> > > the ANA group, it then send NVME_AEN_CFG_NS_ATTR to announce the
> > > new namespaces.  But the new namespace can't reference an ANA
> > > group
> > > that didn't exist, that is a separate event.
> > 
> > That sequence would have a group with no namespaces, which I is
> > okay,
> > but we can tolerant targets that don't use that sequence without
> > much
> > difficulty. This ought to do it:
> 
> This looks like a good simple solution to me...
> 
> > ---
> > diff --git a/drivers/nvme/host/multipath.c
> > b/drivers/nvme/host/multipath.c
> > index 74896be40c17..300cff8c616d 100644
> > --- a/drivers/nvme/host/multipath.c
> > +++ b/drivers/nvme/host/multipath.c
> > @@ -667,6 +667,8 @@ 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 {
> > +                     nvme_read_ana_log(ctrl);

Shouldn't this be nvme_read_ana_log(ns->ctrl)?

Yes, this should work and looks clean too.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Mon, Nov 23, 2020 at 07:29:05PM +0000, George, Martin wrote:
> On Mon, 2020-11-23 at 11:16 -0800, Sagi Grimberg wrote:
> > 
> > This looks like a good simple solution to me...
> > 
> > > ---
> > > diff --git a/drivers/nvme/host/multipath.c
> > > b/drivers/nvme/host/multipath.c
> > > index 74896be40c17..300cff8c616d 100644
> > > --- a/drivers/nvme/host/multipath.c
> > > +++ b/drivers/nvme/host/multipath.c
> > > @@ -667,6 +667,8 @@ 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 {
> > > +                     nvme_read_ana_log(ctrl);
> 
> Shouldn't this be nvme_read_ana_log(ns->ctrl)?

Oops, I just did a quick copy-paste. You are correct.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[George, Martin]

On Mon, 2020-11-23 at 11:43 -0800, Keith Busch wrote:
> On Mon, Nov 23, 2020 at 07:29:05PM +0000, George, Martin wrote:
> > On Mon, 2020-11-23 at 11:16 -0800, Sagi Grimberg wrote:
> > > This looks like a good simple solution to me...
> > > 
> > > > ---
> > > > diff --git a/drivers/nvme/host/multipath.c
> > > > b/drivers/nvme/host/multipath.c
> > > > index 74896be40c17..300cff8c616d 100644
> > > > --- a/drivers/nvme/host/multipath.c
> > > > +++ b/drivers/nvme/host/multipath.c
> > > > @@ -667,6 +667,8 @@ 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 {
> > > > +                     nvme_read_ana_log(ctrl);
> > 
> > Shouldn't this be nvme_read_ana_log(ns->ctrl)?
> 
> Oops, I just did a quick copy-paste. You are correct.

If there is no further feedback, could we move ahead with this fix? Or
does it need to be sent as a separate patch first?

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Hannes Reinecke]

On 11/26/20 5:16 PM, George, Martin wrote:
> On Mon, 2020-11-23 at 11:43 -0800, Keith Busch wrote:
>> On Mon, Nov 23, 2020 at 07:29:05PM +0000, George, Martin wrote:
>>> On Mon, 2020-11-23 at 11:16 -0800, Sagi Grimberg wrote:
>>>> This looks like a good simple solution to me...
>>>>
>>>>> ---
>>>>> diff --git a/drivers/nvme/host/multipath.c
>>>>> b/drivers/nvme/host/multipath.c
>>>>> index 74896be40c17..300cff8c616d 100644
>>>>> --- a/drivers/nvme/host/multipath.c
>>>>> +++ b/drivers/nvme/host/multipath.c
>>>>> @@ -667,6 +667,8 @@ 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 {
>>>>> +                     nvme_read_ana_log(ctrl);
>>>
>>> Shouldn't this be nvme_read_ana_log(ns->ctrl)?
>>
>> Oops, I just did a quick copy-paste. You are correct.
> 
> If there is no further feedback, could we move ahead with this fix? Or
> does it need to be sent as a separate patch first?
> 
I'm not _that_ happy with this approach, as we're doing I/O in the 
middle of an unrecoverable section; if reading the ANA log fails we have 
no way to recover, and end up with a namspace in an undefined state; 
actually it looks as if the namespace will then be presented to the OS
with no ANA information whatsoever, and we can just hope that it's not 
part of a multipathed namespace, otherwise we're screwed.

I'd rather hook in the ana log parsing into the AEN handling code, such 
that we're re-reading the ANA log for every NS CHANGED AEN before 
calling nvme_queue_scan(). That way we have a clear recovery path and 
won't need to worry about duplicate namespaces.

Patch to follow.

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-core: update NS Attr Changed AEN handling for ANA group

[hch]

Getting back to this while I'm reviewing the latest competing patches.

On Mon, Nov 23, 2020 at 03:27:35PM +0000, Knight, Frederick wrote:
> Here is what I would expect to:
> A) create a namespace in existing ANA group:
> 	1) NS Attribute Changed AEN (because the NS List returned has changed)
> 	2) Host gets the list - discovers the NEW NS
> 		Including: Determine which ANAGRPID that NS belongs to (for which the host already knows the state).

Yes.

> 
> B) create a namespace in a NEW ANA group
> 	1) NS Attribute Changed AEN (because the NS List returned has changed)
> 	2) Host gets the list - discovers the new NS
> 		Including: Determine there is a BRAND NEW ANAGRPID that the host knows NOTHING about.
> 		Since the host knows NOTHING about that ANAGRPID, it seems the host should go look.

I strongly disagree.

> It seems that the existing text says there is NO ANA AEN for a "new" namespace.  John quoted the text:
> 
> A controller shall not send this event if:
> a)	the change is due to the creation of a namespace (refer to section 5.20); or
> b)	the change is due to the deletion of a namespace (refer to section 5.20),
> as the Namespace Attribute Changed event is sent for these changes.

Exactly!  But only if the change is due the creation or deletion of
a namespaces.  Note the creation of both a namespace and a ANA group at
the same time.  In fact I don't see anything in NVMe which would allow
for this compound operations.  You always need to create an ANA group
first, and can then create a namespace using it, and you need to send the
Asymmetric Namespace Access Change AEN that a new ANA group has been
created, and then a Namespace Attribute Changed one.


> Christoph claims that the ANA AEN should be send before the NS Attribute Changed AEN.  And, that could be true for an ANAGRPID that changed (although, I don't see text describing any precedence or order requirements for delivery of AENs), but the NS creation case is explicitly excluded.  FWIW – Christoph was one of the people that requested this exclusion to prevent spaming the host with multiple AENs for the same event (a NS create).

Yes, and the exclusion as it exists in the spec makes perfect sense -
the host does not need two AEN to notify that a namespace has been created
and as part of the creation was added to an AEN group.  But the host does
need an AEN when a new group has been created, period.  There also is no
language that allows skipping that.

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

It is clear that the NS attribute change is required:
Namespace Attribute Changed: Indicates a change to one or both of the following:
•	... ; or
•	the Namespace List returned when the Identify command is issued with the CNS field set to 02h.

It seems clear to me that the ANA change IS prohibited:
Asymmetric Namespace Access Change: The Asymmetric Namespace Access information (refer to section 5.14.1.12) related to an ANA Group that contains namespaces attached to this controller has changed (e.g., an ANA state has changed, an ANAGRPID has changed). The current Asymmetric Namespace Access information for attached namespaces is indicated in the Asymmetric Namespace Access log page (refer to section 5.14.1.12).

We CANNOT deliver this AEN unless it is related to an ANA group that contains namespaces attached to this controller.  The creating of a new ANA group (which therefore has no namespaces) does not meet the criteria of "containing namespaces attached to this controller".  So there are 2 possible orders - 1) the ANA group is created before the namespace, or 2) the namespace is created before the namespace.  If the Group is created before the namespace, then there are NO attached namespaces in that (brand new) ANA group, therefore, we are prohibited from sending the AEN.  If the namespace is created before the ANA group, then we hit the next language, that prohibits sending the ANA changed AEN because is it "due to the creation of a namespace".

We are also prohibited from sending this AEN by the following language:
A controller shall not send this event if:
a)	the change is due to the creation of a namespace (refer to section 5.20); or
b)	the change is due to the deletion of a namespace (refer to section 5.20),
as the Namespace Attribute Changed event is sent for these changes.

Therefore, if the change "is due to the creating of a namespace" (as in sending a NS Management command to create a new NS, or other administrative action), then the ANA Change AEN shall not be sent.

So, there are TWO prohibitions, and ZERO requirements for sending an ANA Changed AEN for this situation.  Please show me the text that requires an ANA Changed AEN for this situation.  I just can't find it.  You're welcome to bring a new TPAR to add that requirement, but from my reading, it is a new requirement.

	Fred Knight


-----Original Message-----
From: hch@lst.de <hch@lst.de> 
Sent: Monday, December 7, 2020 10:26 AM
To: Knight, Frederick <Frederick.Knight@netapp.com>
Cc: Meneghini, John <John.Meneghini@netapp.com>; hch@lst.de; George, Martin <Martin.George@netapp.com>; kbusch@kernel.org; linux-nvme@lists.infradead.org; sagi@grimberg.me; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




Getting back to this while I'm reviewing the latest competing patches.

On Mon, Nov 23, 2020 at 03:27:35PM +0000, Knight, Frederick wrote:
> Here is what I would expect to:
> A) create a namespace in existing ANA group:
>       1) NS Attribute Changed AEN (because the NS List returned has changed)
>       2) Host gets the list - discovers the NEW NS
>               Including: Determine which ANAGRPID that NS belongs to (for which the host already knows the state).

Yes.

>
> B) create a namespace in a NEW ANA group
>       1) NS Attribute Changed AEN (because the NS List returned has changed)
>       2) Host gets the list - discovers the new NS
>               Including: Determine there is a BRAND NEW ANAGRPID that the host knows NOTHING about.
>               Since the host knows NOTHING about that ANAGRPID, it seems the host should go look.

I strongly disagree.

> It seems that the existing text says there is NO ANA AEN for a "new" namespace.  John quoted the text:
>
> A controller shall not send this event if:
> a)    the change is due to the creation of a namespace (refer to section 5.20); or
> b)    the change is due to the deletion of a namespace (refer to section 5.20),
> as the Namespace Attribute Changed event is sent for these changes.

Exactly!  But only if the change is due the creation or deletion of a namespaces.  Note the creation of both a namespace and a ANA group at the same time.  In fact I don't see anything in NVMe which would allow for this compound operations.  You always need to create an ANA group first, and can then create a namespace using it, and you need to send the Asymmetric Namespace Access Change AEN that a new ANA group has been created, and then a Namespace Attribute Changed one.


> Christoph claims that the ANA AEN should be send before the NS Attribute Changed AEN.  And, that could be true for an ANAGRPID that changed (although, I don't see text describing any precedence or order requirements for delivery of AENs), but the NS creation case is explicitly excluded.  FWIW – Christoph was one of the people that requested this exclusion to prevent spaming the host with multiple AENs for the same event (a NS create).

Yes, and the exclusion as it exists in the spec makes perfect sense - the host does not need two AEN to notify that a namespace has been created and as part of the creation was added to an AEN group.  But the host does need an AEN when a new group has been created, period.  There also is no language that allows skipping that.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Tue, Dec 08, 2020 at 03:21:16PM +0000, Knight, Frederick wrote:
> It is clear that the NS attribute change is required:
> Namespace Attribute Changed: Indicates a change to one or both of the following:
> •	... ; or
> •	the Namespace List returned when the Identify command is issued with the CNS field set to 02h.
> 
> It seems clear to me that the ANA change IS prohibited:
>
> Asymmetric Namespace Access Change: The Asymmetric Namespace Access
> information (refer to section 5.14.1.12) related to an ANA Group that
> contains namespaces attached to this controller has changed (e.g., an
> ANA state has changed, an ANAGRPID has changed). The current
> Asymmetric Namespace Access information for attached namespaces is
> indicated in the Asymmetric Namespace Access log page (refer to
> section 5.14.1.12).
> 
> We CANNOT deliver this AEN unless it is related to an ANA group that
> contains namespaces attached to this controller.  The creating of a
> new ANA group (which therefore has no namespaces) does not meet the
> criteria of "containing namespaces attached to this controller".  So
> there are 2 possible orders - 1) the ANA group is created before the
> namespace, or 2) the namespace is created before the namespace.  If
> the Group is created before the namespace, then there are NO attached
> namespaces in that (brand new) ANA group, therefore, we are prohibited
> from sending the AEN.  If the namespace is created before the ANA
> group, then we hit the next language, that prohibits sending the ANA
> changed AEN because is it "due to the creation of a namespace".

It can't be "1". If you make a group without a namespace, the host
couldn't read it anyway because the log only contains entries for groups
with namespaces attached to that controller.

As for "2", my understanding from reading this is that it refers to
suppressing the ANA event when a change in the "Number of NSID Values"
within a group occurs rather than a change in "Number of ANA Group
Descriptors" field.

As for text supporting it, we have 8.20.3.6:

  an Asymmetric Namespace Access Change Notice shall be sent by the
  controllers where the change occurred:
   ...
  c) upon entry to the following ANA states:

Your scenario has an ANA group that previously didn't exist entering a
state, so you shall send the event.

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

We can't send the AEN if we create the ANA Group before creating the namespace - because there is no attached NS to which we can send the AER.  And, we can't create a namespace in an ANA group unless the ANA group already exists.

OK, here is the text you are quoting:
8.20.3.6	Asymmetric Namespace Access Change Notifications
If Asymmetric Namespace Access Change Notices are enabled (refer to section 5.21.1.11) on a controller, then an Asymmetric Namespace Access Change Notice shall be sent by the controllers where the change occurred:
a)	if an ANA Group Identifier (refer to Figure 251) changes;
b)	if an asymmetric namespace access state transition fails (e.g., a transition begins, but does not complete and the controller returns to the state that existed before the transition began); or
c)	upon entry to the following ANA states:
•	ANA Optimized State;
•	ANA Non-Optimized State;
•	ANA Inaccessible State; and
•	ANA Persistent Loss State.

So, what you're saying is that storage determining the initial state is a TRANSITION.  So that would also mean that after every connection, you would get a FLOOD of ANA Changed AENs as we discover the initial state of each ANA group, and TRANSITION that group from <unknown> to optimized state (or one of those 4 states).

That honestly makes no sense.  You really want all those AENs every time you connect to a new controller?

Discovery by the host of a new namespace should be the same if it happens at boot, or it happens later when a new namespace is discovered.  It seems like you're asking for 2 different discovery processes.  For some period after a new connection, you don't want the AENs telling you the ANA group entered its initial state, but after that time is up, then you DO want AENs to tell you when an ANA group enters its initial state.

The text never mentions anything about changes to "Number of ANA Group Descriptors" field and any relationship with the AEN, so I have no idea where you're getting that from.  The text does explicitly say that if the ANA changes was due to the creation of a namespace - if the ANA log pages changes because of a namespace being created, then there is NO AEN for that change.

A controller shall not send this event if:
a)	the change is due to the creation of a namespace (refer to section 5.20); or
b)	the change is due to the deletion of a namespace (refer to section 5.20),

	Fred

-----Original Message-----
From: Keith Busch <kbusch@kernel.org> 
Sent: Tuesday, December 8, 2020 12:41 PM
To: Knight, Frederick <Frederick.Knight@netapp.com>
Cc: hch@lst.de; Meneghini, John <John.Meneghini@netapp.com>; George, Martin <Martin.George@netapp.com>; linux-nvme@lists.infradead.org; sagi@grimberg.me; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




On Tue, Dec 08, 2020 at 03:21:16PM +0000, Knight, Frederick wrote:
> It is clear that the NS attribute change is required:
> Namespace Attribute Changed: Indicates a change to one or both of the following:
> •     ... ; or
> •     the Namespace List returned when the Identify command is issued with the CNS field set to 02h.
>
> It seems clear to me that the ANA change IS prohibited:
>
> Asymmetric Namespace Access Change: The Asymmetric Namespace Access 
> information (refer to section 5.14.1.12) related to an ANA Group that 
> contains namespaces attached to this controller has changed (e.g., an 
> ANA state has changed, an ANAGRPID has changed). The current 
> Asymmetric Namespace Access information for attached namespaces is 
> indicated in the Asymmetric Namespace Access log page (refer to 
> section 5.14.1.12).
>
> We CANNOT deliver this AEN unless it is related to an ANA group that 
> contains namespaces attached to this controller.  The creating of a 
> new ANA group (which therefore has no namespaces) does not meet the 
> criteria of "containing namespaces attached to this controller".  So 
> there are 2 possible orders - 1) the ANA group is created before the 
> namespace, or 2) the namespace is created before the namespace.  If 
> the Group is created before the namespace, then there are NO attached 
> namespaces in that (brand new) ANA group, therefore, we are prohibited 
> from sending the AEN.  If the namespace is created before the ANA 
> group, then we hit the next language, that prohibits sending the ANA 
> changed AEN because is it "due to the creation of a namespace".

It can't be "1". If you make a group without a namespace, the host couldn't read it anyway because the log only contains entries for groups with namespaces attached to that controller.

As for "2", my understanding from reading this is that it refers to suppressing the ANA event when a change in the "Number of NSID Values"
within a group occurs rather than a change in "Number of ANA Group Descriptors" field.

As for text supporting it, we have 8.20.3.6:

  an Asymmetric Namespace Access Change Notice shall be sent by the
  controllers where the change occurred:
   ...
  c) upon entry to the following ANA states:

Your scenario has an ANA group that previously didn't exist entering a state, so you shall send the event.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Tue, Dec 08, 2020 at 06:15:08PM +0000, Knight, Frederick wrote:
> We can't send the AEN if we create the ANA Group before creating the namespace - because there is no attached NS to which we can send the AER.  And, we can't create a namespace in an ANA group unless the ANA group already exists.
> 
> OK, here is the text you are quoting:
> 8.20.3.6	Asymmetric Namespace Access Change Notifications
> If Asymmetric Namespace Access Change Notices are enabled (refer to section 5.21.1.11) on a controller, then an Asymmetric Namespace Access Change Notice shall be sent by the controllers where the change occurred:
> a)	if an ANA Group Identifier (refer to Figure 251) changes;
> b)	if an asymmetric namespace access state transition fails (e.g., a transition begins, but does not complete and the controller returns to the state that existed before the transition began); or
> c)	upon entry to the following ANA states:
> •	ANA Optimized State;
> •	ANA Non-Optimized State;
> •	ANA Inaccessible State; and
> •	ANA Persistent Loss State.
> 
> So, what you're saying is that storage determining the initial state
> is a TRANSITION.  So that would also mean that after every connection,
> you would get a FLOOD of ANA Changed AENs as we discover the initial
> state of each ANA group, and TRANSITION that group from <unknown> to
> optimized state (or one of those 4 states).
>
> That honestly makes no sense.  You really want all those AENs every
> time you connect to a new controller?

Why on would you get a flood of ANA on a connection? You haven't even
got an outstanding AEN command to respond to upon controller connection,
nor has the AEN feature configuration been set on connection either.
The initial reading of the log clears all AEN events of that type with a
single read anyway, so you don't send any AEN responses for this.

Now, if the host were to enable ANA events and send an AEN before it
reads the log, then sure, you might send a single AEN response to notify
the host that the ANA log has data the host hasn't read yet.
 
> Discovery by the host of a new namespace should be the same if it
> happens at boot, or it happens later when a new namespace is
> discovered.  It seems like you're asking for 2 different discovery
> processes.  For some period after a new connection, you don't want the
> AENs telling you the ANA group entered its initial state, but after
> that time is up, then you DO want AENs to tell you when an ANA group
> enters its initial state.

The event is latched by the host reading the log with RAE bit. This
isn't some arbitrary temporal descision.

> The text never mentions anything about changes to "Number of ANA Group
> Descriptors" field and any relationship with the AEN, so I have no
> idea where you're getting that from.  The text does explicitly say
> that if the ANA changes was due to the creation of a namespace - if
> the ANA log pages changes because of a namespace being created, then
> there is NO AEN for that change.

I'm only saying the text says a controller that has events enabled
affected from a group entering an ANA state "shall" send an event. A
controller with a group having no state to now having a state certainly
sounds like that controller has an ANA group entering a state.

> A controller shall not send this event if:
> a)	the change is due to the creation of a namespace (refer to section 5.20); or
> b)	the change is due to the deletion of a namespace (refer to section 5.20),

The text also says the controller shall send the event when a group
enters a state. There are no exceptions listed in that section.

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

Setting up a new ANA Group does NOT cause a transition from <UNKNOWN state> to <some known state>.  The establishment of initial state is NOT a change of state as defined in section 8.20.3.6.

An ANA AEN is prohibited if the ANA change is due to the creation of a namespace.

If you want to change the text from the original requirements and design, then please bring a TPAR to make that change.

	Fred

-----Original Message-----
From: Keith Busch <kbusch@kernel.org> 
Sent: Tuesday, December 8, 2020 1:35 PM
To: Knight, Frederick <Frederick.Knight@netapp.com>
Cc: hch@lst.de; Meneghini, John <John.Meneghini@netapp.com>; George, Martin <Martin.George@netapp.com>; linux-nvme@lists.infradead.org; sagi@grimberg.me; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




On Tue, Dec 08, 2020 at 06:15:08PM +0000, Knight, Frederick wrote:
> We can't send the AEN if we create the ANA Group before creating the namespace - because there is no attached NS to which we can send the AER.  And, we can't create a namespace in an ANA group unless the ANA group already exists.
>
> OK, here is the text you are quoting:
> 8.20.3.6      Asymmetric Namespace Access Change Notifications
> If Asymmetric Namespace Access Change Notices are enabled (refer to section 5.21.1.11) on a controller, then an Asymmetric Namespace Access Change Notice shall be sent by the controllers where the change occurred:
> a)    if an ANA Group Identifier (refer to Figure 251) changes;
> b)    if an asymmetric namespace access state transition fails (e.g., a transition begins, but does not complete and the controller returns to the state that existed before the transition began); or
> c)    upon entry to the following ANA states:
> •     ANA Optimized State;
> •     ANA Non-Optimized State;
> •     ANA Inaccessible State; and
> •     ANA Persistent Loss State.
>
> So, what you're saying is that storage determining the initial state 
> is a TRANSITION.  So that would also mean that after every connection, 
> you would get a FLOOD of ANA Changed AENs as we discover the initial 
> state of each ANA group, and TRANSITION that group from <unknown> to 
> optimized state (or one of those 4 states).
>
> That honestly makes no sense.  You really want all those AENs every 
> time you connect to a new controller?

Why on would you get a flood of ANA on a connection? You haven't even got an outstanding AEN command to respond to upon controller connection, nor has the AEN feature configuration been set on connection either.
The initial reading of the log clears all AEN events of that type with a single read anyway, so you don't send any AEN responses for this.

Now, if the host were to enable ANA events and send an AEN before it reads the log, then sure, you might send a single AEN response to notify the host that the ANA log has data the host hasn't read yet.

> Discovery by the host of a new namespace should be the same if it 
> happens at boot, or it happens later when a new namespace is 
> discovered.  It seems like you're asking for 2 different discovery 
> processes.  For some period after a new connection, you don't want the 
> AENs telling you the ANA group entered its initial state, but after 
> that time is up, then you DO want AENs to tell you when an ANA group 
> enters its initial state.

The event is latched by the host reading the log with RAE bit. This isn't some arbitrary temporal descision.

> The text never mentions anything about changes to "Number of ANA Group 
> Descriptors" field and any relationship with the AEN, so I have no 
> idea where you're getting that from.  The text does explicitly say 
> that if the ANA changes was due to the creation of a namespace - if 
> the ANA log pages changes because of a namespace being created, then 
> there is NO AEN for that change.

I'm only saying the text says a controller that has events enabled affected from a group entering an ANA state "shall" send an event. A controller with a group having no state to now having a state certainly sounds like that controller has an ANA group entering a state.

> A controller shall not send this event if:
> a)    the change is due to the creation of a namespace (refer to section 5.20); or
> b)    the change is due to the deletion of a namespace (refer to section 5.20),

The text also says the controller shall send the event when a group enters a state. There are no exceptions listed in that section.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Tue, Dec 08, 2020 at 07:13:00PM +0000, Knight, Frederick wrote:
> Setting up a new ANA Group does NOT cause a transition from <UNKNOWN state> to <some known state>.  The establishment of initial state is NOT a change of state as defined in section 8.20.3.6.
> 
> An ANA AEN is prohibited if the ANA change is due to the creation of a namespace.

Yes, the AEN is prohibited if the ANA change is due to the creation of a
namespace. In this scenario, though, the ANA change is due to a group
entering a state, and you're required to notify that as long as ANA
events are enabled. There is no text in section 8.20.3.6 or anywhere
else that I can find that says creating a group with a valid state
disqualifies it as "entering" that state. However, I do find that
section 8.20.3.5 says transitions may occur from states not visible to
the host, so <UNKNOWN state> to <known state> is a transition according
to the spec.

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

If you want to make that a transition, then bring a TPAR to change it.

When ANA Groups are created, they already have a state.  There is NO transition; there is NO AEN.

	Fred

-----Original Message-----
From: Keith Busch <kbusch@kernel.org> 
Sent: Tuesday, December 8, 2020 4:47 PM
To: Knight, Frederick <Frederick.Knight@netapp.com>
Cc: hch@lst.de; Meneghini, John <John.Meneghini@netapp.com>; George, Martin <Martin.George@netapp.com>; linux-nvme@lists.infradead.org; sagi@grimberg.me; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




On Tue, Dec 08, 2020 at 07:13:00PM +0000, Knight, Frederick wrote:
> Setting up a new ANA Group does NOT cause a transition from <UNKNOWN state> to <some known state>.  The establishment of initial state is NOT a change of state as defined in section 8.20.3.6.
>
> An ANA AEN is prohibited if the ANA change is due to the creation of a namespace.

Yes, the AEN is prohibited if the ANA change is due to the creation of a namespace. In this scenario, though, the ANA change is due to a group entering a state, and you're required to notify that as long as ANA events are enabled. There is no text in section 8.20.3.6 or anywhere else that I can find that says creating a group with a valid state disqualifies it as "entering" that state. However, I do find that section 8.20.3.5 says transitions may occur from states not visible to the host, so <UNKNOWN state> to <known state> is a transition according to the spec.

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

Here are the authors of the original TP:

Technical Proposal Author(s)
Name	Company
Fred Knight	NetApp
David Black	Dell EMC
Curtis Ballard	HPE
Christoph Hellwig	WDC

Myself, David Black, Curtis Ballard and the rest of the weekly FMDS meeting all agreed that an AEN is prohibited during this circumstance ("due to the creation of a namespace").  All three agree that when ANA groups are created, they already have a state and there is NO transition.  They all agree if you want to change that, you need to bring a TPAR to change it.

The fourth author, Christoph, also stated during the TP development that he did NOT want 2 AENs when a namespace was created.  He specifically required there be only 1.

	Fred

-----Original Message-----
From: Keith Busch <kbusch@kernel.org> 
Sent: Tuesday, December 8, 2020 4:47 PM
To: Knight, Frederick <Frederick.Knight@netapp.com>
Cc: hch@lst.de; Meneghini, John <John.Meneghini@netapp.com>; George, Martin <Martin.George@netapp.com>; linux-nvme@lists.infradead.org; sagi@grimberg.me; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




On Tue, Dec 08, 2020 at 07:13:00PM +0000, Knight, Frederick wrote:
> Setting up a new ANA Group does NOT cause a transition from <UNKNOWN state> to <some known state>.  The establishment of initial state is NOT a change of state as defined in section 8.20.3.6.
>
> An ANA AEN is prohibited if the ANA change is due to the creation of a namespace.

Yes, the AEN is prohibited if the ANA change is due to the creation of a namespace. In this scenario, though, the ANA change is due to a group entering a state, and you're required to notify that as long as ANA events are enabled. There is no text in section 8.20.3.6 or anywhere else that I can find that says creating a group with a valid state disqualifies it as "entering" that state. However, I do find that section 8.20.3.5 says transitions may occur from states not visible to the host, so <UNKNOWN state> to <known state> is a transition according to the spec.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Wed, Dec 09, 2020 at 12:07:08AM +0000, Knight, Frederick wrote:
> If you want to make that a transition, then bring a TPAR to change it.
>
> When ANA Groups are created, they already have a state.  There is NO transition; there is NO AEN.

The requirement doesn't even mention the word "transition", so it's a
moot point. The requirement says "upon entry". Whether that entry occurs
because of a transition to it or because it sprung into existence
already having entered that state is not a disticition made in the spec.
If you want that disticition to be written in spec, then feel free to
bring an ECN to clarify it.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Hannes Reinecke]

On 12/9/20 1:20 AM, Keith Busch wrote:
> On Wed, Dec 09, 2020 at 12:07:08AM +0000, Knight, Frederick wrote:
>> If you want to make that a transition, then bring a TPAR to change it.
>>
>> When ANA Groups are created, they already have a state.  There is NO transition; there is NO AEN.
> 
> The requirement doesn't even mention the word "transition", so it's a
> moot point. The requirement says "upon entry". Whether that entry occurs
> because of a transition to it or because it sprung into existence
> already having entered that state is not a disticition made in the spec.
> If you want that disticition to be written in spec, then feel free to
> bring an ECN to clarify it.
> 
What are you discussing here?
The original situation was that a namespace was created within an 
existing subsystem and existing ANA groups and namespaces, where the new 
namespace was _not_ part of any of the existing ANA groups.
And you already had agreed that a namespace _creation_ (which this most 
definitely is) should not send ANA AENs.

So where is the 'transition' here? Is this due to the fact the ANA group 
creation and namespace creation might be two distinct events?

Clearly, the ANA group has to be created at one point.
And namespace creation might be happening at a different time, true; but 
they might also be a single event.
If it's a single event then I think we've already agreed that ANA change 
AENs will be suppressed.

I don't think it'll be possible to create an ANA group _after_ creating 
a namespace, as the definition for the ANAGRPID field in the 'namespace 
identify' data structure says:

   This field indicates the ANA Group Identifier of the ANA group
  (refer to section 8.20.2) of which the namespace is a member.
  Each namespace that is attached to a controller that supports
  Asymmetric Namespace Access Reporting (refer to the CMIC field)
  shall report a valid ANAGRPID.

Which would require us to create the ANA group prior to the namespace.
But then we cannot report the empty ANA group as per definition of the 
ANA log page:

  Number of ANA Group Descriptors: This field indicates the number
  of ANA Group Descriptors available in the log page. The log page
  shall contain one ANA Group Descriptor for each ANA Group that
  contains Namespaces that are attached to the controller.

So even if you are right and we would have to expect an ANA AEN, the log 
page would have no way of reporting it. And we would end up in exactly 
the same situation as we are now, namely having to re-read the ANA log 
page after namespace creation.

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-core: update NS Attr Changed AEN handling for ANA group

[hch]

Fred, stop this bullshit now!  I'm sick and tired of you misrepresenting my
opinion again and again despite me correcting you.  This is not a basis for
productive work.

The only thing I requested is what is said in the document:

"A controller shall not send this event if:

  a) the change is due to the creation of a namespace (refer to section 5.20); or
  b) the change is due to the deletion of a namespace (refer to section 5.20),

as the Namespace Attribute Changed event is sent for these changes."

The Namespace Attribute Changed event has no way of notifying the host
of a creation of new ANA group, so strangely interpreting that entirely
different event to be part of the namespace creation does not make any
sense whatsover.

Creating the group at the spec level is not due to creating a namespace.
That might be the interpretation of your management backend and is an
ok implementation, but nothing in the spec implicitly creates groups as
part of namespaces creation.

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

Then I have no idea what you're asking.

Creating an ANA group does not change the ANA Log page, so there is NO AEN.

When a namespace is created (and contained in an ANA Group), the act of creating that namespace results in a change to the ANA Log page - therefore AEN.  BUT, the text you site specifically says, ONLY the Namespace Attribute Changed event is sent, and the ANA Log page change AEN shall not be sent when this happens.  This is what is described when either the ANA group already exists (and the new namespace is added to it), or when the ANA group does not exist, and the new ANA group along with the new namespace are both added to the ANA log page at the same time.

If you are not asking for an AEN when an ANA group is created, then what are you asking for?  Because I clearly don't understand your ask.

	Fred

-----Original Message-----
From: hch@lst.de <hch@lst.de> 
Sent: Wednesday, December 9, 2020 2:28 AM
To: Knight, Frederick <Frederick.Knight@netapp.com>
Cc: Keith Busch <kbusch@kernel.org>; hch@lst.de; Meneghini, John <John.Meneghini@netapp.com>; George, Martin <Martin.George@netapp.com>; linux-nvme@lists.infradead.org; sagi@grimberg.me; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




Fred, stop this bullshit now!  I'm sick and tired of you misrepresenting my opinion again and again despite me correcting you.  This is not a basis for productive work.

The only thing I requested is what is said in the document:

"A controller shall not send this event if:

  a) the change is due to the creation of a namespace (refer to section 5.20); or
  b) the change is due to the deletion of a namespace (refer to section 5.20),

as the Namespace Attribute Changed event is sent for these changes."

The Namespace Attribute Changed event has no way of notifying the host of a creation of new ANA group, so strangely interpreting that entirely different event to be part of the namespace creation does not make any sense whatsover.

Creating the group at the spec level is not due to creating a namespace.
That might be the interpretation of your management backend and is an ok implementation, but nothing in the spec implicitly creates groups as part of namespaces creation.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Wed, Dec 09, 2020 at 03:20:57PM +0000, Knight, Frederick wrote:
> When a namespace is created (and contained in an ANA Group), the act
> of creating that namespace results in a change to the ANA Log page -
> therefore AEN.  

That is wrong: creating a namespace doesn't do anything to the ANA log.
Only attached namespaces appear in the log. Creating a namespace
doesn't result in automatically attaching it to any controller.

You are thinking of Namespace Attach from section 5.19, and the spec
doesn't define any special exceptions for events resulting from *that*
command, which is also the command that triggered this thread.

If you want to claim that was a mistake in the text (I would agree it is
a mistake), then take it to the technical reflector rather than this
forum. The spec as currently written doesn't align with you.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Hannes Reinecke]

On 12/9/20 4:53 PM, Keith Busch wrote:
> On Wed, Dec 09, 2020 at 03:20:57PM +0000, Knight, Frederick wrote:
>> When a namespace is created (and contained in an ANA Group), the act
>> of creating that namespace results in a change to the ANA Log page -
>> therefore AEN.
> 
> That is wrong: creating a namespace doesn't do anything to the ANA log.
> Only attached namespaces appear in the log. Creating a namespace
> doesn't result in automatically attaching it to any controller.
> 
> You are thinking of Namespace Attach from section 5.19, and the spec
> doesn't define any special exceptions for events resulting from *that*
> command, which is also the command that triggered this thread.
> 
Actually, no, we're not thinking about 'Namespace Attach'.
This is a normal namespace creation on the controller side.

I do agree that 'Namespace Attach' would have different constrains and 
rules under which AENs will be sent, but this is not what our initial 
use-case was about.

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-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Wed, Dec 09, 2020 at 05:02:05PM +0100, Hannes Reinecke wrote:
> On 12/9/20 4:53 PM, Keith Busch wrote:
> > On Wed, Dec 09, 2020 at 03:20:57PM +0000, Knight, Frederick wrote:
> > > When a namespace is created (and contained in an ANA Group), the act
> > > of creating that namespace results in a change to the ANA Log page -
> > > therefore AEN.
> > 
> > That is wrong: creating a namespace doesn't do anything to the ANA log.
> > Only attached namespaces appear in the log. Creating a namespace
> > doesn't result in automatically attaching it to any controller.
> > 
> > You are thinking of Namespace Attach from section 5.19, and the spec
> > doesn't define any special exceptions for events resulting from *that*
> > command, which is also the command that triggered this thread.
> > 
> Actually, no, we're not thinking about 'Namespace Attach'.

This patch is the result from an Attach command, otherwise the host
driver wouldn't have found an unknown ANA group for a new namespace
during the rescan.

> This is a normal namespace creation on the controller side.
>
> I do agree that 'Namespace Attach' would have different constrains and rules
> under which AENs will be sent, but this is not what our initial use-case was
> about.

The use case in this patch is about getting the host to recognize the
ANA group of a newly attached namespace. If you stop at namespace
creation, then this patch doesn't do anything.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Hannes Reinecke]

On 12/9/20 5:19 PM, Keith Busch wrote:
> On Wed, Dec 09, 2020 at 05:02:05PM +0100, Hannes Reinecke wrote:
>> On 12/9/20 4:53 PM, Keith Busch wrote:
>>> On Wed, Dec 09, 2020 at 03:20:57PM +0000, Knight, Frederick wrote:
>>>> When a namespace is created (and contained in an ANA Group), the act
>>>> of creating that namespace results in a change to the ANA Log page -
>>>> therefore AEN.
>>>
>>> That is wrong: creating a namespace doesn't do anything to the ANA log.
>>> Only attached namespaces appear in the log. Creating a namespace
>>> doesn't result in automatically attaching it to any controller.
>>>
>>> You are thinking of Namespace Attach from section 5.19, and the spec
>>> doesn't define any special exceptions for events resulting from *that*
>>> command, which is also the command that triggered this thread.
>>>
>> Actually, no, we're not thinking about 'Namespace Attach'.
> 
> This patch is the result from an Attach command, otherwise the host
> driver wouldn't have found an unknown ANA group for a new namespace
> during the rescan.
> 
>> This is a normal namespace creation on the controller side.
>>
>> I do agree that 'Namespace Attach' would have different constrains and rules
>> under which AENs will be sent, but this is not what our initial use-case was
>> about.
> 
> The use case in this patch is about getting the host to recognize the
> ANA group of a newly attached namespace. If you stop at namespace
> creation, then this patch doesn't do anything.
> 
Now you got me confused.
Of course the namespace is attached to a controller, but this is _not_ 
the result of a 'namespace attach' command from the host.
If that's what you imply.
Not sure, though; as said, I'm confused now.

The use-case I'm trying to describe is that the admin on the storage 
array is creating a namespace and attaching it to an existing subsystem, 
completely without interaction from the host. It just so happens that 
this new namespace has a different ANA group than the existing 
namespaces in this subsystem.
Then the array has to notify the host about this.
And the whole discussion is about which AENs this controller/storage 
array should be sending.

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-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Wed, Dec 09, 2020 at 06:04:29PM +0100, Hannes Reinecke wrote:
> On 12/9/20 5:19 PM, Keith Busch wrote:
> > On Wed, Dec 09, 2020 at 05:02:05PM +0100, Hannes Reinecke wrote:
> > > On 12/9/20 4:53 PM, Keith Busch wrote:
> > > > On Wed, Dec 09, 2020 at 03:20:57PM +0000, Knight, Frederick wrote:
> > > > > When a namespace is created (and contained in an ANA Group), the act
> > > > > of creating that namespace results in a change to the ANA Log page -
> > > > > therefore AEN.
> > > > 
> > > > That is wrong: creating a namespace doesn't do anything to the ANA log.
> > > > Only attached namespaces appear in the log. Creating a namespace
> > > > doesn't result in automatically attaching it to any controller.
> > > > 
> > > > You are thinking of Namespace Attach from section 5.19, and the spec
> > > > doesn't define any special exceptions for events resulting from *that*
> > > > command, which is also the command that triggered this thread.
> > > > 
> > > Actually, no, we're not thinking about 'Namespace Attach'.
> > 
> > This patch is the result from an Attach command, otherwise the host
> > driver wouldn't have found an unknown ANA group for a new namespace
> > during the rescan.
> > 
> > > This is a normal namespace creation on the controller side.
> > > 
> > > I do agree that 'Namespace Attach' would have different constrains and rules
> > > under which AENs will be sent, but this is not what our initial use-case was
> > > about.
> > 
> > The use case in this patch is about getting the host to recognize the
> > ANA group of a newly attached namespace. If you stop at namespace
> > creation, then this patch doesn't do anything.
> > 
> Now you got me confused.
> Of course the namespace is attached to a controller, but this is _not_ the
> result of a 'namespace attach' command from the host.
> If that's what you imply.
> Not sure, though; as said, I'm confused now.
> 
> The use-case I'm trying to describe is that the admin on the storage array
> is creating a namespace and attaching it to an existing subsystem,
> completely without interaction from the host. It just so happens that this
> new namespace has a different ANA group than the existing namespaces in this
> subsystem.
> Then the array has to notify the host about this.
> And the whole discussion is about which AENs this controller/storage array
> should be sending.

Fred keeps saying the spec's rules for NVMe's namespace create command
from section 5.20 allow him to not send events, but it turns out you're
not even using this command? Why would the spec's defined behavior apply
to this proprietary use case?

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[hch]

On Thu, Dec 10, 2020 at 02:39:36AM +0900, Keith Busch wrote:
> > The use-case I'm trying to describe is that the admin on the storage array
> > is creating a namespace and attaching it to an existing subsystem,
> > completely without interaction from the host. It just so happens that this
> > new namespace has a different ANA group than the existing namespaces in this
> > subsystem.
> > Then the array has to notify the host about this.
> > And the whole discussion is about which AENs this controller/storage array
> > should be sending.
> 
> Fred keeps saying the spec's rules for NVMe's namespace create command
> from section 5.20 allow him to not send events, but it turns out you're
> not even using this command? Why would the spec's defined behavior apply
> to this proprietary use case?

I think we are in an even deeper mess here than I though.

 - one issue is the fact that the exception gets creating vs attaching
   and deleting vs detaching wrong, and that probably is my fault.
 - the other one is the wording should be shall not send the event
   when the creation is ONLY due to the creation of a namespace, that
   is not when a new ANA group shows up (and the way how ANA groups
   are created is out of scope)

So if we do a textual interpretation of the spec, the controller does
not only need to send the ANA AEN for the case we are debatting here,
but also for the case I specifically wanted to exclude.  Sigh.

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

"Deeper mess" - I do not know, but this DOES sort out the misunderstandings.  Whether it does what we want is a different question.

Create and attach are discrete actions in NVMe.  VS management tools may have a single command that invokes both actions (making it appear they are a single action, but they are not when it comes to how NVMe describes what happens):

1) Creation of a namespace (no matter how it happens) adds a namespace ID to IDENTIFY (CNS=10h).  This action does NOT change IDENTIFY (CNS=2h).
If we look at the AENs, the Namespace Attribute Changed AEN happens for a change to one or both of the following:
a) the Identify Namespace Data Structure for one or more namespaces; or
b) the Namespace List returned for IDENTIFY (CNS=2h).

Clearly, a create NS changes CNS=10h, and does NOT change CNS=02h, so b) isn't happening.  As for a), that is a little more murky.  Consider, that CNS=2h (attached namespaces) is specifically mentioned, and CNS=10h is specifically absent. Does that suggest the intention was to only send the AEN when something related to an ATTACHED namespaces changes?  Well, the exact text of a) includes CNS=11h.  CNS=11h does contain an Identify Namespace Data Structure, and it DOES change when you create a namespace (although, the namespace is only allocated, and not attached).  So was the intent to send an AEN for the creation of an allocated NS, or just for an attached NS?

What happens to the ANA Log page when a Namespace is created - nothing, therefore NO AEN about changes to that log page (it is only about attached namespaces).

Neither the Identify Namespace data changed AEN, nor the CNS=11h exist prior to rev 1.2.  They were added as part of 1.2 development.  I have not yet been able to dig out the history to see if there are any hints as to the original intent.  Is the a) item supposed to be only about CNS = 0h, or is it intended to apply to CNS = 11h too? When we invented  CNS = 11h, did we forget to exclude that from a) (did we forget to add the word "attached" to a))? OR, was it intentional?

2) Attach of a namespace (no matter how it happens) adds a namespace ID to IDENTIFY (CNS=02h).  This action DOES change IDENTIFY (CNS=0h and CNS=2h). That means the Identify Namespace Attribute Changed AEN is DOES happen (case b) above, and case a) above (CNS = 0h changes)).

In this case, the ANA Log page has a new entry added (either adding just 1 NSID - to an existing ANA Group, or adding a new ANA Group that contains the newly attached NS).  Either way, the Log page changes, and that means the ANA Change AEN happens.

So for case 2 (the attach), there are 2 AENs for effectively the same event.  This is where we are at with the existing text.

Whether this is what the hosts want, that is a good question. What changes are desired (if any)?

	Fred

-----Original Message-----
From: hch@lst.de <hch@lst.de> 
Sent: Wednesday, December 9, 2020 12:47 PM
To: Keith Busch <kbusch@kernel.org>
Cc: Hannes Reinecke <hare@suse.de>; Knight, Frederick <Frederick.Knight@netapp.com>; hch@lst.de; Meneghini, John <John.Meneghini@netapp.com>; George, Martin <Martin.George@netapp.com>; linux-nvme@lists.infradead.org; sagi@grimberg.me; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




On Thu, Dec 10, 2020 at 02:39:36AM +0900, Keith Busch wrote:
> > The use-case I'm trying to describe is that the admin on the storage 
> > array is creating a namespace and attaching it to an existing 
> > subsystem, completely without interaction from the host. It just so 
> > happens that this new namespace has a different ANA group than the 
> > existing namespaces in this subsystem.
> > Then the array has to notify the host about this.
> > And the whole discussion is about which AENs this controller/storage 
> > array should be sending.
>
> Fred keeps saying the spec's rules for NVMe's namespace create command 
> from section 5.20 allow him to not send events, but it turns out 
> you're not even using this command? Why would the spec's defined 
> behavior apply to this proprietary use case?

I think we are in an even deeper mess here than I though.

 - one issue is the fact that the exception gets creating vs attaching
   and deleting vs detaching wrong, and that probably is my fault.
 - the other one is the wording should be shall not send the event
   when the creation is ONLY due to the creation of a namespace, that
   is not when a new ANA group shows up (and the way how ANA groups
   are created is out of scope)

So if we do a textual interpretation of the spec, the controller does not only need to send the ANA AEN for the case we are debatting here, but also for the case I specifically wanted to exclude.  Sigh.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Wed, Dec 09, 2020 at 08:58:06PM +0000, Knight, Frederick wrote:
> So for case 2 (the attach), there are 2 AENs for effectively the same event.  This is where we are at with the existing text.
> 
> Whether this is what the hosts want, that is a good question. What changes are desired (if any)?

Linux hosts currently want an ANA AEN anytime the host needs to refresh
the ANA log. That includes any condition that adds groups that didn't
exist from the host's previous reading.

If the Namespace Attach occurs to an ANA group the host already knows
about, then you don't need to send an ANA AEN because there's nothing
new in the log that the host requires. You just need to send the NS
Notify AEN.

But if a side effect of attaching a namespace results in a new ANA group
becoming visible to the host, then that group creation is considered a
separate event, so the host wants both AENs. I believe this is where
Christoph is trying to steer the interpretation and the text.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[hch]

On Wed, Dec 09, 2020 at 01:34:37PM -0800, Keith Busch wrote:
> Linux hosts currently want an ANA AEN anytime the host needs to refresh
> the ANA log. That includes any condition that adds groups that didn't
> exist from the host's previous reading.
> 
> If the Namespace Attach occurs to an ANA group the host already knows
> about, then you don't need to send an ANA AEN because there's nothing
> new in the log that the host requires. You just need to send the NS
> Notify AEN.
> 
> But if a side effect of attaching a namespace results in a new ANA group
> becoming visible to the host, then that group creation is considered a
> separate event, so the host wants both AENs. I believe this is where
> Christoph is trying to steer the interpretation and the text.

Exactly.  Although as Hannes pointed out, this language:

  If, for an ANA Group, there are no namespaces attached to the
  controller processing the command, then no ANA Group Descriptor is
  returned for that ANA Group (i.e., an ANA Group Descriptor is returned
  only if that ANA Group contains namespaces that are attached to the
  controller processing the command.

creates a bit of a chicken an egg problem in that case of a newly
attached namespace that is the first one referencing an ANA group.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Hannes Reinecke]

On 12/10/20 9:51 AM, hch@lst.de wrote:
> On Wed, Dec 09, 2020 at 01:34:37PM -0800, Keith Busch wrote:
>> Linux hosts currently want an ANA AEN anytime the host needs to refresh
>> the ANA log. That includes any condition that adds groups that didn't
>> exist from the host's previous reading.
>>
>> If the Namespace Attach occurs to an ANA group the host already knows
>> about, then you don't need to send an ANA AEN because there's nothing
>> new in the log that the host requires. You just need to send the NS
>> Notify AEN.
>>
>> But if a side effect of attaching a namespace results in a new ANA group
>> becoming visible to the host, then that group creation is considered a
>> separate event, so the host wants both AENs. I believe this is where
>> Christoph is trying to steer the interpretation and the text.
> 
> Exactly.  Although as Hannes pointed out, this language:
> 
>    If, for an ANA Group, there are no namespaces attached to the
>    controller processing the command, then no ANA Group Descriptor is
>    returned for that ANA Group (i.e., an ANA Group Descriptor is returned
>    only if that ANA Group contains namespaces that are attached to the
>    controller processing the command.
> 
> creates a bit of a chicken an egg problem in that case of a newly
> attached namespace that is the first one referencing an ANA group.
> 
Yes, this was what I tried to outline.

Personally I don't mind whether the controller is required to send an 
ANA change AEN together with the NS change AEN.
Requiring both will pose an ordering issue (as on fabrics we can't 
control the completion ordering), so effectively we'll have to implement 
handling for AENs arriving in any order.
But I'm also fine with just getting one AEN; the patch will solve that 
situation just fine.

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-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Thu, Dec 10, 2020 at 10:13:02AM +0100, Hannes Reinecke wrote:
> On 12/10/20 9:51 AM, hch@lst.de wrote:
> > On Wed, Dec 09, 2020 at 01:34:37PM -0800, Keith Busch wrote:
> > > Linux hosts currently want an ANA AEN anytime the host needs to refresh
> > > the ANA log. That includes any condition that adds groups that didn't
> > > exist from the host's previous reading.
> > > 
> > > If the Namespace Attach occurs to an ANA group the host already knows
> > > about, then you don't need to send an ANA AEN because there's nothing
> > > new in the log that the host requires. You just need to send the NS
> > > Notify AEN.
> > > 
> > > But if a side effect of attaching a namespace results in a new ANA group
> > > becoming visible to the host, then that group creation is considered a
> > > separate event, so the host wants both AENs. I believe this is where
> > > Christoph is trying to steer the interpretation and the text.
> > 
> > Exactly.  Although as Hannes pointed out, this language:
> > 
> >    If, for an ANA Group, there are no namespaces attached to the
> >    controller processing the command, then no ANA Group Descriptor is
> >    returned for that ANA Group (i.e., an ANA Group Descriptor is returned
> >    only if that ANA Group contains namespaces that are attached to the
> >    controller processing the command.
> > 
> > creates a bit of a chicken an egg problem in that case of a newly
> > attached namespace that is the first one referencing an ANA group.
> > 
> Yes, this was what I tried to outline.
> 
> Personally I don't mind whether the controller is required to send an ANA
> change AEN together with the NS change AEN.
> Requiring both will pose an ordering issue (as on fabrics we can't control
> the completion ordering), so effectively we'll have to implement handling
> for AENs arriving in any order.

I was thinking more about this, and it's actually not a problem based on
how the controller is allowed to send AEN completions:

  When the controller posts a completion queue entry for an outstanding
  Asynchronous Event Request command and thus reports an asynchronous
  event, subsequent events of that event type are automatically masked by
  the controller until the host clears that event.

These are both "Notice" event types, so the controller is not allowed to
send both at the same time. The host must acknowledge the first before
the second can be seen, so we can enforce a deterministic sequence.

And while it may make sense if the first event seen is the ANA one
before the NS, it doesn't look like Linux cares if we observe them the
other way around. The only thing the driver needs to do is synchronize
the work queues for each event.

> But I'm also fine with just getting one AEN; the patch will solve that
> situation just fine.

I'm also okay with that patch, too.

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Sagi Grimberg]

Hey all,

Joining late to the discussion, ramping up on an old old backlog...

>>> On Wed, Dec 09, 2020 at 01:34:37PM -0800, Keith Busch wrote:
>>>> Linux hosts currently want an ANA AEN anytime the host needs to refresh
>>>> the ANA log. That includes any condition that adds groups that didn't
>>>> exist from the host's previous reading.
>>>>
>>>> If the Namespace Attach occurs to an ANA group the host already knows
>>>> about, then you don't need to send an ANA AEN because there's nothing
>>>> new in the log that the host requires. You just need to send the NS
>>>> Notify AEN.
>>>>
>>>> But if a side effect of attaching a namespace results in a new ANA group
>>>> becoming visible to the host, then that group creation is considered a
>>>> separate event, so the host wants both AENs. I believe this is where
>>>> Christoph is trying to steer the interpretation and the text.
>>>
>>> Exactly.  Although as Hannes pointed out, this language:
>>>
>>>     If, for an ANA Group, there are no namespaces attached to the
>>>     controller processing the command, then no ANA Group Descriptor is
>>>     returned for that ANA Group (i.e., an ANA Group Descriptor is returned
>>>     only if that ANA Group contains namespaces that are attached to the
>>>     controller processing the command.
>>>
>>> creates a bit of a chicken an egg problem in that case of a newly
>>> attached namespace that is the first one referencing an ANA group.

FWIW, the subsystems I'm familiar with are sending both events in this
case and the interpretation of this paragraph was that although the ANA
group existed since day-0, the host was never informed about it, so
just like lots of other internal resources, it doesn't exist from an
interface perspective.

The alternative would have been to tell the host about every ANA group
that the subsystem is managing, which could be a very large number and
just creates a churn of completely useless huge log pages.

>> Yes, this was what I tried to outline.
>>
>> Personally I don't mind whether the controller is required to send an ANA
>> change AEN together with the NS change AEN.
>> Requiring both will pose an ordering issue (as on fabrics we can't control
>> the completion ordering), so effectively we'll have to implement handling
>> for AENs arriving in any order.
> 
> I was thinking more about this, and it's actually not a problem based on
> how the controller is allowed to send AEN completions:
> 
>    When the controller posts a completion queue entry for an outstanding
>    Asynchronous Event Request command and thus reports an asynchronous
>    event, subsequent events of that event type are automatically masked by
>    the controller until the host clears that event.
> 
> These are both "Notice" event types, so the controller is not allowed to
> send both at the same time. The host must acknowledge the first before
> the second can be seen, so we can enforce a deterministic sequence.

Well that is easy today because there is only one aen at a time...

> And while it may make sense if the first event seen is the ANA one
> before the NS, it doesn't look like Linux cares if we observe them the
> other way around. The only thing the driver needs to do is synchronize
> the work queues for each event.

Yes, the order doesn't matter and we should keep it that way...

>> But I'm also fine with just getting one AEN; the patch will solve that
>> situation just fine.
> 
> I'm also okay with that patch, too.

I would actually be in favor of this patch, would be nice to skip
sending duplicate events for this.

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

RE: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Knight, Frederick]

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

Attached is the proposed clarification.  This has had ZERO review time in the NVMe committee.  So just FYI at this point.

Feel free to share thoughts.

	Fred

-----Original Message-----
From: Sagi Grimberg <sagi@grimberg.me> 
Sent: Wednesday, January 13, 2021 10:09 PM
To: Keith Busch <kbusch@kernel.org>; Hannes Reinecke <hare@suse.de>
Cc: hch@lst.de; Knight, Frederick <Frederick.Knight@netapp.com>; Meneghini, John <John.Meneghini@netapp.com>; George, Martin <Martin.George@netapp.com>; linux-nvme@lists.infradead.org; Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.




Hey all,

Joining late to the discussion, ramping up on an old old backlog...

>>> On Wed, Dec 09, 2020 at 01:34:37PM -0800, Keith Busch wrote:
>>>> Linux hosts currently want an ANA AEN anytime the host needs to 
>>>> refresh the ANA log. That includes any condition that adds groups 
>>>> that didn't exist from the host's previous reading.
>>>>
>>>> If the Namespace Attach occurs to an ANA group the host already 
>>>> knows about, then you don't need to send an ANA AEN because there's 
>>>> nothing new in the log that the host requires. You just need to 
>>>> send the NS Notify AEN.
>>>>
>>>> But if a side effect of attaching a namespace results in a new ANA 
>>>> group becoming visible to the host, then that group creation is 
>>>> considered a separate event, so the host wants both AENs. I believe 
>>>> this is where Christoph is trying to steer the interpretation and the text.
>>>
>>> Exactly.  Although as Hannes pointed out, this language:
>>>
>>>     If, for an ANA Group, there are no namespaces attached to the
>>>     controller processing the command, then no ANA Group Descriptor is
>>>     returned for that ANA Group (i.e., an ANA Group Descriptor is returned
>>>     only if that ANA Group contains namespaces that are attached to the
>>>     controller processing the command.
>>>
>>> creates a bit of a chicken an egg problem in that case of a newly 
>>> attached namespace that is the first one referencing an ANA group.

FWIW, the subsystems I'm familiar with are sending both events in this case and the interpretation of this paragraph was that although the ANA group existed since day-0, the host was never informed about it, so just like lots of other internal resources, it doesn't exist from an interface perspective.

The alternative would have been to tell the host about every ANA group that the subsystem is managing, which could be a very large number and just creates a churn of completely useless huge log pages.

>> Yes, this was what I tried to outline.
>>
>> Personally I don't mind whether the controller is required to send an 
>> ANA change AEN together with the NS change AEN.
>> Requiring both will pose an ordering issue (as on fabrics we can't 
>> control the completion ordering), so effectively we'll have to 
>> implement handling for AENs arriving in any order.
>
> I was thinking more about this, and it's actually not a problem based 
> on how the controller is allowed to send AEN completions:
>
>    When the controller posts a completion queue entry for an outstanding
>    Asynchronous Event Request command and thus reports an asynchronous
>    event, subsequent events of that event type are automatically masked by
>    the controller until the host clears that event.
>
> These are both "Notice" event types, so the controller is not allowed 
> to send both at the same time. The host must acknowledge the first 
> before the second can be seen, so we can enforce a deterministic sequence.

Well that is easy today because there is only one aen at a time...

> And while it may make sense if the first event seen is the ANA one 
> before the NS, it doesn't look like Linux cares if we observe them the 
> other way around. The only thing the driver needs to do is synchronize 
> the work queues for each event.

Yes, the order doesn't matter and we should keep it that way...

>> But I'm also fine with just getting one AEN; the patch will solve 
>> that situation just fine.
>
> I'm also okay with that patch, too.

I would actually be in favor of this patch, would be nice to skip sending duplicate events for this.

[-- Attachment #2: NVMe - NVMe 1.4 ECN-008 2021.01.13.docx --]
[-- Type: application/vnd.openxmlformats-officedocument.wordprocessingml.document, Size: 81284 bytes --]

[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Keith Busch]

On Thu, Jan 14, 2021 at 10:59:09PM +0000, Knight, Frederick wrote:
> Attached is the proposed clarification.  This has had ZERO review time in the NVMe committee.  So just FYI at this point.
> 
> Feel free to share thoughts.

The ECN retains the text that no new ANA event shall be sent if "the
change is due to the creation of a namespace", which seems odd to
continue pointing out given that namespace creation doesn't alter the
ANA log.

Also, if you're going to use this ECN to ensure namespace attachment to
a new ANA group suppresses the ANA event, could you also update section
8.20.3.6 to change: 

  c) upon entry to the following ANA states:

to something like

  c) upon entry to the following ANA states except if the state entry
     occurs as a result of a namespace attachment:

Just so everyone is consistent with the "exceptions".

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

Re: [PATCH] nvme-core: update NS Attr Changed AEN handling for ANA group

[Hannes Reinecke]

On 1/14/21 11:59 PM, Knight, Frederick wrote:
> Attached is the proposed clarification.  This has had ZERO review time in the NVMe committee.  So just FYI at this point.
> 
> Feel free to share thoughts.
> 
Thumbs up from my side.

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