[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[Hannes Reinecke]

When undergoing state transitions I/O might be requeued, hence
we always have to schedule requeue_work whenever the nvme device
is live, independent on whether the old state was live or not.

Reported-by: Martin George <martin.george at netapp.com>
Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/multipath.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 2839bb70badf..f69c9476d663 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -352,8 +352,6 @@ static void nvme_mpath_set_live(struct nvme_ns *ns)
 			__nvme_find_path(head, node);
 		srcu_read_unlock(&head->srcu, srcu_idx);
 	}
-
-	kblockd_schedule_work(&ns->head->requeue_work);
 }
 
 static int nvme_parse_ana_log(struct nvme_ctrl *ctrl, void *data,
@@ -412,8 +410,11 @@ static void nvme_update_ns_ana_state(struct nvme_ana_group_desc *desc,
 	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))
-		nvme_mpath_set_live(ns);
+	if (nvme_state_is_live(ns->ana_state)) {
+		if (!nvme_state_is_live(old))
+			nvme_mpath_set_live(ns);
+		kblockd_schedule_work(&ns->head->requeue_work);
+	}
 	mutex_unlock(&ns->head->lock);
 }
 
@@ -587,6 +588,7 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
 		mutex_lock(&ns->head->lock);
 		ns->ana_state = NVME_ANA_OPTIMIZED; 
 		nvme_mpath_set_live(ns);
+		kblockd_schedule_work(&ns->head->requeue_work);
 		mutex_unlock(&ns->head->lock);
 	}
 }
-- 
2.16.4

[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[Sagi Grimberg]

> When undergoing state transitions I/O might be requeued, hence
> we always have to schedule requeue_work whenever the nvme device
> is live, independent on whether the old state was live or not.

Any reason why not simply update from live to live?

[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[Hannes Reinecke]

On 3/21/19 10:26 PM, Sagi Grimberg wrote:
> 
>> When undergoing state transitions I/O might be requeued, hence
>> we always have to schedule requeue_work whenever the nvme device
>> is live, independent on whether the old state was live or not.
> 
> Any reason why not simply update from live to live?
> 
?

I somewhat fail to parse the answer.
Care to elaborate?

Cheers,

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

[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[Sagi Grimberg]

>>> When undergoing state transitions I/O might be requeued, hence
>>> we always have to schedule requeue_work whenever the nvme device
>>> is live, independent on whether the old state was live or not.
>>
>> Any reason why not simply update from live to live?
>>
> ?
> 
> I somewhat fail to parse the answer.
> Care to elaborate?

--
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 2839bb70badf..f0716f6ce41f 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -404,15 +404,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);
  }
--

[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[Hannes Reinecke]

On 3/22/19 7:57 PM, Sagi Grimberg wrote:
>>>> When undergoing state transitions I/O might be requeued, hence
>>>> we always have to schedule requeue_work whenever the nvme device
>>>> is live, independent on whether the old state was live or not.
>>>
>>> Any reason why not simply update from live to live?
>>>
>> ?
>>
>> I somewhat fail to parse the answer.
>> Care to elaborate?
> 
> -- 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 2839bb70badf..f0716f6ce41f 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -404,15 +404,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);
>  ?}
> -- 
That's what we tried initially (cf thread "[PATCH] nvme-multipath: relax 
ANA state check"), but got rejected by Christoph.

Cheers,

Hannes

[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[Sagi Grimberg]

> That's what we tried initially (cf thread "[PATCH] nvme-multipath: relax 
> ANA state check"), but got rejected by Christoph.

IIRC Christoph was asking why was requests being requeued when moving
from live -> live, and I think you answered that in your patch
description..

[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[Christoph Hellwig]

On Mon, Mar 25, 2019@08:55:47AM -0700, Sagi Grimberg wrote:
>
>> That's what we tried initially (cf thread "[PATCH] nvme-multipath: relax 
>> ANA state check"), but got rejected by Christoph.
>
> IIRC Christoph was asking why was requests being requeued when moving
> from live -> live, and I think you answered that in your patch
> description..

Exactltly.  Asking to please explain WTF is happening is not rejecting,
it is asking for more information to actually understand the issue.

So the patch from Georg with a changelog and comment in the code based
on yours seems like what we need.

[PATCH] nvme: schedule requeue whenever a LIVE state is entered

[Hannes Reinecke]

On 3/27/19 9:37 AM, Christoph Hellwig wrote:
> On Mon, Mar 25, 2019@08:55:47AM -0700, Sagi Grimberg wrote:
>>
>>> That's what we tried initially (cf thread "[PATCH] nvme-multipath: relax
>>> ANA state check"), but got rejected by Christoph.
>>
>> IIRC Christoph was asking why was requests being requeued when moving
>> from live -> live, and I think you answered that in your patch
>> description..
> 
> Exactly.  Asking to please explain WTF is happening is not rejecting,
> it is asking for more information to actually understand the issue.
> 
> So the patch from Georg with a changelog and comment in the code based
> on yours seems like what we need.
> 
Okay, will be resending.

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: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)