[PATCH v2 0/5] btrfs: btrfs_get_extent() cleanup for inline extents

[PATCH v2 0/5] btrfs: btrfs_get_extent() cleanup for inline extents

[Qu Wenruo]

Currently we have some very confusing (and harder to explain) code
inside btrfs_get_extent() for inline extents.

The TL;DR is, those mess is to support inline extents at non-zero file
offset.

This is completely impossible now, as tree-checker will reject any
inline file extents at non-zero offset.

So this series will:

- Update the selftest to get rid of the impossible case

- Simplify the inline extent read

  Since we no longer need to support inline extents at non-zero file
  offset, some variables can be removed.

- Don't reset the inline extent map members

  Those resets are either doing nothing (setting the member to the same
  value), or making no difference (the member is not utilized anyway)

- Remove @new_inline argument for btrfs_extent_item_to_extent_map()

  That argument makes btrfs_get_extent() to skip certain member update.
  Previously it makes a difference only for fiemap.

  But now since fiemap no longer relies on extent_map, the remaining
  use cases won't bother those members anyway.

  Thus we can remove the bool argument and make
  btrfs_extent_item_to_extent_map() to have a consistent behavior.

- Introduce a helper to do inline extents read

  Now the function is so simple that, extracting the helper can only
  reduce indents.

[CHANGELOG]
v2:
- Do better patch split for bisection with better reason

- Put the selftest to the first to help bisection
  Or we may hit ASSERT() during selftest.

Qu Wenruo (5):
  btrfs: selftests: remove impossible inline extent at non-zero file
    offset
  btrfs: make inline extent read calculation much simpler
  btrfs: do not reset extent map members for inline extents read
  btrfs: remove the @new_inline argument from
    btrfs_extent_item_to_extent_map()
  btrfs: extract the inline extent read code into its own function

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

-- 
2.37.3

[PATCH v2 1/5] 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 d5640e695e6b..d1847d9f4841 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 b1c88dd187cb..87b5ee9d853b 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

[PATCH v2 2/5] btrfs: make inline extent read calculation much simpler

[Qu Wenruo]

Currently we calculate inline extent read in a way that inline extent
can start at non-zero offset.

This is consistent with the inode selftest, which puts an inline extent
at file offset 5.

Meanwhile the inline extent creation code will only create inline extent
at file offset 0.

Furthermore with the introduction of tree-checker on file extents, we are
actively rejecting inline extent which starts at non-zero file offset.
And so far we haven't yet seen any report of rejected inline extents at
non-zero file offset.

This all means, the extra calculation to support inline extents at
non-zero file offset is mostly paper weight, and damaging the
readability of the code.

Thus this patch will:

- Add extra ASSERT()s to make sure involved file offset are all 0

- Remove @extent_offset calculation

- Simplify the involved code
  As several variables are now single-use, no need to declare them as
  a variable anymore.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6fde13f62c1d..5df5b0714d40 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6983,41 +6983,43 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	    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;
+		/*
+		 * Inline extent can only exist at file offset 0. This is
+		 * ensured by tree-checker and inline extent creation path.
+		 * Thus all members representing file offsets should be zero.
+		 */
+		ASSERT(page_offset(page) == 0);
+		ASSERT(pg_offset == 0);
+		ASSERT(extent_start == 0);
+		ASSERT(em->start == 0);
+
+		copy_size = min_t(u64, PAGE_SIZE,
+				  btrfs_file_extent_ram_bytes(leaf, item));
+		em->start = extent_start;
 		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);
+				ret = uncompress_inline(path, page, 0, 0, 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);
-				}
+				read_extent_buffer(leaf, map,
+					btrfs_file_extent_inline_start(item),
+					copy_size);
+				if (copy_size < PAGE_SIZE)
+					memset(map + copy_size, 0,
+					       PAGE_SIZE - copy_size);
 				kunmap_local(map);
 			}
 			flush_dcache_page(page);
-- 
2.37.3

[PATCH v2 3/5] btrfs: do not reset extent map members for inline extents read

[Qu Wenruo]

Currently for inline extents read inside btrfs_get_extent(), we will
reset several extent map members:

- em->start

  Reset to extent_start, which is completely unnecessary.
  The extent_start and em->start should have already be zero, ensured by
  tree-checker already.

