Re: [PATCH 0/2] raid1/10: Handle write errors correctly in narrow_write_error()

[Neil Brown <neilb@suse.de>]

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

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> Nate Dailey <nate.dailey@stratus.com> writes:
>> Thank you!
>>
>> I confirmed that this patch prevents the bug.
>>
>> Nate
>
> Awesome, thanks Nate!
>
> Neil once you commit the final version of this patch, please let me
> know.
>
> Cheers,
> Jes
>
>>
>>
>>
>> On 10/22/2015 08:09 PM, Neil Brown wrote:
>>> Nate Dailey <nate.dailey@stratus.com> writes:
>>>
>>>> The problem is that we aren't getting true write (medium) errors.
>>>>
>>>> In this case we're testing device removals. The write errors happen
>>>> because the
>>>> disk goes away. Narrow_write_error returns 1, the bitmap bit is cleared, and
>>>> then when the device is re-added the resync might not include the sectors in
>>>> that chunk (there's some luck involved; if other writes to that chunk happen
>>>> while the disk is removed, we're okay--bug is easier to hit with
>>>> smaller bitmap
>>>> chunks because of this).
>>>>
>>>>
>>> OK, that makes sense.
>>>
>>> The device removal will be noticed when the bad block log is written
>>> out.
>>> When a bad-block is recorded we make sure to write that out promptly
>>> before bio_endio() gets called.  But not before close_write() has called
>>> bitmap_end_write().
>>>
>>> So I guess we need to delay the close_write() call until the
>>> bad-block-log has been written.
>>>
>>> I think this patch should do it.  Can you test?
>>>
>>> Thanks,
>>> NeilBrown
>>>
>>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>>> index c1ad0b075807..1a1c5160c930 100644
>>> --- a/drivers/md/raid1.c
>>> +++ b/drivers/md/raid1.c
>>> @@ -2269,8 +2269,6 @@ static void handle_write_finished(struct r1conf *conf, struct r1bio *r1_bio)
>>>   			rdev_dec_pending(conf->mirrors[m].rdev,
>>>   					 conf->mddev);
>>>   		}
>>> -	if (test_bit(R1BIO_WriteError, &r1_bio->state))
>>> -		close_write(r1_bio);
>>>   	if (fail) {
>>>   		spin_lock_irq(&conf->device_lock);
>>>   		list_add(&r1_bio->retry_list, &conf->bio_end_io_list);
>>> @@ -2396,6 +2394,9 @@ static void raid1d(struct md_thread *thread)
>>>   			r1_bio = list_first_entry(&tmp, struct r1bio,
>>>   						  retry_list);
>>>   			list_del(&r1_bio->retry_list);
>>> +			if (mddev->degraded)
>>> +				set_bit(R1BIO_Degraded, &r1_bio->state);
>>> +			close_write(r1_bio);
>>>   			raid_end_bio_io(r1_bio);
>>>   		}
>>>   	}

I've just pushed out a version (for raid10 as well) in my 'for-linus'
branch.  I'll submit to Linus later today after zero-day comes back with
no errors.

This version contains some extra code which is not needed, but makes the
change more obviously correct.

Thanks,
NeilBrown


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