From 108cef3aa41669610e1836fe638812dd067d72de Mon Sep 17 00:00:00 2001 From: NeilBrown Date: Wed, 3 Dec 2014 16:07:58 +1100 Subject: [PATCH 1/3] md/raid5: fetch_block must fetch all the blocks handle_stripe_dirtying wants. It is critical that fetch_block() and handle_stripe_dirtying() are consistent in their analysis of what needs to be loaded. Otherwise raid5 can wait forever for a block that won't be loaded. Currently when writing to a RAID5 that is resyncing, to a location beyond the resync offset, handle_stripe_dirtying chooses a reconstruct-write cycle, but fetch_block() assumes a read-modify-write, and a lockup can happen. So treat that case just like RAID6, just as we do in handle_stripe_dirtying. RAID6 always does reconstruct-write. This bug was introduced when the behaviour of handle_stripe_dirtying was changed in 3.7, so the patch is suitable for any kernel since, though it will need careful merging for some versions. Cc: stable@vger.kernel.org (v3.7+) Fixes: a7854487cd7128a30a7f4f5259de9f67d5efb95f Reported-by: Henry Cai Signed-off-by: NeilBrown --- drivers/md/raid5.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index 9c66e5997fc8..c1b0d52bfcb0 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -2917,8 +2917,11 @@ static int fetch_block(struct stripe_head *sh, struct stripe_head_state *s, (sh->raid_conf->level <= 5 && s->failed && fdev[0]->towrite && (!test_bit(R5_Insync, &dev->flags) || test_bit(STRIPE_PREREAD_ACTIVE, &sh->state)) && !test_bit(R5_OVERWRITE, &fdev[0]->flags)) || - (sh->raid_conf->level == 6 && s->failed && s->to_write && - s->to_write - s->non_overwrite < sh->raid_conf->raid_disks - 2 && + ((sh->raid_conf->level == 6 || + sh->sector >= sh->raid_conf->mddev->recovery_cp) + && s->failed && s->to_write && + (s->to_write - s->non_overwrite < + sh->raid_conf->raid_disks - sh->raid_conf->max_degraded) && (!test_bit(R5_Insync, &dev->flags) || test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))))) { /* we would like to get this block, possibly by computing it, * otherwise read it if the backing disk is insync From 7d7e64f2ecd4a863b5fbc1fe998bb39d6423664a Mon Sep 17 00:00:00 2001 From: kbuild test robot Date: Wed, 3 Dec 2014 16:07:59 +1100 Subject: [PATCH 2/3] md: fix semicolon.cocci warnings drivers/md/md.c:7175:43-44: Unneeded semicolon Removes unneeded semicolon. Generated by: scripts/coccinelle/misc/semicolon.cocci Signed-off-by: Fengguang Wu Signed-off-by: NeilBrown --- drivers/md/md.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 9233c71138f1..ad5d783ad416 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -6969,7 +6969,7 @@ static unsigned int mdstat_poll(struct file *filp, poll_table *wait) int mask; if (md_unloading) - return POLLIN|POLLRDNORM|POLLERR|POLLPRI;; + return POLLIN|POLLRDNORM|POLLERR|POLLPRI; poll_wait(filp, &md_event_waiters, wait); /* always allow read */ From f851b60db0fd83a10034c5cc9d9e58c758457b1c Mon Sep 17 00:00:00 2001 From: NeilBrown Date: Thu, 11 Dec 2014 10:02:10 +1100 Subject: [PATCH 3/3] md: Check MD_RECOVERY_RUNNING as well as ->sync_thread. A recent change to md started the ->sync_thread from a asynchronously from a work_queue rather than synchronously. This means that there can be a small window between the time when MD_RECOVERY_RUNNING is set and when ->sync_thread is set. So code that checks ->sync_thread might now conclude that the thread has not been started and (because a lock is held) will not be started. That is no longer the case. Most of those places are best fixed by testing MD_RECOVERY_RUNNING as well. To make this completely reliable, we wake_up(&resync_wait) after clearing that flag as well as after clearing ->sync_thread. Other places are better served by flushing the relevant workqueue to ensure that that if the sync thread was starting, it has now started. This is particularly best if we are about to stop the sync thread. Fixes: ac05f256691fe427a3e84c19261adb0b67dd73c0 Signed-off-by: NeilBrown --- drivers/md/md.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index ad5d783ad416..b97ac9861ab2 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -2695,7 +2695,8 @@ static ssize_t new_offset_store(struct md_rdev *rdev, if (kstrtoull(buf, 10, &new_offset) < 0) return -EINVAL; - if (mddev->sync_thread) + if (mddev->sync_thread || + test_bit(MD_RECOVERY_RUNNING,&mddev->recovery)) return -EBUSY; if (new_offset == rdev->data_offset) /* reset is always permitted */ @@ -3272,6 +3273,7 @@ level_store(struct mddev *mddev, const char *buf, size_t len) */ if (mddev->sync_thread || + test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) || mddev->reshape_position != MaxSector || mddev->sysfs_active) return -EBUSY; @@ -4026,6 +4028,7 @@ action_store(struct mddev *mddev, const char *page, size_t len) clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery); if (cmd_match(page, "idle") || cmd_match(page, "frozen")) { + flush_workqueue(md_misc_wq); if (mddev->sync_thread) { set_bit(MD_RECOVERY_INTR, &mddev->recovery); md_reap_sync_thread(mddev); @@ -5044,6 +5047,7 @@ static void md_clean(struct mddev *mddev) static void __md_stop_writes(struct mddev *mddev) { set_bit(MD_RECOVERY_FROZEN, &mddev->recovery); + flush_workqueue(md_misc_wq); if (mddev->sync_thread) { set_bit(MD_RECOVERY_INTR, &mddev->recovery); md_reap_sync_thread(mddev); @@ -5104,19 +5108,22 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev) set_bit(MD_RECOVERY_FROZEN, &mddev->recovery); md_wakeup_thread(mddev->thread); } - if (mddev->sync_thread) { + if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) set_bit(MD_RECOVERY_INTR, &mddev->recovery); + if (mddev->sync_thread) /* Thread might be blocked waiting for metadata update * which will now never happen */ wake_up_process(mddev->sync_thread->tsk); - } + mddev_unlock(mddev); - wait_event(resync_wait, mddev->sync_thread == NULL); + wait_event(resync_wait, !test_bit(MD_RECOVERY_RUNNING, + &mddev->recovery)); mddev_lock_nointr(mddev); mutex_lock(&mddev->open_mutex); if ((mddev->pers && atomic_read(&mddev->openers) > !!bdev) || mddev->sync_thread || + test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) || (bdev && !test_bit(MD_STILL_CLOSED, &mddev->flags))) { printk("md: %s still in use.\n",mdname(mddev)); if (did_freeze) { @@ -5162,20 +5169,24 @@ static int do_md_stop(struct mddev *mddev, int mode, set_bit(MD_RECOVERY_FROZEN, &mddev->recovery); md_wakeup_thread(mddev->thread); } - if (mddev->sync_thread) { + if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) set_bit(MD_RECOVERY_INTR, &mddev->recovery); + if (mddev->sync_thread) /* Thread might be blocked waiting for metadata update * which will now never happen */ wake_up_process(mddev->sync_thread->tsk); - } + mddev_unlock(mddev); - wait_event(resync_wait, mddev->sync_thread == NULL); + wait_event(resync_wait, (mddev->sync_thread == NULL && + !test_bit(MD_RECOVERY_RUNNING, + &mddev->recovery))); mddev_lock_nointr(mddev); mutex_lock(&mddev->open_mutex); if ((mddev->pers && atomic_read(&mddev->openers) > !!bdev) || mddev->sysfs_active || mddev->sync_thread || + test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) || (bdev && !test_bit(MD_STILL_CLOSED, &mddev->flags))) { printk("md: %s still in use.\n",mdname(mddev)); mutex_unlock(&mddev->open_mutex); @@ -5950,7 +5961,8 @@ static int update_size(struct mddev *mddev, sector_t num_sectors) * of each device. If num_sectors is zero, we find the largest size * that fits. */ - if (mddev->sync_thread) + if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) || + mddev->sync_thread) return -EBUSY; if (mddev->ro) return -EROFS; @@ -5981,7 +5993,9 @@ static int update_raid_disks(struct mddev *mddev, int raid_disks) if (raid_disks <= 0 || (mddev->max_disks && raid_disks >= mddev->max_disks)) return -EINVAL; - if (mddev->sync_thread || mddev->reshape_position != MaxSector) + if (mddev->sync_thread || + test_bit(MD_RECOVERY_RUNNING, &mddev->recovery) || + mddev->reshape_position != MaxSector) return -EBUSY; rdev_for_each(rdev, mddev) { @@ -7593,6 +7607,7 @@ static void md_start_sync(struct work_struct *ws) clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery); clear_bit(MD_RECOVERY_CHECK, &mddev->recovery); clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery); + wake_up(&resync_wait); if (test_and_clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery)) if (mddev->sysfs_action) @@ -7761,6 +7776,7 @@ void md_check_recovery(struct mddev *mddev) not_running: if (!mddev->sync_thread) { clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery); + wake_up(&resync_wait); if (test_and_clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery)) if (mddev->sysfs_action) @@ -7779,7 +7795,6 @@ void md_reap_sync_thread(struct mddev *mddev) /* resync has finished, collect result */ md_unregister_thread(&mddev->sync_thread); - wake_up(&resync_wait); if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery) && !test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery)) { /* success...*/ @@ -7807,6 +7822,7 @@ void md_reap_sync_thread(struct mddev *mddev) clear_bit(MD_RECOVERY_RESHAPE, &mddev->recovery); clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery); clear_bit(MD_RECOVERY_CHECK, &mddev->recovery); + wake_up(&resync_wait); /* flag recovery needed just to double check */ set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); sysfs_notify_dirent_safe(mddev->sysfs_action);