[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