On Wed, Jun 14, 2023 at 2:05 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote: > > Hi, > > 在 2023/06/14 11:47, Xiao Ni 写道: > > On Wed, Jun 14, 2023 at 9:48 AM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote: > >> > >> Hi, > >> > >> 在 2023/06/13 22:50, Xiao Ni 写道: > >>> > >>> 在 2023/5/29 下午9:20, Yu Kuai 写道: > >>>> From: Yu Kuai <yukuai3@xxxxxxxxxx> > >>>> > >>>> Our test found a following deadlock in raid10: > >>>> > >>>> 1) Issue a normal write, and such write failed: > >>>> > >>>> raid10_end_write_request > >>>> set_bit(R10BIO_WriteError, &r10_bio->state) > >>>> one_write_done > >>>> reschedule_retry > >>>> > >>>> // later from md thread > >>>> raid10d > >>>> handle_write_completed > >>>> list_add(&r10_bio->retry_list, &conf->bio_end_io_list) > >>>> > >>>> // later from md thread > >>>> raid10d > >>>> if (!test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags)) > >>>> list_move(conf->bio_end_io_list.prev, &tmp) > >>>> r10_bio = list_first_entry(&tmp, struct r10bio, retry_list) > >>>> raid_end_bio_io(r10_bio) > >>>> > >>>> Dependency chain 1: normal io is waiting for updating superblock > >>> > >>> Hi Kuai > >>> > >>> It looks like the above situation is more complex. It only needs a > >>> normal write and md_write_start needs to > >>> > >>> wait until the metadata is written to member disks, right? If so, it > >>> doesn't need to introduce raid10 write failure > >>> > >>> here. I guess, it should be your test case. It's nice, if you can put > >>> your test steps in the patch. But for the analysis > >>> > >>> of the deadlock here, it's better to be simple. > >> > >> Test script can be found here, it's pretty easy to trigger: > >> > >> https://patchwork.kernel.org/project/linux-raid/patch/20230529132826.2125392-4-yukuai1@xxxxxxxxxxxxxxx/ > > > > Thanks for this. > >> > >> While reviewing the related code, I found that io can only be added to > >> list bio_end_io_list from handle_write_completed() if such io failed, so > >> I think io failure is needed to trigger deadlock from daemon thread. > >> > >> I think the key point is how MD_SB_CHANGE_PENDING is set: > >> > >> 1) raid10_error() and rdev_set_badblocks(), trigger by io failure; > >> 2) raid10_write_request() related to reshape; > >> 3) md_write_start() and md_allow_write(), and mddev->in_sync is set, > >> however, I was thinking this is not a common case; > >> > >> 1) is used here because it's quite easy to trigger and this is what > >> we meet in real test. 3) is possible but I will say let's keep 1), I > >> don't think it's necessary to reporduce this deadlock through another > >> path again. > > > > It makes sense. Let's go back to the first path mentioned in the patch. > > > >> 1) Issue a normal write, and such write failed: > >> > >> raid10_end_write_request > >> set_bit(R10BIO_WriteError, &r10_bio->state) > >> one_write_done > >> reschedule_retry > > > > This is good. > >> > >> // later from md thread > >> raid10d > >> handle_write_completed > >> list_add(&r10_bio->retry_list, &conf->bio_end_io_list) > > > > I have a question here. It should run narrow_write_error in > > handle_write_completed. In the test case, will narrow_write_error run > > successfully? Or it fails and will call rdev_set_badblocks and > > md_error. So MD_RECOVERY_PENDING will be set? > > r10_bio will always be added to bio_end_io_list, no matter > narrow_write_error() succeed or not. The dependecy chain 1 here is just > indicate handle this r10_bio will wait for updating super block, it's > not where MD_RECOVERY_PENDING is set... > > And MD_RECOVERY_PENDING can be set from narrow_write_error() and other > places where rdev_set_badblocks() is called. Because in your patch, it doesn't show which step sets MD_RECOVERY_PENDING. It's the reason I need to guess. It's a normal write, so md_write_start can set the flag. In this case, it can cause the same deadlock. So it's better to give which step sets the flag. > > > >> > >> // later from md thread > >> raid10d > >> if (!test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags)) > -> It's here, if the flag is set, bio won't be handled. Yes. > >> list_move(conf->bio_end_io_list.prev, &tmp) > >> r10_bio = list_first_entry(&tmp, struct r10bio, retry_list) > >> raid_end_bio_io(r10_bio) > >> > >> Dependency chain 1: normal io is waiting for updating superblock > > > > It's a little hard to understand. Because it doesn't show how normal > > io waits for a superblock update. And based on your last email, I > > guess you want to say rdev_set_badblock sets MD_RECOVERY_PENDING, but > > the flag can't be cleared, so the bios can't be added to > > bio_end_io_list, so the io rquests can't be finished. > > It's not that bio can't be added to bio_end_io_list, it's that bio in > this list can't be handled if sb_flags is set. Sorry for this. I wanted to say the same thing. I understand the case totally. Regards Xiao > > Thanks, > Kuai > > > > Regards > > Xiao > >> > >> Thanks, > >> Kuai > >>> > >>>> > >>>> 2) Trigger a recovery: > >>>> > >>>> raid10_sync_request > >>>> raise_barrier > >>>> > >>>> Dependency chain 2: sync thread is waiting for normal io > >>>> > >>>> 3) echo idle/frozen to sync_action: > >>>> > >>>> action_store > >>>> mddev_lock > >>>> md_unregister_thread > >>>> kthread_stop > >>>> > >>>> Dependency chain 3: drop 'reconfig_mutex' is waiting for sync thread > >>>> > >>>> 4) md thread can't update superblock: > >>>> > >>>> raid10d > >>>> md_check_recovery > >>>> if (mddev_trylock(mddev)) > >>>> md_update_sb > >>>> > >>>> Dependency chain 4: update superblock is waiting for 'reconfig_mutex' > >>>> > >>>> Hence cyclic dependency exist, in order to fix the problem, we must > >>>> break one of them. Dependency 1 and 2 can't be broken because they are > >>>> foundation design. Dependency 4 may be possible if it can be guaranteed > >>>> that no io can be inflight, however, this requires a new mechanism which > >>>> seems complex. Dependency 3 is a good choice, because idle/frozen only > >>>> requires sync thread to finish, which can be done asynchronously that is > >>>> already implemented, and 'reconfig_mutex' is not needed anymore. > >>>> > >>>> This patch switch 'idle' and 'frozen' to wait sync thread to be done > >>>> asynchronously, and this patch also add a sequence counter to record how > >>>> many times sync thread is done, so that 'idle' won't keep waiting on new > >>>> started sync thread. > >>> > >>> In the patch, sync_seq is added in md_reap_sync_thread. In > >>> idle_sync_thread, if sync_seq isn't equal > >>> > >>> mddev->sync_seq, it should mean there is someone that stops the sync > >>> thread already, right? Why do > >>> > >>> you say 'new started sync thread' here? > >>> > >>> Regards > >>> > >>> Xiao > >>> > >>> > >>>> > >>>> Noted that raid456 has similiar deadlock([1]), and it's verified[2] this > >>>> deadlock can be fixed by this patch as well. > >>>> > >>>> [1] > >>>> https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@xxxxxxxxxxxxx/T/#t > >>>> > >>>> [2] > >>>> https://lore.kernel.org/linux-raid/e9067438-d713-f5f3-0d3d-9e6b0e9efa0e@xxxxxxxxxxxxxxx/ > >>>> > >>>> Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx> > >>>> --- > >>>> drivers/md/md.c | 23 +++++++++++++++++++---- > >>>> drivers/md/md.h | 2 ++ > >>>> 2 files changed, 21 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/md/md.c b/drivers/md/md.c > >>>> index 63a993b52cd7..7912de0e4d12 100644 > >>>> --- a/drivers/md/md.c > >>>> +++ b/drivers/md/md.c > >>>> @@ -652,6 +652,7 @@ void mddev_init(struct mddev *mddev) > >>>> timer_setup(&mddev->safemode_timer, md_safemode_timeout, 0); > >>>> atomic_set(&mddev->active, 1); > >>>> atomic_set(&mddev->openers, 0); > >>>> + atomic_set(&mddev->sync_seq, 0); > >>>> spin_lock_init(&mddev->lock); > >>>> atomic_set(&mddev->flush_pending, 0); > >>>> init_waitqueue_head(&mddev->sb_wait); > >>>> @@ -4776,19 +4777,27 @@ static void stop_sync_thread(struct mddev *mddev) > >>>> if (work_pending(&mddev->del_work)) > >>>> flush_workqueue(md_misc_wq); > >>>> - if (mddev->sync_thread) { > >>>> - set_bit(MD_RECOVERY_INTR, &mddev->recovery); > >>>> - md_reap_sync_thread(mddev); > >>>> - } > >>>> + set_bit(MD_RECOVERY_INTR, &mddev->recovery); > >>>> + /* > >>>> + * Thread might be blocked waiting for metadata update which will > >>>> now > >>>> + * never happen > >>>> + */ > >>>> + md_wakeup_thread_directly(mddev->sync_thread); > >>>> mddev_unlock(mddev); > >>>> } > >>>> static void idle_sync_thread(struct mddev *mddev) > >>>> { > >>>> + int sync_seq = atomic_read(&mddev->sync_seq); > >>>> + > >>>> mutex_lock(&mddev->sync_mutex); > >>>> clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery); > >>>> stop_sync_thread(mddev); > >>>> + > >>>> + wait_event(resync_wait, sync_seq != atomic_read(&mddev->sync_seq) || > >>>> + !test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)); > >>>> + > >>>> mutex_unlock(&mddev->sync_mutex); > >>>> } > >>>> @@ -4797,6 +4806,10 @@ static void frozen_sync_thread(struct mddev > >>>> *mddev) > >>>> mutex_init(&mddev->delete_mutex); > >>>> set_bit(MD_RECOVERY_FROZEN, &mddev->recovery); > >>>> stop_sync_thread(mddev); > >>>> + > >>>> + wait_event(resync_wait, mddev->sync_thread == NULL && > >>>> + !test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)); > >>>> + > >>>> mutex_unlock(&mddev->sync_mutex); > >>>> } > >>>> @@ -9472,6 +9485,8 @@ void md_reap_sync_thread(struct mddev *mddev) > >>>> /* resync has finished, collect result */ > >>>> md_unregister_thread(&mddev->sync_thread); > >>>> + atomic_inc(&mddev->sync_seq); > >>>> + > >>>> if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery) && > >>>> !test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery) && > >>>> mddev->degraded != mddev->raid_disks) { > >>>> diff --git a/drivers/md/md.h b/drivers/md/md.h > >>>> index 2fa903de5bd0..7cab9c7c45b8 100644 > >>>> --- a/drivers/md/md.h > >>>> +++ b/drivers/md/md.h > >>>> @@ -539,6 +539,8 @@ struct mddev { > >>>> /* Used to synchronize idle and frozen for action_store() */ > >>>> struct mutex sync_mutex; > >>>> + /* The sequence number for sync thread */ > >>>> + atomic_t sync_seq; > >>>> bool has_superblocks:1; > >>>> bool fail_last_dev:1; > >>> > >>> -- > >>> dm-devel mailing list > >>> dm-devel@xxxxxxxxxx > >>> https://listman.redhat.com/mailman/listinfo/dm-devel > >> > > > > . > > > -- dm-devel mailing list dm-devel@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel
