Re: [PATCH 5/6] btrfs: Simplify extent type check

[Nikolay Borisov <nborisov@suse.com>]

On 22.08.19 г. 2:47 ч., Qu Wenruo wrote:
> 
> 
> On 2019/8/21 下午11:40, David Sterba wrote:
>> On Mon, Aug 05, 2019 at 05:47:07PM +0300, Nikolay Borisov wrote:
>>> Extent type can only be regular/prealloc/inline. The main branch of the
>>> 'if' already handles the first two, leaving the 'else' to handle inline.
>>> Furthermore, tree-checker ensures that leaf items are correct.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>  fs/btrfs/inode.c | 10 +++-------
>>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 8e24b7641247..6c3f9f3a7ed1 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -1502,18 +1502,14 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>>>  			if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
>>>  				goto out_check;
>>>  			nocow = true;
>>> -		} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>>> -			extent_end = found_key.offset +
>>> -				btrfs_file_extent_ram_bytes(leaf, fi);
>>> -			extent_end = ALIGN(extent_end,
>>> -					   fs_info->sectorsize);
>>> +		} else {
>>> +			extent_end = found_key.offset + ram_bytes;
>>> +			extent_end = ALIGN(extent_end, fs_info->sectorsize);
>>>  			/* Skip extents outside of our requested range */
>>>  			if (extent_end <= start) {
>>>  				path->slots[0]++;
>>>  				goto next_slot;
>>>  			}
>>> -		} else {
>>> -			BUG();
>>
>> I am not sure if we should delete this or leave it (with a message what
>> happened). There are other places that switch value from a known set and
>> have a catch-all branch.
> 
> We can just delete it IHMO.
> 
> That's why we have tree-checker, we have ensured at least EXTENT_DATA
> item read from disk doesn't contain invalid type.
> So removing the BUG() here should be OK.
> 
> Although converting it to a better error handler won't hurt.
> In that case it can catch runtime memory corruption earlier.

IMO it's counter productive to have duplicated checks. That's just more
code that someone needs to grok down the line when they need to
understand the code. If the tree-checker was an optional feature - then
perhaps it makes sense. But since the tree-checker is, so to speak, the
outer layer of protection, ensuring the btrfs shouldn't work with
invalid data then I'd rather remove the check.

> 
> Thanks,
> Qu
> 
>>
>> With your change the 'catch-all' is the inline extent type. It's true
>> that the checker should not let an unknown type appear in this code,
>> however I'd rather make it explicit that something is seriously wrong if
>> there's an unexpected type rather than silently continuing.
>>
>> The BUG can be turned to actual error handling so we don't need to crash
>> at least.
>>
>