[PATCH] nvme-multipath: relax ANA state check

[PATCH] nvme-multipath: relax ANA state check

[martinus.gpy]

From: Martin George <marting@netapp.com>

Always call nvme_mpath_set_live() when transitioning to a live
state, where live is defined as ANA Optimized or Non-Optimized,
irrespective of what the previous asymmetric access state was.
This removes the restriction of only permitting transitions
from states that were not considered live.

Signed-off-by: Martin George <marting at netapp.com>
Signed-off-by: Gargi Srinivas <sring at netapp.com>
---
 drivers/nvme/host/multipath.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index b9fff3b8ed1b..23da7beadd62 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -366,15 +366,12 @@ static inline bool nvme_state_is_live(enum nvme_ana_state state)
 static void nvme_update_ns_ana_state(struct nvme_ana_group_desc *desc,
 		struct nvme_ns *ns)
 {
-	enum nvme_ana_state old;
-
 	mutex_lock(&ns->head->lock);
-	old = ns->ana_state;
 	ns->ana_grpid = le32_to_cpu(desc->grpid);
 	ns->ana_state = desc->state;
 	clear_bit(NVME_NS_ANA_PENDING, &ns->flags);
 
-	if (nvme_state_is_live(ns->ana_state) && !nvme_state_is_live(old))
+	if (nvme_state_is_live(ns->ana_state))
 		nvme_mpath_set_live(ns);
 	mutex_unlock(&ns->head->lock);
 }
-- 
2.17.1

[PATCH] nvme-multipath: relax ANA state check

[Christoph Hellwig]

On Mon, Jan 28, 2019@07:54:46PM +0530, martinus.gpy@gmail.com wrote:
> From: Martin George <marting at netapp.com>
> 
> Always call nvme_mpath_set_live() when transitioning to a live
> state, where live is defined as ANA Optimized or Non-Optimized,
> irrespective of what the previous asymmetric access state was.
> This removes the restriction of only permitting transitions
> from states that were not considered live.

This still doesn't make sense.  We never prohibit a transition here.

What kind of problem do you see that triggered this patch?

[PATCH] nvme-multipath: relax ANA state check

[George, Martin]

On Mon, 2019-01-28@17:30 +0100, Christoph Hellwig wrote:
> 
> On Mon, Jan 28, 2019 at 07:54:46PM +0530, martinus.gpy at gmail.com
> wrote:
> > From: Martin George <marting at netapp.com>
> > 
> > Always call nvme_mpath_set_live() when transitioning to a live
> > state, where live is defined as ANA Optimized or Non-Optimized,
> > irrespective of what the previous asymmetric access state was.
> > This removes the restriction of only permitting transitions
> > from states that were not considered live.
> 
> This still doesn't make sense.  We never prohibit a transition here.
> 
> What kind of problem do you see that triggered this patch?

We saw I/O successfully resume on a namespace that transitioned from
Inaccessible to Optimized on the ANA host. No issues there. But at the
same time, I/O hung when it transitioned from Optimized to Optimized
itself.

After applying the above patch, this problem was resolved and I/O
successfully resumed for this same state Optimized to Optimized
transition scenario. In fact with the above patch, it should also
permit I/O to successfully resume for Optimized to Non-Optimized
scenarios (& vice versa) as well, all of which are valid transition
scenarios as per the NVMe 1.3 TP4004 ANA spec.

-Martin George

[PATCH] nvme-multipath: relax ANA state check

[hch]

On Mon, Jan 28, 2019@05:10:59PM +0000, George, Martin wrote:
> On Mon, 2019-01-28@17:30 +0100, Christoph Hellwig wrote:
> > 
> > On Mon, Jan 28, 2019 at 07:54:46PM +0530, martinus.gpy at gmail.com
> > wrote:
> > > From: Martin George <marting at netapp.com>
> > > 
> > > Always call nvme_mpath_set_live() when transitioning to a live
> > > state, where live is defined as ANA Optimized or Non-Optimized,
> > > irrespective of what the previous asymmetric access state was.
> > > This removes the restriction of only permitting transitions
> > > from states that were not considered live.
> > 
> > This still doesn't make sense.  We never prohibit a transition here.
> > 
> > What kind of problem do you see that triggered this patch?
> 
> We saw I/O successfully resume on a namespace that transitioned from
> Inaccessible to Optimized on the ANA host. No issues there. But at the
> same time, I/O hung when it transitioned from Optimized to Optimized
> itself.

