Re: [PATCH] block: fix deadlock between bd_link_disk_holder and partition scan
From: Yu Kuai <hidden>
Date: 2024-02-19 08:53:42
Also in:
linux-block, lkml
Hi, Christoph 在 2024/02/07 17:27, linan666@huaweicloud.com 写道:
From: Li Nan <redacted>
'open_mutex' of gendisk is used to protect open/close block devices. But
in bd_link_disk_holder(), it is used to protect the creation of symlink
between holding disk and slave bdev, which introduces some issues.
When bd_link_disk_holder() is called, the driver is usually in the process
of initialization/modification and may suspend submitting io. At this
time, any io hold 'open_mutex', such as scanning partitions, can cause
deadlocks. For example, in raid:
T1 T2
bdev_open_by_dev
lock open_mutex [1]
...
efi_partition
...
md_submit_bio
md_ioctl mddev_syspend
-> suspend all io
md_add_new_disk
bind_rdev_to_array
bd_link_disk_holder
try lock open_mutex [2]
md_handle_request
-> wait mddev_resume
T1 scan partition, T2 add a new device to raid. T1 waits for T2 to resume
mddev, but T2 waits for open_mutex held by T1. Deadlock occurs.
Fix it by introducing a local mutex 'holder_mutex' to replace 'open_mutex'.Can you take a look at this patch? I think for raid(perhaps and dm and other drivers), it's reasonable to suspend IO while hot adding new underlying disks. And I think add new slaves to holder is not related to open the holder disk, because caller should already open the holder disk to hot add slaves, hence 'open_mutex' for holder is not necessary here. Actually bd_link_disk_holder() is protected by 'reconfig_mutex' for raid, and 'table_devices_lock' for dm(I'm not sure yet if other drivers have similiar lock). For raid, we do can fix this problem in raid by delay bd_link_disk_holder() while the array is not suspended, however, we'll consider this fix later if you think this patch is not acceptable. Thanks, Kuai
quoted hunk ↗ jump to hunk
Signed-off-by: Li Nan <redacted> --- block/holder.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)diff --git a/block/holder.c b/block/holder.c index 37d18c13d958..5bfb0a674cc7 100644 --- a/block/holder.c +++ b/block/holder.c@@ -8,6 +8,8 @@ struct bd_holder_disk { int refcnt; }; +static DEFINE_MUTEX(holder_mutex); + static struct bd_holder_disk *bd_find_holder_disk(struct block_device *bdev, struct gendisk *disk) {@@ -80,7 +82,7 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk) kobject_get(bdev->bd_holder_dir); mutex_unlock(&bdev->bd_disk->open_mutex); - mutex_lock(&disk->open_mutex); + mutex_lock(&holder_mutex); WARN_ON_ONCE(!bdev->bd_holder); holder = bd_find_holder_disk(bdev, disk);@@ -108,7 +110,7 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk) goto out_del_symlink; list_add(&holder->list, &disk->slave_bdevs); - mutex_unlock(&disk->open_mutex); + mutex_unlock(&holder_mutex); return 0; out_del_symlink:@@ -116,7 +118,7 @@ int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk) out_free_holder: kfree(holder); out_unlock: - mutex_unlock(&disk->open_mutex); + mutex_unlock(&holder_mutex); if (ret) kobject_put(bdev->bd_holder_dir); return ret;@@ -140,7 +142,7 @@ void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk) if (WARN_ON_ONCE(!disk->slave_dir)) return; - mutex_lock(&disk->open_mutex); + mutex_lock(&holder_mutex); holder = bd_find_holder_disk(bdev, disk); if (!WARN_ON_ONCE(holder == NULL) && !--holder->refcnt) { del_symlink(disk->slave_dir, bdev_kobj(bdev));@@ -149,6 +151,6 @@ void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk) list_del_init(&holder->list); kfree(holder); } - mutex_unlock(&disk->open_mutex); + mutex_unlock(&holder_mutex); } EXPORT_SYMBOL_GPL(bd_unlink_disk_holder);