* [PATCH] nvme: drop scan_lock and always kick requeue list when removing namespaces
@ 2021-10-20 5:59 Hannes Reinecke
2021-10-20 15:33 ` Sagi Grimberg
0 siblings, 1 reply; 4+ messages in thread
From: Hannes Reinecke @ 2021-10-20 5:59 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
When reading the partition table on initial scan hits an I/O error the
I/O will hang with the scan_mutex held:
[<0>] do_read_cache_page+0x49b/0x790
[<0>] read_part_sector+0x39/0xe0
[<0>] read_lba+0xf9/0x1d0
[<0>] efi_partition+0xf1/0x7f0
[<0>] bdev_disk_changed+0x1ee/0x550
[<0>] blkdev_get_whole+0x81/0x90
[<0>] blkdev_get_by_dev+0x128/0x2e0
[<0>] device_add_disk+0x377/0x3c0
[<0>] nvme_mpath_set_live+0x130/0x1b0 [nvme_core]
[<0>] nvme_mpath_add_disk+0x150/0x160 [nvme_core]
[<0>] nvme_alloc_ns+0x417/0x950 [nvme_core]
[<0>] nvme_validate_or_alloc_ns+0xe9/0x1e0 [nvme_core]
[<0>] nvme_scan_work+0x168/0x310 [nvme_core]
[<0>] process_one_work+0x231/0x420
and trying to delete the controller will deadlock as it tries to grab
the scan mutex:
[<0>] nvme_mpath_clear_ctrl_paths+0x25/0x80 [nvme_core]
[<0>] nvme_remove_namespaces+0x31/0xf0 [nvme_core]
[<0>] nvme_do_delete_ctrl+0x4b/0x80 [nvme_core]
As we're now properly ordering the namespace list there is no need to
hold the scan_mutex in nvme_mpath_clear_ctrl_paths() anymore.
And we always need to kick the requeue list as the path will be marked
as unusable and I/O will be requeued _without_ a current path.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/multipath.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index fb96e900dd3a..475224ecee8b 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -141,13 +141,12 @@ void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl)
{
struct nvme_ns *ns;
- mutex_lock(&ctrl->scan_lock);
down_read(&ctrl->namespaces_rwsem);
- list_for_each_entry(ns, &ctrl->namespaces, list)
- if (nvme_mpath_clear_current_path(ns))
- kblockd_schedule_work(&ns->head->requeue_work);
+ list_for_each_entry(ns, &ctrl->namespaces, list) {
+ nvme_mpath_clear_current_path(ns);
+ kblockd_schedule_work(&ns->head->requeue_work);
+ }
up_read(&ctrl->namespaces_rwsem);
- mutex_unlock(&ctrl->scan_lock);
}
void nvme_mpath_revalidate_paths(struct nvme_ns *ns)
--
2.29.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] nvme: drop scan_lock and always kick requeue list when removing namespaces
2021-10-20 5:59 [PATCH] nvme: drop scan_lock and always kick requeue list when removing namespaces Hannes Reinecke
@ 2021-10-20 15:33 ` Sagi Grimberg
2021-10-20 15:57 ` Keith Busch
2021-10-20 17:25 ` Christoph Hellwig
0 siblings, 2 replies; 4+ messages in thread
From: Sagi Grimberg @ 2021-10-20 15:33 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Christoph Hellwig, Keith Busch, linux-nvme, Anton Eidelman
On 10/20/21 8:59 AM, Hannes Reinecke wrote:
> When reading the partition table on initial scan hits an I/O error the
> I/O will hang with the scan_mutex held:
>
> [<0>] do_read_cache_page+0x49b/0x790
> [<0>] read_part_sector+0x39/0xe0
> [<0>] read_lba+0xf9/0x1d0
> [<0>] efi_partition+0xf1/0x7f0
> [<0>] bdev_disk_changed+0x1ee/0x550
> [<0>] blkdev_get_whole+0x81/0x90
> [<0>] blkdev_get_by_dev+0x128/0x2e0
> [<0>] device_add_disk+0x377/0x3c0
> [<0>] nvme_mpath_set_live+0x130/0x1b0 [nvme_core]
> [<0>] nvme_mpath_add_disk+0x150/0x160 [nvme_core]
> [<0>] nvme_alloc_ns+0x417/0x950 [nvme_core]
> [<0>] nvme_validate_or_alloc_ns+0xe9/0x1e0 [nvme_core]
> [<0>] nvme_scan_work+0x168/0x310 [nvme_core]
> [<0>] process_one_work+0x231/0x420
>
> and trying to delete the controller will deadlock as it tries to grab
> the scan mutex:
>
> [<0>] nvme_mpath_clear_ctrl_paths+0x25/0x80 [nvme_core]
> [<0>] nvme_remove_namespaces+0x31/0xf0 [nvme_core]
> [<0>] nvme_do_delete_ctrl+0x4b/0x80 [nvme_core]
>
> As we're now properly ordering the namespace list there is no need to
> hold the scan_mutex in nvme_mpath_clear_ctrl_paths() anymore.
> And we always need to kick the requeue list as the path will be marked
> as unusable and I/O will be requeued _without_ a current path.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/nvme/host/multipath.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index fb96e900dd3a..475224ecee8b 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -141,13 +141,12 @@ void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl)
> {
> struct nvme_ns *ns;
>
> - mutex_lock(&ctrl->scan_lock);
> down_read(&ctrl->namespaces_rwsem);
> - list_for_each_entry(ns, &ctrl->namespaces, list)
> - if (nvme_mpath_clear_current_path(ns))
> - kblockd_schedule_work(&ns->head->requeue_work);
> + list_for_each_entry(ns, &ctrl->namespaces, list) {
> + nvme_mpath_clear_current_path(ns);
> + kblockd_schedule_work(&ns->head->requeue_work);
> + }
> up_read(&ctrl->namespaces_rwsem);
> - mutex_unlock(&ctrl->scan_lock);
> }
>
> void nvme_mpath_revalidate_paths(struct nvme_ns *ns)
I think this works,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Christoph, Keith, Hannes, Chaitanya,
Do you see any issues with this?
Anton, can you also give your opinion here?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] nvme: drop scan_lock and always kick requeue list when removing namespaces
2021-10-20 15:33 ` Sagi Grimberg
@ 2021-10-20 15:57 ` Keith Busch
2021-10-20 17:25 ` Christoph Hellwig
1 sibling, 0 replies; 4+ messages in thread
From: Keith Busch @ 2021-10-20 15:57 UTC (permalink / raw)
To: Sagi Grimberg
Cc: Hannes Reinecke, Christoph Hellwig, Keith Busch, linux-nvme,
Anton Eidelman
On Wed, Oct 20, 2021 at 06:33:48PM +0300, Sagi Grimberg wrote:
> On 10/20/21 8:59 AM, Hannes Reinecke wrote:
> > When reading the partition table on initial scan hits an I/O error the
> > I/O will hang with the scan_mutex held:
> >
> > [<0>] do_read_cache_page+0x49b/0x790
> > [<0>] read_part_sector+0x39/0xe0
> > [<0>] read_lba+0xf9/0x1d0
> > [<0>] efi_partition+0xf1/0x7f0
> > [<0>] bdev_disk_changed+0x1ee/0x550
> > [<0>] blkdev_get_whole+0x81/0x90
> > [<0>] blkdev_get_by_dev+0x128/0x2e0
> > [<0>] device_add_disk+0x377/0x3c0
> > [<0>] nvme_mpath_set_live+0x130/0x1b0 [nvme_core]
> > [<0>] nvme_mpath_add_disk+0x150/0x160 [nvme_core]
> > [<0>] nvme_alloc_ns+0x417/0x950 [nvme_core]
> > [<0>] nvme_validate_or_alloc_ns+0xe9/0x1e0 [nvme_core]
> > [<0>] nvme_scan_work+0x168/0x310 [nvme_core]
> > [<0>] process_one_work+0x231/0x420
> >
> > and trying to delete the controller will deadlock as it tries to grab
> > the scan mutex:
> >
> > [<0>] nvme_mpath_clear_ctrl_paths+0x25/0x80 [nvme_core]
> > [<0>] nvme_remove_namespaces+0x31/0xf0 [nvme_core]
> > [<0>] nvme_do_delete_ctrl+0x4b/0x80 [nvme_core]
> >
> > As we're now properly ordering the namespace list there is no need to
> > hold the scan_mutex in nvme_mpath_clear_ctrl_paths() anymore.
> > And we always need to kick the requeue list as the path will be marked
> > as unusable and I/O will be requeued _without_ a current path.
> >
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> > drivers/nvme/host/multipath.c | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> > index fb96e900dd3a..475224ecee8b 100644
> > --- a/drivers/nvme/host/multipath.c
> > +++ b/drivers/nvme/host/multipath.c
> > @@ -141,13 +141,12 @@ void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl)
> > {
> > struct nvme_ns *ns;
> > - mutex_lock(&ctrl->scan_lock);
> > down_read(&ctrl->namespaces_rwsem);
> > - list_for_each_entry(ns, &ctrl->namespaces, list)
> > - if (nvme_mpath_clear_current_path(ns))
> > - kblockd_schedule_work(&ns->head->requeue_work);
> > + list_for_each_entry(ns, &ctrl->namespaces, list) {
> > + nvme_mpath_clear_current_path(ns);
> > + kblockd_schedule_work(&ns->head->requeue_work);
> > + }
> > up_read(&ctrl->namespaces_rwsem);
> > - mutex_unlock(&ctrl->scan_lock);
> > }
> > void nvme_mpath_revalidate_paths(struct nvme_ns *ns)
>
> I think this works,
>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
>
> Christoph, Keith, Hannes, Chaitanya,
> Do you see any issues with this?
No issue that I can see. The namespaces_rwsem looks like all we need
here.
Reviewed-by:
Keith Busch <kbusch@kernel.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] nvme: drop scan_lock and always kick requeue list when removing namespaces
2021-10-20 15:33 ` Sagi Grimberg
2021-10-20 15:57 ` Keith Busch
@ 2021-10-20 17:25 ` Christoph Hellwig
1 sibling, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2021-10-20 17:25 UTC (permalink / raw)
To: Sagi Grimberg
Cc: Hannes Reinecke, Christoph Hellwig, Keith Busch, linux-nvme,
Anton Eidelman
On Wed, Oct 20, 2021 at 06:33:48PM +0300, Sagi Grimberg wrote:
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
>
> Christoph, Keith, Hannes, Chaitanya,
> Do you see any issues with this?
No. I'll add it to nvme-5.16.
I'd love to kill the scan mutex entirely. For that we'll probably
need a per-tagset freeze in the block layer, which seems like a
pretty good idea to start with.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-20 17:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-20 5:59 [PATCH] nvme: drop scan_lock and always kick requeue list when removing namespaces Hannes Reinecke
2021-10-20 15:33 ` Sagi Grimberg
2021-10-20 15:57 ` Keith Busch
2021-10-20 17:25 ` Christoph Hellwig
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.