From: Yu Kuai <yukuai1@huaweicloud.com> To: guoqing.jiang@linux.dev, agk@redhat.com, snitzer@kernel.org, dm-devel@redhat.com, song@kernel.org Cc: linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org, yukuai3@huawei.com, yukuai1@huaweicloud.com, yi.zhang@huawei.com, yangerkun@huawei.com Subject: [PATCH -next v2 1/6] Revert "md: unlock mddev before reap sync_thread in action_store" Date: Mon, 29 May 2023 21:20:32 +0800 [thread overview] Message-ID: <20230529132037.2124527-2-yukuai1@huaweicloud.com> (raw) In-Reply-To: <20230529132037.2124527-1-yukuai1@huaweicloud.com> From: Yu Kuai <yukuai3@huawei.com> This reverts commit 9dfbdafda3b34e262e43e786077bab8e476a89d1. Because it will introduce a defect that sync_thread can be running while MD_RECOVERY_RUNNING is cleared, which will cause some unexpected problems, for example: list_add corruption. prev->next should be next (ffff0001ac1daba0), but was ffff0000ce1a02a0. (prev=ffff0000ce1a02a0). Call trace: __list_add_valid+0xfc/0x140 insert_work+0x78/0x1a0 __queue_work+0x500/0xcf4 queue_work_on+0xe8/0x12c md_check_recovery+0xa34/0xf30 raid10d+0xb8/0x900 [raid10] md_thread+0x16c/0x2cc kthread+0x1a4/0x1ec ret_from_fork+0x10/0x18 This is because work is requeued while it's still inside workqueue: t1: t2: action_store mddev_lock if (mddev->sync_thread) mddev_unlock md_unregister_thread // first sync_thread is done md_check_recovery mddev_try_lock /* * once MD_RECOVERY_DONE is set, new sync_thread * can start. */ set_bit(MD_RECOVERY_RUNNING, &mddev->recovery) INIT_WORK(&mddev->del_work, md_start_sync) queue_work(md_misc_wq, &mddev->del_work) test_and_set_bit(WORK_STRUCT_PENDING_BIT, ...) // set pending bit insert_work list_add_tail mddev_unlock mddev_lock_nointr md_reap_sync_thread // MD_RECOVERY_RUNNING is cleared mddev_unlock t3: // before queued work started from t2 md_check_recovery // MD_RECOVERY_RUNNING is not set, a new sync_thread can be started INIT_WORK(&mddev->del_work, md_start_sync) work->data = 0 // work pending bit is cleared queue_work(md_misc_wq, &mddev->del_work) insert_work list_add_tail // list is corrupted The above commit is reverted to fix the problem, the deadlock this commit tries to fix will be fixed in following patches. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- drivers/md/dm-raid.c | 1 - drivers/md/md.c | 19 ++----------------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c index 8846bf510a35..1f22bef27841 100644 --- a/drivers/md/dm-raid.c +++ b/drivers/md/dm-raid.c @@ -3725,7 +3725,6 @@ static int raid_message(struct dm_target *ti, unsigned int argc, char **argv, if (!strcasecmp(argv[0], "idle") || !strcasecmp(argv[0], "frozen")) { if (mddev->sync_thread) { set_bit(MD_RECOVERY_INTR, &mddev->recovery); - md_unregister_thread(&mddev->sync_thread); md_reap_sync_thread(mddev); } } else if (decipher_sync_action(mddev, mddev->recovery) != st_idle) diff --git a/drivers/md/md.c b/drivers/md/md.c index a5a7af2f4e59..9b97731e1fe4 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -4772,19 +4772,6 @@ action_store(struct mddev *mddev, const char *page, size_t len) if (work_pending(&mddev->del_work)) flush_workqueue(md_misc_wq); if (mddev->sync_thread) { - sector_t save_rp = mddev->reshape_position; - - mddev_unlock(mddev); - set_bit(MD_RECOVERY_INTR, &mddev->recovery); - md_unregister_thread(&mddev->sync_thread); - mddev_lock_nointr(mddev); - /* - * set RECOVERY_INTR again and restore reshape - * position in case others changed them after - * got lock, eg, reshape_position_store and - * md_check_recovery. - */ - mddev->reshape_position = save_rp; set_bit(MD_RECOVERY_INTR, &mddev->recovery); md_reap_sync_thread(mddev); } @@ -6184,7 +6171,6 @@ static void __md_stop_writes(struct mddev *mddev) flush_workqueue(md_misc_wq); if (mddev->sync_thread) { set_bit(MD_RECOVERY_INTR, &mddev->recovery); - md_unregister_thread(&mddev->sync_thread); md_reap_sync_thread(mddev); } @@ -9336,7 +9322,6 @@ void md_check_recovery(struct mddev *mddev) * ->spare_active and clear saved_raid_disk */ set_bit(MD_RECOVERY_INTR, &mddev->recovery); - md_unregister_thread(&mddev->sync_thread); md_reap_sync_thread(mddev); clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery); clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery); @@ -9372,7 +9357,6 @@ void md_check_recovery(struct mddev *mddev) goto unlock; } if (mddev->sync_thread) { - md_unregister_thread(&mddev->sync_thread); md_reap_sync_thread(mddev); goto unlock; } @@ -9452,7 +9436,8 @@ void md_reap_sync_thread(struct mddev *mddev) sector_t old_dev_sectors = mddev->dev_sectors; bool is_reshaped = false; - /* sync_thread should be unregistered, collect result */ + /* resync has finished, collect result */ + md_unregister_thread(&mddev->sync_thread); if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery) && !test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery) && mddev->degraded != mddev->raid_disks) { -- 2.39.2
WARNING: multiple messages have this Message-ID (diff)
From: Yu Kuai <yukuai1@huaweicloud.com> To: guoqing.jiang@linux.dev, agk@redhat.com, snitzer@kernel.org, dm-devel@redhat.com, song@kernel.org Cc: yi.zhang@huawei.com, yangerkun@huawei.com, linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org, yukuai1@huaweicloud.com, yukuai3@huawei.com Subject: [dm-devel] [PATCH -next v2 1/6] Revert "md: unlock mddev before reap sync_thread in action_store" Date: Mon, 29 May 2023 21:20:32 +0800 [thread overview] Message-ID: <20230529132037.2124527-2-yukuai1@huaweicloud.com> (raw) In-Reply-To: <20230529132037.2124527-1-yukuai1@huaweicloud.com> From: Yu Kuai <yukuai3@huawei.com> This reverts commit 9dfbdafda3b34e262e43e786077bab8e476a89d1. Because it will introduce a defect that sync_thread can be running while MD_RECOVERY_RUNNING is cleared, which will cause some unexpected problems, for example: list_add corruption. prev->next should be next (ffff0001ac1daba0), but was ffff0000ce1a02a0. (prev=ffff0000ce1a02a0). Call trace: __list_add_valid+0xfc/0x140 insert_work+0x78/0x1a0 __queue_work+0x500/0xcf4 queue_work_on+0xe8/0x12c md_check_recovery+0xa34/0xf30 raid10d+0xb8/0x900 [raid10] md_thread+0x16c/0x2cc kthread+0x1a4/0x1ec ret_from_fork+0x10/0x18 This is because work is requeued while it's still inside workqueue: t1: t2: action_store mddev_lock if (mddev->sync_thread) mddev_unlock md_unregister_thread // first sync_thread is done md_check_recovery mddev_try_lock /* * once MD_RECOVERY_DONE is set, new sync_thread * can start. */ set_bit(MD_RECOVERY_RUNNING, &mddev->recovery) INIT_WORK(&mddev->del_work, md_start_sync) queue_work(md_misc_wq, &mddev->del_work) test_and_set_bit(WORK_STRUCT_PENDING_BIT, ...) // set pending bit insert_work list_add_tail mddev_unlock mddev_lock_nointr md_reap_sync_thread // MD_RECOVERY_RUNNING is cleared mddev_unlock t3: // before queued work started from t2 md_check_recovery // MD_RECOVERY_RUNNING is not set, a new sync_thread can be started INIT_WORK(&mddev->del_work, md_start_sync) work->data = 0 // work pending bit is cleared queue_work(md_misc_wq, &mddev->del_work) insert_work list_add_tail // list is corrupted The above commit is reverted to fix the problem, the deadlock this commit tries to fix will be fixed in following patches. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- drivers/md/dm-raid.c | 1 - drivers/md/md.c | 19 ++----------------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c index 8846bf510a35..1f22bef27841 100644 --- a/drivers/md/dm-raid.c +++ b/drivers/md/dm-raid.c @@ -3725,7 +3725,6 @@ static int raid_message(struct dm_target *ti, unsigned int argc, char **argv, if (!strcasecmp(argv[0], "idle") || !strcasecmp(argv[0], "frozen")) { if (mddev->sync_thread) { set_bit(MD_RECOVERY_INTR, &mddev->recovery); - md_unregister_thread(&mddev->sync_thread); md_reap_sync_thread(mddev); } } else if (decipher_sync_action(mddev, mddev->recovery) != st_idle) diff --git a/drivers/md/md.c b/drivers/md/md.c index a5a7af2f4e59..9b97731e1fe4 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -4772,19 +4772,6 @@ action_store(struct mddev *mddev, const char *page, size_t len) if (work_pending(&mddev->del_work)) flush_workqueue(md_misc_wq); if (mddev->sync_thread) { - sector_t save_rp = mddev->reshape_position; - - mddev_unlock(mddev); - set_bit(MD_RECOVERY_INTR, &mddev->recovery); - md_unregister_thread(&mddev->sync_thread); - mddev_lock_nointr(mddev); - /* - * set RECOVERY_INTR again and restore reshape - * position in case others changed them after - * got lock, eg, reshape_position_store and - * md_check_recovery. - */ - mddev->reshape_position = save_rp; set_bit(MD_RECOVERY_INTR, &mddev->recovery); md_reap_sync_thread(mddev); } @@ -6184,7 +6171,6 @@ static void __md_stop_writes(struct mddev *mddev) flush_workqueue(md_misc_wq); if (mddev->sync_thread) { set_bit(MD_RECOVERY_INTR, &mddev->recovery); - md_unregister_thread(&mddev->sync_thread); md_reap_sync_thread(mddev); } @@ -9336,7 +9322,6 @@ void md_check_recovery(struct mddev *mddev) * ->spare_active and clear saved_raid_disk */ set_bit(MD_RECOVERY_INTR, &mddev->recovery); - md_unregister_thread(&mddev->sync_thread); md_reap_sync_thread(mddev); clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery); clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery); @@ -9372,7 +9357,6 @@ void md_check_recovery(struct mddev *mddev) goto unlock; } if (mddev->sync_thread) { - md_unregister_thread(&mddev->sync_thread); md_reap_sync_thread(mddev); goto unlock; } @@ -9452,7 +9436,8 @@ void md_reap_sync_thread(struct mddev *mddev) sector_t old_dev_sectors = mddev->dev_sectors; bool is_reshaped = false; - /* sync_thread should be unregistered, collect result */ + /* resync has finished, collect result */ + md_unregister_thread(&mddev->sync_thread); if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery) && !test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery) && mddev->degraded != mddev->raid_disks) { -- 2.39.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
next prev parent reply other threads:[~2023-05-29 13:24 UTC|newest] Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-05-29 13:20 [PATCH -next v2 0/6] md: fix that MD_RECOVERY_RUNNING can be cleared while sync_thread is still running Yu Kuai 2023-05-29 13:20 ` [dm-devel] " Yu Kuai 2023-05-29 13:20 ` Yu Kuai [this message] 2023-05-29 13:20 ` [dm-devel] [PATCH -next v2 1/6] Revert "md: unlock mddev before reap sync_thread in action_store" Yu Kuai 2023-06-13 6:25 ` Xiao Ni 2023-06-13 6:25 ` [dm-devel] " Xiao Ni 2023-06-13 11:58 ` Yu Kuai 2023-06-13 11:58 ` [dm-devel] " Yu Kuai 2023-05-29 13:20 ` [PATCH -next v2 2/6] md: refactor action_store() for 'idle' and 'frozen' Yu Kuai 2023-05-29 13:20 ` [dm-devel] " Yu Kuai 2023-06-13 8:02 ` Xiao Ni 2023-06-13 8:02 ` Xiao Ni 2023-06-13 12:00 ` Yu Kuai 2023-06-13 12:00 ` Yu Kuai 2023-06-13 12:25 ` Xiao Ni 2023-06-13 12:25 ` Xiao Ni 2023-06-13 12:44 ` Yu Kuai 2023-06-13 12:44 ` Yu Kuai 2023-06-13 14:14 ` Xiao Ni 2023-06-13 14:14 ` Xiao Ni 2023-05-29 13:20 ` [PATCH -next v2 3/6] md: add a mutex to synchronize idle and frozen in action_store() Yu Kuai 2023-05-29 13:20 ` [dm-devel] " Yu Kuai 2023-06-13 14:43 ` Xiao Ni 2023-06-13 14:43 ` Xiao Ni 2023-06-14 1:15 ` Yu Kuai 2023-06-14 1:15 ` Yu Kuai 2023-06-16 6:41 ` Song Liu 2023-06-16 6:41 ` Song Liu 2023-05-29 13:20 ` [PATCH -next v2 4/6] md: refactor idle/frozen_sync_thread() to fix deadlock Yu Kuai 2023-05-29 13:20 ` [dm-devel] " Yu Kuai 2023-06-13 14:50 ` Xiao Ni 2023-06-13 14:50 ` Xiao Ni 2023-06-14 1:48 ` Yu Kuai 2023-06-14 1:48 ` Yu Kuai 2023-06-14 2:04 ` Yu Kuai 2023-06-14 2:04 ` Yu Kuai 2023-06-14 7:12 ` Xiao Ni 2023-06-14 7:12 ` Xiao Ni 2023-06-14 7:38 ` Yu Kuai 2023-06-14 7:38 ` Yu Kuai 2023-06-14 7:57 ` Xiao Ni 2023-06-14 7:57 ` Xiao Ni 2023-06-14 8:28 ` Yu Kuai 2023-06-14 8:28 ` Yu Kuai 2023-06-14 9:08 ` Xiao Ni 2023-06-14 9:08 ` Xiao Ni 2023-06-15 1:28 ` Yu Kuai 2023-06-15 1:28 ` Yu Kuai 2023-06-15 8:01 ` Xiao Ni 2023-06-15 8:01 ` Xiao Ni 2023-06-15 8:17 ` Xiao Ni 2023-06-15 8:17 ` Xiao Ni 2023-06-15 9:05 ` Yu Kuai 2023-06-15 9:05 ` Yu Kuai 2023-06-15 9:14 ` Xiao Ni 2023-06-15 9:14 ` Xiao Ni 2023-06-14 3:47 ` Xiao Ni 2023-06-14 3:47 ` Xiao Ni 2023-06-14 6:04 ` Yu Kuai 2023-06-14 6:04 ` Yu Kuai 2023-06-14 6:37 ` Xiao Ni 2023-06-14 6:37 ` Xiao Ni 2023-05-29 13:20 ` [PATCH -next v2 5/6] md: wake up 'resync_wait' at last in md_reap_sync_thread() Yu Kuai 2023-05-29 13:20 ` [dm-devel] " Yu Kuai 2023-06-14 7:20 ` Xiao Ni 2023-06-14 7:20 ` Xiao Ni 2023-05-29 13:20 ` [PATCH -next v2 6/6] md: enhance checking in md_check_recovery() Yu Kuai 2023-05-29 13:20 ` [dm-devel] " Yu Kuai 2023-06-14 7:24 ` Xiao Ni 2023-06-14 7:24 ` Xiao Ni 2023-06-08 2:41 ` [PATCH -next v2 0/6] md: fix that MD_RECOVERY_RUNNING can be cleared while sync_thread is still running Yu Kuai 2023-06-08 2:41 ` [dm-devel] " Yu Kuai 2023-06-09 4:44 ` Song Liu 2023-06-09 4:44 ` [dm-devel] " Song Liu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230529132037.2124527-2-yukuai1@huaweicloud.com \ --to=yukuai1@huaweicloud.com \ --cc=agk@redhat.com \ --cc=dm-devel@redhat.com \ --cc=guoqing.jiang@linux.dev \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-raid@vger.kernel.org \ --cc=snitzer@kernel.org \ --cc=song@kernel.org \ --cc=yangerkun@huawei.com \ --cc=yi.zhang@huawei.com \ --cc=yukuai3@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.