Re: [PATCH RFC] btrfs: fix read corrpution from disks of different generation

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

On 3/20/19 2:27 PM, Qu Wenruo wrote:
> 
> 
> On 2019/3/20 下午1:47, Anand Jain wrote:
>>
>>
>>>>>> A tree based integrity verification
>>>>>>     is important for all data, which is missing.
>>>>>>       Fix:
>>>>>>       In this RFC patch it proposes to use same disk from with the
>>>>>> metadata
>>>>>>     is read to read the data.
>>>>>
>>>>> The obvious problem I found is, the idea only works for RAID1/10.
>>>>>
>>>>> For striped profile it makes no sense, or even have a worse chance to
>>>>> get stale data.
>>>>>
>>>>>
>>>>> To me, the idea of using possible better mirror makes some sense, but
>>>>> very profile limited.
>>>>
>>>>    Yep. This problem and fix is only for the mirror based profiles
>>>>    such as raid1/raid10.
>>>
>>> Then current implementation lacks such check.
>>>
>>> Further more, data and metadata can lie in different chunks and have
>>> different chunk types.
>>
>>   Right. Current tests for this RFC were only for raid1.
>>
>>   But the final patch can fix that.
>>
>>   In fact current patch works for all the cases except for the case of
>>   replace is running and mix of metadata:raid1 and data:raid56
>>
>>   We need some cleanups in mirror_num, basically we need to bring it
>>   under #define. and handle it accordingly in __btrfs_map_block()
> 
> 
> Wait for a minute.
> 
> There is a hidden pitfall from the very beginning.
> 
> Consider such chunk layout:
> Chunk A Type DATA|RAID1
> Stripe 1: Dev 1
> Stripe 2: Dev 2
> 
> Chunk B Type METADATA|RAID1
> Stripe 1: Dev 2
> Stripe 2: Dev 1

  Right this fix can't solve this case.
  So one down. The other choice I had is to
  Quarantine whole disk by comparing dev1 and dev2 generation # in the 
superblock during mount. or
  Quarantine whole chunk(s) by comparing dev1 and dev2 generation # in 
the chunk tree during mount. or
  Always read eb from both dev1 and dev2, if fails then fail its 
corresponding data block as well if any. or
  During mount compare dev1 and dev2 generation #, record the range of 
generation number missing on the disk. But this need ondisk format 
changes. (But nodatacow, notrunc reg data extent write must change gen#). or
  Similar to btree eb read pages, nest the extent data read as well.
  (But nodatacow, notrunc reg data extent write must change gen#).

Thanks, Anand


> Then when we found stale metadata in chunk B mirror 1, caused by dev 2,
> then your patch consider device 2 stale, and try to use mirror num 2 to
> read from data chunk.
> 
> However in data chunk, mirror num 2 means it's still from device 2, and
> we get stale data.
> 
> So the eb->mirror_num can still map to bad/stale device, due to the
> flexibility provided by btrfs per-chunk mapping.
> 
> Thanks,
> Qu
> 
>>
>>>>> Another idea I get inspired from the idea is, make it more generic so
>>>>> that bad/stale device get a lower priority.
>>>>
>>>>    When it comes to reading junk data, its not about the priority its
>>>>    about the eliminating. When the problem is only few blocks, I am
>>>>    against making the whole disk as bad.
>>>>
>>>>> Although it suffers the same problem as I described.
>>>>>
>>>>> To make the point short, the use case looks very limited.
>>>>
>>>>    It applies to raid1/raid10 with nodatacow (which implies nodatasum).
>>>>    In my understanding that's not rare.
>>>>
>>>>    Any comments on the fix offered here?
>>>
>>> The implementation part is, is eb->read_mirror reliable?
>>>
>>> E.g. if the data and the eb are in different chunks, and the stale
>>> happens in the chunk of eb but not in the data chunk?
>>
>>
>>   eb and regular data are indeed in different chunks always. But eb
>>   can never be stale as there is parent transid which is verified against
>>   the read eb. However we do not have such a check for the data (this is
>>   the core of the issue here) and so we return the junk data silently.
>>
>>   Also any idea why the generation number for the extent data is not
>>   incremented [2] when -o nodatacow and notrunc option is used, is it
>>   a bug? the dump-tree is taken with the script as below [1]
>>   (this corruption is seen with or without generation number is
>>   being incremented, but as another way to fix for the corruption we can
>>   verify the inode EXTENT_DATA generation from the same disk from which
>>   the data is read).
>>
>> [1]
>>   umount /btrfs; mkfs.btrfs -fq -dsingle -msingle /dev/sdb && \
>>   mount -o notreelog,max_inline=0,nodatasum /dev/sdb /btrfs && \
>>   echo 1st write: && \
>>   dd status=none if=/dev/urandom of=/btrfs/anand bs=4096 count=1
>> conv=fsync,notrunc && sync && \
>>   btrfs in dump-tree /dev/sdb | egrep -A7 "257 INODE_ITEM 0\) item" && \
>>   echo --- && \
>>   btrfs in dump-tree /dev/sdb  | grep -A4 "257 EXTENT_DATA" && \
>>   echo 2nd write: && \
>>   dd status=none if=/dev/urandom of=/btrfs/anand bs=4096 count=1
>> conv=fsync,notrunc && sync && \
>>   btrfs in dump-tree /dev/sdb | egrep -A7 "257 INODE_ITEM 0\) item" && \
>>   echo --- && \
>>   btrfs in dump-tree /dev/sdb  | grep -A4 "257 EXTENT_DATA"
>>
>>
>> 1st write:
>>      item 4 key (257 INODE_ITEM 0) itemoff 15881 itemsize 160
>>          generation 6 transid 6 size 4096 nbytes 4096
>>          block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
>>          sequence 1 flags 0x3(NODATASUM|NODATACOW)
>>          atime 1553058460.163985452 (2019-03-20 13:07:40)
>>          ctime 1553058460.163985452 (2019-03-20 13:07:40)
>>          mtime 1553058460.163985452 (2019-03-20 13:07:40)
>>          otime 1553058460.163985452 (2019-03-20 13:07:40)
>> ---
>>      item 6 key (257 EXTENT_DATA 0) itemoff 15813 itemsize 53
>>          generation 6 type 1 (regular)
>>          extent data disk byte 13631488 nr 4096
>>          extent data offset 0 nr 4096 ram 4096
>>          extent compression 0 (none)
>> 2nd write:
>>      item 4 key (257 INODE_ITEM 0) itemoff 15881 itemsize 160
>>          generation 6 transid 7 size 4096 nbytes 4096
>>          block group 0 mode 100644 links 1 uid 0 gid 0 rdev 0
>>          sequence 2 flags 0x3(NODATASUM|NODATACOW)
>>          atime 1553058460.163985452 (2019-03-20 13:07:40)
>>          ctime 1553058460.189985450 (2019-03-20 13:07:40)
>>          mtime 1553058460.189985450 (2019-03-20 13:07:40)
>>          otime 1553058460.163985452 (2019-03-20 13:07:40)
>> ---
>>      item 6 key (257 EXTENT_DATA 0) itemoff 15813 itemsize 53
>>          generation 6 type 1 (regular)   <----- [2]
>>          extent data disk byte 13631488 nr 4096
>>          extent data offset 0 nr 4096 ram 4096
>>          extent compression 0 (none)
>>
>>
>> Thanks, Anand
>