* [PATCH 00/10] Cleanup metadata page reading path
@ 2020-09-09 9:49 Nikolay Borisov
2020-09-09 9:49 ` [PATCH 01/10] btrfs: Remove btree_readpage Nikolay Borisov
` (9 more replies)
0 siblings, 10 replies; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
This series streamlines the page read path for metadata pages. Currently
__do_readpage is shared between ordinary data inodes as well as the btree_inode.
This is unnecenssary and brings clutter to the interface of __do_readpage et al
in the form of needlessly passed parameters and unnecessary actions performed
in btree_get_extent just to appease the interface/logic of __do_readpage.
To simplify the code metadata read is switched from calling __do_readpage to
directly calling submit_extent_page in read_extent_buffer_pages which is sufficient
to grab metadata pages. This in turn paves the way for further cleanups by
removing a lot of arguments from top level functions and sinking them in
lowe-level worker functions.
Patch 1 and 2 remove unused function and a parameter from btrfs_get_extent.
Patch 3 switches read_extent_buffer_pages (sole metadata read path) to using
submit_extent_page which enables further simplifications. Following patches
now-unused functions and removes certain indirection now that __do_readpage
is used only for data read.
Every patch has survived a -g quick and the overall series survived a full
xfstest run.
Nikolay Borisov (10):
btrfs: Remove btree_readpage
btrfs: Remove pg_offset from btrfs_get_extent
btrfs: Simplify metadata pages reading
btrfs: Remove btree_get_extent
btrfs: Remove btrfs_get_extent indirection from __do_readpage
btrfs: Remove mirror_num argument from extent_read_full_page
btrfs: Promote extent_read_full_page to btrfs_readpage
btrfs: Sink mirror_num argument in extent_read_full_page
btrfs: Sink read_flags argument into extent_read_full_page
btrfs: Sink mirror_num argument in __do_readpage
fs/btrfs/ctree.h | 3 +-
fs/btrfs/disk-io.c | 53 --------------------------
fs/btrfs/disk-io.h | 3 --
fs/btrfs/extent_io.c | 74 ++++++++++++------------------------
fs/btrfs/extent_io.h | 10 +++--
fs/btrfs/file.c | 12 +++---
fs/btrfs/inode.c | 37 +++++++++---------
fs/btrfs/ioctl.c | 2 +-
fs/btrfs/tests/inode-tests.c | 42 ++++++++++----------
9 files changed, 79 insertions(+), 157 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 01/10] btrfs: Remove btree_readpage
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-09 10:37 ` Johannes Thumshirn
2020-09-09 9:49 ` [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent Nikolay Borisov
` (8 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/disk-io.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 7147237d9bf0..d63498f3c75f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -949,11 +949,6 @@ static int btree_writepages(struct address_space *mapping,
return btree_write_cache_pages(mapping, wbc);
}
-static int btree_readpage(struct file *file, struct page *page)
-{
- return extent_read_full_page(page, btree_get_extent, 0);
-}
-
static int btree_releasepage(struct page *page, gfp_t gfp_flags)
{
if (PageWriteback(page) || PageDirty(page))
@@ -993,7 +988,6 @@ static int btree_set_page_dirty(struct page *page)
}
static const struct address_space_operations btree_aops = {
- .readpage = btree_readpage,
.writepages = btree_writepages,
.releasepage = btree_releasepage,
.invalidatepage = btree_invalidatepage,
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
2020-09-09 9:49 ` [PATCH 01/10] btrfs: Remove btree_readpage Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-09 10:40 ` Johannes Thumshirn
` (2 more replies)
2020-09-09 9:49 ` [PATCH 03/10] btrfs: Simplify metadata pages reading Nikolay Borisov
` (7 subsequent siblings)
9 siblings, 3 replies; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
Btrfs doesn't support subpage blocksize io as such pg_offset is always
zero. Remove this argument to cleanup the parameter list.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/ctree.h | 3 +--
fs/btrfs/disk-io.c | 3 +--
fs/btrfs/disk-io.h | 3 +--
fs/btrfs/extent_io.c | 8 ++++---
fs/btrfs/extent_io.h | 4 ++--
fs/btrfs/file.c | 12 +++++------
fs/btrfs/inode.c | 28 +++++++++++-------------
fs/btrfs/ioctl.c | 2 +-
fs/btrfs/tests/inode-tests.c | 42 ++++++++++++++++++------------------
9 files changed, 50 insertions(+), 55 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 98c5f6178efc..7c7afa823f71 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3000,8 +3000,7 @@ struct inode *btrfs_iget_path(struct super_block *s, u64 ino,
struct btrfs_root *root, struct btrfs_path *path);
struct inode *btrfs_iget(struct super_block *s, u64 ino, struct btrfs_root *root);
struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
- struct page *page, size_t pg_offset,
- u64 start, u64 end);
+ struct page *page, u64 start, u64 end);
int btrfs_update_inode(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
struct inode *inode);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d63498f3c75f..c6c9b6b13bf0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -209,8 +209,7 @@ void btrfs_set_buffer_lockdep_class(u64 objectid, struct extent_buffer *eb,
* that covers the entire device
*/
struct extent_map *btree_get_extent(struct btrfs_inode *inode,
- struct page *page, size_t pg_offset,
- u64 start, u64 len)
+ struct page *page, u64 start, u64 len)
{
struct extent_map_tree *em_tree = &inode->extent_tree;
struct extent_map *em;
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 00dc39d47ed3..dbc8c353c86c 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -124,8 +124,7 @@ struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,
int btree_lock_page_hook(struct page *page, void *data,
void (*flush_fn)(void *));
struct extent_map *btree_get_extent(struct btrfs_inode *inode,
- struct page *page, size_t pg_offset,
- u64 start, u64 len);
+ struct page *page, u64 start, u64 len);
int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags);
int __init btrfs_end_io_wq_init(void);
void __cold btrfs_end_io_wq_exit(void);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a1e070ec7ad8..ac92c0ab1402 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3127,7 +3127,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
*em_cached = NULL;
}
- em = get_extent(BTRFS_I(inode), page, pg_offset, start, len);
+ em = get_extent(BTRFS_I(inode), page, start, len);
if (em_cached && !IS_ERR_OR_NULL(em)) {
BUG_ON(*em_cached);
refcount_inc(&em->refs);
@@ -3161,7 +3161,7 @@ static int __do_readpage(struct page *page,
int ret = 0;
int nr = 0;
size_t pg_offset = 0;
- size_t iosize;
+ size_t iosize = 0;
size_t disk_io_size;
size_t blocksize = inode->i_sb->s_blocksize;
unsigned long this_bio_flag = 0;
@@ -3208,6 +3208,8 @@ static int __do_readpage(struct page *page,
cur + iosize - 1, &cached);
break;
}
+ if (pg_offset != 0)
+ trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
em = __get_extent_map(inode, page, pg_offset, cur,
end - cur + 1, get_extent, em_cached);
if (IS_ERR_OR_NULL(em)) {
@@ -3540,7 +3542,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
page_end, 1);
break;
}
- em = btrfs_get_extent(inode, NULL, 0, cur, end - cur + 1);
+ em = btrfs_get_extent(inode, NULL, cur, end - cur + 1);
if (IS_ERR_OR_NULL(em)) {
SetPageError(page);
ret = PTR_ERR_OR_ZERO(em);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 06611947a9f7..41621731a4fe 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -187,8 +187,8 @@ static inline int extent_compress_type(unsigned long bio_flags)
struct extent_map_tree;
typedef struct extent_map *(get_extent_t)(struct btrfs_inode *inode,
- struct page *page, size_t pg_offset,
- u64 start, u64 len);
+ struct page *page, u64 start,
+ u64 len);
int try_release_extent_mapping(struct page *page, gfp_t mask);
int try_release_extent_buffer(struct page *page);
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index af4eab9cbc51..0020c6780035 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -466,7 +466,7 @@ static int btrfs_find_new_delalloc_bytes(struct btrfs_inode *inode,
u64 em_len;
int ret = 0;
- em = btrfs_get_extent(inode, NULL, 0, search_start, search_len);
+ em = btrfs_get_extent(inode, NULL, search_start, search_len);
if (IS_ERR(em))
return PTR_ERR(em);
@@ -2511,7 +2511,7 @@ static int find_first_non_hole(struct inode *inode, u64 *start, u64 *len)
struct extent_map *em;
int ret = 0;
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0,
+ em = btrfs_get_extent(BTRFS_I(inode), NULL,
round_down(*start, fs_info->sectorsize),
round_up(*len, fs_info->sectorsize));
if (IS_ERR(em))
@@ -3113,7 +3113,7 @@ static int btrfs_zero_range_check_range_boundary(struct btrfs_inode *inode,
int ret;
offset = round_down(offset, sectorsize);
- em = btrfs_get_extent(inode, NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(inode, NULL, offset, sectorsize);
if (IS_ERR(em))
return PTR_ERR(em);
@@ -3146,7 +3146,7 @@ static int btrfs_zero_range(struct inode *inode,
inode_dio_wait(inode);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, alloc_start,
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, alloc_start,
alloc_end - alloc_start);
if (IS_ERR(em)) {
ret = PTR_ERR(em);
@@ -3190,7 +3190,7 @@ static int btrfs_zero_range(struct inode *inode,
if (BTRFS_BYTES_TO_BLKS(fs_info, offset) ==
BTRFS_BYTES_TO_BLKS(fs_info, offset + len - 1)) {
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, alloc_start,
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, alloc_start,
sectorsize);
if (IS_ERR(em)) {
ret = PTR_ERR(em);
@@ -3430,7 +3430,7 @@ static long btrfs_fallocate(struct file *file, int mode,
/* First, check if we exceed the qgroup limit */
INIT_LIST_HEAD(&reserve_list);
while (cur_offset < alloc_end) {
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, cur_offset,
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, cur_offset,
alloc_end - cur_offset);
if (IS_ERR(em)) {
ret = PTR_ERR(em);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index cce6f8789a4e..a7b62b93246b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4722,7 +4722,7 @@ int btrfs_cont_expand(struct inode *inode, loff_t oldsize, loff_t size)
block_end - 1, &cached_state);
cur_offset = hole_start;
while (1) {
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, cur_offset,
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, cur_offset,
block_end - cur_offset);
if (IS_ERR(em)) {
err = PTR_ERR(em);
@@ -6530,8 +6530,7 @@ static noinline int uncompress_inline(struct btrfs_path *path,
* Return: ERR_PTR on error, non-NULL extent_map on success.
*/
struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
- struct page *page, size_t pg_offset,
- u64 start, u64 len)
+ struct page *page, u64 start, u64 len)
{
struct btrfs_fs_info *fs_info = inode->root->fs_info;
int ret = 0;
@@ -6678,9 +6677,8 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
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);
+ extent_offset = page_offset(page) - extent_start;
+ copy_size = min_t(u64, PAGE_SIZE, size - extent_offset);
em->start = extent_start + extent_offset;
em->len = ALIGN(copy_size, fs_info->sectorsize);
em->orig_block_len = em->len;
@@ -6691,18 +6689,16 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
if (!PageUptodate(page)) {
if (btrfs_file_extent_compression(leaf, item) !=
BTRFS_COMPRESS_NONE) {
- ret = uncompress_inline(path, page, pg_offset,
+ ret = uncompress_inline(path, page, 0,
extent_offset, item);
if (ret)
goto out;
} else {
map = kmap(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, ptr, copy_size);
+ if (copy_size < PAGE_SIZE) {
+ memset(map + copy_size, 0,
+ PAGE_SIZE - copy_size);
}
kunmap(page);
}
@@ -6754,7 +6750,7 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
u64 delalloc_end;
int err = 0;
- em = btrfs_get_extent(inode, NULL, 0, start, len);
+ em = btrfs_get_extent(inode, NULL, start, len);
if (IS_ERR(em))
return em;
/*
@@ -7397,7 +7393,7 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
goto err;
}
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, start, len);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, start, len);
if (IS_ERR(em)) {
ret = PTR_ERR(em);
goto unlock_err;
@@ -10044,7 +10040,7 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
struct btrfs_block_group *bg;
u64 len = isize - start;
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, start, len);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, start, len);
if (IS_ERR(em)) {
ret = PTR_ERR(em);
goto out;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b31949df7bfc..31ebbe918156 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1162,7 +1162,7 @@ static struct extent_map *defrag_lookup_extent(struct inode *inode, u64 start)
/* get the big lock and read metadata off disk */
lock_extent_bits(io_tree, start, end, &cached);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, start, len);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, start, len);
unlock_extent_cached(io_tree, start, end, &cached);
if (IS_ERR(em))
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index 894a63a92236..6bcb392e7367 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -263,7 +263,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
/* First with no extents */
BTRFS_I(inode)->root = root;
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, 0, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, sectorsize);
if (IS_ERR(em)) {
em = NULL;
test_err("got an error when we shouldn't have");
@@ -283,7 +283,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
*/
setup_file_extents(root, sectorsize);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, 0, (u64)-1);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, (u64)-1);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -305,7 +305,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -333,7 +333,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -356,7 +356,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
free_extent_map(em);
/* Regular extent */
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -384,7 +384,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
free_extent_map(em);
/* The next 3 are split extents */
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -413,7 +413,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -435,7 +435,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -469,7 +469,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
free_extent_map(em);
/* Prealloc extent */
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -498,7 +498,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
free_extent_map(em);
/* The next 3 are a half written prealloc extent */
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -528,7 +528,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -561,7 +561,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -596,7 +596,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
free_extent_map(em);
/* Now for the compressed extent */
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -630,7 +630,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
free_extent_map(em);
/* Split compressed extent */
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -665,7 +665,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -692,7 +692,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -727,7 +727,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
free_extent_map(em);
/* A hole between regular extents but no hole extent */
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset + 6, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset + 6, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -754,7 +754,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, SZ_4M);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, SZ_4M);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -787,7 +787,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
offset = em->start + em->len;
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -871,7 +871,7 @@ static int test_hole_first(u32 sectorsize, u32 nodesize)
insert_inode_item_key(root);
insert_extent(root, sectorsize, sectorsize, sectorsize, 0, sectorsize,
sectorsize, BTRFS_FILE_EXTENT_REG, 0, 1);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, 0, 2 * sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, 2 * sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
@@ -893,7 +893,7 @@ static int test_hole_first(u32 sectorsize, u32 nodesize)
}
free_extent_map(em);
- em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, sectorsize, 2 * sectorsize);
+ em = btrfs_get_extent(BTRFS_I(inode), NULL, sectorsize, 2 * sectorsize);
if (IS_ERR(em)) {
test_err("got an error when we shouldn't have");
goto out;
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 03/10] btrfs: Simplify metadata pages reading
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
2020-09-09 9:49 ` [PATCH 01/10] btrfs: Remove btree_readpage Nikolay Borisov
2020-09-09 9:49 ` [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-09 11:20 ` Qu Wenruo
2020-09-10 14:56 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 04/10] btrfs: Remove btree_get_extent Nikolay Borisov
` (6 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
Metadata pages currently use __do_readpage to read metadata pages,
unfortunately this function is also used to deal with ordinary data
pages. This makes the metadata pages reading code to go through multiple
hoops in order to adhere to __do_readpage invariants. Most of these are
necessary for data pages which could be compressed. For metadata it's
enough to simply build a bio and submit it.
To this effect simply call submit_extent_page directly from
read_extent_buffer_pages which is the only callpath used to populate
extent_buffers with data. This in turn enables further cleanups.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/extent_io.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index ac92c0ab1402..1789a7931312 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
}
ClearPageError(page);
- err = __extent_read_full_page(page,
- btree_get_extent, &bio,
- mirror_num, &bio_flags,
- REQ_META);
+ err = submit_extent_page(REQ_OP_READ | REQ_META, NULL,
+ page, page_offset(page), PAGE_SIZE, 0,
+ &bio, end_bio_extent_readpage,
+ mirror_num, 0, 0, false);
if (err) {
ret = err;
- /*
- * We use &bio in above __extent_read_full_page,
- * so we ensure that if it returns error, the
- * current page fails to add itself to bio and
- * it's been unlocked.
- *
- * We must dec io_pages by ourselves.
- */
+ SetPageError(page);
+ unlock_page(page);
atomic_dec(&eb->io_pages);
}
} else {
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 04/10] btrfs: Remove btree_get_extent
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
` (2 preceding siblings ...)
2020-09-09 9:49 ` [PATCH 03/10] btrfs: Simplify metadata pages reading Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-10 14:57 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage Nikolay Borisov
` (5 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
The sole purpose of this function was to satisfy the requirements of
__do_readpage. Since that function is no longer used to read metadata
pages the need to keep btree_get_extent around has also disappeared.
Simply remove it.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/disk-io.c | 46 ----------------------------------------------
fs/btrfs/disk-io.h | 2 --
2 files changed, 48 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c6c9b6b13bf0..2a5aadcf6b54 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -204,52 +204,6 @@ void btrfs_set_buffer_lockdep_class(u64 objectid, struct extent_buffer *eb,
#endif
-/*
- * extents on the btree inode are pretty simple, there's one extent
- * that covers the entire device
- */
-struct extent_map *btree_get_extent(struct btrfs_inode *inode,
- struct page *page, u64 start, u64 len)
-{
- struct extent_map_tree *em_tree = &inode->extent_tree;
- struct extent_map *em;
- int ret;
-
- read_lock(&em_tree->lock);
- em = lookup_extent_mapping(em_tree, start, len);
- if (em) {
- read_unlock(&em_tree->lock);
- goto out;
- }
- read_unlock(&em_tree->lock);
-
- em = alloc_extent_map();
- if (!em) {
- em = ERR_PTR(-ENOMEM);
- goto out;
- }
- em->start = 0;
- em->len = (u64)-1;
- em->block_len = (u64)-1;
- em->block_start = 0;
-
- write_lock(&em_tree->lock);
- ret = add_extent_mapping(em_tree, em, 0);
- if (ret == -EEXIST) {
- free_extent_map(em);
- em = lookup_extent_mapping(em_tree, start, len);
- if (!em)
- em = ERR_PTR(-EIO);
- } else if (ret) {
- free_extent_map(em);
- em = ERR_PTR(ret);
- }
- write_unlock(&em_tree->lock);
-
-out:
- return em;
-}
-
/*
* Compute the csum of a btree block and store the result to provided buffer.
*/
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index dbc8c353c86c..89b6a709a184 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -123,8 +123,6 @@ struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,
u64 objectid);
int btree_lock_page_hook(struct page *page, void *data,
void (*flush_fn)(void *));
-struct extent_map *btree_get_extent(struct btrfs_inode *inode,
- struct page *page, u64 start, u64 len);
int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags);
int __init btrfs_end_io_wq_init(void);
void __cold btrfs_end_io_wq_exit(void);
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
` (3 preceding siblings ...)
2020-09-09 9:49 ` [PATCH 04/10] btrfs: Remove btree_get_extent Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-09 11:24 ` Qu Wenruo
2020-09-09 9:49 ` [PATCH 06/10] btrfs: Remove mirror_num argument from extent_read_full_page Nikolay Borisov
` (4 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
Now that this function is only responsible for reading data pages it's
no longer necessary to pass get_extent_t parameter across several
layers of functions. This patch removes this parameter from multiple
functions: __get_extent_map/__do_readpage/__extent_read_full_page/
extent_read_full_page and simply calls btrfs_get_extent directly in
__get_extent_map.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/extent_io.c | 31 ++++++++++++-------------------
fs/btrfs/extent_io.h | 3 +--
fs/btrfs/inode.c | 2 +-
3 files changed, 14 insertions(+), 22 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1789a7931312..4c04d3655538 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3110,8 +3110,7 @@ void set_page_extent_mapped(struct page *page)
static struct extent_map *
__get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
- u64 start, u64 len, get_extent_t *get_extent,
- struct extent_map **em_cached)
+ u64 start, u64 len, struct extent_map **em_cached)
{
struct extent_map *em;
@@ -3127,7 +3126,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
*em_cached = NULL;
}
- em = get_extent(BTRFS_I(inode), page, start, len);
+ em = btrfs_get_extent(BTRFS_I(inode), page, start, len);
if (em_cached && !IS_ERR_OR_NULL(em)) {
BUG_ON(*em_cached);
refcount_inc(&em->refs);
@@ -3142,9 +3141,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
* XXX JDM: This needs looking at to ensure proper page locking
* return 0 on success, otherwise return error
*/
-static int __do_readpage(struct page *page,
- get_extent_t *get_extent,
- struct extent_map **em_cached,
+static int __do_readpage(struct page *page, struct extent_map **em_cached,
struct bio **bio, int mirror_num,
unsigned long *bio_flags, unsigned int read_flags,
u64 *prev_em_start)
@@ -3211,7 +3208,7 @@ static int __do_readpage(struct page *page,
if (pg_offset != 0)
trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
em = __get_extent_map(inode, page, pg_offset, cur,
- end - cur + 1, get_extent, em_cached);
+ end - cur + 1, em_cached);
if (IS_ERR_OR_NULL(em)) {
SetPageError(page);
unlock_extent(tree, cur, end);
@@ -3364,16 +3361,14 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
for (index = 0; index < nr_pages; index++) {
- __do_readpage(pages[index], btrfs_get_extent, em_cached,
- bio, 0, bio_flags, REQ_RAHEAD, prev_em_start);
+ __do_readpage(pages[index], em_cached, bio, 0, bio_flags,
+ REQ_RAHEAD, prev_em_start);
put_page(pages[index]);
}
}
-static int __extent_read_full_page(struct page *page,
- get_extent_t *get_extent,
- struct bio **bio, int mirror_num,
- unsigned long *bio_flags,
+static int __extent_read_full_page(struct page *page, struct bio **bio,
+ int mirror_num, unsigned long *bio_flags,
unsigned int read_flags)
{
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
@@ -3383,20 +3378,18 @@ static int __extent_read_full_page(struct page *page,
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
- ret = __do_readpage(page, get_extent, NULL, bio, mirror_num,
- bio_flags, read_flags, NULL);
+ ret = __do_readpage(page, NULL, bio, mirror_num, bio_flags, read_flags,
+ NULL);
return ret;
}
-int extent_read_full_page(struct page *page, get_extent_t *get_extent,
- int mirror_num)
+int extent_read_full_page(struct page *page, int mirror_num)
{
struct bio *bio = NULL;
unsigned long bio_flags = 0;
int ret;
- ret = __extent_read_full_page(page, get_extent, &bio, mirror_num,
- &bio_flags, 0);
+ ret = __extent_read_full_page(page, &bio, mirror_num, &bio_flags, 0);
if (bio)
ret = submit_one_bio(bio, mirror_num, bio_flags);
return ret;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 41621731a4fe..57786feffdbf 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -193,8 +193,7 @@ typedef struct extent_map *(get_extent_t)(struct btrfs_inode *inode,
int try_release_extent_mapping(struct page *page, gfp_t mask);
int try_release_extent_buffer(struct page *page);
-int extent_read_full_page(struct page *page, get_extent_t *get_extent,
- int mirror_num);
+int extent_read_full_page(struct page *page, int mirror_num);
int extent_write_full_page(struct page *page, struct writeback_control *wbc);
int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
int mode);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a7b62b93246b..c8d1d935c8c7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8036,7 +8036,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
int btrfs_readpage(struct file *file, struct page *page)
{
- return extent_read_full_page(page, btrfs_get_extent, 0);
+ return extent_read_full_page(page, 0);
}
static int btrfs_writepage(struct page *page, struct writeback_control *wbc)
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 06/10] btrfs: Remove mirror_num argument from extent_read_full_page
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
` (4 preceding siblings ...)
2020-09-09 9:49 ` [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-10 14:58 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 07/10] btrfs: Promote extent_read_full_page to btrfs_readpage Nikolay Borisov
` (3 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
It's called only from btrfs_readpage which always passes 0 so just sink
the argument into extent_read_full_page.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/extent_io.c | 6 +++---
fs/btrfs/extent_io.h | 2 +-
fs/btrfs/inode.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4c04d3655538..d2b71e97218c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3383,15 +3383,15 @@ static int __extent_read_full_page(struct page *page, struct bio **bio,
return ret;
}
-int extent_read_full_page(struct page *page, int mirror_num)
+int extent_read_full_page(struct page *page)
{
struct bio *bio = NULL;
unsigned long bio_flags = 0;
int ret;
- ret = __extent_read_full_page(page, &bio, mirror_num, &bio_flags, 0);
+ ret = __extent_read_full_page(page, &bio, 0, &bio_flags, 0);
if (bio)
- ret = submit_one_bio(bio, mirror_num, bio_flags);
+ ret = submit_one_bio(bio, 0, bio_flags);
return ret;
}
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 57786feffdbf..96bc9f0c2981 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -193,7 +193,7 @@ typedef struct extent_map *(get_extent_t)(struct btrfs_inode *inode,
int try_release_extent_mapping(struct page *page, gfp_t mask);
int try_release_extent_buffer(struct page *page);
-int extent_read_full_page(struct page *page, int mirror_num);
+int extent_read_full_page(struct page *page);
int extent_write_full_page(struct page *page, struct writeback_control *wbc);
int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
int mode);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c8d1d935c8c7..3fd73573bbad 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8036,7 +8036,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
int btrfs_readpage(struct file *file, struct page *page)
{
- return extent_read_full_page(page, 0);
+ return extent_read_full_page(page);
}
static int btrfs_writepage(struct page *page, struct writeback_control *wbc)
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 07/10] btrfs: Promote extent_read_full_page to btrfs_readpage
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
` (5 preceding siblings ...)
2020-09-09 9:49 ` [PATCH 06/10] btrfs: Remove mirror_num argument from extent_read_full_page Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-10 15:01 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 08/10] btrfs: Sink mirror_num argument in extent_read_full_page Nikolay Borisov
` (2 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
Now that btrfs_readpage is the only caller of extent_read_full_page the
latter can be opencoded in the former. Use the occassion to rename
__extent_read_full_page to extent_read_full_page. To facillitate this
change submit_one_bio has to be exported as well.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/extent_io.c | 21 ++++-----------------
fs/btrfs/extent_io.h | 5 ++++-
fs/btrfs/inode.c | 9 ++++++++-
3 files changed, 16 insertions(+), 19 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d2b71e97218c..fa5717fe27f0 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -160,8 +160,8 @@ static int add_extent_changeset(struct extent_state *state, unsigned bits,
return ret;
}
-static int __must_check submit_one_bio(struct bio *bio, int mirror_num,
- unsigned long bio_flags)
+int __must_check submit_one_bio(struct bio *bio, int mirror_num,
+ unsigned long bio_flags)
{
blk_status_t ret = 0;
struct extent_io_tree *tree = bio->bi_private;
@@ -3367,9 +3367,8 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
}
}
-static int __extent_read_full_page(struct page *page, struct bio **bio,
- int mirror_num, unsigned long *bio_flags,
- unsigned int read_flags)
+int extent_read_full_page(struct page *page, struct bio **bio, int mirror_num,
+ unsigned long *bio_flags, unsigned int read_flags)
{
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
u64 start = page_offset(page);
@@ -3383,18 +3382,6 @@ static int __extent_read_full_page(struct page *page, struct bio **bio,
return ret;
}
-int extent_read_full_page(struct page *page)
-{
- struct bio *bio = NULL;
- unsigned long bio_flags = 0;
- int ret;
-
- ret = __extent_read_full_page(page, &bio, 0, &bio_flags, 0);
- if (bio)
- ret = submit_one_bio(bio, 0, bio_flags);
- return ret;
-}
-
static void update_nr_written(struct writeback_control *wbc,
unsigned long nr_written)
{
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 96bc9f0c2981..ff1895994994 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -193,7 +193,10 @@ typedef struct extent_map *(get_extent_t)(struct btrfs_inode *inode,
int try_release_extent_mapping(struct page *page, gfp_t mask);
int try_release_extent_buffer(struct page *page);
-int extent_read_full_page(struct page *page);
+int __must_check submit_one_bio(struct bio *bio, int mirror_num,
+ unsigned long bio_flags);
+int extent_read_full_page(struct page *page, struct bio **bio, int mirror_num,
+ unsigned long *bio_flags, unsigned int read_flags);
int extent_write_full_page(struct page *page, struct writeback_control *wbc);
int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
int mode);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3fd73573bbad..dcc8b5de1b9c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8036,7 +8036,14 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
int btrfs_readpage(struct file *file, struct page *page)
{
- return extent_read_full_page(page);
+ struct bio *bio = NULL;
+ unsigned long bio_flags = 0;
+ int ret;
+
+ ret = extent_read_full_page(page, &bio, 0, &bio_flags, 0);
+ if (bio)
+ ret = submit_one_bio(bio, 0, bio_flags);
+ return ret;
}
static int btrfs_writepage(struct page *page, struct writeback_control *wbc)
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 08/10] btrfs: Sink mirror_num argument in extent_read_full_page
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
` (6 preceding siblings ...)
2020-09-09 9:49 ` [PATCH 07/10] btrfs: Promote extent_read_full_page to btrfs_readpage Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-10 15:02 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 09/10] btrfs: Sink read_flags argument into extent_read_full_page Nikolay Borisov
2020-09-09 9:49 ` [PATCH 10/10] btrfs: Sink mirror_num argument in __do_readpage Nikolay Borisov
9 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
It's always set to 0 from the sole caller - btrfs_readpage.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/extent_io.c | 5 ++---
fs/btrfs/extent_io.h | 2 +-
fs/btrfs/inode.c | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fa5717fe27f0..dbbb9a35c1d9 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3367,7 +3367,7 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
}
}
-int extent_read_full_page(struct page *page, struct bio **bio, int mirror_num,
+int extent_read_full_page(struct page *page, struct bio **bio,
unsigned long *bio_flags, unsigned int read_flags)
{
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
@@ -3377,8 +3377,7 @@ int extent_read_full_page(struct page *page, struct bio **bio, int mirror_num,
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
- ret = __do_readpage(page, NULL, bio, mirror_num, bio_flags, read_flags,
- NULL);
+ ret = __do_readpage(page, NULL, bio, 0, bio_flags, read_flags, NULL);
return ret;
}
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index ff1895994994..a42fc8322b0f 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -195,7 +195,7 @@ int try_release_extent_buffer(struct page *page);
int __must_check submit_one_bio(struct bio *bio, int mirror_num,
unsigned long bio_flags);
-int extent_read_full_page(struct page *page, struct bio **bio, int mirror_num,
+int extent_read_full_page(struct page *page, struct bio **bio,
unsigned long *bio_flags, unsigned int read_flags);
int extent_write_full_page(struct page *page, struct writeback_control *wbc);
int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index dcc8b5de1b9c..8c1f99d7a3c2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8040,7 +8040,7 @@ int btrfs_readpage(struct file *file, struct page *page)
unsigned long bio_flags = 0;
int ret;
- ret = extent_read_full_page(page, &bio, 0, &bio_flags, 0);
+ ret = extent_read_full_page(page, &bio, &bio_flags, 0);
if (bio)
ret = submit_one_bio(bio, 0, bio_flags);
return ret;
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 09/10] btrfs: Sink read_flags argument into extent_read_full_page
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
` (7 preceding siblings ...)
2020-09-09 9:49 ` [PATCH 08/10] btrfs: Sink mirror_num argument in extent_read_full_page Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-10 15:03 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 10/10] btrfs: Sink mirror_num argument in __do_readpage Nikolay Borisov
9 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
It's always set to 0 by its sole caller - btrfs_readpage. Simply remove
it.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/extent_io.c | 4 ++--
fs/btrfs/extent_io.h | 2 +-
fs/btrfs/inode.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index dbbb9a35c1d9..bb30e34b4f53 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3368,7 +3368,7 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
}
int extent_read_full_page(struct page *page, struct bio **bio,
- unsigned long *bio_flags, unsigned int read_flags)
+ unsigned long *bio_flags)
{
struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
u64 start = page_offset(page);
@@ -3377,7 +3377,7 @@ int extent_read_full_page(struct page *page, struct bio **bio,
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
- ret = __do_readpage(page, NULL, bio, 0, bio_flags, read_flags, NULL);
+ ret = __do_readpage(page, NULL, bio, 0, bio_flags, 0, NULL);
return ret;
}
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index a42fc8322b0f..98bfc3eee930 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -196,7 +196,7 @@ int try_release_extent_buffer(struct page *page);
int __must_check submit_one_bio(struct bio *bio, int mirror_num,
unsigned long bio_flags);
int extent_read_full_page(struct page *page, struct bio **bio,
- unsigned long *bio_flags, unsigned int read_flags);
+ unsigned long *bio_flags);
int extent_write_full_page(struct page *page, struct writeback_control *wbc);
int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
int mode);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8c1f99d7a3c2..fcc7b4139831 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8040,7 +8040,7 @@ int btrfs_readpage(struct file *file, struct page *page)
unsigned long bio_flags = 0;
int ret;
- ret = extent_read_full_page(page, &bio, &bio_flags, 0);
+ ret = extent_read_full_page(page, &bio, &bio_flags);
if (bio)
ret = submit_one_bio(bio, 0, bio_flags);
return ret;
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 10/10] btrfs: Sink mirror_num argument in __do_readpage
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
` (8 preceding siblings ...)
2020-09-09 9:49 ` [PATCH 09/10] btrfs: Sink read_flags argument into extent_read_full_page Nikolay Borisov
@ 2020-09-09 9:49 ` Nikolay Borisov
2020-09-10 15:04 ` Josef Bacik
9 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 9:49 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
It's always set to 0 by the 2 callers so move it inside __do_readpage.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/extent_io.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index bb30e34b4f53..f87c19668e0d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3142,9 +3142,8 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
* return 0 on success, otherwise return error
*/
static int __do_readpage(struct page *page, struct extent_map **em_cached,
- struct bio **bio, int mirror_num,
- unsigned long *bio_flags, unsigned int read_flags,
- u64 *prev_em_start)
+ struct bio **bio, unsigned long *bio_flags,
+ unsigned int read_flags, u64 *prev_em_start)
{
struct inode *inode = page->mapping->host;
u64 start = page_offset(page);
@@ -3324,7 +3323,7 @@ static int __do_readpage(struct page *page, struct extent_map **em_cached,
ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
page, offset, disk_io_size,
pg_offset, bio,
- end_bio_extent_readpage, mirror_num,
+ end_bio_extent_readpage, 0,
*bio_flags,
this_bio_flag,
force_bio_submit);
@@ -3361,7 +3360,7 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
for (index = 0; index < nr_pages; index++) {
- __do_readpage(pages[index], em_cached, bio, 0, bio_flags,
+ __do_readpage(pages[index], em_cached, bio, bio_flags,
REQ_RAHEAD, prev_em_start);
put_page(pages[index]);
}
@@ -3377,7 +3376,7 @@ int extent_read_full_page(struct page *page, struct bio **bio,
btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
- ret = __do_readpage(page, NULL, bio, 0, bio_flags, 0, NULL);
+ ret = __do_readpage(page, NULL, bio, bio_flags, 0, NULL);
return ret;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 01/10] btrfs: Remove btree_readpage
2020-09-09 9:49 ` [PATCH 01/10] btrfs: Remove btree_readpage Nikolay Borisov
@ 2020-09-09 10:37 ` Johannes Thumshirn
2020-09-09 11:13 ` Qu Wenruo
0 siblings, 1 reply; 28+ messages in thread
From: Johannes Thumshirn @ 2020-09-09 10:37 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 09/09/2020 11:49, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/disk-io.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 7147237d9bf0..d63498f3c75f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -949,11 +949,6 @@ static int btree_writepages(struct address_space *mapping,
> return btree_write_cache_pages(mapping, wbc);
> }
>
> -static int btree_readpage(struct file *file, struct page *page)
> -{
> - return extent_read_full_page(page, btree_get_extent, 0);
> -}
> -
> static int btree_releasepage(struct page *page, gfp_t gfp_flags)
> {
> if (PageWriteback(page) || PageDirty(page))
> @@ -993,7 +988,6 @@ static int btree_set_page_dirty(struct page *page)
> }
>
> static const struct address_space_operations btree_aops = {
> - .readpage = btree_readpage,
> .writepages = btree_writepages,
> .releasepage = btree_releasepage,
> .invalidatepage = btree_invalidatepage,
>
Maybe you could add a little explanation why we can remove btree_readpage.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent
2020-09-09 9:49 ` [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent Nikolay Borisov
@ 2020-09-09 10:40 ` Johannes Thumshirn
2020-09-09 11:15 ` Qu Wenruo
2020-09-09 20:46 ` kernel test robot
2 siblings, 0 replies; 28+ messages in thread
From: Johannes Thumshirn @ 2020-09-09 10:40 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 09/09/2020 11:50, Nikolay Borisov wrote:
> + if (pg_offset != 0)
> + trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
That looks like a leftover from development
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 01/10] btrfs: Remove btree_readpage
2020-09-09 10:37 ` Johannes Thumshirn
@ 2020-09-09 11:13 ` Qu Wenruo
0 siblings, 0 replies; 28+ messages in thread
From: Qu Wenruo @ 2020-09-09 11:13 UTC (permalink / raw)
To: Johannes Thumshirn, Nikolay Borisov, linux-btrfs
On 2020/9/9 下午6:37, Johannes Thumshirn wrote:
> On 09/09/2020 11:49, Nikolay Borisov wrote:
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> fs/btrfs/disk-io.c | 6 ------
>> 1 file changed, 6 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 7147237d9bf0..d63498f3c75f 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -949,11 +949,6 @@ static int btree_writepages(struct address_space *mapping,
>> return btree_write_cache_pages(mapping, wbc);
>> }
>>
>> -static int btree_readpage(struct file *file, struct page *page)
>> -{
>> - return extent_read_full_page(page, btree_get_extent, 0);
>> -}
>> -
>> static int btree_releasepage(struct page *page, gfp_t gfp_flags)
>> {
>> if (PageWriteback(page) || PageDirty(page))
>> @@ -993,7 +988,6 @@ static int btree_set_page_dirty(struct page *page)
>> }
>>
>> static const struct address_space_operations btree_aops = {
>> - .readpage = btree_readpage,
>> .writepages = btree_writepages,
>> .releasepage = btree_releasepage,
>> .invalidatepage = btree_invalidatepage,
>>
>
> Maybe you could add a little explanation why we can remove btree_readpage.
>
Same idea here.
The main concern is, wouldn't the exposed readpage callback being used
by some cache or whatever from the VFS layer?
Thanks,
Qu
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent
2020-09-09 9:49 ` [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent Nikolay Borisov
2020-09-09 10:40 ` Johannes Thumshirn
@ 2020-09-09 11:15 ` Qu Wenruo
2020-09-09 20:46 ` kernel test robot
2 siblings, 0 replies; 28+ messages in thread
From: Qu Wenruo @ 2020-09-09 11:15 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 2020/9/9 下午5:49, Nikolay Borisov wrote:
> Btrfs doesn't support subpage blocksize io as such pg_offset is always
> zero. Remove this argument to cleanup the parameter list.
Oh, I prefer not to remove it, as the subpage support is not that far away.
Especially data subpage read just needs a few very small patches to
enable...
Thanks,
Qu
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/ctree.h | 3 +--
> fs/btrfs/disk-io.c | 3 +--
> fs/btrfs/disk-io.h | 3 +--
> fs/btrfs/extent_io.c | 8 ++++---
> fs/btrfs/extent_io.h | 4 ++--
> fs/btrfs/file.c | 12 +++++------
> fs/btrfs/inode.c | 28 +++++++++++-------------
> fs/btrfs/ioctl.c | 2 +-
> fs/btrfs/tests/inode-tests.c | 42 ++++++++++++++++++------------------
> 9 files changed, 50 insertions(+), 55 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 98c5f6178efc..7c7afa823f71 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3000,8 +3000,7 @@ struct inode *btrfs_iget_path(struct super_block *s, u64 ino,
> struct btrfs_root *root, struct btrfs_path *path);
> struct inode *btrfs_iget(struct super_block *s, u64 ino, struct btrfs_root *root);
> struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
> - struct page *page, size_t pg_offset,
> - u64 start, u64 end);
> + struct page *page, u64 start, u64 end);
> int btrfs_update_inode(struct btrfs_trans_handle *trans,
> struct btrfs_root *root,
> struct inode *inode);
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index d63498f3c75f..c6c9b6b13bf0 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -209,8 +209,7 @@ void btrfs_set_buffer_lockdep_class(u64 objectid, struct extent_buffer *eb,
> * that covers the entire device
> */
> struct extent_map *btree_get_extent(struct btrfs_inode *inode,
> - struct page *page, size_t pg_offset,
> - u64 start, u64 len)
> + struct page *page, u64 start, u64 len)
> {
> struct extent_map_tree *em_tree = &inode->extent_tree;
> struct extent_map *em;
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 00dc39d47ed3..dbc8c353c86c 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -124,8 +124,7 @@ struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans,
> int btree_lock_page_hook(struct page *page, void *data,
> void (*flush_fn)(void *));
> struct extent_map *btree_get_extent(struct btrfs_inode *inode,
> - struct page *page, size_t pg_offset,
> - u64 start, u64 len);
> + struct page *page, u64 start, u64 len);
> int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags);
> int __init btrfs_end_io_wq_init(void);
> void __cold btrfs_end_io_wq_exit(void);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index a1e070ec7ad8..ac92c0ab1402 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3127,7 +3127,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
> *em_cached = NULL;
> }
>
> - em = get_extent(BTRFS_I(inode), page, pg_offset, start, len);
> + em = get_extent(BTRFS_I(inode), page, start, len);
> if (em_cached && !IS_ERR_OR_NULL(em)) {
> BUG_ON(*em_cached);
> refcount_inc(&em->refs);
> @@ -3161,7 +3161,7 @@ static int __do_readpage(struct page *page,
> int ret = 0;
> int nr = 0;
> size_t pg_offset = 0;
> - size_t iosize;
> + size_t iosize = 0;
> size_t disk_io_size;
> size_t blocksize = inode->i_sb->s_blocksize;
> unsigned long this_bio_flag = 0;
> @@ -3208,6 +3208,8 @@ static int __do_readpage(struct page *page,
> cur + iosize - 1, &cached);
> break;
> }
> + if (pg_offset != 0)
> + trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
> em = __get_extent_map(inode, page, pg_offset, cur,
> end - cur + 1, get_extent, em_cached);
> if (IS_ERR_OR_NULL(em)) {
> @@ -3540,7 +3542,7 @@ static noinline_for_stack int __extent_writepage_io(struct btrfs_inode *inode,
> page_end, 1);
> break;
> }
> - em = btrfs_get_extent(inode, NULL, 0, cur, end - cur + 1);
> + em = btrfs_get_extent(inode, NULL, cur, end - cur + 1);
> if (IS_ERR_OR_NULL(em)) {
> SetPageError(page);
> ret = PTR_ERR_OR_ZERO(em);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 06611947a9f7..41621731a4fe 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -187,8 +187,8 @@ static inline int extent_compress_type(unsigned long bio_flags)
> struct extent_map_tree;
>
> typedef struct extent_map *(get_extent_t)(struct btrfs_inode *inode,
> - struct page *page, size_t pg_offset,
> - u64 start, u64 len);
> + struct page *page, u64 start,
> + u64 len);
>
> int try_release_extent_mapping(struct page *page, gfp_t mask);
> int try_release_extent_buffer(struct page *page);
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index af4eab9cbc51..0020c6780035 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -466,7 +466,7 @@ static int btrfs_find_new_delalloc_bytes(struct btrfs_inode *inode,
> u64 em_len;
> int ret = 0;
>
> - em = btrfs_get_extent(inode, NULL, 0, search_start, search_len);
> + em = btrfs_get_extent(inode, NULL, search_start, search_len);
> if (IS_ERR(em))
> return PTR_ERR(em);
>
> @@ -2511,7 +2511,7 @@ static int find_first_non_hole(struct inode *inode, u64 *start, u64 *len)
> struct extent_map *em;
> int ret = 0;
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0,
> + em = btrfs_get_extent(BTRFS_I(inode), NULL,
> round_down(*start, fs_info->sectorsize),
> round_up(*len, fs_info->sectorsize));
> if (IS_ERR(em))
> @@ -3113,7 +3113,7 @@ static int btrfs_zero_range_check_range_boundary(struct btrfs_inode *inode,
> int ret;
>
> offset = round_down(offset, sectorsize);
> - em = btrfs_get_extent(inode, NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(inode, NULL, offset, sectorsize);
> if (IS_ERR(em))
> return PTR_ERR(em);
>
> @@ -3146,7 +3146,7 @@ static int btrfs_zero_range(struct inode *inode,
>
> inode_dio_wait(inode);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, alloc_start,
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, alloc_start,
> alloc_end - alloc_start);
> if (IS_ERR(em)) {
> ret = PTR_ERR(em);
> @@ -3190,7 +3190,7 @@ static int btrfs_zero_range(struct inode *inode,
>
> if (BTRFS_BYTES_TO_BLKS(fs_info, offset) ==
> BTRFS_BYTES_TO_BLKS(fs_info, offset + len - 1)) {
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, alloc_start,
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, alloc_start,
> sectorsize);
> if (IS_ERR(em)) {
> ret = PTR_ERR(em);
> @@ -3430,7 +3430,7 @@ static long btrfs_fallocate(struct file *file, int mode,
> /* First, check if we exceed the qgroup limit */
> INIT_LIST_HEAD(&reserve_list);
> while (cur_offset < alloc_end) {
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, cur_offset,
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, cur_offset,
> alloc_end - cur_offset);
> if (IS_ERR(em)) {
> ret = PTR_ERR(em);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index cce6f8789a4e..a7b62b93246b 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4722,7 +4722,7 @@ int btrfs_cont_expand(struct inode *inode, loff_t oldsize, loff_t size)
> block_end - 1, &cached_state);
> cur_offset = hole_start;
> while (1) {
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, cur_offset,
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, cur_offset,
> block_end - cur_offset);
> if (IS_ERR(em)) {
> err = PTR_ERR(em);
> @@ -6530,8 +6530,7 @@ static noinline int uncompress_inline(struct btrfs_path *path,
> * Return: ERR_PTR on error, non-NULL extent_map on success.
> */
> struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
> - struct page *page, size_t pg_offset,
> - u64 start, u64 len)
> + struct page *page, u64 start, u64 len)
> {
> struct btrfs_fs_info *fs_info = inode->root->fs_info;
> int ret = 0;
> @@ -6678,9 +6677,8 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
> 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);
> + extent_offset = page_offset(page) - extent_start;
> + copy_size = min_t(u64, PAGE_SIZE, size - extent_offset);
> em->start = extent_start + extent_offset;
> em->len = ALIGN(copy_size, fs_info->sectorsize);
> em->orig_block_len = em->len;
> @@ -6691,18 +6689,16 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
> if (!PageUptodate(page)) {
> if (btrfs_file_extent_compression(leaf, item) !=
> BTRFS_COMPRESS_NONE) {
> - ret = uncompress_inline(path, page, pg_offset,
> + ret = uncompress_inline(path, page, 0,
> extent_offset, item);
> if (ret)
> goto out;
> } else {
> map = kmap(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, ptr, copy_size);
> + if (copy_size < PAGE_SIZE) {
> + memset(map + copy_size, 0,
> + PAGE_SIZE - copy_size);
> }
> kunmap(page);
> }
> @@ -6754,7 +6750,7 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
> u64 delalloc_end;
> int err = 0;
>
> - em = btrfs_get_extent(inode, NULL, 0, start, len);
> + em = btrfs_get_extent(inode, NULL, start, len);
> if (IS_ERR(em))
> return em;
> /*
> @@ -7397,7 +7393,7 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
> goto err;
> }
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, start, len);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, start, len);
> if (IS_ERR(em)) {
> ret = PTR_ERR(em);
> goto unlock_err;
> @@ -10044,7 +10040,7 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
> struct btrfs_block_group *bg;
> u64 len = isize - start;
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, start, len);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, start, len);
> if (IS_ERR(em)) {
> ret = PTR_ERR(em);
> goto out;
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index b31949df7bfc..31ebbe918156 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1162,7 +1162,7 @@ static struct extent_map *defrag_lookup_extent(struct inode *inode, u64 start)
>
> /* get the big lock and read metadata off disk */
> lock_extent_bits(io_tree, start, end, &cached);
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, start, len);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, start, len);
> unlock_extent_cached(io_tree, start, end, &cached);
>
> if (IS_ERR(em))
> diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
> index 894a63a92236..6bcb392e7367 100644
> --- a/fs/btrfs/tests/inode-tests.c
> +++ b/fs/btrfs/tests/inode-tests.c
> @@ -263,7 +263,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
>
> /* First with no extents */
> BTRFS_I(inode)->root = root;
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, 0, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, sectorsize);
> if (IS_ERR(em)) {
> em = NULL;
> test_err("got an error when we shouldn't have");
> @@ -283,7 +283,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> */
> setup_file_extents(root, sectorsize);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, 0, (u64)-1);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, (u64)-1);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -305,7 +305,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -333,7 +333,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -356,7 +356,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> free_extent_map(em);
>
> /* Regular extent */
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -384,7 +384,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> free_extent_map(em);
>
> /* The next 3 are split extents */
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -413,7 +413,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -435,7 +435,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -469,7 +469,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> free_extent_map(em);
>
> /* Prealloc extent */
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -498,7 +498,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> free_extent_map(em);
>
> /* The next 3 are a half written prealloc extent */
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -528,7 +528,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -561,7 +561,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -596,7 +596,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> free_extent_map(em);
>
> /* Now for the compressed extent */
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -630,7 +630,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> free_extent_map(em);
>
> /* Split compressed extent */
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -665,7 +665,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -692,7 +692,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -727,7 +727,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> free_extent_map(em);
>
> /* A hole between regular extents but no hole extent */
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset + 6, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset + 6, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -754,7 +754,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, SZ_4M);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, SZ_4M);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -787,7 +787,7 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
> offset = em->start + em->len;
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, offset, sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -871,7 +871,7 @@ static int test_hole_first(u32 sectorsize, u32 nodesize)
> insert_inode_item_key(root);
> insert_extent(root, sectorsize, sectorsize, sectorsize, 0, sectorsize,
> sectorsize, BTRFS_FILE_EXTENT_REG, 0, 1);
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, 0, 2 * sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, 2 * sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
> @@ -893,7 +893,7 @@ static int test_hole_first(u32 sectorsize, u32 nodesize)
> }
> free_extent_map(em);
>
> - em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, sectorsize, 2 * sectorsize);
> + em = btrfs_get_extent(BTRFS_I(inode), NULL, sectorsize, 2 * sectorsize);
> if (IS_ERR(em)) {
> test_err("got an error when we shouldn't have");
> goto out;
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 03/10] btrfs: Simplify metadata pages reading
2020-09-09 9:49 ` [PATCH 03/10] btrfs: Simplify metadata pages reading Nikolay Borisov
@ 2020-09-09 11:20 ` Qu Wenruo
2020-09-14 8:08 ` Nikolay Borisov
2020-09-10 14:56 ` Josef Bacik
1 sibling, 1 reply; 28+ messages in thread
From: Qu Wenruo @ 2020-09-09 11:20 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 2020/9/9 下午5:49, Nikolay Borisov wrote:
> Metadata pages currently use __do_readpage to read metadata pages,
> unfortunately this function is also used to deal with ordinary data
> pages. This makes the metadata pages reading code to go through multiple
> hoops in order to adhere to __do_readpage invariants. Most of these are
> necessary for data pages which could be compressed. For metadata it's
> enough to simply build a bio and submit it.
>
> To this effect simply call submit_extent_page directly from
> read_extent_buffer_pages which is the only callpath used to populate
> extent_buffers with data. This in turn enables further cleanups.
This is awesome!!!
And the code also looks pretty good to me.
Reviewed-by: Qu Wenruo <wqu@suse.com>
Just a note for further enhancement inlined below.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/extent_io.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index ac92c0ab1402..1789a7931312 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
> }
>
> ClearPageError(page);
> - err = __extent_read_full_page(page,
> - btree_get_extent, &bio,
> - mirror_num, &bio_flags,
> - REQ_META);
> + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL,
> + page, page_offset(page), PAGE_SIZE, 0,
It would be better to enhance the comment for submit_extent_page() of
@offset.
It's in fact btrfs logical bytenr.
For metadata, page_offset(page) is also the btrfs logical bytenr so it's
completely fine.
But it can be different for data inodes, so it's better to make it more
clear in the comment.
Thanks,
Qu
> + &bio, end_bio_extent_readpage,
> + mirror_num, 0, 0, false);
> if (err) {
> ret = err;
> - /*
> - * We use &bio in above __extent_read_full_page,
> - * so we ensure that if it returns error, the
> - * current page fails to add itself to bio and
> - * it's been unlocked.
> - *
> - * We must dec io_pages by ourselves.
> - */
> + SetPageError(page);
> + unlock_page(page);
> atomic_dec(&eb->io_pages);
> }
> } else {
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage
2020-09-09 9:49 ` [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage Nikolay Borisov
@ 2020-09-09 11:24 ` Qu Wenruo
2020-09-09 11:56 ` Nikolay Borisov
0 siblings, 1 reply; 28+ messages in thread
From: Qu Wenruo @ 2020-09-09 11:24 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 2020/9/9 下午5:49, Nikolay Borisov wrote:
> Now that this function is only responsible for reading data pages it's
> no longer necessary to pass get_extent_t parameter across several
> layers of functions. This patch removes this parameter from multiple
> functions: __get_extent_map/__do_readpage/__extent_read_full_page/
> extent_read_full_page and simply calls btrfs_get_extent directly in
> __get_extent_map.
Then it would be much nicer to see a patch renaming all these functions
to specifically name as like
get_data_extent_map/do_data_readpage/data_extent_read_full_page.
The current extent/page naming is too generic, not really distinguish
the completely different path between data and metadata.
And maybe split extent_io into meta_io and data_io. <- That may be
overkilled I guess...
Thanks,
Qu
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/extent_io.c | 31 ++++++++++++-------------------
> fs/btrfs/extent_io.h | 3 +--
> fs/btrfs/inode.c | 2 +-
> 3 files changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 1789a7931312..4c04d3655538 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3110,8 +3110,7 @@ void set_page_extent_mapped(struct page *page)
>
> static struct extent_map *
> __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
> - u64 start, u64 len, get_extent_t *get_extent,
> - struct extent_map **em_cached)
> + u64 start, u64 len, struct extent_map **em_cached)
> {
> struct extent_map *em;
>
> @@ -3127,7 +3126,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
> *em_cached = NULL;
> }
>
> - em = get_extent(BTRFS_I(inode), page, start, len);
> + em = btrfs_get_extent(BTRFS_I(inode), page, start, len);
> if (em_cached && !IS_ERR_OR_NULL(em)) {
> BUG_ON(*em_cached);
> refcount_inc(&em->refs);
> @@ -3142,9 +3141,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
> * XXX JDM: This needs looking at to ensure proper page locking
> * return 0 on success, otherwise return error
> */
> -static int __do_readpage(struct page *page,
> - get_extent_t *get_extent,
> - struct extent_map **em_cached,
> +static int __do_readpage(struct page *page, struct extent_map **em_cached,
> struct bio **bio, int mirror_num,
> unsigned long *bio_flags, unsigned int read_flags,
> u64 *prev_em_start)
> @@ -3211,7 +3208,7 @@ static int __do_readpage(struct page *page,
> if (pg_offset != 0)
> trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
> em = __get_extent_map(inode, page, pg_offset, cur,
> - end - cur + 1, get_extent, em_cached);
> + end - cur + 1, em_cached);
> if (IS_ERR_OR_NULL(em)) {
> SetPageError(page);
> unlock_extent(tree, cur, end);
> @@ -3364,16 +3361,14 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
> btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
>
> for (index = 0; index < nr_pages; index++) {
> - __do_readpage(pages[index], btrfs_get_extent, em_cached,
> - bio, 0, bio_flags, REQ_RAHEAD, prev_em_start);
> + __do_readpage(pages[index], em_cached, bio, 0, bio_flags,
> + REQ_RAHEAD, prev_em_start);
> put_page(pages[index]);
> }
> }
>
> -static int __extent_read_full_page(struct page *page,
> - get_extent_t *get_extent,
> - struct bio **bio, int mirror_num,
> - unsigned long *bio_flags,
> +static int __extent_read_full_page(struct page *page, struct bio **bio,
> + int mirror_num, unsigned long *bio_flags,
> unsigned int read_flags)
> {
> struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
> @@ -3383,20 +3378,18 @@ static int __extent_read_full_page(struct page *page,
>
> btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
>
> - ret = __do_readpage(page, get_extent, NULL, bio, mirror_num,
> - bio_flags, read_flags, NULL);
> + ret = __do_readpage(page, NULL, bio, mirror_num, bio_flags, read_flags,
> + NULL);
> return ret;
> }
>
> -int extent_read_full_page(struct page *page, get_extent_t *get_extent,
> - int mirror_num)
> +int extent_read_full_page(struct page *page, int mirror_num)
> {
> struct bio *bio = NULL;
> unsigned long bio_flags = 0;
> int ret;
>
> - ret = __extent_read_full_page(page, get_extent, &bio, mirror_num,
> - &bio_flags, 0);
> + ret = __extent_read_full_page(page, &bio, mirror_num, &bio_flags, 0);
> if (bio)
> ret = submit_one_bio(bio, mirror_num, bio_flags);
> return ret;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 41621731a4fe..57786feffdbf 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -193,8 +193,7 @@ typedef struct extent_map *(get_extent_t)(struct btrfs_inode *inode,
> int try_release_extent_mapping(struct page *page, gfp_t mask);
> int try_release_extent_buffer(struct page *page);
>
> -int extent_read_full_page(struct page *page, get_extent_t *get_extent,
> - int mirror_num);
> +int extent_read_full_page(struct page *page, int mirror_num);
> int extent_write_full_page(struct page *page, struct writeback_control *wbc);
> int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
> int mode);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a7b62b93246b..c8d1d935c8c7 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8036,7 +8036,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>
> int btrfs_readpage(struct file *file, struct page *page)
> {
> - return extent_read_full_page(page, btrfs_get_extent, 0);
> + return extent_read_full_page(page, 0);
> }
>
> static int btrfs_writepage(struct page *page, struct writeback_control *wbc)
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage
2020-09-09 11:24 ` Qu Wenruo
@ 2020-09-09 11:56 ` Nikolay Borisov
0 siblings, 0 replies; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-09 11:56 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
On 9.09.20 г. 14:24 ч., Qu Wenruo wrote:
>
>
> On 2020/9/9 下午5:49, Nikolay Borisov wrote:
>> Now that this function is only responsible for reading data pages it's
>> no longer necessary to pass get_extent_t parameter across several
>> layers of functions. This patch removes this parameter from multiple
>> functions: __get_extent_map/__do_readpage/__extent_read_full_page/
>> extent_read_full_page and simply calls btrfs_get_extent directly in
>> __get_extent_map.
>
> Then it would be much nicer to see a patch renaming all these functions
> to specifically name as like
> get_data_extent_map/do_data_readpage/data_extent_read_full_page.
>
> The current extent/page naming is too generic, not really distinguish
> the completely different path between data and metadata.
>
> And maybe split extent_io into meta_io and data_io. <- That may be
> overkilled I guess...
I will mull over this suggestion in any case it is outside the scope of
the current series.
>
> Thanks,
> Qu
>
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> fs/btrfs/extent_io.c | 31 ++++++++++++-------------------
>> fs/btrfs/extent_io.h | 3 +--
>> fs/btrfs/inode.c | 2 +-
>> 3 files changed, 14 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 1789a7931312..4c04d3655538 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -3110,8 +3110,7 @@ void set_page_extent_mapped(struct page *page)
>>
>> static struct extent_map *
>> __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
>> - u64 start, u64 len, get_extent_t *get_extent,
>> - struct extent_map **em_cached)
>> + u64 start, u64 len, struct extent_map **em_cached)
>> {
>> struct extent_map *em;
>>
>> @@ -3127,7 +3126,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
>> *em_cached = NULL;
>> }
>>
>> - em = get_extent(BTRFS_I(inode), page, start, len);
>> + em = btrfs_get_extent(BTRFS_I(inode), page, start, len);
>> if (em_cached && !IS_ERR_OR_NULL(em)) {
>> BUG_ON(*em_cached);
>> refcount_inc(&em->refs);
>> @@ -3142,9 +3141,7 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
>> * XXX JDM: This needs looking at to ensure proper page locking
>> * return 0 on success, otherwise return error
>> */
>> -static int __do_readpage(struct page *page,
>> - get_extent_t *get_extent,
>> - struct extent_map **em_cached,
>> +static int __do_readpage(struct page *page, struct extent_map **em_cached,
>> struct bio **bio, int mirror_num,
>> unsigned long *bio_flags, unsigned int read_flags,
>> u64 *prev_em_start)
>> @@ -3211,7 +3208,7 @@ static int __do_readpage(struct page *page,
>> if (pg_offset != 0)
>> trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
>> em = __get_extent_map(inode, page, pg_offset, cur,
>> - end - cur + 1, get_extent, em_cached);
>> + end - cur + 1, em_cached);
>> if (IS_ERR_OR_NULL(em)) {
>> SetPageError(page);
>> unlock_extent(tree, cur, end);
>> @@ -3364,16 +3361,14 @@ static inline void contiguous_readpages(struct page *pages[], int nr_pages,
>> btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
>>
>> for (index = 0; index < nr_pages; index++) {
>> - __do_readpage(pages[index], btrfs_get_extent, em_cached,
>> - bio, 0, bio_flags, REQ_RAHEAD, prev_em_start);
>> + __do_readpage(pages[index], em_cached, bio, 0, bio_flags,
>> + REQ_RAHEAD, prev_em_start);
>> put_page(pages[index]);
>> }
>> }
>>
>> -static int __extent_read_full_page(struct page *page,
>> - get_extent_t *get_extent,
>> - struct bio **bio, int mirror_num,
>> - unsigned long *bio_flags,
>> +static int __extent_read_full_page(struct page *page, struct bio **bio,
>> + int mirror_num, unsigned long *bio_flags,
>> unsigned int read_flags)
>> {
>> struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
>> @@ -3383,20 +3378,18 @@ static int __extent_read_full_page(struct page *page,
>>
>> btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
>>
>> - ret = __do_readpage(page, get_extent, NULL, bio, mirror_num,
>> - bio_flags, read_flags, NULL);
>> + ret = __do_readpage(page, NULL, bio, mirror_num, bio_flags, read_flags,
>> + NULL);
>> return ret;
>> }
>>
>> -int extent_read_full_page(struct page *page, get_extent_t *get_extent,
>> - int mirror_num)
>> +int extent_read_full_page(struct page *page, int mirror_num)
>> {
>> struct bio *bio = NULL;
>> unsigned long bio_flags = 0;
>> int ret;
>>
>> - ret = __extent_read_full_page(page, get_extent, &bio, mirror_num,
>> - &bio_flags, 0);
>> + ret = __extent_read_full_page(page, &bio, mirror_num, &bio_flags, 0);
>> if (bio)
>> ret = submit_one_bio(bio, mirror_num, bio_flags);
>> return ret;
>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
>> index 41621731a4fe..57786feffdbf 100644
>> --- a/fs/btrfs/extent_io.h
>> +++ b/fs/btrfs/extent_io.h
>> @@ -193,8 +193,7 @@ typedef struct extent_map *(get_extent_t)(struct btrfs_inode *inode,
>> int try_release_extent_mapping(struct page *page, gfp_t mask);
>> int try_release_extent_buffer(struct page *page);
>>
>> -int extent_read_full_page(struct page *page, get_extent_t *get_extent,
>> - int mirror_num);
>> +int extent_read_full_page(struct page *page, int mirror_num);
>> int extent_write_full_page(struct page *page, struct writeback_control *wbc);
>> int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
>> int mode);
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index a7b62b93246b..c8d1d935c8c7 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -8036,7 +8036,7 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>>
>> int btrfs_readpage(struct file *file, struct page *page)
>> {
>> - return extent_read_full_page(page, btrfs_get_extent, 0);
>> + return extent_read_full_page(page, 0);
>> }
>>
>> static int btrfs_writepage(struct page *page, struct writeback_control *wbc)
>>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent
2020-09-09 9:49 ` [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent Nikolay Borisov
2020-09-09 10:40 ` Johannes Thumshirn
2020-09-09 11:15 ` Qu Wenruo
@ 2020-09-09 20:46 ` kernel test robot
2 siblings, 0 replies; 28+ messages in thread
From: kernel test robot @ 2020-09-09 20:46 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 11973 bytes --]
Hi Nikolay,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20200909]
[cannot apply to btrfs/next v5.9-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Cleanup-metadata-page-reading-path/20200909-175201
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: nds32-randconfig-r026-20200909 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
fs/btrfs/extent_io.c: In function '__do_readpage':
>> fs/btrfs/extent_io.c:3212:31: warning: format '%lu' expects argument of type 'long unsigned int', but argument 2 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
3212 | trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
| ~~^ ~~~~~~~~~
| | |
| long unsigned int size_t {aka unsigned int}
| %u
fs/btrfs/extent_io.c:3212:43: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
3212 | trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
| ~~^ ~~~~~~
| | |
| | size_t {aka unsigned int}
| long unsigned int
| %u
# https://github.com/0day-ci/linux/commit/b8861d64c218aa7a3e62c322e7cd99544a1a7cec
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Nikolay-Borisov/Cleanup-metadata-page-reading-path/20200909-175201
git checkout b8861d64c218aa7a3e62c322e7cd99544a1a7cec
vim +3212 fs/btrfs/extent_io.c
3110
3111 static struct extent_map *
3112 __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
3113 u64 start, u64 len, get_extent_t *get_extent,
3114 struct extent_map **em_cached)
3115 {
3116 struct extent_map *em;
3117
3118 if (em_cached && *em_cached) {
3119 em = *em_cached;
3120 if (extent_map_in_tree(em) && start >= em->start &&
3121 start < extent_map_end(em)) {
3122 refcount_inc(&em->refs);
3123 return em;
3124 }
3125
3126 free_extent_map(em);
3127 *em_cached = NULL;
3128 }
3129
3130 em = get_extent(BTRFS_I(inode), page, start, len);
3131 if (em_cached && !IS_ERR_OR_NULL(em)) {
3132 BUG_ON(*em_cached);
3133 refcount_inc(&em->refs);
3134 *em_cached = em;
3135 }
3136 return em;
3137 }
3138 /*
3139 * basic readpage implementation. Locked extent state structs are inserted
3140 * into the tree that are removed when the IO is done (by the end_io
3141 * handlers)
3142 * XXX JDM: This needs looking at to ensure proper page locking
3143 * return 0 on success, otherwise return error
3144 */
3145 static int __do_readpage(struct page *page,
3146 get_extent_t *get_extent,
3147 struct extent_map **em_cached,
3148 struct bio **bio, int mirror_num,
3149 unsigned long *bio_flags, unsigned int read_flags,
3150 u64 *prev_em_start)
3151 {
3152 struct inode *inode = page->mapping->host;
3153 u64 start = page_offset(page);
3154 const u64 end = start + PAGE_SIZE - 1;
3155 u64 cur = start;
3156 u64 extent_offset;
3157 u64 last_byte = i_size_read(inode);
3158 u64 block_start;
3159 u64 cur_end;
3160 struct extent_map *em;
3161 int ret = 0;
3162 int nr = 0;
3163 size_t pg_offset = 0;
3164 size_t iosize = 0;
3165 size_t disk_io_size;
3166 size_t blocksize = inode->i_sb->s_blocksize;
3167 unsigned long this_bio_flag = 0;
3168 struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree;
3169
3170 set_page_extent_mapped(page);
3171
3172 if (!PageUptodate(page)) {
3173 if (cleancache_get_page(page) == 0) {
3174 BUG_ON(blocksize != PAGE_SIZE);
3175 unlock_extent(tree, start, end);
3176 goto out;
3177 }
3178 }
3179
3180 if (page->index == last_byte >> PAGE_SHIFT) {
3181 char *userpage;
3182 size_t zero_offset = offset_in_page(last_byte);
3183
3184 if (zero_offset) {
3185 iosize = PAGE_SIZE - zero_offset;
3186 userpage = kmap_atomic(page);
3187 memset(userpage + zero_offset, 0, iosize);
3188 flush_dcache_page(page);
3189 kunmap_atomic(userpage);
3190 }
3191 }
3192 while (cur <= end) {
3193 bool force_bio_submit = false;
3194 u64 offset;
3195
3196 if (cur >= last_byte) {
3197 char *userpage;
3198 struct extent_state *cached = NULL;
3199
3200 iosize = PAGE_SIZE - pg_offset;
3201 userpage = kmap_atomic(page);
3202 memset(userpage + pg_offset, 0, iosize);
3203 flush_dcache_page(page);
3204 kunmap_atomic(userpage);
3205 set_extent_uptodate(tree, cur, cur + iosize - 1,
3206 &cached, GFP_NOFS);
3207 unlock_extent_cached(tree, cur,
3208 cur + iosize - 1, &cached);
3209 break;
3210 }
3211 if (pg_offset != 0)
> 3212 trace_printk("PG offset: %lu iosize: %lu\n", pg_offset, iosize);
3213 em = __get_extent_map(inode, page, pg_offset, cur,
3214 end - cur + 1, get_extent, em_cached);
3215 if (IS_ERR_OR_NULL(em)) {
3216 SetPageError(page);
3217 unlock_extent(tree, cur, end);
3218 break;
3219 }
3220 extent_offset = cur - em->start;
3221 BUG_ON(extent_map_end(em) <= cur);
3222 BUG_ON(end < cur);
3223
3224 if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) {
3225 this_bio_flag |= EXTENT_BIO_COMPRESSED;
3226 extent_set_compress_type(&this_bio_flag,
3227 em->compress_type);
3228 }
3229
3230 iosize = min(extent_map_end(em) - cur, end - cur + 1);
3231 cur_end = min(extent_map_end(em) - 1, end);
3232 iosize = ALIGN(iosize, blocksize);
3233 if (this_bio_flag & EXTENT_BIO_COMPRESSED) {
3234 disk_io_size = em->block_len;
3235 offset = em->block_start;
3236 } else {
3237 offset = em->block_start + extent_offset;
3238 disk_io_size = iosize;
3239 }
3240 block_start = em->block_start;
3241 if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
3242 block_start = EXTENT_MAP_HOLE;
3243
3244 /*
3245 * If we have a file range that points to a compressed extent
3246 * and it's followed by a consecutive file range that points
3247 * to the same compressed extent (possibly with a different
3248 * offset and/or length, so it either points to the whole extent
3249 * or only part of it), we must make sure we do not submit a
3250 * single bio to populate the pages for the 2 ranges because
3251 * this makes the compressed extent read zero out the pages
3252 * belonging to the 2nd range. Imagine the following scenario:
3253 *
3254 * File layout
3255 * [0 - 8K] [8K - 24K]
3256 * | |
3257 * | |
3258 * points to extent X, points to extent X,
3259 * offset 4K, length of 8K offset 0, length 16K
3260 *
3261 * [extent X, compressed length = 4K uncompressed length = 16K]
3262 *
3263 * If the bio to read the compressed extent covers both ranges,
3264 * it will decompress extent X into the pages belonging to the
3265 * first range and then it will stop, zeroing out the remaining
3266 * pages that belong to the other range that points to extent X.
3267 * So here we make sure we submit 2 bios, one for the first
3268 * range and another one for the third range. Both will target
3269 * the same physical extent from disk, but we can't currently
3270 * make the compressed bio endio callback populate the pages
3271 * for both ranges because each compressed bio is tightly
3272 * coupled with a single extent map, and each range can have
3273 * an extent map with a different offset value relative to the
3274 * uncompressed data of our extent and different lengths. This
3275 * is a corner case so we prioritize correctness over
3276 * non-optimal behavior (submitting 2 bios for the same extent).
3277 */
3278 if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags) &&
3279 prev_em_start && *prev_em_start != (u64)-1 &&
3280 *prev_em_start != em->start)
3281 force_bio_submit = true;
3282
3283 if (prev_em_start)
3284 *prev_em_start = em->start;
3285
3286 free_extent_map(em);
3287 em = NULL;
3288
3289 /* we've found a hole, just zero and go on */
3290 if (block_start == EXTENT_MAP_HOLE) {
3291 char *userpage;
3292 struct extent_state *cached = NULL;
3293
3294 userpage = kmap_atomic(page);
3295 memset(userpage + pg_offset, 0, iosize);
3296 flush_dcache_page(page);
3297 kunmap_atomic(userpage);
3298
3299 set_extent_uptodate(tree, cur, cur + iosize - 1,
3300 &cached, GFP_NOFS);
3301 unlock_extent_cached(tree, cur,
3302 cur + iosize - 1, &cached);
3303 cur = cur + iosize;
3304 pg_offset += iosize;
3305 continue;
3306 }
3307 /* the get_extent function already copied into the page */
3308 if (test_range_bit(tree, cur, cur_end,
3309 EXTENT_UPTODATE, 1, NULL)) {
3310 check_page_uptodate(tree, page);
3311 unlock_extent(tree, cur, cur + iosize - 1);
3312 cur = cur + iosize;
3313 pg_offset += iosize;
3314 continue;
3315 }
3316 /* we have an inline extent but it didn't get marked up
3317 * to date. Error out
3318 */
3319 if (block_start == EXTENT_MAP_INLINE) {
3320 SetPageError(page);
3321 unlock_extent(tree, cur, cur + iosize - 1);
3322 cur = cur + iosize;
3323 pg_offset += iosize;
3324 continue;
3325 }
3326
3327 ret = submit_extent_page(REQ_OP_READ | read_flags, NULL,
3328 page, offset, disk_io_size,
3329 pg_offset, bio,
3330 end_bio_extent_readpage, mirror_num,
3331 *bio_flags,
3332 this_bio_flag,
3333 force_bio_submit);
3334 if (!ret) {
3335 nr++;
3336 *bio_flags = this_bio_flag;
3337 } else {
3338 SetPageError(page);
3339 unlock_extent(tree, cur, cur + iosize - 1);
3340 goto out;
3341 }
3342 cur = cur + iosize;
3343 pg_offset += iosize;
3344 }
3345 out:
3346 if (!nr) {
3347 if (!PageError(page))
3348 SetPageUptodate(page);
3349 unlock_page(page);
3350 }
3351 return ret;
3352 }
3353
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31240 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 03/10] btrfs: Simplify metadata pages reading
2020-09-09 9:49 ` [PATCH 03/10] btrfs: Simplify metadata pages reading Nikolay Borisov
2020-09-09 11:20 ` Qu Wenruo
@ 2020-09-10 14:56 ` Josef Bacik
1 sibling, 0 replies; 28+ messages in thread
From: Josef Bacik @ 2020-09-10 14:56 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 9/9/20 5:49 AM, Nikolay Borisov wrote:
> Metadata pages currently use __do_readpage to read metadata pages,
> unfortunately this function is also used to deal with ordinary data
> pages. This makes the metadata pages reading code to go through multiple
> hoops in order to adhere to __do_readpage invariants. Most of these are
> necessary for data pages which could be compressed. For metadata it's
> enough to simply build a bio and submit it.
>
> To this effect simply call submit_extent_page directly from
> read_extent_buffer_pages which is the only callpath used to populate
> extent_buffers with data. This in turn enables further cleanups.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/extent_io.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index ac92c0ab1402..1789a7931312 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
> }
>
> ClearPageError(page);
> - err = __extent_read_full_page(page,
> - btree_get_extent, &bio,
> - mirror_num, &bio_flags,
> - REQ_META);
> + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL,
> + page, page_offset(page), PAGE_SIZE, 0,
> + &bio, end_bio_extent_readpage,
> + mirror_num, 0, 0, false);
> if (err) {
> ret = err;
> - /*
> - * We use &bio in above __extent_read_full_page,
> - * so we ensure that if it returns error, the
> - * current page fails to add itself to bio and
> - * it's been unlocked.
> - *
> - * We must dec io_pages by ourselves.
> - */
I'd rather change the comment to indicate that we failed to submit the bio, thus
the page is our responsibility to clean up, and thus we need to do the
error/unlock and io_pages dance. Thanks,
Josef
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 04/10] btrfs: Remove btree_get_extent
2020-09-09 9:49 ` [PATCH 04/10] btrfs: Remove btree_get_extent Nikolay Borisov
@ 2020-09-10 14:57 ` Josef Bacik
0 siblings, 0 replies; 28+ messages in thread
From: Josef Bacik @ 2020-09-10 14:57 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 9/9/20 5:49 AM, Nikolay Borisov wrote:
> The sole purpose of this function was to satisfy the requirements of
> __do_readpage. Since that function is no longer used to read metadata
> pages the need to keep btree_get_extent around has also disappeared.
> Simply remove it.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 06/10] btrfs: Remove mirror_num argument from extent_read_full_page
2020-09-09 9:49 ` [PATCH 06/10] btrfs: Remove mirror_num argument from extent_read_full_page Nikolay Borisov
@ 2020-09-10 14:58 ` Josef Bacik
0 siblings, 0 replies; 28+ messages in thread
From: Josef Bacik @ 2020-09-10 14:58 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 9/9/20 5:49 AM, Nikolay Borisov wrote:
> It's called only from btrfs_readpage which always passes 0 so just sink
> the argument into extent_read_full_page.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 07/10] btrfs: Promote extent_read_full_page to btrfs_readpage
2020-09-09 9:49 ` [PATCH 07/10] btrfs: Promote extent_read_full_page to btrfs_readpage Nikolay Borisov
@ 2020-09-10 15:01 ` Josef Bacik
0 siblings, 0 replies; 28+ messages in thread
From: Josef Bacik @ 2020-09-10 15:01 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 9/9/20 5:49 AM, Nikolay Borisov wrote:
> Now that btrfs_readpage is the only caller of extent_read_full_page the
> latter can be opencoded in the former. Use the occassion to rename
> __extent_read_full_page to extent_read_full_page. To facillitate this
> change submit_one_bio has to be exported as well.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 08/10] btrfs: Sink mirror_num argument in extent_read_full_page
2020-09-09 9:49 ` [PATCH 08/10] btrfs: Sink mirror_num argument in extent_read_full_page Nikolay Borisov
@ 2020-09-10 15:02 ` Josef Bacik
0 siblings, 0 replies; 28+ messages in thread
From: Josef Bacik @ 2020-09-10 15:02 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 9/9/20 5:49 AM, Nikolay Borisov wrote:
> It's always set to 0 from the sole caller - btrfs_readpage.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 09/10] btrfs: Sink read_flags argument into extent_read_full_page
2020-09-09 9:49 ` [PATCH 09/10] btrfs: Sink read_flags argument into extent_read_full_page Nikolay Borisov
@ 2020-09-10 15:03 ` Josef Bacik
0 siblings, 0 replies; 28+ messages in thread
From: Josef Bacik @ 2020-09-10 15:03 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 9/9/20 5:49 AM, Nikolay Borisov wrote:
> It's always set to 0 by its sole caller - btrfs_readpage. Simply remove
> it.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 10/10] btrfs: Sink mirror_num argument in __do_readpage
2020-09-09 9:49 ` [PATCH 10/10] btrfs: Sink mirror_num argument in __do_readpage Nikolay Borisov
@ 2020-09-10 15:04 ` Josef Bacik
0 siblings, 0 replies; 28+ messages in thread
From: Josef Bacik @ 2020-09-10 15:04 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 9/9/20 5:49 AM, Nikolay Borisov wrote:
> It's always set to 0 by the 2 callers so move it inside __do_readpage.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks,
Josef
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 03/10] btrfs: Simplify metadata pages reading
2020-09-09 11:20 ` Qu Wenruo
@ 2020-09-14 8:08 ` Nikolay Borisov
2020-09-14 8:22 ` Qu Wenruo
0 siblings, 1 reply; 28+ messages in thread
From: Nikolay Borisov @ 2020-09-14 8:08 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
On 9.09.20 г. 14:20 ч., Qu Wenruo wrote:
>
>
> On 2020/9/9 下午5:49, Nikolay Borisov wrote:
>> Metadata pages currently use __do_readpage to read metadata pages,
>> unfortunately this function is also used to deal with ordinary data
>> pages. This makes the metadata pages reading code to go through multiple
>> hoops in order to adhere to __do_readpage invariants. Most of these are
>> necessary for data pages which could be compressed. For metadata it's
>> enough to simply build a bio and submit it.
>>
>> To this effect simply call submit_extent_page directly from
>> read_extent_buffer_pages which is the only callpath used to populate
>> extent_buffers with data. This in turn enables further cleanups.
>
> This is awesome!!!
>
> And the code also looks pretty good to me.
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> Just a note for further enhancement inlined below.
>
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> fs/btrfs/extent_io.c | 18 ++++++------------
>> 1 file changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index ac92c0ab1402..1789a7931312 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>> }
>>
>> ClearPageError(page);
>> - err = __extent_read_full_page(page,
>> - btree_get_extent, &bio,
>> - mirror_num, &bio_flags,
>> - REQ_META);
>> + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL,
>> + page, page_offset(page), PAGE_SIZE, 0,
>
> It would be better to enhance the comment for submit_extent_page() of
> @offset.
> It's in fact btrfs logical bytenr.
>
> For metadata, page_offset(page) is also the btrfs logical bytenr so it's
> completely fine.
> But it can be different for data inodes, so it's better to make it more
> clear in the comment.
How can it be different for data node? page_offset is always page->index
<< PAGE_SHIFT, meaning it's the offset in the file that this page refers
to. I.e page 5 would refer to 20k.
>
> Thanks,
> Qu
>
>> + &bio, end_bio_extent_readpage,
>> + mirror_num, 0, 0, false);
>> if (err) {
>> ret = err;
>> - /*
>> - * We use &bio in above __extent_read_full_page,
>> - * so we ensure that if it returns error, the
>> - * current page fails to add itself to bio and
>> - * it's been unlocked.
>> - *
>> - * We must dec io_pages by ourselves.
>> - */
>> + SetPageError(page);
>> + unlock_page(page);
>> atomic_dec(&eb->io_pages);
>> }
>> } else {
>>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 03/10] btrfs: Simplify metadata pages reading
2020-09-14 8:08 ` Nikolay Borisov
@ 2020-09-14 8:22 ` Qu Wenruo
0 siblings, 0 replies; 28+ messages in thread
From: Qu Wenruo @ 2020-09-14 8:22 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 2020/9/14 下午4:08, Nikolay Borisov wrote:
>
>
> On 9.09.20 г. 14:20 ч., Qu Wenruo wrote:
>>
>>
>> On 2020/9/9 下午5:49, Nikolay Borisov wrote:
>>> Metadata pages currently use __do_readpage to read metadata pages,
>>> unfortunately this function is also used to deal with ordinary data
>>> pages. This makes the metadata pages reading code to go through multiple
>>> hoops in order to adhere to __do_readpage invariants. Most of these are
>>> necessary for data pages which could be compressed. For metadata it's
>>> enough to simply build a bio and submit it.
>>>
>>> To this effect simply call submit_extent_page directly from
>>> read_extent_buffer_pages which is the only callpath used to populate
>>> extent_buffers with data. This in turn enables further cleanups.
>>
>> This is awesome!!!
>>
>> And the code also looks pretty good to me.
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Just a note for further enhancement inlined below.
>>
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>> fs/btrfs/extent_io.c | 18 ++++++------------
>>> 1 file changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index ac92c0ab1402..1789a7931312 100644
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -5575,20 +5575,14 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>>> }
>>>
>>> ClearPageError(page);
>>> - err = __extent_read_full_page(page,
>>> - btree_get_extent, &bio,
>>> - mirror_num, &bio_flags,
>>> - REQ_META);
>>> + err = submit_extent_page(REQ_OP_READ | REQ_META, NULL,
>>> + page, page_offset(page), PAGE_SIZE, 0,
>>
>> It would be better to enhance the comment for submit_extent_page() of
>> @offset.
>> It's in fact btrfs logical bytenr.
>>
>> For metadata, page_offset(page) is also the btrfs logical bytenr so it's
>> completely fine.
>> But it can be different for data inodes, so it's better to make it more
>> clear in the comment.
>
> How can it be different for data node? page_offset is always page->index
> << PAGE_SHIFT, meaning it's the offset in the file that this page refers
> to. I.e page 5 would refer to 20k.
For data inode, its page_offset() is the file offset, not logical bytenr.
Thanks,
Qu
>
>>
>> Thanks,
>> Qu
>>
>>> + &bio, end_bio_extent_readpage,
>>> + mirror_num, 0, 0, false);
>>> if (err) {
>>> ret = err;
>>> - /*
>>> - * We use &bio in above __extent_read_full_page,
>>> - * so we ensure that if it returns error, the
>>> - * current page fails to add itself to bio and
>>> - * it's been unlocked.
>>> - *
>>> - * We must dec io_pages by ourselves.
>>> - */
>>> + SetPageError(page);
>>> + unlock_page(page);
>>> atomic_dec(&eb->io_pages);
>>> }
>>> } else {
>>>
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2020-09-14 8:22 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09 9:49 [PATCH 00/10] Cleanup metadata page reading path Nikolay Borisov
2020-09-09 9:49 ` [PATCH 01/10] btrfs: Remove btree_readpage Nikolay Borisov
2020-09-09 10:37 ` Johannes Thumshirn
2020-09-09 11:13 ` Qu Wenruo
2020-09-09 9:49 ` [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent Nikolay Borisov
2020-09-09 10:40 ` Johannes Thumshirn
2020-09-09 11:15 ` Qu Wenruo
2020-09-09 20:46 ` kernel test robot
2020-09-09 9:49 ` [PATCH 03/10] btrfs: Simplify metadata pages reading Nikolay Borisov
2020-09-09 11:20 ` Qu Wenruo
2020-09-14 8:08 ` Nikolay Borisov
2020-09-14 8:22 ` Qu Wenruo
2020-09-10 14:56 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 04/10] btrfs: Remove btree_get_extent Nikolay Borisov
2020-09-10 14:57 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage Nikolay Borisov
2020-09-09 11:24 ` Qu Wenruo
2020-09-09 11:56 ` Nikolay Borisov
2020-09-09 9:49 ` [PATCH 06/10] btrfs: Remove mirror_num argument from extent_read_full_page Nikolay Borisov
2020-09-10 14:58 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 07/10] btrfs: Promote extent_read_full_page to btrfs_readpage Nikolay Borisov
2020-09-10 15:01 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 08/10] btrfs: Sink mirror_num argument in extent_read_full_page Nikolay Borisov
2020-09-10 15:02 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 09/10] btrfs: Sink read_flags argument into extent_read_full_page Nikolay Borisov
2020-09-10 15:03 ` Josef Bacik
2020-09-09 9:49 ` [PATCH 10/10] btrfs: Sink mirror_num argument in __do_readpage Nikolay Borisov
2020-09-10 15:04 ` Josef Bacik
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.