RE: Possible bug in BTRFS w/ Duplication

[Sam Winchenbach <swichenbach@tethers.com>]

That is correct. This bug is present on a single device with duplication enabled. 

Here, in __btrfs_map_block the read length can be reduced to the stripe length when duplication is enabled.

	if (map->type & (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1 |
			 BTRFS_BLOCK_GROUP_RAID1C3 | BTRFS_BLOCK_GROUP_RAID1C4 |
			 BTRFS_BLOCK_GROUP_RAID5 | BTRFS_BLOCK_GROUP_RAID6 |
			 BTRFS_BLOCK_GROUP_RAID10 |
			 BTRFS_BLOCK_GROUP_DUP)) {
		/* we limit the length of each bio to what fits in a stripe */
		*length = min_t(u64, ce->size - offset,
			      map->stripe_len - stripe_offset);
	} else {
		*length = ce->size - offset;
	}

read_extent_data fails to read the complete length. I implemented a patch similar to yours and it worked perfectly.  I am a little sad you beat me to submitting it.


Thanks,
Sam Winchenbach

-----Original Message-----
From: Qu Wenruo <quwenruo.btrfs@gmx.com> 
Sent: Friday, December 30, 2022 7:10 PM
To: Sam Winchenbach <swichenbach@tethers.com>; Heinrich Schuchardt <xypron.glpk@gmx.de>; Marek Behún <kabel@kernel.org>
Cc: u-boot@lists.denx.de; Qu Wenruo <wqu@suse.com>; linux-btrfs@vger.kernel.org
Subject: Re: Possible bug in BTRFS w/ Duplication



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@lists.denx.de> On Behalf Of Qu Wenruo
> Sent: Thursday, December 29, 2022 7:01 PM
> To: Heinrich Schuchardt <xypron.glpk@gmx.de>; Sam Winchenbach 
> <swichenbach@tethers.com>; Marek Behún <kabel@kernel.org>
> Cc: u-boot@lists.denx.de; Qu Wenruo <wqu@suse.com>; 
> linux-btrfs@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@kernel.org> (maintainer:BTRFS) Qu Wenruo 
>> <wqu@suse.com> (reviewer:BTRFS) linux-btrfs@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@tethers.com | C: 207-974-6934
>>> 11711 North Creek Pkwy # D113, Bothell, WA 98011-8808, USA
>>