[PATCH 0/3] nvme: fixup race with namespace removal

[PATCH 0/3] nvme: fixup race with namespace removal

[Hannes Reinecke]

Hi all,

nvme namespace scanning might race with controller reset, causing the
system to crash in myriad ways if nvme_scan_ns_list() decides to remove
a namespace while resetting is active on another thread.
The main problem here is that namespace removal is not atomic.
The namespace mutex needs to be dropped before calling nvme_ns_remove(),
so if nvme_ns_remove() is called while traversing the list of namespaces
there always will be a race condition.

This patchset tries to fix this problem by shortening the race window
and check for valid namespaces in nvme_start_queue()/nvme_stop_queue().

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  nvme: do not quiesce or unquiesce invalid namespaces
  nvme: shorten race window in nvme_ns_remove()
  nvme-multipath: test namespace state when selecting a new path

 drivers/nvme/host/core.c      | 18 ++++++++++++------
 drivers/nvme/host/multipath.c |  6 ++++--
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.16.4

[PATCH 1/3] nvme: do not quiesce or unquiesce invalid namespaces

[Hannes Reinecke]

nvme_(start,stop)_queues() might race with namespace scanning.
As namespaces might be removed during scanning, but the removal
from the list is not atomic, those functions might trip
over namespaces which are partially deleted, causing really
interesting kernel crashes.
So validate the namespaces in nvme_(start,stop)_queues().

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 470601980794..c583735383ca 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3857,8 +3857,11 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl)
 	struct nvme_ns *ns;
 
 	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		if (test_bit(NVME_NS_REMOVING, &ns->flags))
+			continue;
 		blk_mq_quiesce_queue(ns->queue);
+	}
 	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_stop_queues);
@@ -3868,8 +3871,11 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
 	struct nvme_ns *ns;
 
 	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		if (test_bit(NVME_NS_REMOVING, &ns->flags))
+			continue;
 		blk_mq_unquiesce_queue(ns->queue);
+	}
 	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_start_queues);
-- 
2.16.4

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Hannes Reinecke]

nvme_ns_remove() sets the 'NVME_NS_REMOVING' bit at first, but
only removes the namespace from the list at the very end.
This opens a rether large race window during which other processes
traversing the namespaces via list_for_each() will access partially
deleted namespaces, causing unpredictable results.
This patch shortens the race window by removing the namespace
from the list as early as possible.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c583735383ca..8c87ef584e6d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
 		return;
 
+	down_write(&ns->ctrl->namespaces_rwsem);
+	list_del_init(&ns->list);
+	up_write(&ns->ctrl->namespaces_rwsem);
+
 	nvme_fault_inject_fini(ns);
 	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
 		del_gendisk(ns->disk);
@@ -3329,10 +3333,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 	nvme_mpath_clear_current_path(ns);
 	mutex_unlock(&ns->ctrl->subsys->lock);
 
-	down_write(&ns->ctrl->namespaces_rwsem);
-	list_del_init(&ns->list);
-	up_write(&ns->ctrl->namespaces_rwsem);
-
 	synchronize_srcu(&ns->head->srcu);
 	nvme_mpath_check_last_path(ns);
 	nvme_put_ns(ns);
-- 
2.16.4

[PATCH 3/3] nvme-multipath: test namespace state when selecting a new path

[Hannes Reinecke]

When selecting a new path we need to check if the namespace is
not about to be removed.

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

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index f69c9476d663..a87462efc927 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -130,7 +130,8 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
 
 	list_for_each_entry_rcu(ns, &head->list, siblings) {
 		if (ns->ctrl->state != NVME_CTRL_LIVE ||
-		    test_bit(NVME_NS_ANA_PENDING, &ns->flags))
+		    test_bit(NVME_NS_ANA_PENDING, &ns->flags) ||
+		    test_bit(NVME_NS_REMOVING, &ns->flags))
 			continue;
 
 		if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
@@ -185,7 +186,8 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
 	     ns != old;
 	     ns = nvme_next_ns(head, ns)) {
 		if (ns->ctrl->state != NVME_CTRL_LIVE ||
-		    test_bit(NVME_NS_ANA_PENDING, &ns->flags))
+		    test_bit(NVME_NS_ANA_PENDING, &ns->flags) ||
+		    test_bit(NVME_NS_REMOVING, &ns->flags))
 			continue;
 
 		if (ns->ana_state == NVME_ANA_OPTIMIZED) {
-- 
2.16.4

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Christoph Hellwig]

On Wed, Mar 27, 2019@09:09:28AM +0100, Hannes Reinecke wrote:
> nvme_ns_remove() sets the 'NVME_NS_REMOVING' bit at first, but
> only removes the namespace from the list at the very end.
> This opens a rether large race window during which other processes
> traversing the namespaces via list_for_each() will access partially
> deleted namespaces, causing unpredictable results.
> This patch shortens the race window by removing the namespace
> from the list as early as possible.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>

With this patch I don't see any reason for your patch 1, given that
the window between setting the REMOVING bit and removing from the
list is so tiny.

That being said I think we really should also remove the namespace
from ->siblings and clear the current path ASAP as well, which might
remove the need for patch 3 as well.

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Keith Busch]

On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
>  		return;
>  
> +	down_write(&ns->ctrl->namespaces_rwsem);
> +	list_del_init(&ns->list);
> +	up_write(&ns->ctrl->namespaces_rwsem);
> +
>  	nvme_fault_inject_fini(ns);
>  	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
>  		del_gendisk(ns->disk);

I think I understand the problem you've described, but I'm a little
concerned with the early removal. This will call blk_cleanup_queue
after the namespace is removed from our lists, and that does a blocking
queue_freeze. If commands happen to be dispatched to that namespace,
and the controller for some reason stops responding, we may not be able
to recover during reset when the namespace queue is removed from the
controller list. That's at least problematic if we have to go with the
per-queue tag iterators that Jianchao proposed.

I may be wrong, or there might be another way to address this. It's a bit
late where I'm at the moment, I'll revisit again with fresh eyes tomorrow.

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Hannes Reinecke]

