Re: [PATCH 1/2] btrfs: refactor the inline extent read code inside btrfs_get_extent()

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

On 2022/9/15 23:40, Josef Bacik wrote:
> On Thu, Sep 15, 2022 at 04:22:51PM +0800, Qu Wenruo wrote:
>> [PROBLEM]
>> In btrfs_get_extent() we can fill the inline extent directly.
>>
>> But there are something not that straightforward for the functionality:
>>
>> - It's behind two levels of indent
>>
>> - It has a weird reset for extent map values
>>    This includes resetting:
>>    * em->start
>>
>>      This makes no sense, as our current tree-checker ensures that
>>      inline extent can only at file offset 0.
>>
>>      This means not only the @extent_start (key.offset) is always 0,
>>      but also @pg_offset and @extent_offset should also be 0.
>>
>>    * em->len
>>
>>      In btrfs_extent_item_to_extent_map(), we already initialize em->len
>>      to "btrfs_file_extent_end() - extent_start".
>>      Since @extent_start can only be 0 for inline extents, and
>>      btrfs_file_extent_end() is already doing ALIGN() (which is rounding
>>      up).
>>
>>      So em->len is always sector_size for inlined extent now.
>>
>>    * em->orig_block_len/orig_start
>>
>>      They doesn't make much sense for inlined extent anyway.
>>
>> - Extra complex calculation for inline extent read
>>
>>    This is mostly caused by the fact it's still assuming we can have
>>    inline file extents at non-zero file offset.
>>
>>    Such offset calculation is now a dead code which we will never reach,
>>    just damaging the readability.
>>
>> - We have an extra bool for btrfs_extent_item_to_extent_map()
>>
>>    Which is making no difference for now.
>>
>> [ENHANCEMENT]
>> This patch will enhance the behavior by:
>>
>> - Extract the read code into a new helper, read_inline_extent()
>>
>> - Much simpler calculation for inline extent read
>>
>> - Don't touch extent map when reading inline extents
>>
>> - Remove the bool argument from btrfs_extent_item_to_extent_map()
>>
>> - New ASSERT()s to ensure inline extents only start at file offset 0
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/ctree.h     |  1 -
>>   fs/btrfs/file-item.c |  6 +--
>>   fs/btrfs/inode.c     | 93 +++++++++++++++++++++++++-------------------
>>   fs/btrfs/ioctl.c     |  2 +-
>>   4 files changed, 57 insertions(+), 45 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 05df502c3c9d..e8ce86516ec8 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -3356,7 +3356,6 @@ int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
>>   void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>>   				     const struct btrfs_path *path,
>>   				     struct btrfs_file_extent_item *fi,
>> -				     const bool new_inline,
>>   				     struct extent_map *em);
>>   int btrfs_inode_clear_file_extent_range(struct btrfs_inode *inode, u64 start,
>>   					u64 len);
>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>> index 29999686d234..d9c3b58b63bf 100644
>> --- a/fs/btrfs/file-item.c
>> +++ b/fs/btrfs/file-item.c
>> @@ -1196,7 +1196,6 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
>>   void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>>   				     const struct btrfs_path *path,
>>   				     struct btrfs_file_extent_item *fi,
>> -				     const bool new_inline,
>>   				     struct extent_map *em)
>>   {
>>   	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>> @@ -1248,10 +1247,9 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
>>   		 */
>>   		em->orig_start = EXTENT_MAP_HOLE;
>>   		em->block_len = (u64)-1;
>> -		if (!new_inline && compress_type != BTRFS_COMPRESS_NONE) {
>> +		em->compress_type = compress_type;
>> +		if (em->compress_type != BTRFS_COMPRESS_NONE)
>>   			set_bit(EXTENT_FLAG_COMPRESSED, &em->flags);
>> -			em->compress_type = compress_type;
>> -		}
>
> I spent most of my time reviewing this patch trying to decide if this mattered
> or not, why it was introduced in the first place, and if it was ok to change.
> Additionally it's not related to the bulk of the change which is making the
> btrfs_get_extent thing cleaner.
>
> So break this out into it's own patch, with a detailed explanation of why this
> particular change is ok.  In fact I'd prefer if you made all changes to how we
> mess with the em their own changes, since bugs here can be super subtle, I'd
> rather have more discreete areas to bisect to.  Thanks,

Sure, in fact I'm also a little unsure if I should put all these changes
into one patch.

Will split all these changes into separate ones.

Thanks,
Qu

>
> Josef