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