Re: [PATCH] btrfs: repair all bad mirrors

[Nikolay Borisov <nborisov@suse.com>]

On 21.06.22 г. 18:46 ч., Christoph Hellwig wrote:
> On Tue, Jun 21, 2022 at 06:19:19PM +0300, Nikolay Borisov wrote:
>>> +
>>> +	mirror = failrec->this_mirror;
>>> +	do {
>>> +		mirror = prev_mirror(failrec, mirror);
>>> +		repair_io_failure(fs_info, ino, start, failrec->len,
>>> +				  failrec->logical, page, pg_offset, mirror);
>>> +	} while (mirror != failrec->orig_mirror);
>>
>> But does this work as intended? Say we have a raid1c4 and we read from
>> mirror 3 which is bad, in this case failrec->orig_mirror = 3 and
>> ->this_mirror = 4. The read from mirror 4 returns good data and
>> clean_io_failure is called with mirror= 3 in which case only mirror 3 is
>> repaired (assume 1/2 were also bad we don't know it yet, because the
>> original bio request didn't submit to them based on the PID policy).
> 
> Yes.  Although that is what I intended as we don't want to read
> data we don't otherwise have to. Maybe it should state "all known bad
> mirrors" instead of "all mirrors".  I think if we want to check all
> mirror we need to defer to the scrub code.
> 

My point is won't this loop ever fix at most 1 mirror? Consider again 
raid1c4, where the 4th copy is the good one. First we read from 0 -> bad 
we submit io to mirror 1 (orig_mirror = 0, this_mirror=1). The same 
thing is repeated until we get to orig_mirror = 3, this_mirror =4. This 
time the repair would be completed and so for this_mirror = 4 we'll 
execute clean_io_failure in which case the do{} while() loop will only 
fix the bad copy for mirror 3.