* [PATCH 0/3] nvme: fixup race with namespace removal
@ 2019-03-27 8:09 Hannes Reinecke
2019-03-27 8:09 ` [PATCH 1/3] nvme: do not quiesce or unquiesce invalid namespaces Hannes Reinecke
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Hannes Reinecke @ 2019-03-27 8:09 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/3] nvme: do not quiesce or unquiesce invalid namespaces 2019-03-27 8:09 [PATCH 0/3] nvme: fixup race with namespace removal Hannes Reinecke @ 2019-03-27 8:09 ` Hannes Reinecke 2019-03-27 8:09 ` [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() Hannes Reinecke 2019-03-27 8:09 ` [PATCH 3/3] nvme-multipath: test namespace state when selecting a new path Hannes Reinecke 2 siblings, 0 replies; 21+ messages in thread From: Hannes Reinecke @ 2019-03-27 8:09 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-27 8:09 [PATCH 0/3] nvme: fixup race with namespace removal Hannes Reinecke 2019-03-27 8:09 ` [PATCH 1/3] nvme: do not quiesce or unquiesce invalid namespaces Hannes Reinecke @ 2019-03-27 8:09 ` Hannes Reinecke 2019-03-27 14:02 ` Christoph Hellwig 2019-03-27 14:20 ` Keith Busch 2019-03-27 8:09 ` [PATCH 3/3] nvme-multipath: test namespace state when selecting a new path Hannes Reinecke 2 siblings, 2 replies; 21+ messages in thread From: Hannes Reinecke @ 2019-03-27 8:09 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-27 8:09 ` [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() Hannes Reinecke @ 2019-03-27 14:02 ` Christoph Hellwig 2019-03-27 14:41 ` Hannes Reinecke 2019-03-27 14:20 ` Keith Busch 1 sibling, 1 reply; 21+ messages in thread From: Christoph Hellwig @ 2019-03-27 14:02 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-27 14:02 ` Christoph Hellwig @ 2019-03-27 14:41 ` Hannes Reinecke 0 siblings, 0 replies; 21+ messages in thread From: Hannes Reinecke @ 2019-03-27 14:41 UTC (permalink / raw) 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-27 8:09 ` [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() Hannes Reinecke 2019-03-27 14:02 ` Christoph Hellwig @ 2019-03-27 14:20 ` Keith Busch 2019-03-27 15:12 ` Hannes Reinecke 1 sibling, 1 reply; 21+ messages in thread From: Keith Busch @ 2019-03-27 14:20 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-27 14:20 ` Keith Busch @ 2019-03-27 15:12 ` Hannes Reinecke 2019-03-27 18:50 ` James Smart 0 siblings, 1 reply; 21+ messages in thread From: Hannes Reinecke @ 2019-03-27 15:12 UTC (permalink / raw) 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-27 15:12 ` Hannes Reinecke @ 2019-03-27 18:50 ` James Smart 2019-03-28 9:27 ` Hannes Reinecke 0 siblings, 1 reply; 21+ messages in thread From: James Smart @ 2019-03-27 18:50 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-27 18:50 ` James Smart @ 2019-03-28 9:27 ` Hannes Reinecke 2019-03-28 11:07 ` Ming Lei 2019-03-28 15:05 ` James Smart 0 siblings, 2 replies; 21+ messages in thread From: Hannes Reinecke @ 2019-03-28 9:27 UTC (permalink / raw) 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) ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 9:27 ` Hannes Reinecke @ 2019-03-28 11:07 ` Ming Lei 2019-03-28 11:48 ` Hannes Reinecke 2019-03-28 15:05 ` James Smart 1 sibling, 1 reply; 21+ messages in thread From: Ming Lei @ 2019-03-28 11:07 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 11:07 ` Ming Lei @ 2019-03-28 11:48 ` Hannes Reinecke 2019-03-28 12:11 ` Hannes Reinecke 0 siblings, 1 reply; 21+ messages in thread From: Hannes Reinecke @ 2019-03-28 11:48 UTC (permalink / raw) 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 11:48 ` Hannes Reinecke @ 2019-03-28 12:11 ` Hannes Reinecke 2019-03-28 15:19 ` Ming Lei 2019-03-28 15:46 ` James Smart 0 siblings, 2 replies; 21+ messages in thread From: Hannes Reinecke @ 2019-03-28 12:11 UTC (permalink / raw) 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) ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 12:11 ` Hannes Reinecke @ 2019-03-28 15:19 ` Ming Lei 2019-03-28 15:56 ` James Smart 2019-03-28 15:46 ` James Smart 1 sibling, 1 reply; 21+ messages in thread From: Ming Lei @ 2019-03-28 15:19 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 15:19 ` Ming Lei @ 2019-03-28 15:56 ` James Smart 0 siblings, 0 replies; 21+ messages in thread From: James Smart @ 2019-03-28 15:56 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 12:11 ` Hannes Reinecke 2019-03-28 15:19 ` Ming Lei @ 2019-03-28 15:46 ` James Smart 2019-03-28 15:57 ` Hannes Reinecke 1 sibling, 1 reply; 21+ messages in thread From: James Smart @ 2019-03-28 15:46 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 15:46 ` James Smart @ 2019-03-28 15:57 ` Hannes Reinecke 2019-03-28 17:50 ` James Smart 0 siblings, 1 reply; 21+ messages in thread From: Hannes Reinecke @ 2019-03-28 15:57 UTC (permalink / raw) 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 15:57 ` Hannes Reinecke @ 2019-03-28 17:50 ` James Smart 0 siblings, 0 replies; 21+ messages in thread From: James Smart @ 2019-03-28 17:50 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 9:27 ` Hannes Reinecke 2019-03-28 11:07 ` Ming Lei @ 2019-03-28 15:05 ` James Smart 2019-03-28 15:50 ` Hannes Reinecke 1 sibling, 1 reply; 21+ messages in thread From: James Smart @ 2019-03-28 15:05 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 15:05 ` James Smart @ 2019-03-28 15:50 ` Hannes Reinecke 2019-03-29 0:40 ` Ming Lei 0 siblings, 1 reply; 21+ messages in thread From: Hannes Reinecke @ 2019-03-28 15:50 UTC (permalink / raw) 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() 2019-03-28 15:50 ` Hannes Reinecke @ 2019-03-29 0:40 ` Ming Lei 0 siblings, 0 replies; 21+ messages in thread From: Ming Lei @ 2019-03-29 0:40 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/3] nvme-multipath: test namespace state when selecting a new path 2019-03-27 8:09 [PATCH 0/3] nvme: fixup race with namespace removal Hannes Reinecke 2019-03-27 8:09 ` [PATCH 1/3] nvme: do not quiesce or unquiesce invalid namespaces Hannes Reinecke 2019-03-27 8:09 ` [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() Hannes Reinecke @ 2019-03-27 8:09 ` Hannes Reinecke 2 siblings, 0 replies; 21+ messages in thread From: Hannes Reinecke @ 2019-03-27 8:09 UTC (permalink / raw) 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-03-29 0:40 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-27 8:09 [PATCH 0/3] nvme: fixup race with namespace removal Hannes Reinecke 2019-03-27 8:09 ` [PATCH 1/3] nvme: do not quiesce or unquiesce invalid namespaces Hannes Reinecke 2019-03-27 8:09 ` [PATCH 2/3] nvme: shorten race window in nvme_ns_remove() Hannes Reinecke 2019-03-27 14:02 ` Christoph Hellwig 2019-03-27 14:41 ` Hannes Reinecke 2019-03-27 14:20 ` Keith Busch 2019-03-27 15:12 ` Hannes Reinecke 2019-03-27 18:50 ` James Smart 2019-03-28 9:27 ` Hannes Reinecke 2019-03-28 11:07 ` Ming Lei 2019-03-28 11:48 ` Hannes Reinecke 2019-03-28 12:11 ` Hannes Reinecke 2019-03-28 15:19 ` Ming Lei 2019-03-28 15:56 ` James Smart 2019-03-28 15:46 ` James Smart 2019-03-28 15:57 ` Hannes Reinecke 2019-03-28 17:50 ` James Smart 2019-03-28 15:05 ` James Smart 2019-03-28 15:50 ` Hannes Reinecke 2019-03-29 0:40 ` Ming Lei 2019-03-27 8:09 ` [PATCH 3/3] nvme-multipath: test namespace state when selecting a new path Hannes Reinecke
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.