Possible bug in BTRFS w/ Duplication

[Qu Wenruo]

On 2022/12/30 23:28, Sam Winchenbach wrote:
> I believe you fixed the issue with the patch you presented. I was in the process of testing a similar fix for release and it solved the issue I encountered.

But I still want to make sure that only RAID0 on single device can cause 
the problem.

For multi-device RAID0/RAID10/RAID5/6, we don't support them until we 
can scan all devices in U-boot...

Thanks,
Qu
> 
> Thanks,
> Sam Winchenbach
> 
> -----Original Message-----
> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Qu Wenruo
> Sent: Thursday, December 29, 2022 7:01 PM
> To: Heinrich Schuchardt <xypron.glpk at gmx.de>; Sam Winchenbach <swichenbach at tethers.com>; Marek Behún <kabel at kernel.org>
> Cc: u-boot at lists.denx.de; Qu Wenruo <wqu at suse.com>; linux-btrfs at vger.kernel.org
> Subject: Re: Possible bug in BTRFS w/ Duplication
> 
> 
> 
> On 2022/12/29 22:12, Heinrich Schuchardt wrote:
>> On 12/28/22 21:51, Sam Winchenbach wrote:
>>> Hello,
>>>
>>> Hello, I have hit the following situation when trying to load files
>>> from a BTRFS partition with duplication enabled.
> 
> You mean multi-device?
> 
> For DUP/RAID1 duplication, they don't have stripe limitation at all.
> 
> Thus I believe you're talking about RAID0 (which doesn't have any duplication/extra mirrors) or RAID10 or RAID5/6?
> 
> But for now, we don't support multi-device in U-boot yet, thus I'm not sure what situation you're talking about.
> 
> Mind to run the following command?
> 
>    # btrfs fi usage <mnt of the btrfs>
> 
>>>
>>> In the first example I read a 16KiB file - __btrfs_map_block()
>>> changes the length to something larger than the file being read. This
>>> works fine, as length is later clamped to the file size.
>>>
>>> In the second example, __btrfs_map_block() changes the length
>>> parameter to something smaller than the file (the size of a stripe).
>>> This seems to break this check here:
>>>
>>>       read = len;
>>>       num_copies = btrfs_num_copies(fs_info, logical, len);
>>>       for (i = 1; i <= num_copies; i++) {
>>>           ret = read_extent_data(fs_info, dest, logical, &read, i);
>>>           if (ret < 0 || read != len) {
>>>               continue;
>>>           }
>>>           finished = true;
>>>           break;
>>>       }
>>>
>>> The problem being that read is always less than len.
>>>
>>> I am not sure if __btrfs_map_block is changing "len" to the incorrect
>>> value, or if there is some logic in "read_extent_data" that isn't
>>> correct. Any pointers on how this code is supposed to work would be
>>> greatly appreciated.
>>> Thanks.
>>
>> Thanks for reporting the issue
>>
>> $ scripts/get_maintainer.pl -f fs/btrfs/volumes.c
>>
>> suggests to include
>>
>> "Marek Behún" <kabel at kernel.org> (maintainer:BTRFS) Qu Wenruo
>> <wqu at suse.com> (reviewer:BTRFS) linux-btrfs at vger.kernel.org
>>
>> to the communication.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> === EXAMPLE 2 ===
>>> Zynq> load mmc 1:0 0 16K
>>> [btrfs_file_read,fs/btrfs/inode.c:710] === read the aligned part ===
>>> [btrfs_read_extent_reg,fs/btrfs/inode.c:458] before read_extent_data
>>> (ret = 0, read = 16384, len = 16384)
>>> [read_extent_data,fs/btrfs/disk-io.c:547] before __btrfs_map_block
>>> (len = 16384) [read_extent_data,fs/btrfs/disk-io.c:550] after
>>> __btrfs_map_block (len = 28672)
>>> [read_extent_data,fs/btrfs/disk-io.c:565] before __btrfs_devread (len
>>> = 16384) [read_extent_data,fs/btrfs/disk-io.c:568] after
>>> __btrfs_devread (len =
>>> 16384)
>>> [btrfs_read_extent_reg,fs/btrfs/inode.c:460] after read_extent_data
>>> (ret = 0, read = 16384, len = 16384)
>>> cur: 0, extent_num_bytes: 16384, aligned_end: 16384
>>> 16384 bytes read in 100 ms (159.2 KiB/s)
>>>
>>> === EXAMPLE 2 ===
>>> Zynq> load mmc 1:0 0 32K
>>> [btrfs_file_read,fs/btrfs/inode.c:710] === read the aligned part ===
>>> [btrfs_read_extent_reg,fs/btrfs/inode.c:458] before read_extent_data
>>> (ret = 0, read = 32768, len = 32768)
>>> [read_extent_data,fs/btrfs/disk-io.c:547] before __btrfs_map_block
>>> (len = 32768) [read_extent_data,fs/btrfs/disk-io.c:550] after
>>> __btrfs_map_block (len = 12288)
>>> [read_extent_data,fs/btrfs/disk-io.c:565] before __btrfs_devread (len
>>> = 12288) [read_extent_data,fs/btrfs/disk-io.c:568] after
>>> __btrfs_devread (len =
>>> 12288)
> 
> So the first 3 sectors are before the stripe boundary and we read it correctly.
> 
>>> [btrfs_read_extent_reg,fs/btrfs/inode.c:460] after read_extent_data
>>> (ret = 0, read = 12288, len = 32768)
>>> [btrfs_read_extent_reg,fs/btrfs/inode.c:458] before read_extent_data
>>> (ret = 0, read = 12288, len = 32768)
>>> [read_extent_data,fs/btrfs/disk-io.c:547] before __btrfs_map_block
>>> (len = 12288) [read_extent_data,fs/btrfs/disk-io.c:550] after
>>> __btrfs_map_block (len = 12288)
> 
> I believe this is the problem.
> 
> If we're reading the full 32K, and the first 12K is in the first stripe, we should then try to map the remaining 20K, not the 12K again.
> 
> I'll look into the situation.
> But if you can provide the image or the dump, it can greatly help the debugging.
> 
> Thanks,
> Qu
> 
>>> [read_extent_data,fs/btrfs/disk-io.c:565] before __btrfs_devread (len
>>> = 12288) [read_extent_data,fs/btrfs/disk-io.c:568] after
>>> __btrfs_devread (len =
>>> 12288)
>>> [btrfs_read_extent_reg,fs/btrfs/inode.c:460] after read_extent_data
>>> (ret = 0, read = 12288, len = 32768)
>>> file: fs/btrfs/inode.c, line: 468
>>> cur: 0, extent_num_bytes: 32768, aligned_end: 32768
>>> -----> btrfs_read_extent_reg: -5, line: 758
>>> BTRFS: An error occurred while reading file 32K Failed to load '32K'
>>>
>>>
>>>
>>>
>>>
>>> Sam Winchenbach
>>> Embedded Software Engineer III
>>> Tethers Unlimited, Inc. | Connect Your Universe | www.tethers.com
>>> swinchenbach at tethers.com | C: 207-974-6934
>>> 11711 North Creek Pkwy # D113, Bothell, WA 98011-8808, USA
>>