From: Li Nan <[email protected]>
'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'.
Signed-off-by: Li Nan <[email protected]>
---
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);
--
2.39.2
On Wed, Feb 7, 2024 at 1:32 AM <[email protected]> wrote:
>
> From: Li Nan <[email protected]>
>
> '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'.
Is this to fix [1]? Do we need some Fixes and/or Closes tags?
Could you please add steps to reproduce this issue?
Thanks,
Song
[1] https://bugzilla.kernel.org/show_bug.cgi?id=218459
>
> Signed-off-by: Li Nan <[email protected]>
> ---
> 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);
> --
> 2.39.2
>
在 2024/2/8 14:50, Song Liu 写道:
> On Wed, Feb 7, 2024 at 1:32 AM <[email protected]> wrote:
>>
>> From: Li Nan <[email protected]>
>>
>> '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'.
>
> Is this to fix [1]? Do we need some Fixes and/or Closes tags?
>
No. Just use another way to fix [2], and both [2] and this patch can fix
the issue. I am not sure about the root cause of [1] yet.
[2] https://patchwork.kernel.org/project/linux-raid/list/?series=812045
> Could you please add steps to reproduce this issue?
We need to modify the kernel, add sleep in md_submit_bio() and md_ioctl()
as below, and then:
1. mdadm -CR /dev/md0 -l1 -n2 /dev/sd[bc] #create a raid
2. echo 1 > /sys/module/md_mod/parameters/error_inject #enable sleep
3. 'mdadm --add /dev/md0 /dev/sda' #add a disk to raid
4. submit ioctl BLKRRPART to raid within 10s.
Changes of kernel:
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 350f5b22ba6f..ce16d319edf2 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -76,6 +76,8 @@ static DEFINE_SPINLOCK(pers_lock);
static const struct kobj_type md_ktype;
+static bool error_inject = false;
+
struct md_cluster_operations *md_cluster_ops;
EXPORT_SYMBOL(md_cluster_ops);
static struct module *md_cluster_mod;
@@ -372,6 +374,8 @@ static bool is_suspended(struct mddev *mddev, struct
bio *bio)
void md_handle_request(struct mddev *mddev, struct bio *bio)
{
+ if (error_inject)
+ ssleep(10);
check_suspended:
if (is_suspended(mddev, bio)) {
DEFINE_WAIT(__wait);
@@ -7752,6 +7756,8 @@ static int md_ioctl(struct block_device *bdev,
blk_mode_t mode,
*/
if (mddev->pers) {
mdu_disk_info_t info;
+ if (error_inject)
+ ssleep(10);
if (copy_from_user(&info, argp, sizeof(info)))
err = -EFAULT;
else if (!(info.state & (1<<MD_DISK_SYNC)))
@@ -10120,6 +10126,7 @@ module_param_call(start_ro, set_ro, get_ro, NULL,
S_IRUSR|S_IWUSR);
module_param(start_dirty_degraded, int, S_IRUGO|S_IWUSR);
module_param_call(new_array, add_named_array, NULL, NULL, S_IWUSR);
module_param(create_on_open, bool, S_IRUSR|S_IWUSR);
+module_param(error_inject, bool, S_IRUSR|S_IWUSR);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("MD RAID framework");
--
Thanks,
Nan
On Thu, Feb 8, 2024 at 12:44 AM Li Nan <[email protected]> wrote:
>
>
>
> 在 2024/2/8 14:50, Song Liu 写道:
> > On Wed, Feb 7, 2024 at 1:32 AM <[email protected]> wrote:
> >>
> >> From: Li Nan <[email protected]>
> >>
> >> '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'.
> >
> > Is this to fix [1]? Do we need some Fixes and/or Closes tags?
> >
>
> No. Just use another way to fix [2], and both [2] and this patch can fix
> the issue. I am not sure about the root cause of [1] yet.
>
> [2] https://patchwork.kernel.org/project/linux-raid/list/?series=812045
>
> > Could you please add steps to reproduce this issue?
>
> We need to modify the kernel, add sleep in md_submit_bio() and md_ioctl()
> as below, and then:
> 1. mdadm -CR /dev/md0 -l1 -n2 /dev/sd[bc] #create a raid
> 2. echo 1 > /sys/module/md_mod/parameters/error_inject #enable sleep
> 3. 'mdadm --add /dev/md0 /dev/sda' #add a disk to raid
> 4. submit ioctl BLKRRPART to raid within 10s.
The analysis makes sense. I also hit the issue a couple times without adding
extra delays. But I am not sure whether this is the best fix (I didn't find real
issues with it either).
Maybe we don't need to suspend the array for ADD_NEW_DISK? So that
something like the following might just work?
Thanks,
Song
@@ -7573,7 +7577,6 @@ static inline bool md_ioctl_valid(unsigned int cmd)
static bool md_ioctl_need_suspend(unsigned int cmd)
{
switch (cmd) {
- case ADD_NEW_DISK:
case HOT_ADD_DISK:
case HOT_REMOVE_DISK:
case SET_BITMAP_FILE:
On Thu, Feb 8, 2024 at 4:49 PM Song Liu <[email protected]> wrote:
>
> On Thu, Feb 8, 2024 at 12:44 AM Li Nan <[email protected]> wrote:
> >
> >
> >
> > 在 2024/2/8 14:50, Song Liu 写道:
> > > On Wed, Feb 7, 2024 at 1:32 AM <[email protected]> wrote:
> > >>
> > >> From: Li Nan <[email protected]>
> > >>
> > >> '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'.
> > >
> > > Is this to fix [1]? Do we need some Fixes and/or Closes tags?
> > >
> >
> > No. Just use another way to fix [2], and both [2] and this patch can fix
> > the issue. I am not sure about the root cause of [1] yet.
> >
> > [2] https://patchwork.kernel.org/project/linux-raid/list/?series=812045
> >
> > > Could you please add steps to reproduce this issue?
> >
> > We need to modify the kernel, add sleep in md_submit_bio() and md_ioctl()
> > as below, and then:
> > 1. mdadm -CR /dev/md0 -l1 -n2 /dev/sd[bc] #create a raid
> > 2. echo 1 > /sys/module/md_mod/parameters/error_inject #enable sleep
> > 3. 'mdadm --add /dev/md0 /dev/sda' #add a disk to raid
> > 4. submit ioctl BLKRRPART to raid within 10s.
>
> The analysis makes sense. I also hit the issue a couple times without adding
> extra delays. But I am not sure whether this is the best fix (I didn't find real
> issues with it either).
To be extra safe and future proof, we can do something like the
following to only
suspend the array for ADD_NEW_DISK on not-running arrays.
This appear to solve the problem reported in
https://bugzilla.kernel.org/show_bug.cgi?id=218459
Thanks,
Song
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 9e41a9aaba8b..395911d5f4d6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7570,10 +7570,11 @@ static inline bool md_ioctl_valid(unsigned int cmd)
}
}
-static bool md_ioctl_need_suspend(unsigned int cmd)
+static bool md_ioctl_need_suspend(struct mddev *mddev, unsigned int cmd)
{
switch (cmd) {
case ADD_NEW_DISK:
+ return mddev->pers != NULL;
case HOT_ADD_DISK:
case HOT_REMOVE_DISK:
case SET_BITMAP_FILE:
@@ -7625,6 +7626,7 @@ static int md_ioctl(struct block_device *bdev,
blk_mode_t mode,
void __user *argp = (void __user *)arg;
struct mddev *mddev = NULL;
bool did_set_md_closing = false;
+ bool need_suspend;
if (!md_ioctl_valid(cmd))
return -ENOTTY;
@@ -7716,8 +7718,11 @@ static int md_ioctl(struct block_device *bdev,
blk_mode_t mode,
if (!md_is_rdwr(mddev))
flush_work(&mddev->sync_work);
- err = md_ioctl_need_suspend(cmd) ? mddev_suspend_and_lock(mddev) :
- mddev_lock(mddev);
+ need_suspend = md_ioctl_need_suspend(mddev, cmd);
+ if (need_suspend)
+ err = mddev_suspend_and_lock(mddev);
+ else
+ err = mddev_lock(mddev);
if (err) {
pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n",
err, cmd);
@@ -7846,8 +7851,10 @@ static int md_ioctl(struct block_device *bdev,
blk_mode_t mode,
err != -EINVAL)
mddev->hold_active = 0;
- md_ioctl_need_suspend(cmd) ? mddev_unlock_and_resume(mddev) :
- mddev_unlock(mddev);
+ if (need_suspend)
+ mddev_unlock_and_resume(mddev);
+ else
+ mddev_unlock(mddev);
out:
if(did_set_md_closing)
Hi,
在 2024/02/17 3:03, Song Liu 写道:
> On Thu, Feb 8, 2024 at 4:49 PM Song Liu <[email protected]> wrote:
>>
>> On Thu, Feb 8, 2024 at 12:44 AM Li Nan <[email protected]> wrote:
>>>
>>>
>>>
>>> 在 2024/2/8 14:50, Song Liu 写道:
>>>> On Wed, Feb 7, 2024 at 1:32 AM <[email protected]> wrote:
>>>>>
>>>>> From: Li Nan <[email protected]>
>>>>>
>>>>> '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'.
>>>>
>>>> Is this to fix [1]? Do we need some Fixes and/or Closes tags?
>>>>
>>>
>>> No. Just use another way to fix [2], and both [2] and this patch can fix
>>> the issue. I am not sure about the root cause of [1] yet.
>>>
>>> [2] https://patchwork.kernel.org/project/linux-raid/list/?series=812045
>>>
>>>> Could you please add steps to reproduce this issue?
>>>
>>> We need to modify the kernel, add sleep in md_submit_bio() and md_ioctl()
>>> as below, and then:
>>> 1. mdadm -CR /dev/md0 -l1 -n2 /dev/sd[bc] #create a raid
>>> 2. echo 1 > /sys/module/md_mod/parameters/error_inject #enable sleep
>>> 3. 'mdadm --add /dev/md0 /dev/sda' #add a disk to raid
>>> 4. submit ioctl BLKRRPART to raid within 10s.
>>
>> The analysis makes sense. I also hit the issue a couple times without adding
>> extra delays. But I am not sure whether this is the best fix (I didn't find real
>> issues with it either).
>
> To be extra safe and future proof, we can do something like the
> following to only
> suspend the array for ADD_NEW_DISK on not-running arrays.
>
> This appear to solve the problem reported in
>
> https://bugzilla.kernel.org/show_bug.cgi?id=218459
>
> Thanks,
> Song
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 9e41a9aaba8b..395911d5f4d6 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7570,10 +7570,11 @@ static inline bool md_ioctl_valid(unsigned int cmd)
> }
> }
>
> -static bool md_ioctl_need_suspend(unsigned int cmd)
> +static bool md_ioctl_need_suspend(struct mddev *mddev, unsigned int cmd)
> {
> switch (cmd) {
> case ADD_NEW_DISK:
> + return mddev->pers != NULL;
Did you check already that this problem is not related that 'active_io'
is leaked for flush IO?
I don't understand the problem reported yet. If 'mddev->pers' is not set
yet, md_submit_bio() will return directly, and 'active_io' should not be
grabbed in the first place.
md_run() is the only place to convert 'mddev->pers' from NULL to a real
personality, and it's protected by 'reconfig_mutex', however,
md_ioctl_need_suspend() is called without 'reconfig_mutex', hence there
is a race condition:
md_ioctl_need_suspend array_state_store
// mddev->pers is NULL, return false
mddev_lock
do_md_run
mddev->pers = xxx
mddev_unlock
// mddev_suspend is not called
mddev_lock
md_add_new_disk
if (mddev->pers)
md_import_device
bind_rdev_to_array
add_bound_rdev
mddev->pers->hot_add_disk
-> hot add disk without suspending
Thanks,
Kuai
> case HOT_ADD_DISK:
> case HOT_REMOVE_DISK:
> case SET_BITMAP_FILE:
> @@ -7625,6 +7626,7 @@ static int md_ioctl(struct block_device *bdev,
> blk_mode_t mode,
> void __user *argp = (void __user *)arg;
> struct mddev *mddev = NULL;
> bool did_set_md_closing = false;
> + bool need_suspend;
>
> if (!md_ioctl_valid(cmd))
> return -ENOTTY;
> @@ -7716,8 +7718,11 @@ static int md_ioctl(struct block_device *bdev,
> blk_mode_t mode,
> if (!md_is_rdwr(mddev))
> flush_work(&mddev->sync_work);
>
> - err = md_ioctl_need_suspend(cmd) ? mddev_suspend_and_lock(mddev) :
> - mddev_lock(mddev);
> + need_suspend = md_ioctl_need_suspend(mddev, cmd);
> + if (need_suspend)
> + err = mddev_suspend_and_lock(mddev);
> + else
> + err = mddev_lock(mddev);
> if (err) {
> pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n",
> err, cmd);
> @@ -7846,8 +7851,10 @@ static int md_ioctl(struct block_device *bdev,
> blk_mode_t mode,
> err != -EINVAL)
> mddev->hold_active = 0;
>
> - md_ioctl_need_suspend(cmd) ? mddev_unlock_and_resume(mddev) :
> - mddev_unlock(mddev);
> + if (need_suspend)
> + mddev_unlock_and_resume(mddev);
> + else
> + mddev_unlock(mddev);
>
> out:
> if(did_set_md_closing)
> .
>
On Sat, Feb 17, 2024 at 11:47 PM Yu Kuai <[email protected]> wrote:
>
> Hi,
>
> 在 2024/02/17 3:03, Song Liu 写道:
> > On Thu, Feb 8, 2024 at 4:49 PM Song Liu <[email protected]> wrote:
> >>
> >> On Thu, Feb 8, 2024 at 12:44 AM Li Nan <[email protected]> wrote:
> >>>
> >>>
> >>>
> >>> 在 2024/2/8 14:50, Song Liu 写道:
> >>>> On Wed, Feb 7, 2024 at 1:32 AM <[email protected]> wrote:
> >>>>>
> >>>>> From: Li Nan <[email protected]>
> >>>>>
> >>>>> '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'.
> >>>>
> >>>> Is this to fix [1]? Do we need some Fixes and/or Closes tags?
> >>>>
> >>>
> >>> No. Just use another way to fix [2], and both [2] and this patch can fix
> >>> the issue. I am not sure about the root cause of [1] yet.
> >>>
> >>> [2] https://patchwork.kernel.org/project/linux-raid/list/?series=812045
> >>>
> >>>> Could you please add steps to reproduce this issue?
> >>>
> >>> We need to modify the kernel, add sleep in md_submit_bio() and md_ioctl()
> >>> as below, and then:
> >>> 1. mdadm -CR /dev/md0 -l1 -n2 /dev/sd[bc] #create a raid
> >>> 2. echo 1 > /sys/module/md_mod/parameters/error_inject #enable sleep
> >>> 3. 'mdadm --add /dev/md0 /dev/sda' #add a disk to raid
> >>> 4. submit ioctl BLKRRPART to raid within 10s.
> >>
> >> The analysis makes sense. I also hit the issue a couple times without adding
> >> extra delays. But I am not sure whether this is the best fix (I didn't find real
> >> issues with it either).
> >
> > To be extra safe and future proof, we can do something like the
> > following to only
> > suspend the array for ADD_NEW_DISK on not-running arrays.
> >
> > This appear to solve the problem reported in
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=218459
> >
> > Thanks,
> > Song
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 9e41a9aaba8b..395911d5f4d6 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -7570,10 +7570,11 @@ static inline bool md_ioctl_valid(unsigned int cmd)
> > }
> > }
> >
> > -static bool md_ioctl_need_suspend(unsigned int cmd)
> > +static bool md_ioctl_need_suspend(struct mddev *mddev, unsigned int cmd)
> > {
> > switch (cmd) {
> > case ADD_NEW_DISK:
> > + return mddev->pers != NULL;
>
> Did you check already that this problem is not related that 'active_io'
> is leaked for flush IO?
>
> I don't understand the problem reported yet. If 'mddev->pers' is not set
> yet, md_submit_bio() will return directly, and 'active_io' should not be
> grabbed in the first place.
AFAICT, this is not related to the active_io issue.
>
> md_run() is the only place to convert 'mddev->pers' from NULL to a real
> personality, and it's protected by 'reconfig_mutex', however,
> md_ioctl_need_suspend() is called without 'reconfig_mutex', hence there
> is a race condition:
>
> md_ioctl_need_suspend array_state_store
> // mddev->pers is NULL, return false
> mddev_lock
> do_md_run
> mddev->pers = xxx
> mddev_unlock
>
> // mddev_suspend is not called
> mddev_lock
> md_add_new_disk
> if (mddev->pers)
> md_import_device
> bind_rdev_to_array
> add_bound_rdev
> mddev->pers->hot_add_disk
> -> hot add disk without suspending
Yeah, this race condition exists. We probably need some
trick with suspend and lock here.
Thanks,
Song
Hi, Christoph
?? 2024/02/07 17:27, [email protected] д??:
> From: Li Nan <[email protected]>
>
> '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
>
> Signed-off-by: Li Nan <[email protected]>
> ---
> 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);
>
On Mon, Feb 19, 2024 at 04:53:36PM +0800, Yu Kuai wrote:
> 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.
Yes, not taking open_lock here seems reasonable, open_lock or it's
previous name has always been a bit of a catchall without very well
defined semantics. I'd give the symbol a blk_ prefix, though.