[PATCH v2 0/9] Cleanup metadata page reading path

[PATCH v2 0/9] Cleanup metadata page reading path

[Nikolay Borisov]

Here is v2 of the metadata readout cleanups [0]. This series incorporates the
feedback I received, namely:

* Added justification why removing btree_readpage is safe in Patch 1
* Dropped Patch 2 (pg_offset remove from btrfs_get_extent) as Qu intends on using
it in his subpage blocksize work.
* Add a comment about caller's responsibility for cleanup in Patch 3
* Added RB for patches which haven't changed since v1 and got RB by Josef.

[0] https://lore.kernel.org/linux-btrfs/20200909094914.29721-1-nborisov@suse.com/T/#t

Nikolay Borisov (9):
  btrfs: Remove btree_readpage
  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/disk-io.c   | 53 ---------------------------------
 fs/btrfs/disk-io.h   |  3 --
 fs/btrfs/extent_io.c | 71 +++++++++++++++-----------------------------
 fs/btrfs/extent_io.h |  6 ++--
 fs/btrfs/inode.c     |  9 +++++-
 5 files changed, 36 insertions(+), 106 deletions(-)

--
2.17.1

[PATCH v2 1/9] btrfs: Remove btree_readpage

[Nikolay Borisov]

There is no way for this function to be called as ->readpage() since
it's called from
generic_file_buffered_read/filemap_fault/do_read_cache_page/readhead
code. BTRFS doesn't utilize the first 3 for the btree inode and
implements it's owon readhead mechanism. So simply remove the function.

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

[PATCH v2 2/9] 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 | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a1e070ec7ad8..0a6cda4c30ed 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5573,20 +5573,19 @@ 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.
+				 * We failed to submit the bio so it's the
+				 * caller's responsibility to perform cleanup
+				 * i.e unlock page/set error bit.
 				 */
+				ret = err;
+				SetPageError(page);
+				unlock_page(page);
 				atomic_dec(&eb->io_pages);
 			}
 		} else {
-- 
2.17.1

[PATCH v2 3/9] 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>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c | 47 ----------------------------------------------
 fs/btrfs/disk-io.h |  3 ---
 2 files changed, 50 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d63498f3c75f..2a5aadcf6b54 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -204,53 +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, size_t pg_offset,
-				    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 00dc39d47ed3..89b6a709a184 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -123,9 +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, size_t pg_offset,
-				    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

[PATCH v2 4/9] 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 0a6cda4c30ed..4a0675ec90fa 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, pg_offset, start, len);
+	em = btrfs_get_extent(BTRFS_I(inode), page, pg_offset, 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)
@@ -3209,7 +3206,7 @@ static int __do_readpage(struct page *page,
 			break;
 		}
 		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);
@@ -3362,16 +3359,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);
@@ -3381,20 +3376,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 06611947a9f7..272d5281bd4d 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 cce6f8789a4e..df8a4008b139 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8040,7 +8040,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

[PATCH v2 5/9] 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>
Reviewed-by: Josef Bacik <josef@toxicpanda.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 4a0675ec90fa..355db40a1cb5 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3381,15 +3381,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 272d5281bd4d..0ccb2dabc291 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 df8a4008b139..e78099d1db34 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8040,7 +8040,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

[PATCH v2 6/9] 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>
Reviewed-by: Josef Bacik <josef@toxicpanda.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 355db40a1cb5..402b88ddcbca 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;
@@ -3365,9 +3365,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);
@@ -3381,18 +3380,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 0ccb2dabc291..8fec00c50846 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 e78099d1db34..f01066607901 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8040,7 +8040,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

[PATCH v2 7/9] 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>
Reviewed-by: Josef Bacik <josef@toxicpanda.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 402b88ddcbca..f43827bee7e6 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3365,7 +3365,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);
@@ -3375,8 +3375,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 8fec00c50846..3562c9203de3 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 f01066607901..31a154c7fb00 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8044,7 +8044,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

[PATCH v2 8/9] 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>
Reviewed-by: Josef Bacik <josef@toxicpanda.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 f43827bee7e6..1c959d66195c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3366,7 +3366,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);
@@ -3375,7 +3375,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 3562c9203de3..5fa248570145 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 31a154c7fb00..c50a907742a1 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8044,7 +8044,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

