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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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.

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.
>