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

[Josef Bacik <josef@toxicpanda.com>]

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,

Josef