[PATCH v2 9/9] 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>
Reviewed-by: Josef Bacik <josef@toxicpanda.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 1c959d66195c..1ef857c0d784 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);
@@ -3322,7 +3321,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);
@@ -3359,7 +3358,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]);
 	}
@@ -3375,7 +3374,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 v2 7/9] btrfs: Sink mirror_num argument in extent_read_full_page

[Johannes Thumshirn]

On 14/09/2020 11:37, Nikolay Borisov wrote:
> @@ -3375,8 +3375,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;
>  }

	return _do_readpage(page, NULL, bio, 0, bio_flags, read_flags, NULL);

Re: [PATCH v2 7/9] btrfs: Sink mirror_num argument in extent_read_full_page

[Nikolay Borisov]

On 14.09.20 г. 13:35 ч., Johannes Thumshirn wrote:
> On 14/09/2020 11:37, Nikolay Borisov wrote:
>> @@ -3375,8 +3375,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;
>>  }
> 
> 	return _do_readpage(page, NULL, bio, 0, bio_flags, read_flags, NULL);
> 

Ah yes, but actually I think extent_read_full_page should be opencoded
in btrfs_readpage ..

Re: [PATCH v2 0/9] Cleanup metadata page reading path

[Johannes Thumshirn]

On 14/09/2020 11:37, Nikolay Borisov wrote:
> Here is v2 of the metadata readout cleanups [0]. This series incorporates the
> feedback I received, namely:
> 
> * Added justification why removing btree_readpage is safe in Patch 1
> * Dropped Patch 2 (pg_offset remove from btrfs_get_extent) as Qu intends on using
> it in his subpage blocksize work.
> * Add a comment about caller's responsibility for cleanup in Patch 3
> * Added RB for patches which haven't changed since v1 and got RB by Josef.
> 
> [0] https://lore.kernel.org/linux-btrfs/20200909094914.29721-1-nborisov@suse.com/T/#t

Apart from the nit in 7/9
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 1/9] btrfs: Remove btree_readpage

[Qu Wenruo]

On 2020/9/14 下午5:37, Nikolay Borisov wrote:
> There is no way for this function to be called as ->readpage() since
> it's called from
> generic_file_buffered_read/filemap_fault/do_read_cache_page/readhead
> code. BTRFS doesn't utilize the first 3 for the btree inode and
> implements it's owon readhead mechanism. So simply remove the function.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

With the new commit message, it's way easier to know why that function
is not needed.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>  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,
>

Re: [PATCH v2 2/9] btrfs: Simplify metadata pages reading

[Qu Wenruo]

On 2020/9/14 下午5:37, 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>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>  fs/btrfs/extent_io.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index a1e070ec7ad8..0a6cda4c30ed 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5573,20 +5573,19 @@ 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.
> +				 * We failed to submit the bio so it's the
> +				 * caller's responsibility to perform cleanup
> +				 * i.e unlock page/set error bit.
>  				 */
> +				ret = err;
> +				SetPageError(page);
> +				unlock_page(page);
>  				atomic_dec(&eb->io_pages);
>  			}
>  		} else {
>

[PATCH] btrfs: Opencode extent_read_full_page to its sole caller

[Nikolay Borisov]

This makes reading the code a tad easier by decreasing the level of
indirection by one.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
David,

Here is a followup patch based on Johaness' feedback if you could merge it
after having merged the whole series I'll much appreciated it.

 fs/btrfs/extent_io.c | 24 +++++-------------------
 fs/btrfs/extent_io.h |  5 +++--
 fs/btrfs/inode.c     |  9 +++++++--
 3 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1ef857c0d784..afac70ef0cc5 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3141,9 +3141,9 @@ __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, struct extent_map **em_cached,
-			 struct bio **bio, unsigned long *bio_flags,
-			 unsigned int read_flags, u64 *prev_em_start)
+int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
+		      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);
@@ -3358,26 +3358,12 @@ 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, bio_flags,
-			      REQ_RAHEAD, prev_em_start);
+		btrfs_do_readpage(pages[index], em_cached, bio, bio_flags,
+				  REQ_RAHEAD, prev_em_start);
 		put_page(pages[index]);
 	}
 }