On 3/27/19 3:02 PM, Christoph Hellwig wrote:
> On Wed, Mar 27, 2019@09:09:28AM +0100, Hannes Reinecke wrote:
>> nvme_ns_remove() sets the 'NVME_NS_REMOVING' bit at first, but
>> only removes the namespace from the list at the very end.
>> This opens a rether large race window during which other processes
>> traversing the namespaces via list_for_each() will access partially
>> deleted namespaces, causing unpredictable results.
>> This patch shortens the race window by removing the namespace
>> from the list as early as possible.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.com>
> 
> With this patch I don't see any reason for your patch 1, given that
> the window between setting the REMOVING bit and removing from the
> list is so tiny.
> 
Tiny doesn't equal non-existent.
I'd rather make it explicit to make life easier for others having to 
debug issues here.

> That being said I think we really should also remove the namespace
> from ->siblings and clear the current path ASAP as well, which might
> remove the need for patch 3 as well.
> 
Okay, can do that.

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 2/3] nvme: shorten race window in nvme_ns_remove()

[Hannes Reinecke]

On 3/27/19 3:20 PM, Keith Busch wrote:
> On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
>> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>   	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
>>   		return;
>>   
>> +	down_write(&ns->ctrl->namespaces_rwsem);
>> +	list_del_init(&ns->list);
>> +	up_write(&ns->ctrl->namespaces_rwsem);
>> +
>>   	nvme_fault_inject_fini(ns);
>>   	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
>>   		del_gendisk(ns->disk);
> 
> I think I understand the problem you've described, but I'm a little
> concerned with the early removal. This will call blk_cleanup_queue
> after the namespace is removed from our lists, and that does a blocking
> queue_freeze. If commands happen to be dispatched to that namespace,
> and the controller for some reason stops responding, we may not be able
> to recover during reset when the namespace queue is removed from the
> controller list. That's at least problematic if we have to go with the
> per-queue tag iterators that Jianchao proposed.
> 
Hmm. I sort of see where you coming from.
But I was slightly worried about all the other callers where we iterate
the namespace list; one would need to to a _really_ careful audit which 
are safe to traverse even with namespace removal running; some have to, 
others must not, and some simply don't care.
Hence this approach to make things easier.

> I may be wrong, or there might be another way to address this. It's a bit
> late where I'm at the moment, I'll revisit again with fresh eyes tomorrow.
> 
Thanks.

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 2/3] nvme: shorten race window in nvme_ns_remove()

[James Smart]

On 3/27/2019 8:12 AM, Hannes Reinecke wrote:
> On 3/27/19 3:20 PM, Keith Busch wrote:
>> On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
>>> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>> ????? if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
>>> ????????? return;
>>> ? +??? down_write(&ns->ctrl->namespaces_rwsem);
>>> +??? list_del_init(&ns->list);
>>> +??? up_write(&ns->ctrl->namespaces_rwsem);
>>> +
>>> ????? nvme_fault_inject_fini(ns);
>>> ????? if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
>>> ????????? del_gendisk(ns->disk);
>>
>> I think I understand the problem you've described, but I'm a little
>> concerned with the early removal. This will call blk_cleanup_queue
>> after the namespace is removed from our lists, and that does a blocking
>> queue_freeze. If commands happen to be dispatched to that namespace,
>> and the controller for some reason stops responding, we may not be able
>> to recover during reset when the namespace queue is removed from the
>> controller list. That's at least problematic if we have to go with the
>> per-queue tag iterators that Jianchao proposed.
>>
> Hmm. I sort of see where you coming from.
> But I was slightly worried about all the other callers where we iterate
> the namespace list; one would need to to a _really_ careful audit 
> which are safe to traverse even with namespace removal running; some 
> have to, others must not, and some simply don't care.
> Hence this approach to make things easier.

I like and agree with Hannes's later points.

But, Keiths statements make me nervous. With Hannes's patches, I could 
easily see the case where the ns->queue was quiesced when ns_remove is 
called, which I think causes issues with blk_freeze_queue. We would need 
to have ns_remove, after unlinking, to release the queue. is this right ?

-- james

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Hannes Reinecke]

On 3/27/19 7:50 PM, James Smart wrote:
> On 3/27/2019 8:12 AM, Hannes Reinecke wrote:
>> On 3/27/19 3:20 PM, Keith Busch wrote:
>>> On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
>>>> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>>> ????? if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
>>>> ????????? return;
>>>> ? +??? down_write(&ns->ctrl->namespaces_rwsem);
>>>> +??? list_del_init(&ns->list);
>>>> +??? up_write(&ns->ctrl->namespaces_rwsem);
>>>> +
>>>> ????? nvme_fault_inject_fini(ns);
>>>> ????? if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
>>>> ????????? del_gendisk(ns->disk);
>>>
>>> I think I understand the problem you've described, but I'm a little
>>> concerned with the early removal. This will call blk_cleanup_queue
>>> after the namespace is removed from our lists, and that does a blocking
>>> queue_freeze. If commands happen to be dispatched to that namespace,
>>> and the controller for some reason stops responding, we may not be able
>>> to recover during reset when the namespace queue is removed from the
>>> controller list. That's at least problematic if we have to go with the
>>> per-queue tag iterators that Jianchao proposed.
>>>
>> Hmm. I sort of see where you coming from.
>> But I was slightly worried about all the other callers where we iterate
>> the namespace list; one would need to to a _really_ careful audit 
>> which are safe to traverse even with namespace removal running; some 
>> have to, others must not, and some simply don't care.
>> Hence this approach to make things easier.
> 
> I like and agree with Hannes's later points.
> 
> But, Keiths statements make me nervous. With Hannes's patches, I could 
> easily see the case where the ns->queue was quiesced when ns_remove is 
> called, which I think causes issues with blk_freeze_queue. We would need 
> to have ns_remove, after unlinking, to release the queue. is this right ?
> 
Hmm. But maybe it's as simple as this:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4e5ad111b68b..67451e5eec31 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
  {
         if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
                 return -EBUSY;
+       flush_work(&ctrl->scan_work);
         if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
                 return -EBUSY;

James? Keith?

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)

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Ming Lei]

