Re: MD RAID 1 fail/remove/add corruption in 3.10

[NeilBrown <neilb@suse.de>]

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

On Tue, 16 Jul 2013 14:49:20 -0400 Joe Lawrence <joe.lawrence@stratus.com>
wrote:

> Hi Neil, Martin,
> 
> While testing patches to fix RAID1 repair GPF crash w/3.10-rc7
> ( http://thread.gmane.org/gmane.linux.raid/43351 ), I encountered disk
> corruption when repeatedly failing, removing, and adding MD RAID1
> component disks to their array.  The RAID1 was created with an internal
> write bitmap and the test was run against alternating disks in the
> set.  I bisected this behavior back to commit 7ceb17e8 "md: Allow
> devices to be re-added to a read-only array", specifically these lines
> of code:
> 
> remove_and_add_spares:
> 
> +		if (rdev->saved_raid_disk >= 0 && mddev->in_sync) {
> +			spin_lock_irq(&mddev->write_lock);
> +			if (mddev->in_sync)
> +				/* OK, this device, which is in_sync,
> +				 * will definitely be noticed before
> +				 * the next write, so recovery isn't
> +				 * needed.
> +				 */
> +				rdev->recovery_offset = mddev->recovery_cp;
> +			spin_unlock_irq(&mddev->write_lock);
> +		}
> +		if (mddev->ro && rdev->recovery_offset != MaxSector)
> +			/* not safe to add this disk now */
> +			continue;
> 
> when I #ifdef 0 these lines out, leaving rdev->recovery_offset = 0,
> then my tests run without incident.
> 
> If there is any instrumentation I can apply to remove_and_add_spares
> I'll be happy to gather more data.  I'll send an attached copy of
> my test programs in a reply so this mail doesn't get bounced by
> any spam filters.
> 
>

Thanks for the report Joe.

That code has problems.

If the array has a bitmap, then 'saved_raid_disk >= 0' means that the device
is fairly close, but the bitmap based resync is required first.  This code
skips that.

If the array does not have a bitmap, then 'saved_raid_disk >= 0' means that
this device was exactly right for this slot before, but there is no locking
to prevent updates going to the array between then super_1_validate checked
the event count, and when remove_and_add_spares tried to add it.

I suspect I should  just rip that code out and go back to the drawing board.

NeilBrown

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