-int extent_read_full_page(struct page *page, struct bio **bio,
-			  unsigned long *bio_flags)
-{
-	struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
-	u64 start = page_offset(page);
-	u64 end = start + PAGE_SIZE - 1;
-	int ret;
-
-	btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
-
-	ret = __do_readpage(page, NULL, bio, bio_flags, 0, NULL);
-	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 5fa248570145..3bbc25b816ea 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -195,8 +195,9 @@ 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);
+int btrfs_do_readpage(struct page *page, struct extent_map **em_cached,
+		      struct bio **bio, unsigned long *bio_flags,
+		      unsigned int read_flags, u64 *prev_em_start);
 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 c50a907742a1..245c80aaf7a3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8040,11 +8040,16 @@ static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,

 int btrfs_readpage(struct file *file, struct page *page)
 {
-	struct bio *bio = NULL;
+	struct btrfs_inode *inode = BTRFS_I(page->mapping->host);
+	u64 start = page_offset(page);
+	u64 end = start + PAGE_SIZE - 1;
 	unsigned long bio_flags = 0;
+	struct bio *bio = NULL;
 	int ret;

-	ret = extent_read_full_page(page, &bio, &bio_flags);
+	btrfs_lock_and_flush_ordered_range(inode, start, end, NULL);
+
+	ret = btrfs_do_readpage(page, NULL, &bio, &bio_flags, 0, NULL);
 	if (bio)
 		ret = submit_one_bio(bio, 0, bio_flags);
 	return ret;
--
2.17.1

Re: [PATCH v2 0/9] Cleanup metadata page reading path

[Nikolay Borisov]

On 14.09.20 г. 13:39 ч., Johannes Thumshirn wrote:
> On 14/09/2020 11:37, Nikolay Borisov wrote:
>> Here is v2 of the metadata readout cleanups [0]. This series incorporates the
>> feedback I received, namely:
>>
>> * Added justification why removing btree_readpage is safe in Patch 1
>> * Dropped Patch 2 (pg_offset remove from btrfs_get_extent) as Qu intends on using
>> it in his subpage blocksize work.
>> * Add a comment about caller's responsibility for cleanup in Patch 3
>> * Added RB for patches which haven't changed since v1 and got RB by Josef.
>>
>> [0] https://lore.kernel.org/linux-btrfs/20200909094914.29721-1-nborisov@suse.com/T/#t
> 
> Apart from the nit in 7/9
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 


I believe your feedback has been addressed by the follow on patch I just
sent.

Re: [PATCH v2 0/9] Cleanup metadata page reading path

[Johannes Thumshirn]

On 14/09/2020 13:40, Nikolay Borisov wrote:
> I believe your feedback has been addressed by the follow on patch I just
> sent.
> 
Yep looks good, thanks :)

Re: [PATCH] btrfs: Opencode extent_read_full_page to its sole caller

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH v2 0/9] Cleanup metadata page reading path

[David Sterba]

On Mon, Sep 14, 2020 at 12:37:02PM +0300, Nikolay Borisov wrote:
> Here is v2 of the metadata readout cleanups [0]. This series incorporates the
> feedback I received, namely:
> 
> * Added justification why removing btree_readpage is safe in Patch 1
> * Dropped Patch 2 (pg_offset remove from btrfs_get_extent) as Qu intends on using
> it in his subpage blocksize work.
> * Add a comment about caller's responsibility for cleanup in Patch 3
> * Added RB for patches which haven't changed since v1 and got RB by Josef.
> 
> [0] https://lore.kernel.org/linux-btrfs/20200909094914.29721-1-nborisov@suse.com/T/#t
> 
> Nikolay Borisov (9):
>   btrfs: Remove btree_readpage
>   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

Added to misc-next, thanks.

Re: [PATCH] btrfs: Opencode extent_read_full_page to its sole caller

[David Sterba]

On Mon, Sep 14, 2020 at 02:39:16PM +0300, Nikolay Borisov wrote:
> This makes reading the code a tad easier by decreasing the level of
> indirection by one.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> David,
> 
> Here is a followup patch based on Johaness' feedback if you could merge it
> after having merged the whole series I'll much appreciated it.

Sure, added to the rest.