[PATCH 0/2] Fixes for dm-raid faulty device restoration

[PATCH 0/2] Fixes for dm-raid faulty device restoration

[Jonathan Brassow]

Neil,

I've discovered a bug in the recently accepted (for 3.11) patch:
"[PATCH - v2] DM RAID: Add ability to restore transiently failed devices
on resume"

I have two follow-on patches to fix the issue:
- [PATCH 1/2] DM RAID:  Break-up untidy function
- [PATCH 2/2] DM RAID:  Fix raid_resume not reviving failed devices in
all cases

Thanks,
 brassow

[PATCH 1/2] DM RAID: Break-up untidy function

[Jonathan Brassow]

DM RAID:  Break-up untidy function

Clean-up excessive indentation by moving some code in raid_resume()
into its own function.

Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>

Index: linux-upstream/drivers/md/dm-raid.c
===================================================================
--- linux-upstream.orig/drivers/md/dm-raid.c
+++ linux-upstream/drivers/md/dm-raid.c
@@ -1572,15 +1572,51 @@ static void raid_postsuspend(struct dm_t
 	mddev_suspend(&rs->md);
 }
 
-static void raid_resume(struct dm_target *ti)
+static void attempt_restore_of_faulty_devices(struct raid_set *rs)
 {
 	int i;
 	uint64_t failed_devices, cleared_failed_devices = 0;
 	unsigned long flags;
 	struct dm_raid_superblock *sb;
-	struct raid_set *rs = ti->private;
 	struct md_rdev *r;
 
+	for (i = 0; i < rs->md.raid_disks; i++) {
+		r = &rs->dev[i].rdev;
+		if (test_bit(Faulty, &r->flags) && r->sb_page &&
+		    sync_page_io(r, 0, r->sb_size, r->sb_page, READ, 1)) {
+			DMINFO("Faulty %s device #%d has readable super block."
+			       "  Attempting to revive it.",
+			       rs->raid_type->name, i);
+			r->raid_disk = i;
+			r->saved_raid_disk = i;
+			flags = r->flags;
+			clear_bit(Faulty, &r->flags);
+			clear_bit(WriteErrorSeen, &r->flags);
+			clear_bit(In_sync, &r->flags);
+			if (r->mddev->pers->hot_add_disk(r->mddev, r)) {
+				r->raid_disk = -1;
+				r->saved_raid_disk = -1;
+				r->flags = flags;
+			} else {
+				r->recovery_offset = 0;
+				cleared_failed_devices |= 1 << i;
+			}
+		}
+	}
+	if (cleared_failed_devices) {
+		rdev_for_each(r, &rs->md) {
+			sb = page_address(r->sb_page);
+			failed_devices = le64_to_cpu(sb->failed_devices);
+			failed_devices &= ~cleared_failed_devices;
+			sb->failed_devices = cpu_to_le64(failed_devices);
+		}
+	}
+}
+
+static void raid_resume(struct dm_target *ti)
+{
+	struct raid_set *rs = ti->private;
+
 	set_bit(MD_CHANGE_DEVS, &rs->md.flags);
 	if (!rs->bitmap_loaded) {
 		bitmap_load(&rs->md);
@@ -1591,37 +1627,7 @@ static void raid_resume(struct dm_target
 		 * Take this opportunity to check whether any failed
 		 * devices are reachable again.
 		 */
-		for (i = 0; i < rs->md.raid_disks; i++) {
-			r = &rs->dev[i].rdev;
-			if (test_bit(Faulty, &r->flags) && r->sb_page &&
-			    sync_page_io(r, 0, r->sb_size,
-					 r->sb_page, READ, 1)) {
-				DMINFO("Faulty device #%d has readable super"
-				       "block.  Attempting to revive it.", i);
-				r->raid_disk = i;
-				r->saved_raid_disk = i;
-				flags = r->flags;
-				clear_bit(Faulty, &r->flags);
-				clear_bit(WriteErrorSeen, &r->flags);
-				clear_bit(In_sync, &r->flags);
-				if (r->mddev->pers->hot_add_disk(r->mddev, r)) {
-					r->raid_disk = -1;
-					r->saved_raid_disk = -1;
-					r->flags = flags;
-				} else {
-					r->recovery_offset = 0;
-					cleared_failed_devices |= 1 << i;
-				}
-			}
-		}
-		if (cleared_failed_devices) {
-			rdev_for_each(r, &rs->md) {
-				sb = page_address(r->sb_page);
-				failed_devices = le64_to_cpu(sb->failed_devices);
-				failed_devices &= ~cleared_failed_devices;
-				sb->failed_devices = cpu_to_le64(failed_devices);
-			}
-		}
+		attempt_restore_of_faulty_devices(rs);
 	}
 
 	clear_bit(MD_RECOVERY_FROZEN, &rs->md.recovery);

[PATCH 2/2] DM RAID: Fix raid_resume not reviving failed devices in all cases

[Jonathan Brassow]

DM RAID:  Fix raid_resume not reviving failed devices in all cases

When a device fails in a RAID array, it is marked as Faulty.  Later,
md_check_recovery is called which (through the call chain) calls
'hot_remove_disk' in order to have the personalities remove the device
from use in the array.

Sometimes, it is possible for the array to be suspended before the
personalities get their chance to perform 'hot_remove_disk'.  This is
normally not an issue.  If the array is deactivated, then the failed
device will be noticed when the array is reinstantiated.  If the
array is resumed and the disk is still missing, md_check_recovery will
be called upon resume and 'hot_remove_disk' will be called at that
time.  However, (for dm-raid) if the device has been restored,
a resume on the array would cause it to attempt to revive the device
by calling 'hot_add_disk'.  If 'hot_remove_disk' had not been called,
a situation is then created where the device is thought to concurrently
be the replacement and the device to be replaced.  Thus, the device
is first sync'ed with the rest of the array (because it is the replacement
device) and then marked Faulty and removed from the array (because
it is also the device being replaced).

The solution is to check and see if the device had properly been removed
before the array was suspended.  This is done by seeing whether the
device's 'raid_disk' field is -1 - a condition that implies that
'md_check_recovery -> remove_and_add_spares (where raid_disk is set to -1)
-> hot_remove_disk' has been called.  If 'raid_disk' is not -1, then
'hot_remove_disk' must be called to complete the removal of the previously
faulty device before it can be revived via 'hot_add_disk'.

Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>

Index: linux-upstream/drivers/md/dm-raid.c
===================================================================
--- linux-upstream.orig/drivers/md/dm-raid.c
+++ linux-upstream/drivers/md/dm-raid.c
@@ -1587,6 +1587,21 @@ static void attempt_restore_of_faulty_de
 			DMINFO("Faulty %s device #%d has readable super block."
 			       "  Attempting to revive it.",
 			       rs->raid_type->name, i);
+
+			/*
+			 * Faulty bit may be set, but sometimes the array can
+			 * be suspended before the personalities can respond
+			 * by removing the device from the array (i.e. calling
+			 * 'hot_remove_disk').  If they haven't yet removed
+			 * the failed device, its 'raid_disk' number will be
+			 * '>= 0' - meaning we must call this function
+			 * ourselves.
+			 */
+			if ((r->raid_disk >= 0) &&
+			    (r->mddev->pers->hot_remove_disk(r->mddev, r) != 0))
+				/* Failed to revive this device, try next */
+				continue;
+
 			r->raid_disk = i;
 			r->saved_raid_disk = i;
 			flags = r->flags;

Re: [PATCH 0/2] Fixes for dm-raid faulty device restoration

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

On Wed, 08 May 2013 17:54:57 -0500 Jonathan Brassow <jbrassow@redhat.com>
wrote:

> Neil,
> 
> I've discovered a bug in the recently accepted (for 3.11) patch:
> "[PATCH - v2] DM RAID: Add ability to restore transiently failed devices
> on resume"
> 
> I have two follow-on patches to fix the issue:
> - [PATCH 1/2] DM RAID:  Break-up untidy function
> - [PATCH 2/2] DM RAID:  Fix raid_resume not reviving failed devices in
> all cases
> 
> Thanks,
>  brassow

Thanks.  I've applied these.

I also fixed that compiler warning.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]