[PATCH 00/10] Cleanup metadata page reading path

[PATCH 00/10] Cleanup metadata page reading path

[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

[PATCH 01/10] btrfs: Remove btree_readpage

[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

Re: [PATCH 01/10] btrfs: Remove btree_readpage

[Johannes Thumshirn]

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.

Re: [PATCH 01/10] btrfs: Remove btree_readpage

[Qu Wenruo]

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

[PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent

[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

Re: [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent

[Johannes Thumshirn]

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

Re: [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent

[Qu Wenruo]

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;
>

Re: [PATCH 02/10] btrfs: Remove pg_offset from btrfs_get_extent

[kernel test robot]

[-- 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 --]

[PATCH 03/10] btrfs: Simplify metadata pages reading

[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

Re: [PATCH 03/10] btrfs: Simplify metadata pages reading

[Qu Wenruo]

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 {
>

Re: [PATCH 03/10] btrfs: Simplify metadata pages reading

[Nikolay Borisov]

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 {
>>
> 

Re: [PATCH 03/10] btrfs: Simplify metadata pages reading

[Qu Wenruo]

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 {
>>>
>>

Re: [PATCH 03/10] btrfs: Simplify metadata pages reading

[Josef Bacik]

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

[PATCH 04/10] btrfs: Remove btree_get_extent

[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

Re: [PATCH 04/10] btrfs: Remove btree_get_extent

[Josef Bacik]

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

[PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage

[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

Re: [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage

[Qu Wenruo]

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)
>

Re: [PATCH 05/10] btrfs: Remove btrfs_get_extent indirection from __do_readpage

[Nikolay Borisov]

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)
>>
> 

[PATCH 06/10] btrfs: Remove mirror_num argument from extent_read_full_page

[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

Re: [PATCH 06/10] btrfs: Remove mirror_num argument from extent_read_full_page

[Josef Bacik]

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

[PATCH 07/10] btrfs: Promote extent_read_full_page to btrfs_readpage

[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

Re: [PATCH 07/10] btrfs: Promote extent_read_full_page to btrfs_readpage

[Josef Bacik]

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

[PATCH 08/10] btrfs: Sink mirror_num argument in extent_read_full_page

[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

Re: [PATCH 08/10] btrfs: Sink mirror_num argument in extent_read_full_page

[Josef Bacik]

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

[PATCH 09/10] btrfs: Sink read_flags argument into extent_read_full_page

[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

Re: [PATCH 09/10] btrfs: Sink read_flags argument into extent_read_full_page

[Josef Bacik]

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

[PATCH 10/10] btrfs: Sink mirror_num argument in __do_readpage

[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

Re: [PATCH 10/10] btrfs: Sink mirror_num argument in __do_readpage

[Josef Bacik]

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