On Thu, Mar 28, 2019@5:28 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 3/27/19 7:50 PM, James Smart wrote:
> > On 3/27/2019 8:12 AM, Hannes Reinecke wrote:
> >> On 3/27/19 3:20 PM, Keith Busch wrote:
> >>> On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
> >>>> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
> >>>>       if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
> >>>>           return;
> >>>>   +    down_write(&ns->ctrl->namespaces_rwsem);
> >>>> +    list_del_init(&ns->list);
> >>>> +    up_write(&ns->ctrl->namespaces_rwsem);
> >>>> +
> >>>>       nvme_fault_inject_fini(ns);
> >>>>       if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
> >>>>           del_gendisk(ns->disk);
> >>>
> >>> I think I understand the problem you've described, but I'm a little
> >>> concerned with the early removal. This will call blk_cleanup_queue
> >>> after the namespace is removed from our lists, and that does a blocking
> >>> queue_freeze. If commands happen to be dispatched to that namespace,
> >>> and the controller for some reason stops responding, we may not be able
> >>> to recover during reset when the namespace queue is removed from the
> >>> controller list. That's at least problematic if we have to go with the
> >>> per-queue tag iterators that Jianchao proposed.
> >>>
> >> Hmm. I sort of see where you coming from.
> >> But I was slightly worried about all the other callers where we iterate
> >> the namespace list; one would need to to a _really_ careful audit
> >> which are safe to traverse even with namespace removal running; some
> >> have to, others must not, and some simply don't care.
> >> Hence this approach to make things easier.
> >
> > I like and agree with Hannes's later points.
> >
> > But, Keiths statements make me nervous. With Hannes's patches, I could
> > easily see the case where the ns->queue was quiesced when ns_remove is
> > called, which I think causes issues with blk_freeze_queue. We would need
> > to have ns_remove, after unlinking, to release the queue. is this right ?
> >
> Hmm. But maybe it's as simple as this:
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4e5ad111b68b..67451e5eec31 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
>   {
>          if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>                  return -EBUSY;
> +       flush_work(&ctrl->scan_work);
>          if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
>                  return -EBUSY;
>

This way might cause deadlock because there is sync IO in scan
work context.

thanks,
Ming

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Hannes Reinecke]

On 3/28/19 12:07 PM, Ming Lei wrote:
> On Thu, Mar 28, 2019@5:28 PM Hannes Reinecke <hare@suse.de> wrote:
>>
>> On 3/27/19 7:50 PM, James Smart wrote:
>>> On 3/27/2019 8:12 AM, Hannes Reinecke wrote:
>>>> On 3/27/19 3:20 PM, Keith Busch wrote:
>>>>> On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
>>>>>> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>>>>>        if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
>>>>>>            return;
>>>>>>    +    down_write(&ns->ctrl->namespaces_rwsem);
>>>>>> +    list_del_init(&ns->list);
>>>>>> +    up_write(&ns->ctrl->namespaces_rwsem);
>>>>>> +
>>>>>>        nvme_fault_inject_fini(ns);
>>>>>>        if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
>>>>>>            del_gendisk(ns->disk);
>>>>>
>>>>> I think I understand the problem you've described, but I'm a little
>>>>> concerned with the early removal. This will call blk_cleanup_queue
>>>>> after the namespace is removed from our lists, and that does a blocking
>>>>> queue_freeze. If commands happen to be dispatched to that namespace,
>>>>> and the controller for some reason stops responding, we may not be able
>>>>> to recover during reset when the namespace queue is removed from the
>>>>> controller list. That's at least problematic if we have to go with the
>>>>> per-queue tag iterators that Jianchao proposed.
>>>>>
>>>> Hmm. I sort of see where you coming from.
>>>> But I was slightly worried about all the other callers where we iterate
>>>> the namespace list; one would need to to a _really_ careful audit
>>>> which are safe to traverse even with namespace removal running; some
>>>> have to, others must not, and some simply don't care.
>>>> Hence this approach to make things easier.
>>>
>>> I like and agree with Hannes's later points.
>>>
>>> But, Keiths statements make me nervous. With Hannes's patches, I could
>>> easily see the case where the ns->queue was quiesced when ns_remove is
>>> called, which I think causes issues with blk_freeze_queue. We would need
>>> to have ns_remove, after unlinking, to release the queue. is this right ?
>>>
>> Hmm. But maybe it's as simple as this:
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 4e5ad111b68b..67451e5eec31 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
>>    {
>>           if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>>                   return -EBUSY;
>> +       flush_work(&ctrl->scan_work);
>>           if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
>>                   return -EBUSY;
>>
> 
> This way might cause deadlock because there is sync IO in scan
> work context.
> 
But I sort of hoped that I/O would be aborted when the controller is 
resetting, which should cause the sync I/O to complete, right?

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 2/3] nvme: shorten race window in nvme_ns_remove()

[Hannes Reinecke]

On 3/28/19 12:48 PM, Hannes Reinecke wrote:
> On 3/28/19 12:07 PM, Ming Lei wrote:
>> On Thu, Mar 28, 2019@5:28 PM Hannes Reinecke <hare@suse.de> wrote:
>>>
>>> On 3/27/19 7:50 PM, James Smart wrote:
[ .. ]
>>>> But, Keiths statements make me nervous. With Hannes's patches, I could
>>>> easily see the case where the ns->queue was quiesced when ns_remove is
>>>> called, which I think causes issues with blk_freeze_queue. We would 
>>>> need
>>>> to have ns_remove, after unlinking, to release the queue. is this 
>>>> right ?
>>>>
>>> Hmm. But maybe it's as simple as this:
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index 4e5ad111b68b..67451e5eec31 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
>>> ?? {
>>> ????????? if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>>> ????????????????? return -EBUSY;
>>> +?????? flush_work(&ctrl->scan_work);
>>> ????????? if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
>>> ????????????????? return -EBUSY;
>>>
>>
>> This way might cause deadlock because there is sync IO in scan
>> work context.
>>
> But I sort of hoped that I/O would be aborted when the controller is 
> resetting, which should cause the sync I/O to complete, right?
> 
But indeed, you're right (of course).

This should be better:
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 67451e5eec31..81900735a380 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3452,6 +3452,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
  {
         nvme_mpath_stop(ctrl);
         nvme_stop_keep_alive(ctrl);
+       flush_work(&ctrl->scan_work);
         flush_work(&ctrl->async_event_work);
         cancel_work_sync(&ctrl->fw_act_work);
  }
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index d42bada25b54..ef20935a7202 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1761,8 +1761,8 @@ static void nvme_rdma_reset_ctrl_work(struct 
work_struct *work)
         int ret;
         bool changed;

-       nvme_stop_ctrl(&ctrl->ctrl);
         nvme_rdma_shutdown_ctrl(ctrl, false);
+       nvme_stop_ctrl(&ctrl->ctrl);

         if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
                 /* state change failure should never happen */

(We need to twiddle the order on RDMA, as we'd need to abort the I/O 
first before we can flush the scan_work element).
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)

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[James Smart]

