[11/17] btrfs: extract the extent buffer verification from btree_readpage_end_io_hook()

Commit Message

Qu Wenruo Sept. 8, 2020, 7:52 a.m. UTC

Currently btree_readpage_end_io_hook() only needs to handle one extent
buffer as currently one page only maps to one extent buffer.

But for incoming subpage support, one page can be mapped to multiple
extent buffers, thus we can no longer use current code.

This refactor would allow us to call btrfs_check_extent_buffer() on
all involved extent buffers at btree_readpage_end_io_hook() and other
locations.

Signed-off-by: Qu Wenruo <[e-mail]>
---
 fs/btrfs/disk-io.c | 78 ++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 33 deletions(-)

Comments

Nikolay Borisov Sept. 11, 2020, 1 p.m. UTC | #1

On 8.09.20 г. 10:52 ч., Qu Wenruo wrote:
> Currently btree_readpage_end_io_hook() only needs to handle one extent
> buffer as currently one page only maps to one extent buffer.
> 
> But for incoming subpage support, one page can be mapped to multiple
> extent buffers, thus we can no longer use current code.
> 
> This refactor would allow us to call btrfs_check_extent_buffer() on
> all involved extent buffers at btree_readpage_end_io_hook() and other
> locations.
> 
> Signed-off-by: Qu Wenruo <[e-mail]>

Reviewed-by: Nikolay Borisov <[e-mail]>

> ---
>  fs/btrfs/disk-io.c | 78 ++++++++++++++++++++++++++--------------------
>  1 file changed, 45 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 62dbd9bbd381..f6e562979682 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -574,60 +574,37 @@ static int check_tree_block_fsid(struct extent_buffer *eb)
>  	return ret;
>  }
>  

<snip>
> +static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> +				      u64 phy_offset, struct page *page,
> +				      u64 start, u64 end, int mirror)
> +{
> +	struct extent_buffer *eb;
> +	int ret = 0;
> +	int reads_done;

nit: while at it just turn it into a bool.

> +
> +	if (!page->private)
> +		goto out;
> +

nit: metadata pages are guaranteed to have their ->private val set to
the pointer of extent buffer so this check can go away.

> +	eb = (struct extent_buffer *)page->private;
> +
> +	/*
> +	 * The pending IO might have been the only thing that kept this buffer
> +	 * in memory.  Make sure we have a ref for all this other checks
> +	 */
> +	atomic_inc(&eb->refs);
> +
> +	reads_done = atomic_dec_and_test(&eb->io_pages);
> +	if (!reads_done)
> +		goto err;
> +
> +	eb->read_mirror = mirror;
> +	if (test_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags)) {
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	ret = btrfs_check_extent_buffer(eb);
> +
>  err:
>  	if (reads_done &&
>  	    test_and_clear_bit(EXTENT_BUFFER_READAHEAD, &eb->bflags))
>

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 62dbd9bbd381..f6e562979682 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -574,60 +574,37 @@  static int check_tree_block_fsid(struct extent_buffer *eb)
 	return ret;
 }
 
-static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
-				      u64 phy_offset, struct page *page,
-				      u64 start, u64 end, int mirror)
+/* Do basic extent buffer check at read time */
+static int btrfs_check_extent_buffer(struct extent_buffer *eb)
 {
-	u64 found_start;
-	int found_level;
-	struct extent_buffer *eb;
-	struct btrfs_fs_info *fs_info;
+	struct btrfs_fs_info *fs_info = eb->fs_info;
 	u16 csum_size;
-	int ret = 0;
+	u64 found_start;
+	u8 found_level;
 	u8 result[BTRFS_CSUM_SIZE];
-	int reads_done;
-
-	if (!page->private)
-		goto out;
+	int ret = 0;
 
-	eb = (struct extent_buffer *)page->private;
-	fs_info = eb->fs_info;
 	csum_size = btrfs_super_csum_size(fs_info->super_copy);
 
-	/* the pending IO might have been the only thing that kept this buffer
-	 * in memory.  Make sure we have a ref for all this other checks
-	 */
-	atomic_inc(&eb->refs);
-
-	reads_done = atomic_dec_and_test(&eb->io_pages);
-	if (!reads_done)
-		goto err;
-
-	eb->read_mirror = mirror;
-	if (test_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags)) {
-		ret = -EIO;
-		goto err;
-	}
-
 	found_start = btrfs_header_bytenr(eb);
 	if (found_start != eb->start) {
 		btrfs_err_rl(fs_info, "bad tree block start, want %llu have %llu",
 			     eb->start, found_start);
 		ret = -EIO;
-		goto err;
+		goto out;
 	}
 	if (check_tree_block_fsid(eb)) {
 		btrfs_err_rl(fs_info, "bad fsid on block %llu",
 			     eb->start);
 		ret = -EIO;
-		goto err;
+		goto out;
 	}
 	found_level = btrfs_header_level(eb);
 	if (found_level >= BTRFS_MAX_LEVEL) {
 		btrfs_err(fs_info, "bad tree block level %d on %llu",
 			  (int)btrfs_header_level(eb), eb->start);
 		ret = -EIO;
-		goto err;
+		goto out;
 	}
 
 	btrfs_set_buffer_lockdep_class(btrfs_header_owner(eb),
@@ -647,7 +624,7 @@  static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
 			      fs_info->sb->s_id, eb->start,
 			      val, found, btrfs_header_level(eb));
 		ret = -EUCLEAN;
-		goto err;
+		goto out;
 	}
 
 	/*
@@ -669,6 +646,41 @@  static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
 		btrfs_err(fs_info,
 			  "block=%llu read time tree block corruption detected",
 			  eb->start);
+out:
+	return ret;
+}
+
+static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
+				      u64 phy_offset, struct page *page,
+				      u64 start, u64 end, int mirror)
+{
+	struct extent_buffer *eb;
+	int ret = 0;
+	int reads_done;
+
+	if (!page->private)
+		goto out;
+
+	eb = (struct extent_buffer *)page->private;
+
+	/*
+	 * The pending IO might have been the only thing that kept this buffer
+	 * in memory.  Make sure we have a ref for all this other checks
+	 */
+	atomic_inc(&eb->refs);
+
+	reads_done = atomic_dec_and_test(&eb->io_pages);
+	if (!reads_done)
+		goto err;
+
+	eb->read_mirror = mirror;
+	if (test_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags)) {
+		ret = -EIO;
+		goto err;
+	}
+
+	ret = btrfs_check_extent_buffer(eb);
+
 err:
 	if (reads_done &&
 	    test_and_clear_bit(EXTENT_BUFFER_READAHEAD, &eb->bflags))