Re: RAD1 doesn't fail WRITE that was written only on a rebuilding drive

[NeilBrown <neilb@suse.de>]

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

On Tue, 28 May 2013 23:12:38 +0300 Alexander Lyakas <alex.bolshoy@gmail.com>
wrote:

> Hi Neil,
> I have found why recovery is triggered again and again.
> 
> After initial recovery aborts, raid1d() calls md_check_recovery(),
> which calls remove_and_add_spares().
> So remove_and_add_spares wants to eject the rebuilding drive from the
> array, but cannot because rdev->nr_pending>0:
> 		if (rdev->raid_disk >= 0 &&
> 		    !test_bit(Blocked, &rdev->flags) &&
> 		    (test_bit(Faulty, &rdev->flags) ||
> 		     ! test_bit(In_sync, &rdev->flags)) &&
> 		    atomic_read(&rdev->nr_pending)==0) {
> if (mddev->pers->hot_remove_disk(
> ...
> 
> So instead it goes ahead and triggers another recovery in the
> following condition (by incrementing "spares"):
> 	rdev_for_each(rdev, mddev) {
> 		if (rdev->raid_disk >= 0 &&
> 		    !test_bit(In_sync, &rdev->flags) &&
> 		    !test_bit(Faulty, &rdev->flags))
> 			spares++;
> 
> So another recovery starts, aborts, is triggered again, aborts and so forth.
> 
> Because conditions:
> test_bit(Faulty, &rdev->flags) || ! test_bit(In_sync, &rdev->flags)
> and
> !test_bit(In_sync, &rdev->flags) &&  !test_bit(Faulty, &rdev->flags))
> can both be true at the same time.
> 
> Problem is that IOs keep coming in, because MD does not fail them as I
> discussed in the previous email. So nr_pending never gets to zero, or
> it takes it a long time to get to zero (totally racy), and we keep
> triggering and aborting recovery.
> 
> Can we fix remove_and_add_spares() the following way:
> 
> "if a drive qualifies for being removed from array, but cannot be
> removed because of nr_pending, don't try to trigger another recovery
> on it by returning doing spares++".

I'm not sure...  While that approach would address this particular problem
I'm not convinced that it would always be correct.

The 'recovery_disabled' field is supposed to stop this sort of thing, but it
doesn't get activated here because it only stops the adding of devices to the
array, and we never add a device.

I would probably remove the nr_pending test from remove_add_and_spares and
leave it to the personality routine to do that test (I think they already do).
Then get ->hot_remove_disk to have a particular error status which means
"don't try any recovery", which is returned if the ->recovery_disabled fields
match.
Then remove_and_add_spares need to check for this return value and act
accordingly.
Something like that.

Do you think that would  work?

Thanks,
NeilBrown


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