On 3/28/2019 2:27 AM, Hannes Reinecke wrote:
> On 3/27/19 7:50 PM, James Smart wrote:
>> On 3/27/2019 8:12 AM, Hannes Reinecke wrote:
>>> On 3/27/19 3:20 PM, Keith Busch wrote:
>>>> On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
>>>>> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>>>> ????? if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
>>>>> ????????? return;
>>>>> ? +??? down_write(&ns->ctrl->namespaces_rwsem);
>>>>> +??? list_del_init(&ns->list);
>>>>> +??? up_write(&ns->ctrl->namespaces_rwsem);
>>>>> +
>>>>> ????? nvme_fault_inject_fini(ns);
>>>>> ????? if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
>>>>> ????????? del_gendisk(ns->disk);
>>>>
>>>> I think I understand the problem you've described, but I'm a little
>>>> concerned with the early removal. This will call blk_cleanup_queue
>>>> after the namespace is removed from our lists, and that does a 
>>>> blocking
>>>> queue_freeze. If commands happen to be dispatched to that namespace,
>>>> and the controller for some reason stops responding, we may not be 
>>>> able
>>>> to recover during reset when the namespace queue is removed from the
>>>> controller list. That's at least problematic if we have to go with the
>>>> per-queue tag iterators that Jianchao proposed.
>>>>
>>> Hmm. I sort of see where you coming from.
>>> But I was slightly worried about all the other callers where we iterate
>>> the namespace list; one would need to to a _really_ careful audit 
>>> which are safe to traverse even with namespace removal running; some 
>>> have to, others must not, and some simply don't care.
>>> Hence this approach to make things easier.
>>
>> I like and agree with Hannes's later points.
>>
>> But, Keiths statements make me nervous. With Hannes's patches, I 
>> could easily see the case where the ns->queue was quiesced when 
>> ns_remove is called, which I think causes issues with 
>> blk_freeze_queue. We would need to have ns_remove, after unlinking, 
>> to release the queue. is this right ?
>>
> Hmm. But maybe it's as simple as this:
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4e5ad111b68b..67451e5eec31 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
> ?{
> ??????? if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
> ??????????????? return -EBUSY;
> +?????? flush_work(&ctrl->scan_work);
> ??????? if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
> ??????????????? return -EBUSY;
>
> James? Keith?
>
No...

As mentioned in the other thread. If the reset processing is held off, 
then nothing will kill the io that the scan thread has issued and is 
pending on the link. In the best case, you must wait for the command 
timeout to occur, which will want to re-invoke the reset path, which 
you've noop'd here, so I'm not fully sure it will clean up the io.? And, 
as reset hasn't run, the io queues on this controller have yet to be 
quiesced so you're going to start seeing filesystem errors.? The other 
headache - even if that 1st io is aborted and returned, just because the 
1 scan io errored, it doesn't stop the scan thread. The thread is coded 
to keep looping on new namespaces so it'll attempt to send another/many 
commands.? Which means the new io's will fall into the pass through the 
check_ready routines as they will fall into the path that is only 
prevented by the connection state being nil, which would not have been 
set because the reset path was held off.?? Minimally a new change has to 
be put there.?? This is not something that can be solved by scheduling 
work elements at the transport level. The transport has no idea what 
ns's are using its io request queue what states they are in. It's a ns 
state issue.

-- james

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Ming Lei]

