[PATCH 0/2] btrfs: btrfs_get_extent() cleanup

[PATCH 0/2] btrfs: btrfs_get_extent() cleanup

[Qu Wenruo]

There are some small but weird behavior in btrfs_get_extent() for inline
extent handling

- Resetting em members only for inline extent read

  If btrfs_get_extent() is called with @page (aka, for file read path),
  it will reset @em members even it's already updated by
  btrfs_extent_item_to_extent_map()

  The behavior itself is no longer needed as tree-checker has ensured
  inline extents are only valid if they have 0 file offset.

  Thus this means, in that path, @extent_offset must be 0, and a lot of
  calculations can be simplified and the em member reset is unnecessary.

- Unnecessarily complex handling for inline extent read

  The truth is, since inline extents can only exist at file offset 0, we
  don't need such complex calculation at all.

- Unnecessary argument for btrfs_extent_item_to_extent_map()

- Unnecessarily complex selftest for btrfs_get_extent()
  It has an inline extent at file offset 5, which is no longer valid.

The root cause is, the old code just assumes we can have inline extents
at non-zero file offset.

The patchset will replace those complex code, with just ASSERT()s, and
use much cleaner code to implement the same behavior, and also to update
the selftest to reflect the modern behavior.

Qu Wenruo (2):
  btrfs: refactor the inline extent read code inside btrfs_get_extent()
  btrfs: selftests: remove impossible inline extent at non-zero file
    offset

 fs/btrfs/ctree.h             |  1 -
 fs/btrfs/extent_map.c        |  7 +++
 fs/btrfs/file-item.c         |  6 +--
 fs/btrfs/inode.c             | 93 +++++++++++++++++++++---------------
 fs/btrfs/ioctl.c             |  2 +-
 fs/btrfs/tests/inode-tests.c | 56 +++++++---------------
 6 files changed, 82 insertions(+), 83 deletions(-)

-- 
2.37.3

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

[Qu Wenruo]

