* [PATCH 1/2] btrfs: refactor the inline extent read code inside btrfs_get_extent()
2022-09-15 8:22 [PATCH 0/2] btrfs: btrfs_get_extent() cleanup Qu Wenruo
@ 2022-09-15 8:22 ` Qu Wenruo
2022-09-15 15:40 ` Josef Bacik
2022-09-15 8:22 ` [PATCH 2/2] btrfs: selftests: remove impossible inline extent at non-zero file offset Qu Wenruo
1 sibling, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2022-09-15 8:22 UTC (permalink / raw)
To: linux-btrfs
[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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] btrfs: selftests: remove impossible inline extent at non-zero file offset
2022-09-15 8:22 [PATCH 0/2] btrfs: btrfs_get_extent() cleanup Qu Wenruo
2022-09-15 8:22 ` [PATCH 1/2] btrfs: refactor the inline extent read code inside btrfs_get_extent() Qu Wenruo
@ 2022-09-15 8:22 ` Qu Wenruo
1 sibling, 0 replies; 5+ messages in thread
From: Qu Wenruo @ 2022-09-15 8:22 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 5+ messages in thread