Re: [PATCH 2/4] Btrfs: fix data corruption in raid6

[Anand Jain <anand.jain@oracle.com>]

On 11/09/2017 03:53 AM, Liu Bo wrote:
> On Tue, Nov 07, 2017 at 04:32:55PM +0800, Anand Jain wrote:
>>
>>
>> On 11/02/2017 08:54 AM, Liu Bo wrote:
>>> With raid6 profile, btrfs can end up with data corruption by the
>>> following steps.
>>>
>>> Say we have a 5 disks that are set up with raid6 profile,
>>>
>>> 1) mount this btrfs
>>> 2) one disk gets pulled out
>>> 3) write something to btrfs and sync
>>> 4) another disk gets pulled out
>>> 5) write something to btrfs and sync
>>> 6) umount this btrfs
>>> 7) bring the two disk back
>>> 8) reboot
>>> 9) mount this btrfs
>>>
>>> Chances are mount will fail (even with -odegraded) because of failure
>>> on reading metadata blocks, IOW, our raid6 setup is not able to
>>> protect two disk failures in some cases.
>>>
>>> So it turns out that there is a bug in raid6's recover code, that is,
>>>
>>> if we have one of stripes in the raid6 layout as shown here,
>>>
>>> | D1(*)  | D2(*)  | D3 | D4 | D5    |
>>> -------------------------------------
>>> | data1  | data2  | P  | Q  | data0 |
>>
>>
>>> D1 and D2 are the two disks which got pulled out and brought back.
>>> When mount reads data1 and finds out that data1 doesn't match its crc,
>>> btrfs goes to recover data1 from other stripes, what it'll be doing is
>>>
>>> 1) read data2, parity P, parity Q and data0
>>>
>>> 2) as we have a valid parity P and two data stripes, it goes to
>>>     recover in raid5 style.
>>
>>
>>> (However, since disk D2 was pulled out and data2 on D2 could be stale,
>>
>>   data2 should end up crc error, if not then raid5 recover is as
>>   expected OR this example is confusing to explain the context of
>>   two data stipe missing.
> 
> The assumption you have is not true, 

> when doing reconstruction, we
> don't verify checksum for each data stripe that is read from disk.

  Why ? what if data0 (which has InSync flag) was wrong ?

  I am wary about the In_sync approach. IMO we are loosing the
  advantage of FS being merged with volume manager - which is to
  know exactly which active stripes were lost for quicker reconstruct.

  Lets say RAID6 is 50% full and disk1 is pulled out, and at 75%
  full disk2 is pulled out and reaches 90% full.

  So now when all the disks are back, two disks will not have
  In_sync flag? hope I understood this correctly.

  Now.
  15% of the data have lost two stripes.
  25% of the data have lost one stripe.
  50% of the data did not loose any stripe at all.

  Now for read and reconstruct..
  50% of the data does not need any reconstruction.
  25% of the data needs RAID5 style reconstruction as they have lost 
only one stripe.
  15% of the data needs RAID6 reconstruction since they have lost two 
stripes.

  Does InSync design here help to work like this ?

  Now for RAID1, lets say disk1 lost first 10% of the stripes and
  disk2 lost the last 10% of the stripes and these stripes are mutually
  exclusive. That means there is no single stripe which has lost
  completely.

  There is no code yet where I can see when the In_sync will be reset,
  which is fine for this eg. Assume no reconstruction is done. But now
  when both the disks are being mounted and as the In_sync will be based
  on dev_item.generation, one of it will get In_sync to me it looks like
  there will be 10% of stripes which will fail even though its not
  practically true in this example.


Thanks, Anand



> Thanks,
> 
> -liubo
>>
>> Thanks, Anand
>>
>>
>>> we still get the wrong data1 from this reconstruction.)
>>>
>>> 3) btrfs continues to try to reconstruct data1 from parity Q, data2
>>>     and data0, we still get the wrong one for the same reason.
>>>
>>> The fix here is to take advantage of the device flag, ie. 'In_sync',
>>> all data on a device might be stale if 'In_sync' has not been set.
>>>
>>> With this, we can build the correct data2 from parity P, Q and data1.
>>>
>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>> ---
>>>   fs/btrfs/raid56.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
>>> index 67262f8..3c0ce61 100644
>>> --- a/fs/btrfs/raid56.c
>>> +++ b/fs/btrfs/raid56.c
>>> @@ -1076,7 +1076,8 @@ static int rbio_add_io_page(struct btrfs_raid_bio *rbio,
>>>   	disk_start = stripe->physical + (page_index << PAGE_SHIFT);
>>>   	/* if the device is missing, just fail this stripe */
>>> -	if (!stripe->dev->bdev)
>>> +	if (!stripe->dev->bdev ||
>>> +	    !test_bit(In_sync, &stripe->dev->flags))
>>>   		return fail_rbio_index(rbio, stripe_nr);
>>>   	/* see if we can add this page onto our existing bio */
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>