[PATCH] nvme: drop scan_lock and always kick requeue list when removing namespaces

[Keith Busch]

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 at 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 at 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 at kernel.org>