[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;
-		}
 	} else {
 		btrfs_err(fs_info,
 			  "unknown file extent item type %d, inode %llu, offset %llu, "
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 10849db7f3a5..57fbd8665221 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6830,6 +6830,47 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 	return ret;
 }
 
+/* To read the inline extent into the correct page position. */
+static int read_inline_extent(struct btrfs_inode *inode,
+			      struct btrfs_path *path,
+			      struct extent_map *em,
+			      struct page *page)
+{
+	struct btrfs_file_extent_item *fi;
+	u64 ram_bytes;
+	size_t copy_size;
+	void *kaddr;
+	int ret;
+
+	if (!page || PageUptodate(page))
+		return 0;
+
+	fi = btrfs_item_ptr(path->nodes[0], path->slots[0],
+			    struct btrfs_file_extent_item);
+
+	if (btrfs_file_extent_compression(path->nodes[0], fi)) {
+		ret = uncompress_inline(path, page, 0, 0, fi);
+		if (ret < 0)
+			return ret;
+		flush_dcache_page(page);
+		return 0;
+	}
+
+	ram_bytes = btrfs_file_extent_ram_bytes(path->nodes[0], fi);
+	copy_size = min_t(u64, PAGE_SIZE, ram_bytes);
+
+
+	kaddr = kmap_local_page(page);
+	read_extent_buffer(path->nodes[0], kaddr,
+			   btrfs_file_extent_inline_start(fi), copy_size);
+	kunmap_local(kaddr);
+	/* Zero the remaining part inside the page. */
+	if (copy_size < PAGE_SIZE)
+		memzero_page(page, copy_size, PAGE_SIZE - copy_size);
+	flush_dcache_page(page);
+	return 0;
+}
+
 /**
  * btrfs_get_extent - Lookup the first extent overlapping a range in a file.
  * @inode:	file to search in
@@ -6982,51 +7023,25 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 		goto insert;
 	}
 
-	btrfs_extent_item_to_extent_map(inode, path, item, !page, em);
+	btrfs_extent_item_to_extent_map(inode, path, item, em);
 
 	if (extent_type == BTRFS_FILE_EXTENT_REG ||
 	    extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
 		goto insert;
 	} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
-		unsigned long ptr;
-		char *map;
-		size_t size;
-		size_t extent_offset;
-		size_t copy_size;
-
-		if (!page)
-			goto out;
-
-		size = btrfs_file_extent_ram_bytes(leaf, item);
-		extent_offset = page_offset(page) + pg_offset - extent_start;
-		copy_size = min_t(u64, PAGE_SIZE - pg_offset,
-				  size - extent_offset);
-		em->start = extent_start + extent_offset;
-		em->len = ALIGN(copy_size, fs_info->sectorsize);
-		em->orig_block_len = em->len;
-		em->orig_start = em->start;
-		ptr = btrfs_file_extent_inline_start(item) + extent_offset;
-
-		if (!PageUptodate(page)) {
-			if (btrfs_file_extent_compression(leaf, item) !=
-			    BTRFS_COMPRESS_NONE) {
-				ret = uncompress_inline(path, page, pg_offset,
-							extent_offset, item);
-				if (ret)
-					goto out;
-			} else {
-				map = kmap_local_page(page);
-				read_extent_buffer(leaf, map + pg_offset, ptr,
-						   copy_size);
-				if (pg_offset + copy_size < PAGE_SIZE) {
-					memset(map + pg_offset + copy_size, 0,
-					       PAGE_SIZE - pg_offset -
-					       copy_size);
-				}
-				kunmap_local(map);
-			}
-			flush_dcache_page(page);
+		/*
+		 * If there is an inline extent, the file offset,
+		 * page_offset(), and @pg_offset must all be zero,
+		 * as inline extent can only exist at file offset 0.
+		 */
+		ASSERT(em->start == 0);
+		if (page) {
+			ASSERT(page_offset(page) == 0);
+			ASSERT(pg_offset == 0);
 		}
+		ret = read_inline_extent(inode, path, em, page);
+		if (ret < 0)
+			goto out;
 		goto insert;
 	}
 not_found:
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index fe0cc816b4eb..a623bd37d010 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1159,7 +1159,7 @@ static struct extent_map *defrag_get_extent(struct btrfs_inode *inode,
 			goto next;
 
 		/* Now this extent covers @start, convert it to em */
-		btrfs_extent_item_to_extent_map(inode, &path, fi, false, em);
+		btrfs_extent_item_to_extent_map(inode, &path, fi, em);
 		break;
 next:
 		ret = btrfs_next_item(root, &path);
-- 
2.37.3

[PATCH 2/2] btrfs: selftests: remove impossible inline extent at non-zero file offset

[Qu Wenruo]

In our inode-tests.c, we create an inline offset at file offset 5, which
is no longer possible since the introduction of tree-checker.

Thus I don't think we should spend time maintaining some corner cases
which are already ruled out by tree-checker.

So this patch will:

- Change the inline extent to start at file offset 0

  Also change its length to 6 to cover the original length

- Add an extra ASSERT() for btrfs_add_extent_mapping()

  This is to make sure tree-checker is working correctly.

- Update the inode selftest

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_map.c        |  7 +++++
 fs/btrfs/tests/inode-tests.c | 56 ++++++++++++------------------------
 2 files changed, 25 insertions(+), 38 deletions(-)

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 6fee14ce2e6b..59e5a035359b 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -610,6 +610,13 @@ int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
 	int ret;
 	struct extent_map *em = *em_in;
 
+	/*
+	 * Tree-checker should have rejected any inline extent with non-zero
+	 * file offset. Here just do a sanity check.
+	 */
+	if (em->block_start == EXTENT_MAP_INLINE)
+		ASSERT(em->start == 0);
+
 	ret = add_extent_mapping(em_tree, em, 0);
 	/* it is possible that someone inserted the extent into the tree
 	 * while we had the lock dropped.  It is also possible that
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index cac89c388131..0521c31610a4 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -72,8 +72,8 @@ static void insert_inode_item_key(struct btrfs_root *root)
  * diagram of how the extents will look though this may not be possible we still
  * want to make sure everything acts normally (the last number is not inclusive)
  *
- * [0 - 5][5 -  6][     6 - 4096     ][ 4096 - 4100][4100 - 8195][8195 - 12291]
- * [hole ][inline][hole but no extent][  hole   ][   regular ][regular1 split]
+ * [0  - 6][     6 - 4096     ][ 4096 - 4100][4100 - 8195][8195  -  12291]
+ * [inline][hole but no extent][    hole    ][   regular ][regular1 split]
  *
  * [12291 - 16387][16387 - 24579][24579 - 28675][ 28675 - 32771][32771 - 36867 ]
  * [    hole    ][regular1 split][   prealloc ][   prealloc1  ][prealloc1 written]
@@ -90,19 +90,12 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 	u64 disk_bytenr = SZ_1M;
 	u64 offset = 0;
 
-	/* First we want a hole */
-	insert_extent(root, offset, 5, 5, 0, 0, 0, BTRFS_FILE_EXTENT_REG, 0,
-		      slot);
-	slot++;
-	offset += 5;
-
 	/*
-	 * Now we want an inline extent, I don't think this is possible but hey
-	 * why not?  Also keep in mind if we have an inline extent it counts as
-	 * the whole first page.  If we were to expand it we would have to cow
-	 * and we wouldn't have an inline extent anymore.
+	 * Tree-checker has strict limits on inline extents that they can only
+	 * exist at file offset 0, thus we can only have one inline file extent
+	 * at most.
 	 */
-	insert_extent(root, offset, 1, 1, 0, 0, 0, BTRFS_FILE_EXTENT_INLINE, 0,
+	insert_extent(root, offset, 6, 6, 0, 0, 0, BTRFS_FILE_EXTENT_INLINE, 0,
 		      slot);
 	slot++;
 	offset = sectorsize;
@@ -281,37 +274,24 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
 		test_err("got an error when we shouldn't have");
 		goto out;
 	}
-	if (em->block_start != EXTENT_MAP_HOLE) {
-		test_err("expected a hole, got %llu", em->block_start);
-		goto out;
-	}
-	if (em->start != 0 || em->len != 5) {
-		test_err(
-		"unexpected extent wanted start 0 len 5, got start %llu len %llu",
-			em->start, em->len);
-		goto out;
-	}
-	if (em->flags != 0) {
-		test_err("unexpected flags set, want 0 have %lu", em->flags);
-		goto out;
-	}
-	offset = em->start + em->len;
-	free_extent_map(em);
-
-	em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
-	if (IS_ERR(em)) {
-		test_err("got an error when we shouldn't have");
-		goto out;
-	}
 	if (em->block_start != EXTENT_MAP_INLINE) {
 		test_err("expected an inline, got %llu", em->block_start);
 		goto out;
 	}
 
-	if (em->start != offset || em->len != (sectorsize - 5)) {
+	/*
+	 * For inline extent, we always round up the em to sectorsize, as
+	 * they are either:
+	 * a) a hidden hole
+	 *    The range will be zeroed at inline extent read time.
+	 *
+	 * b) a file extent with unaligned bytenr
+	 *    Tree checker will reject it.
+	 */
+	if (em->start != 0 || em->len != sectorsize) {
 		test_err(
-	"unexpected extent wanted start %llu len 1, got start %llu len %llu",
-			offset, em->start, em->len);
+	"unexpected extent wanted start 0 len %u, got start %llu len %llu",
+			sectorsize, em->start, em->len);
 		goto out;
 	}
 	if (em->flags != 0) {
-- 
2.37.3

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

[Josef Bacik]

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

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

[Qu Wenruo]

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