- em->len

  Reset the round_up(copy_size, fs_info->sectorsize), which is again
  unnecessary.

- em->orig_block_len

  Reset to em->len (sectorsize), while it is originally unset from
  btrfs_extent_item_to_extent_map().

  This makes no difference, as all extent map handling paths will
  ignore the orig_block_len if they found it's an inlined extent.

  Such inline extent orig_block_len ignoring examples can be found in
  btrfs_drop_extent_cache().

- em->orig_start

  Reset to em->start (0), while it is originally set to EXTENT_MAP_HOLE.

  This makes no difference either, as all extent map handling paths will
  ignore the em->orig_start if they found it's an inline extent.

Thus all these em members resetting are unnecessary.

Replace them with ASSERT()s checking the only two members (block_start
and length) that make sense.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5df5b0714d40..41562d9d2113 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7001,10 +7001,15 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 
 		copy_size = min_t(u64, PAGE_SIZE,
 				  btrfs_file_extent_ram_bytes(leaf, item));
-		em->start = extent_start;
-		em->len = ALIGN(copy_size, fs_info->sectorsize);
-		em->orig_block_len = em->len;
-		em->orig_start = em->start;
+
+		/*
+		 * btrfs_extent_item_to_extent_map() should have properly
+		 * initialized em members already.
+		 *
+		 * Other members are not utilized for inline extents.
+		 */
+		ASSERT(em->block_start == EXTENT_MAP_INLINE);
+		ASSERT(em->len = fs_info->sectorsize);
 
 		if (!PageUptodate(page)) {
 			if (btrfs_file_extent_compression(leaf, item) !=
-- 
2.37.3

[PATCH v2 4/5] btrfs: remove the @new_inline argument from btrfs_extent_item_to_extent_map()

[Qu Wenruo]

The argument @new_inline changes the following members of extent_map:

- em->compress_type
- EXTENT_FLAG_COMPRESSED of em->flags

However neither members makes a difference for inline extents:

- Inline extent read never use above em members

  As inside btrfs_get_extent() we directly use the file extent item to
  do the read.

- Inline extents are never to be split

  Thus code really needs em->compress_type or that flag will never be
  executed on inlined extents.
  (btrfs_drop_extent_cache() would be one example)

- Fiemap no longer relies on extent maps

  Recent fiemap optimization makes fiemap to search subvolume tree
  directly, without using any extent map at all.

  Thus those members make no difference for inline extents any more.

Furthermore such exception without much explanation is really a source
of confusion.

Thus this patch will completely remove the argument, and always set the
involved members, unifying the behavior.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h     | 1 -
 fs/btrfs/file-item.c | 6 ++----
 fs/btrfs/inode.c     | 2 +-
 fs/btrfs/ioctl.c     | 2 +-
 4 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 8b7b7a212da0..7deec2977d28 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 45949261c699..ba4624b541f5 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 (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 41562d9d2113..27fe46ef5e86 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6977,7 +6977,7 @@ 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) {
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d5dd8bed1488..ebcb2d31eb16 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 v2 5/5] btrfs: extract the inline extent read code into its own function

[Qu Wenruo]

Currently we have inline extent read code behind two levels of indent,
just extract them into a new function, read_inline_extent(), to make it
a little easier to read.

Since we're here, also remove @extent_offset and @pg_offset arguments
from uncompress_inline() function, as it's not possible to have inline
extents at non-inline file offset.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 73 +++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 27fe46ef5e86..871c65f72822 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6784,7 +6784,6 @@ static int btrfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 
 static noinline int uncompress_inline(struct btrfs_path *path,
 				      struct page *page,
-				      size_t pg_offset, u64 extent_offset,
 				      struct btrfs_file_extent_item *item)
 {
 	int ret;
@@ -6795,7 +6794,6 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 	unsigned long ptr;
 	int compress_type;
 
-	WARN_ON(pg_offset != 0);
 	compress_type = btrfs_file_extent_compression(leaf, item);
 	max_size = btrfs_file_extent_ram_bytes(leaf, item);
 	inline_size = btrfs_file_extent_inline_item_len(leaf, path->slots[0]);
@@ -6807,8 +6805,8 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 	read_extent_buffer(leaf, tmp, ptr, inline_size);
 
 	max_size = min_t(unsigned long, PAGE_SIZE, max_size);
-	ret = btrfs_decompress(compress_type, tmp, page,
-			       extent_offset, inline_size, max_size);
+	ret = btrfs_decompress(compress_type, tmp, page, 0, inline_size,
+			       max_size);
 
 	/*
 	 * decompression code contains a memset to fill in any space between the end
@@ -6818,13 +6816,43 @@ static noinline int uncompress_inline(struct btrfs_path *path,
 	 * cover that region here.
 	 */
 
-	if (max_size + pg_offset < PAGE_SIZE)
-		memzero_page(page,  pg_offset + max_size,
-			     PAGE_SIZE - max_size - pg_offset);
+	if (max_size < PAGE_SIZE)
+		memzero_page(page,  max_size, PAGE_SIZE - max_size);
 	kfree(tmp);
 	return ret;
 }
 
+static int read_inline_extent(struct btrfs_inode *inode,
+			      struct btrfs_path *path,
+			      struct page *page)
+{
+	struct btrfs_file_extent_item *fi;
+	void *kaddr;
+	size_t copy_size;
+
+	if (!page || PageUptodate(page))
+		return 0;
+
+	ASSERT(page_offset(page) == 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) !=
+	    BTRFS_COMPRESS_NONE)
+		return uncompress_inline(path, page, fi);
+
+	copy_size = min_t(u64, PAGE_SIZE,
+			  btrfs_file_extent_ram_bytes(path->nodes[0], fi));
+	kaddr = kmap_local_page(page);
+	read_extent_buffer(path->nodes[0], kaddr,
+			   btrfs_file_extent_inline_start(fi), copy_size);
+	kunmap_local(kaddr);
+	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
@@ -6983,25 +7011,15 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	    extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
 		goto insert;
 	} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
-		char *map;
-		size_t copy_size;
-
-		if (!page)
-			goto out;
-
 		/*
 		 * Inline extent can only exist at file offset 0. This is
 		 * ensured by tree-checker and inline extent creation path.
 		 * Thus all members representing file offsets should be zero.
 		 */
-		ASSERT(page_offset(page) == 0);
 		ASSERT(pg_offset == 0);
 		ASSERT(extent_start == 0);
 		ASSERT(em->start == 0);
 
-		copy_size = min_t(u64, PAGE_SIZE,
-				  btrfs_file_extent_ram_bytes(leaf, item));
-
 		/*
 		 * btrfs_extent_item_to_extent_map() should have properly
 		 * initialized em members already.
@@ -7011,24 +7029,9 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 		ASSERT(em->block_start == EXTENT_MAP_INLINE);
 		ASSERT(em->len = fs_info->sectorsize);
 
-		if (!PageUptodate(page)) {
-			if (btrfs_file_extent_compression(leaf, item) !=
-			    BTRFS_COMPRESS_NONE) {
-				ret = uncompress_inline(path, page, 0, 0, item);
-				if (ret)
-					goto out;
-			} else {
-				map = kmap_local_page(page);
-				read_extent_buffer(leaf, map,
-					btrfs_file_extent_inline_start(item),
-					copy_size);
-				if (copy_size < PAGE_SIZE)
-					memset(map + copy_size, 0,
-					       PAGE_SIZE - copy_size);
-				kunmap_local(map);
-			}
-			flush_dcache_page(page);
-		}
+		ret = read_inline_extent(inode, path, page);
+		if (ret < 0)
+			goto out;
 		goto insert;
 	}
 not_found:
-- 
2.37.3

Re: [PATCH v2 0/5] btrfs: btrfs_get_extent() cleanup for inline extents

[David Sterba]

On Fri, Sep 16, 2022 at 03:28:34PM +0800, Qu Wenruo wrote:
> Currently we have some very confusing (and harder to explain) code
> inside btrfs_get_extent() for inline extents.
> 
> The TL;DR is, those mess is to support inline extents at non-zero file
> offset.
> 
> This is completely impossible now, as tree-checker will reject any
> inline file extents at non-zero offset.

The extent item check has been in place since 2017, but I don't remember
if it was ever possible the create inline uncompressed extent other than
at offset 0. I know there is the inconsistency with compressed extent
but for plain uncompressed it should not be normally possible.

> So this series will:
> 
> - Update the selftest to get rid of the impossible case
> 
> - Simplify the inline extent read
> 
>   Since we no longer need to support inline extents at non-zero file
>   offset, some variables can be removed.
> 
> - Don't reset the inline extent map members
> 
>   Those resets are either doing nothing (setting the member to the same
>   value), or making no difference (the member is not utilized anyway)
> 
> - Remove @new_inline argument for btrfs_extent_item_to_extent_map()
> 
>   That argument makes btrfs_get_extent() to skip certain member update.
>   Previously it makes a difference only for fiemap.
> 
>   But now since fiemap no longer relies on extent_map, the remaining
>   use cases won't bother those members anyway.
> 
>   Thus we can remove the bool argument and make
>   btrfs_extent_item_to_extent_map() to have a consistent behavior.
> 
> - Introduce a helper to do inline extents read
> 
>   Now the function is so simple that, extracting the helper can only
>   reduce indents.
> 
> [CHANGELOG]
> v2:
> - Do better patch split for bisection with better reason
> 
> - Put the selftest to the first to help bisection
>   Or we may hit ASSERT() during selftest.
> 
> Qu Wenruo (5):
>   btrfs: selftests: remove impossible inline extent at non-zero file
>     offset
>   btrfs: make inline extent read calculation much simpler
>   btrfs: do not reset extent map members for inline extents read
>   btrfs: remove the @new_inline argument from
>     btrfs_extent_item_to_extent_map()
>   btrfs: extract the inline extent read code into its own function

I did a quick review, code looks ok for inclusion to for-next.

Re: [PATCH v2 5/5] btrfs: extract the inline extent read code into its own function

[David Sterba]

On Fri, Sep 16, 2022 at 03:28:39PM +0800, Qu Wenruo wrote:
> Currently we have inline extent read code behind two levels of indent,
> just extract them into a new function, read_inline_extent(), to make it
> a little easier to read.
> 
> Since we're here, also remove @extent_offset and @pg_offset arguments
> from uncompress_inline() function, as it's not possible to have inline
> extents at non-inline file offset.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/inode.c | 73 +++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 27fe46ef5e86..871c65f72822 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> +static int read_inline_extent(struct btrfs_inode *inode,
> +			      struct btrfs_path *path,
> +			      struct page *page)
> +{
> +	struct btrfs_file_extent_item *fi;
> +	void *kaddr;
> +	size_t copy_size;
> +
> +	if (!page || PageUptodate(page))
> +		return 0;
> +
> +	ASSERT(page_offset(page) == 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) !=
> +	    BTRFS_COMPRESS_NONE)
> +		return uncompress_inline(path, page, fi);
> +
> +	copy_size = min_t(u64, PAGE_SIZE,
> +			  btrfs_file_extent_ram_bytes(path->nodes[0], fi));
> +	kaddr = kmap_local_page(page);
> +	read_extent_buffer(path->nodes[0], kaddr,
> +			   btrfs_file_extent_inline_start(fi), copy_size);
> +	kunmap_local(kaddr);
> +	if (copy_size < PAGE_SIZE)
> +		memzero_page(page, copy_size, PAGE_SIZE - copy_size);
> +	flush_dcache_page(page);

memzero_page already does flush_dcache_page so it's not needed.

> +	return 0;
> +}
> +
>  /**
>   * btrfs_get_extent - Lookup the first extent overlapping a range in a file.
>   * @inode:	file to search in

Re: [PATCH v2 0/5] btrfs: btrfs_get_extent() cleanup for inline extents

[David Sterba]

On Tue, Oct 25, 2022 at 04:08:51PM +0200, David Sterba wrote:
> > v2:
> > - Do better patch split for bisection with better reason
> > 
> > - Put the selftest to the first to help bisection
> >   Or we may hit ASSERT() during selftest.
> > 
> > Qu Wenruo (5):
> >   btrfs: selftests: remove impossible inline extent at non-zero file
> >     offset
> >   btrfs: make inline extent read calculation much simpler
> >   btrfs: do not reset extent map members for inline extents read
> >   btrfs: remove the @new_inline argument from
> >     btrfs_extent_item_to_extent_map()
> >   btrfs: extract the inline extent read code into its own function
> 
> I did a quick review, code looks ok for inclusion to for-next.

Moved to misc-next, thanks.