Re: [PATCH U-BOOT v3 25/30] fs: btrfs: Implement btrfs_file_read()

From: Qu Wenruo <wqu@suse.com>
To: Tom Rini <trini@konsulko.com>, Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: "Marek Behún" <marek.behun@nic.cz>,
	u-boot@lists.denx.de,
	"Alberto Sánchez Molero" <alsamolero@gmail.com>,
	"Marek Vasut" <marex@denx.de>,
	"Pierre Bourdon" <delroth@gmail.com>,
	"Simon Glass" <sjg@chromium.org>,
	"Yevgeny Popovych" <yevgenyp@pointgrab.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH U-BOOT v3 25/30] fs: btrfs: Implement btrfs_file_read()
Date: Tue, 8 Sep 2020 08:59:34 +0800	[thread overview]
Message-ID: <9588696f-9013-29c6-c3de-d0fae7ac339a@suse.com> (raw)
In-Reply-To: <20200908005646.GG7259@bill-the-cat>


On 2020/9/8 上午8:56, Tom Rini wrote:
> On Tue, Sep 08, 2020 at 08:26:27AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/9/8 上午6:35, Tom Rini wrote:
>>> On Wed, Jun 24, 2020 at 06:03:11PM +0200, Marek Behún wrote:
>>>
>>>> From: Qu Wenruo <wqu@suse.com>
>>>>
>>>> This version of btrfs_file_read() has the following new features:
>>>> - Tries all mirrors
>>>> - More handling on unaligned size
>>>> - Better compressed extent handling
>>>>   The old implementation doesn't handle compressed extent with offset
>>>>   properly: we need to read out the whole compressed extent, then
>>>>   decompress the whole extent, and only then copy the requested part.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> Reviewed-by: Marek Behún <marek.behun@nic.cz>
>>>
>>> Note that this introduces a warning with LLVM-10:
>>> fs/btrfs/btrfs.c:246:6: warning: variable 'real_size' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>>>         if (!len) {
>>>             ^~~~
>>> fs/btrfs/btrfs.c:255:12: note: uninitialized use occurs here
>>>         if (len > real_size - offset)
>>>                   ^~~~~~~~~
>>> fs/btrfs/btrfs.c:246:2: note: remove the 'if' if its condition is always true
>>>         if (!len) {
>>>         ^~~~~~~~~~
>>> fs/btrfs/btrfs.c:228:18: note: initialize the variable 'real_size' to silence this warning
>>>         loff_t real_size;
>>>                         ^
>>>                          = 0
>>> 1 warning generated.
>>>
>>> and I have silenced as suggested.  I'm not 100% happy with that, but
>>> leave fixing it here and in upstream btrfs-progs to the btrfs-experts.
>>
>> My bad. The warning is correct, and since the code is U-boot specific,
>> it doesn't affect kernel (using page) nor btrfs-progs (not really do
>> file read with offset).
>>
>> The fix is a little complex.
>>
>> In fact we need to always call btrfs_size() to grab the real_size, and
>> then modify @len using real_size, either using real_size directly, or do
>> some basic calculation.
> 
> Ah, thanks.  I'll fold in your changes.
> 
>>
>> BTW, I didn't see the btrfs rebuild work merged upstream. Is this
>> planned or you just grab from some specific branch?
> 
> Yes, I'm testing them for -next right now.
> 
That's awesome!

Thanks for your effort,
Qu