On Thu, Mar 28, 2019@8:11 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 3/28/19 12:48 PM, Hannes Reinecke wrote:
> > On 3/28/19 12:07 PM, Ming Lei wrote:
> >> On Thu, Mar 28, 2019@5:28 PM Hannes Reinecke <hare@suse.de> wrote:
> >>>
> >>> On 3/27/19 7:50 PM, James Smart wrote:
> [ .. ]
> >>>> But, Keiths statements make me nervous. With Hannes's patches, I could
> >>>> easily see the case where the ns->queue was quiesced when ns_remove is
> >>>> called, which I think causes issues with blk_freeze_queue. We would
> >>>> need
> >>>> to have ns_remove, after unlinking, to release the queue. is this
> >>>> right ?
> >>>>
> >>> Hmm. But maybe it's as simple as this:
> >>>
> >>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> >>> index 4e5ad111b68b..67451e5eec31 100644
> >>> --- a/drivers/nvme/host/core.c
> >>> +++ b/drivers/nvme/host/core.c
> >>> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
> >>>    {
> >>>           if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
> >>>                   return -EBUSY;
> >>> +       flush_work(&ctrl->scan_work);
> >>>           if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
> >>>                   return -EBUSY;
> >>>
> >>
> >> This way might cause deadlock because there is sync IO in scan
> >> work context.
> >>
> > But I sort of hoped that I/O would be aborted when the controller is
> > resetting, which should cause the sync I/O to complete, right?
> >
> But indeed, you're right (of course).
>
> This should be better:
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 67451e5eec31..81900735a380 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3452,6 +3452,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
>   {
>          nvme_mpath_stop(ctrl);
>          nvme_stop_keep_alive(ctrl);
> +       flush_work(&ctrl->scan_work);
>          flush_work(&ctrl->async_event_work);
>          cancel_work_sync(&ctrl->fw_act_work);
>   }
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index d42bada25b54..ef20935a7202 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1761,8 +1761,8 @@ static void nvme_rdma_reset_ctrl_work(struct
> work_struct *work)
>          int ret;
>          bool changed;
>
> -       nvme_stop_ctrl(&ctrl->ctrl);
>          nvme_rdma_shutdown_ctrl(ctrl, false);
> +       nvme_stop_ctrl(&ctrl->ctrl);

After the controller is shutdown, no any IO can be done at all, then
flush_work(&ctrl->scan_work) will wait for ever.

Thanks,
Ming Lei

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[James Smart]

On 3/28/2019 5:11 AM, Hannes Reinecke wrote:
> On 3/28/19 12:48 PM, Hannes Reinecke wrote:
>> On 3/28/19 12:07 PM, Ming Lei wrote:
>>> On Thu, Mar 28, 2019@5:28 PM Hannes Reinecke <hare@suse.de> wrote:
>>>>
>>>> On 3/27/19 7:50 PM, James Smart wrote:
> [ .. ]
>>>>> But, Keiths statements make me nervous. With Hannes's patches, I 
>>>>> could
>>>>> easily see the case where the ns->queue was quiesced when 
>>>>> ns_remove is
>>>>> called, which I think causes issues with blk_freeze_queue. We 
>>>>> would need
>>>>> to have ns_remove, after unlinking, to release the queue. is this 
>>>>> right ?
>>>>>
>>>> Hmm. But maybe it's as simple as this:
>>>>
>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>> index 4e5ad111b68b..67451e5eec31 100644
>>>> --- a/drivers/nvme/host/core.c
>>>> +++ b/drivers/nvme/host/core.c
>>>> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
>>>> ?? {
>>>> ????????? if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>>>> ????????????????? return -EBUSY;
>>>> +?????? flush_work(&ctrl->scan_work);
>>>> ????????? if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
>>>> ????????????????? return -EBUSY;
>>>>
>>>
>>> This way might cause deadlock because there is sync IO in scan
>>> work context.
>>>
>> But I sort of hoped that I/O would be aborted when the controller is 
>> resetting, which should cause the sync I/O to complete, right?
>>
> But indeed, you're right (of course).
>
> This should be better:
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 67451e5eec31..81900735a380 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3452,6 +3452,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
> ?{
> ??????? nvme_mpath_stop(ctrl);
> ??????? nvme_stop_keep_alive(ctrl);
> +?????? flush_work(&ctrl->scan_work);
> ??????? flush_work(&ctrl->async_event_work);
> ??????? cancel_work_sync(&ctrl->fw_act_work);
> ?}
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index d42bada25b54..ef20935a7202 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1761,8 +1761,8 @@ static void nvme_rdma_reset_ctrl_work(struct 
> work_struct *work)
> ??????? int ret;
> ??????? bool changed;
>
> -?????? nvme_stop_ctrl(&ctrl->ctrl);
> ??????? nvme_rdma_shutdown_ctrl(ctrl, false);
> +?????? nvme_stop_ctrl(&ctrl->ctrl);
>
> ??????? if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
> ??????????????? /* state change failure should never happen */
>
> (We need to twiddle the order on RDMA, as we'd need to abort the I/O 
> first before we can flush the scan_work element).
> Cheers,
>
> Hannes

Looking at the flows - it looks like it should be ok. You're proving me 
wrong. Like changes need to be done in the other transports and they 
have to be audited for the same flows or the check_ready checks won't 
work right to continue to bounce ios so the flush can exit. I'm not sure 
having some of the async flows continue to run when doing the aborts as 
it may have side effects. But, I expect the issues aren't new. Simple 
change, but any change in this area can be dramatic.

It still bothers me how "happy path" the ns_scan path is. That the 
"validate" tears down the namespace on any error, even on a short 
connectivity disruption, shouldn't be happening. Deleting the namespace 
can have harsh consequences.

-- james

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Hannes Reinecke]

On 3/28/19 4:05 PM, James Smart wrote:
> 
> 
> On 3/28/2019 2:27 AM, Hannes Reinecke wrote:
>> On 3/27/19 7:50 PM, James Smart wrote:
>>> On 3/27/2019 8:12 AM, Hannes Reinecke wrote:
>>>> On 3/27/19 3:20 PM, Keith Busch wrote:
>>>>> On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
>>>>>> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>>>>> ????? if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
>>>>>> ????????? return;
>>>>>> ? +??? down_write(&ns->ctrl->namespaces_rwsem);
>>>>>> +??? list_del_init(&ns->list);
>>>>>> +??? up_write(&ns->ctrl->namespaces_rwsem);
>>>>>> +
>>>>>> ????? nvme_fault_inject_fini(ns);
>>>>>> ????? if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
>>>>>> ????????? del_gendisk(ns->disk);
>>>>>
>>>>> I think I understand the problem you've described, but I'm a little
>>>>> concerned with the early removal. This will call blk_cleanup_queue
>>>>> after the namespace is removed from our lists, and that does a 
>>>>> blocking
>>>>> queue_freeze. If commands happen to be dispatched to that namespace,
>>>>> and the controller for some reason stops responding, we may not be 
>>>>> able
>>>>> to recover during reset when the namespace queue is removed from the
>>>>> controller list. That's at least problematic if we have to go with the
>>>>> per-queue tag iterators that Jianchao proposed.
>>>>>
>>>> Hmm. I sort of see where you coming from.
>>>> But I was slightly worried about all the other callers where we iterate
>>>> the namespace list; one would need to to a _really_ careful audit 
>>>> which are safe to traverse even with namespace removal running; some 
>>>> have to, others must not, and some simply don't care.
>>>> Hence this approach to make things easier.
>>>
>>> I like and agree with Hannes's later points.
>>>
>>> But, Keiths statements make me nervous. With Hannes's patches, I 
>>> could easily see the case where the ns->queue was quiesced when 
>>> ns_remove is called, which I think causes issues with 
>>> blk_freeze_queue. We would need to have ns_remove, after unlinking, 
>>> to release the queue. is this right ?
>>>
>> Hmm. But maybe it's as simple as this:
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 4e5ad111b68b..67451e5eec31 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
>> ?{
>> ??????? if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>> ??????????????? return -EBUSY;
>> +?????? flush_work(&ctrl->scan_work);
>> ??????? if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
>> ??????????????? return -EBUSY;
>>
>> James? Keith?
>>
> No...
> 
> As mentioned in the other thread. If the reset processing is held off, 
> then nothing will kill the io that the scan thread has issued and is 
> pending on the link. In the best case, you must wait for the command 
> timeout to occur, which will want to re-invoke the reset path, which 
> you've noop'd here, so I'm not fully sure it will clean up the io.? And, 
> as reset hasn't run, the io queues on this controller have yet to be 
> quiesced so you're going to start seeing filesystem errors.? The other 
> headache - even if that 1st io is aborted and returned, just because the 
> 1 scan io errored, it doesn't stop the scan thread. The thread is coded 
> to keep looping on new namespaces so it'll attempt to send another/many 
> commands.? Which means the new io's will fall into the pass through the 
> check_ready routines as they will fall into the path that is only 
> prevented by the connection state being nil, which would not have been 
> set because the reset path was held off.?? Minimally a new change has to 
> be put there.?? This is not something that can be solved by scheduling 
> work elements at the transport level. The transport has no idea what 
> ns's are using its io request queue what states they are in. It's a ns 
> state issue.
> 
Please do check with my reply to Ming Lei.
We should be able to flush scan_work from nvme_stop_ctrl(); FC already 
stops all I/O before nvme_stop_ctrl() (so any sync I/O will be 
completed), and for RDMA we should be able to reshuffle nvme_stop_ctrl() 
and nvme_rdma_stop_io() to get to the same behaviour.

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 2/3] nvme: shorten race window in nvme_ns_remove()

[James Smart]

On 3/28/2019 8:19 AM, Ming Lei wrote:
> On Thu, Mar 28, 2019@8:11 PM Hannes Reinecke <hare@suse.de> wrote:
>> On 3/28/19 12:48 PM, Hannes Reinecke wrote:
>>> On 3/28/19 12:07 PM, Ming Lei wrote:
>>>> On Thu, Mar 28, 2019@5:28 PM Hannes Reinecke <hare@suse.de> wrote:
>>>>> On 3/27/19 7:50 PM, James Smart wrote:
>> [ .. ]
>>>>>> But, Keiths statements make me nervous. With Hannes's patches, I could
>>>>>> easily see the case where the ns->queue was quiesced when ns_remove is
>>>>>> called, which I think causes issues with blk_freeze_queue. We would
>>>>>> need
>>>>>> to have ns_remove, after unlinking, to release the queue. is this
>>>>>> right ?
>>>>>>
>>>>> Hmm. But maybe it's as simple as this:
>>>>>
>>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>>> index 4e5ad111b68b..67451e5eec31 100644
>>>>> --- a/drivers/nvme/host/core.c
>>>>> +++ b/drivers/nvme/host/core.c
>>>>> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
>>>>>     {
>>>>>            if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>>>>>                    return -EBUSY;
>>>>> +       flush_work(&ctrl->scan_work);
>>>>>            if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
>>>>>                    return -EBUSY;
>>>>>
>>>> This way might cause deadlock because there is sync IO in scan
>>>> work context.
>>>>
>>> But I sort of hoped that I/O would be aborted when the controller is
>>> resetting, which should cause the sync I/O to complete, right?
>>>
>> But indeed, you're right (of course).
>>
>> This should be better:
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 67451e5eec31..81900735a380 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3452,6 +3452,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
>>    {
>>           nvme_mpath_stop(ctrl);
>>           nvme_stop_keep_alive(ctrl);
>> +       flush_work(&ctrl->scan_work);
>>           flush_work(&ctrl->async_event_work);
>>           cancel_work_sync(&ctrl->fw_act_work);
>>    }
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index d42bada25b54..ef20935a7202 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1761,8 +1761,8 @@ static void nvme_rdma_reset_ctrl_work(struct
>> work_struct *work)
>>           int ret;
>>           bool changed;
>>
>> -       nvme_stop_ctrl(&ctrl->ctrl);
>>           nvme_rdma_shutdown_ctrl(ctrl, false);
>> +       nvme_stop_ctrl(&ctrl->ctrl);
> After the controller is shutdown, no any IO can be done at all, then
> flush_work(&ctrl->scan_work) will wait for ever.
>
> Thanks,
> Ming Lei

There's no guarantee io could have been done even prior to the shutdown 
- it would likely be accepted and be waiting for completion. Key is the 
shutdown has to abort/complete anything outstanding and set the 
state/flags to bounce anything that arrives after. How its bounced 
depends on the type of io - it may be a resource busy or an outright 
error. With scan io (admin cmds), it should be a hard error.? Thus 
scan_work thread will see a stream of io errors from that point on.

-- james

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Hannes Reinecke]

On 3/28/19 4:46 PM, James Smart wrote:
> 
> 
> On 3/28/2019 5:11 AM, Hannes Reinecke wrote:
>> On 3/28/19 12:48 PM, Hannes Reinecke wrote:
>>> On 3/28/19 12:07 PM, Ming Lei wrote:
>>>> On Thu, Mar 28, 2019@5:28 PM Hannes Reinecke <hare@suse.de> wrote:
>>>>>
>>>>> On 3/27/19 7:50 PM, James Smart wrote:
>> [ .. ]
>>>>>> But, Keiths statements make me nervous. With Hannes's patches, I 
>>>>>> could
>>>>>> easily see the case where the ns->queue was quiesced when 
>>>>>> ns_remove is
>>>>>> called, which I think causes issues with blk_freeze_queue. We 
>>>>>> would need
>>>>>> to have ns_remove, after unlinking, to release the queue. is this 
>>>>>> right ?
>>>>>>
>>>>> Hmm. But maybe it's as simple as this:
>>>>>
>>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>>> index 4e5ad111b68b..67451e5eec31 100644
>>>>> --- a/drivers/nvme/host/core.c
>>>>> +++ b/drivers/nvme/host/core.c
>>>>> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
>>>>> ?? {
>>>>> ????????? if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>>>>> ????????????????? return -EBUSY;
>>>>> +?????? flush_work(&ctrl->scan_work);
>>>>> ????????? if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
>>>>> ????????????????? return -EBUSY;
>>>>>
>>>>
>>>> This way might cause deadlock because there is sync IO in scan
>>>> work context.
>>>>
>>> But I sort of hoped that I/O would be aborted when the controller is 
>>> resetting, which should cause the sync I/O to complete, right?
>>>
>> But indeed, you're right (of course).
>>
>> This should be better:
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 67451e5eec31..81900735a380 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3452,6 +3452,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
>> ?{
>> ??????? nvme_mpath_stop(ctrl);
>> ??????? nvme_stop_keep_alive(ctrl);
>> +?????? flush_work(&ctrl->scan_work);
>> ??????? flush_work(&ctrl->async_event_work);
>> ??????? cancel_work_sync(&ctrl->fw_act_work);
>> ?}
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index d42bada25b54..ef20935a7202 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1761,8 +1761,8 @@ static void nvme_rdma_reset_ctrl_work(struct 
>> work_struct *work)
>> ??????? int ret;
>> ??????? bool changed;
>>
>> -?????? nvme_stop_ctrl(&ctrl->ctrl);
>> ??????? nvme_rdma_shutdown_ctrl(ctrl, false);
>> +?????? nvme_stop_ctrl(&ctrl->ctrl);
>>
>> ??????? if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
>> ??????????????? /* state change failure should never happen */
>>
>> (We need to twiddle the order on RDMA, as we'd need to abort the I/O 
>> first before we can flush the scan_work element).
>> Cheers,
>>
>> Hannes
> 
> Looking at the flows - it looks like it should be ok. You're proving me 
> wrong. Like changes need to be done in the other transports and they 
> have to be audited for the same flows or the check_ready checks won't 
> work right to continue to bounce ios so the flush can exit. I'm not sure 
> having some of the async flows continue to run when doing the aborts as 
> it may have side effects. But, I expect the issues aren't new. Simple 
> change, but any change in this area can be dramatic.
> 
> It still bothers me how "happy path" the ns_scan path is. That the 
> "validate" tears down the namespace on any error, even on a short 
> connectivity disruption, shouldn't be happening. Deleting the namespace 
> can have harsh consequences.
> 
Oh, I'm fully with you there.
It only took me some months to properly debug this issue.
Personally, I'd mark the namespace as 'faulty', and requeue I/O for 
these cases (much like we do already when an error is returned).
And then we can tear down namespaces after controller reset as we then 
have a stable state.

But that'll be quite a change to the flow of control, and I guess the 
powers that be have some opinion here.

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)

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[James Smart]

On 3/28/2019 8:57 AM, Hannes Reinecke wrote:
> On 3/28/19 4:46 PM, James Smart wrote:
>>
>>
>> On 3/28/2019 5:11 AM, Hannes Reinecke wrote:
>>> On 3/28/19 12:48 PM, Hannes Reinecke wrote:
>>>> On 3/28/19 12:07 PM, Ming Lei wrote:
>>>>> On Thu, Mar 28, 2019@5:28 PM Hannes Reinecke <hare@suse.de> wrote:
>>>>>>
>>>>>> On 3/27/19 7:50 PM, James Smart wrote:
>>> [ .. ]
>>>>>>> But, Keiths statements make me nervous. With Hannes's patches, I 
>>>>>>> could
>>>>>>> easily see the case where the ns->queue was quiesced when 
>>>>>>> ns_remove is
>>>>>>> called, which I think causes issues with blk_freeze_queue. We 
>>>>>>> would need
>>>>>>> to have ns_remove, after unlinking, to release the queue. is 
>>>>>>> this right ?
>>>>>>>
>>>>>> Hmm. But maybe it's as simple as this:
>>>>>>
>>>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>>>> index 4e5ad111b68b..67451e5eec31 100644
>>>>>> --- a/drivers/nvme/host/core.c
>>>>>> +++ b/drivers/nvme/host/core.c
>>>>>> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
>>>>>> ?? {
>>>>>> ????????? if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>>>>>> ????????????????? return -EBUSY;
>>>>>> +?????? flush_work(&ctrl->scan_work);
>>>>>> ????????? if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
>>>>>> ????????????????? return -EBUSY;
>>>>>>
>>>>>
>>>>> This way might cause deadlock because there is sync IO in scan
>>>>> work context.
>>>>>
>>>> But I sort of hoped that I/O would be aborted when the controller 
>>>> is resetting, which should cause the sync I/O to complete, right?
>>>>
>>> But indeed, you're right (of course).
>>>
>>> This should be better:
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index 67451e5eec31..81900735a380 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -3452,6 +3452,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
>>> ?{
>>> ??????? nvme_mpath_stop(ctrl);
>>> ??????? nvme_stop_keep_alive(ctrl);
>>> +?????? flush_work(&ctrl->scan_work);
>>> ??????? flush_work(&ctrl->async_event_work);
>>> ??????? cancel_work_sync(&ctrl->fw_act_work);
>>> ?}
>>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>>> index d42bada25b54..ef20935a7202 100644
>>> --- a/drivers/nvme/host/rdma.c
>>> +++ b/drivers/nvme/host/rdma.c
>>> @@ -1761,8 +1761,8 @@ static void nvme_rdma_reset_ctrl_work(struct 
>>> work_struct *work)
>>> ??????? int ret;
>>> ??????? bool changed;
>>>
>>> -?????? nvme_stop_ctrl(&ctrl->ctrl);
>>> ??????? nvme_rdma_shutdown_ctrl(ctrl, false);
>>> +?????? nvme_stop_ctrl(&ctrl->ctrl);
>>>
>>> ??????? if (!nvme_change_ctrl_state(&ctrl->ctrl, 
>>> NVME_CTRL_CONNECTING)) {
>>> ??????????????? /* state change failure should never happen */
>>>
>>> (We need to twiddle the order on RDMA, as we'd need to abort the I/O 
>>> first before we can flush the scan_work element).
>>> Cheers,
>>>
>>> Hannes
>>
>> Looking at the flows - it looks like it should be ok. You're proving 
>> me wrong. Like changes need to be done in the other transports and 
>> they have to be audited for the same flows or the check_ready checks 
>> won't work right to continue to bounce ios so the flush can exit. I'm 
>> not sure having some of the async flows continue to run when doing 
>> the aborts as it may have side effects. But, I expect the issues 
>> aren't new. Simple change, but any change in this area can be dramatic.
>>
>> It still bothers me how "happy path" the ns_scan path is. That the 
>> "validate" tears down the namespace on any error, even on a short 
>> connectivity disruption, shouldn't be happening. Deleting the 
>> namespace can have harsh consequences.
>>
> Oh, I'm fully with you there.
> It only took me some months to properly debug this issue.
> Personally, I'd mark the namespace as 'faulty', and requeue I/O for 
> these cases (much like we do already when an error is returned).
> And then we can tear down namespaces after controller reset as we then 
> have a stable state.
>
> But that'll be quite a change to the flow of control, and I guess the 
> powers that be have some opinion here.
>
> Cheers,
>
> Hannes

Actually, we didn't solve anything.... got caught up with - can 
scan_work make progress.

The error scenario is:
transport enters reset: it must quiesce queues, mark connections 
invalid, terminate io, unquiesce queues.
This part has to run as the terminate is required to return the scan io 
so that scan can make progress. Issue is, scan can make progress by 
terminating the ns, which means the ns may be tripping on the bitmap 
when the transport calls the unquiesce queues.

your patch didn't change this sequence as the sequence is in the 
"shutdown" path. It doesn't matter that scan is waited for later.

I'm back to the original fix: changing quiesce/unquiesce and adding an 
unquiesce to ns_remove.

-- james

[PATCH 2/3] nvme: shorten race window in nvme_ns_remove()

[Ming Lei]

On Thu, Mar 28, 2019@11:50 PM Hannes Reinecke <hare@suse.com> wrote:
>
> On 3/28/19 4:05 PM, James Smart wrote:
> >
> >
> > On 3/28/2019 2:27 AM, Hannes Reinecke wrote:
> >> On 3/27/19 7:50 PM, James Smart wrote:
> >>> On 3/27/2019 8:12 AM, Hannes Reinecke wrote:
> >>>> On 3/27/19 3:20 PM, Keith Busch wrote:
> >>>>> On Wed, Mar 27, 2019@01:09:28AM -0700, Hannes Reinecke wrote:
> >>>>>> @@ -3316,6 +3316,10 @@ static void nvme_ns_remove(struct nvme_ns *ns)
> >>>>>>       if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
> >>>>>>           return;
> >>>>>>   +    down_write(&ns->ctrl->namespaces_rwsem);
> >>>>>> +    list_del_init(&ns->list);
> >>>>>> +    up_write(&ns->ctrl->namespaces_rwsem);
> >>>>>> +
> >>>>>>       nvme_fault_inject_fini(ns);
> >>>>>>       if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
> >>>>>>           del_gendisk(ns->disk);
> >>>>>
> >>>>> I think I understand the problem you've described, but I'm a little
> >>>>> concerned with the early removal. This will call blk_cleanup_queue
> >>>>> after the namespace is removed from our lists, and that does a
> >>>>> blocking
> >>>>> queue_freeze. If commands happen to be dispatched to that namespace,
> >>>>> and the controller for some reason stops responding, we may not be
> >>>>> able
> >>>>> to recover during reset when the namespace queue is removed from the
> >>>>> controller list. That's at least problematic if we have to go with the
> >>>>> per-queue tag iterators that Jianchao proposed.
> >>>>>
> >>>> Hmm. I sort of see where you coming from.
> >>>> But I was slightly worried about all the other callers where we iterate
> >>>> the namespace list; one would need to to a _really_ careful audit
> >>>> which are safe to traverse even with namespace removal running; some
> >>>> have to, others must not, and some simply don't care.
> >>>> Hence this approach to make things easier.
> >>>
> >>> I like and agree with Hannes's later points.
> >>>
> >>> But, Keiths statements make me nervous. With Hannes's patches, I
> >>> could easily see the case where the ns->queue was quiesced when
> >>> ns_remove is called, which I think causes issues with
> >>> blk_freeze_queue. We would need to have ns_remove, after unlinking,
> >>> to release the queue. is this right ?
> >>>
> >> Hmm. But maybe it's as simple as this:
> >>
> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> >> index 4e5ad111b68b..67451e5eec31 100644
> >> --- a/drivers/nvme/host/core.c
> >> +++ b/drivers/nvme/host/core.c
> >> @@ -131,6 +131,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl)
> >>  {
> >>         if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
> >>                 return -EBUSY;
> >> +       flush_work(&ctrl->scan_work);
> >>         if (!queue_work(nvme_reset_wq, &ctrl->reset_work))
> >>                 return -EBUSY;
> >>
> >> James? Keith?
> >>
> > No...
> >
> > As mentioned in the other thread. If the reset processing is held off,
> > then nothing will kill the io that the scan thread has issued and is
> > pending on the link. In the best case, you must wait for the command
> > timeout to occur, which will want to re-invoke the reset path, which
> > you've noop'd here, so I'm not fully sure it will clean up the io.  And,
> > as reset hasn't run, the io queues on this controller have yet to be
> > quiesced so you're going to start seeing filesystem errors.  The other
> > headache - even if that 1st io is aborted and returned, just because the
> > 1 scan io errored, it doesn't stop the scan thread. The thread is coded
> > to keep looping on new namespaces so it'll attempt to send another/many
> > commands.  Which means the new io's will fall into the pass through the
> > check_ready routines as they will fall into the path that is only
> > prevented by the connection state being nil, which would not have been
> > set because the reset path was held off.   Minimally a new change has to
> > be put there.   This is not something that can be solved by scheduling
> > work elements at the transport level. The transport has no idea what
> > ns's are using its io request queue what states they are in. It's a ns
> > state issue.
> >
> Please do check with my reply to Ming Lei.
> We should be able to flush scan_work from nvme_stop_ctrl(); FC already
> stops all I/O before nvme_stop_ctrl() (so any sync I/O will be
> completed),

Not at all, these in-flight IOs are canceled from device queue, and they are
actually retried and requeued to blk-mq, and will be dispatched again after
the reset is done.

> and for RDMA we should be able to reshuffle nvme_stop_ctrl()
> and nvme_rdma_stop_io() to get to the same behaviour.


Thanks,
Ming Lei