Checking ->sync_thread without holding the mddev_lock() isn't really safe, even after flushing the workqueue which ensures md_start_sync() has been run. While this code is waiting for the lock, md_check_recovery could reap the thread itself, and then start another thread (e.g. recovery might finish, then reshape starts). When this thread gets the lock md_start_sync() hasn't run so it doesn't get reaped, but MD_RECOVERY_RUNNING gets cleared. This allows two threads to start which leads to confusion. So don't both if MD_RECOVERY_RUNNING isn't set, but if it is do the flush and the test and the reap all under the mddev_lock to avoid any race with md_check_recovery.
Unified · Split
| @@ -4217,13 +4217,14 @@ action_store(struct mddev *mddev, const char *page, size_t len) · 4216 unmodified lines | ||
| 4217 | 4217 | set_bit(MD_RECOVERY_FROZEN, &mddev->recovery); |
| 4218 | 4218 | else |
| 4219 | 4219 | clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery); |
| 4220 | − flush_workqueue(md_misc_wq); | |
| 4221 | − if (mddev->sync_thread) { | |
| 4222 | − set_bit(MD_RECOVERY_INTR, &mddev->recovery); | |
| 4223 | − if (mddev_lock(mddev) == 0) { | |
| 4220 | + if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) && | |
| 4221 | + mddev_lock(mddev) == 0) { | |
| 4222 | + flush_workqueue(md_misc_wq); | |
| 4223 | + if (mddev->sync_thread) { | |
| 4224 | + set_bit(MD_RECOVERY_INTR, &mddev->recovery); | |
| 4224 | 4225 | md_reap_sync_thread(mddev); |
| 4225 | − mddev_unlock(mddev); | |
| 4226 | 4226 | } |
| 4227 | + mddev_unlock(mddev); | |
| 4227 | 4228 | } |
| 4228 | 4229 | } else if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) || |
| 4229 | 4230 | test_bit(MD_RECOVERY_NEEDED, &mddev->recovery)) |