Well, I/O should haver never stopped in that case.  Can you please
try to help debugging why it even stopped?
> 
> After applying the above patch, this problem was resolved and I/O
> successfully resumed for this same state Optimized to Optimized
> transition scenario. In fact with the above patch, it should also
> permit I/O to successfully resume for Optimized to Non-Optimized
> scenarios (& vice versa) as well, all of which are valid transition
> scenarios as per the NVMe 1.3 TP4004 ANA spec.

Again, there is nothing in the code that prohibits transitions, so this
explanation doesn't make sense.  It it obvious that you saw a problem
and that we have an issue somewhere.  But the patch description is plain
wrong, and I suspect that the problem might be elsewhere as well.

[PATCH] nvme-multipath: relax ANA state check

[George, Martin]

On Mon, 2019-01-28@08:43 +0100, Christoph Hellwig wrote:
> 
> This description does not seem to match the patch:
> 
> >  static void nvme_update_ns_ana_state(struct nvme_ana_group_desc
> > *desc,
> >               struct nvme_ns *ns)
> >  {
> > -     enum nvme_ana_state old;
> > -
> >       mutex_lock(&ns->head->lock);
> > -     old = ns->ana_state;
> >       ns->ana_grpid = le32_to_cpu(desc->grpid);
> >       ns->ana_state = desc->state;
> >       clear_bit(NVME_NS_ANA_PENDING, &ns->flags);
> > 
> > -     if (nvme_state_is_live(ns->ana_state) &&
> > !nvme_state_is_live(old))
> > +     if (nvme_state_is_live(ns->ana_state))
> >               nvme_mpath_set_live(ns);
> 
> What the patch does is to always call nvme_mpath_set_live when
> transitioning to a live state, where live is defined as optimized or
> non-optimized, instead of limiting that call to transitions from
> states that were not considered live.

Ok. Will modify the description accordingly and resend the patch. Thank
you.

-Martin George

[PATCH] nvme-multipath: relax ANA state check

[Christoph Hellwig]

On Fri, Jan 25, 2019@04:42:36PM +0530, martinus.gpy@gmail.com wrote:
> From: Martin George <marting at netapp.com>
> 
> The current nvme multipath code prevents access to a namespace
> through a controller that has transitioned from ANA Optimized
> to Non-Optimized and vice versa.

Note: controllers do not have ANA states, ANA groups and thus
namespaces have states.

> As per the NVMe 1.3 TP4004
> spec, "The change from one asymmetric namespace access state
> to another asymmetric namespace access state is called a
> transition". The emphasis here is on the change itself,
> whatever the change may be. So a change from ANA Optimized to
> Non-Optimized and vice versa, are actually valid transition
> scenarios. In fact, if one were to exit and reenter the same
> state, that too is a change i.e. a change to the same state
> is also a valid transition scenario.
> 
> So remove the "old" state check during ANA state update, so
> that transition from ANA Optimized to Non-Optimized and vice
> versa, along with transition to the same state, are all
> permitted.

This description does not seem to match the patch:

>  static void nvme_update_ns_ana_state(struct nvme_ana_group_desc *desc,
>  		struct nvme_ns *ns)
>  {
> -	enum nvme_ana_state old;
> -
>  	mutex_lock(&ns->head->lock);
> -	old = ns->ana_state;
>  	ns->ana_grpid = le32_to_cpu(desc->grpid);
>  	ns->ana_state = desc->state;
>  	clear_bit(NVME_NS_ANA_PENDING, &ns->flags);
>  
> -	if (nvme_state_is_live(ns->ana_state) && !nvme_state_is_live(old))
> +	if (nvme_state_is_live(ns->ana_state))
>  		nvme_mpath_set_live(ns);

What the patch does is to always call nvme_mpath_set_live when
transitioning to a live state, where live is defined as optimized or
non-optimized, instead of limiting that call to transitions from
states that were not considered live.

[PATCH] nvme-multipath: relax ANA state check

[Hannes Reinecke]

On 1/25/19 12:12 PM, martinus.gpy@gmail.com wrote:
> From: Martin George <marting at netapp.com>
> 
> The current nvme multipath code prevents access to a namespace
> through a controller that has transitioned from ANA Optimized
> to Non-Optimized and vice versa. As per the NVMe 1.3 TP4004
> spec, "The change from one asymmetric namespace access state
> to another asymmetric namespace access state is called a
> transition". The emphasis here is on the change itself,
> whatever the change may be. So a change from ANA Optimized to
> Non-Optimized and vice versa, are actually valid transition
> scenarios. In fact, if one were to exit and reenter the same
> state, that too is a change i.e. a change to the same state
> is also a valid transition scenario.
> 
> So remove the "old" state check during ANA state update, so
> that transition from ANA Optimized to Non-Optimized and vice
> versa, along with transition to the same state, are all
> permitted.
> 
> Signed-off-by: Martin George <marting at netapp.com>
> Signed-off-by: Gargi Srinivas <sring at netapp.com>
> ---
>   drivers/nvme/host/multipath.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index b9fff3b8ed1b..23da7beadd62 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -366,15 +366,12 @@ static inline bool nvme_state_is_live(enum nvme_ana_state state)
>   static void nvme_update_ns_ana_state(struct nvme_ana_group_desc *desc,
>   		struct nvme_ns *ns)
>   {
> -	enum nvme_ana_state old;
> -
>   	mutex_lock(&ns->head->lock);
> -	old = ns->ana_state;
>   	ns->ana_grpid = le32_to_cpu(desc->grpid);
>   	ns->ana_state = desc->state;
>   	clear_bit(NVME_NS_ANA_PENDING, &ns->flags);
>   
> -	if (nvme_state_is_live(ns->ana_state) && !nvme_state_is_live(old))
> +	if (nvme_state_is_live(ns->ana_state))
>   		nvme_mpath_set_live(ns);
>   	mutex_unlock(&ns->head->lock);
>   }
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

[PATCH] nvme-multipath: relax ANA state check

[martinus.gpy]

From: Martin George <marting@netapp.com>

The current nvme multipath code prevents access to a namespace
through a controller that has transitioned from ANA Optimized
to Non-Optimized and vice versa. As per the NVMe 1.3 TP4004
spec, "The change from one asymmetric namespace access state
to another asymmetric namespace access state is called a
transition". The emphasis here is on the change itself,
whatever the change may be. So a change from ANA Optimized to
Non-Optimized and vice versa, are actually valid transition
scenarios. In fact, if one were to exit and reenter the same
state, that too is a change i.e. a change to the same state
is also a valid transition scenario.

So remove the "old" state check during ANA state update, so
that transition from ANA Optimized to Non-Optimized and vice
versa, along with transition to the same state, are all
permitted.

Signed-off-by: Martin George <marting at netapp.com>
Signed-off-by: Gargi Srinivas <sring at netapp.com>
---
 drivers/nvme/host/multipath.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index b9fff3b8ed1b..23da7beadd62 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -366,15 +366,12 @@ static inline bool nvme_state_is_live(enum nvme_ana_state state)
 static void nvme_update_ns_ana_state(struct nvme_ana_group_desc *desc,
 		struct nvme_ns *ns)
 {
-	enum nvme_ana_state old;
-
 	mutex_lock(&ns->head->lock);
-	old = ns->ana_state;
 	ns->ana_grpid = le32_to_cpu(desc->grpid);
 	ns->ana_state = desc->state;
 	clear_bit(NVME_NS_ANA_PENDING, &ns->flags);
 
-	if (nvme_state_is_live(ns->ana_state) && !nvme_state_is_live(old))
+	if (nvme_state_is_live(ns->ana_state))
 		nvme_mpath_set_live(ns);
 	mutex_unlock(&ns->head->lock);
 }
-- 
2.17.1