[PATCH 0/2] btrfs: merge the two different super block read functions into one

[PATCH 0/2] btrfs: merge the two different super block read functions into one

[Qu Wenruo]

We have two functions to read a super block from a block device:

- btrfs_read_dev_one_super()
  Exported from disk-io.c

- btrfs_read_disk_super()
  Local to volumes.c

Which are doing mostly the same thing, so the first patch merge them
into btrfs_read_disk_super().

The second patch removes the btrfs_read_dev_super() function, as it
never really implement the full super blocks scan, and can be replaced
by a btrfs_read_disk_super() call reading the primary super block.

This not only reduce the duplication, but also removes the confusion
between all the similiarly named functions.

Qu Wenruo (2):
  btrfs: merge btrfs_read_dev_one_super() into btrfs_read_disk_super()
  btrfs: get rid of btrfs_read_dev_super()

 fs/btrfs/disk-io.c | 81 +-----------------------------------------
 fs/btrfs/disk-io.h |  3 --
 fs/btrfs/super.c   |  2 +-
 fs/btrfs/volumes.c | 88 +++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.h |  2 ++
 5 files changed, 48 insertions(+), 128 deletions(-)

-- 
2.49.0

[PATCH 1/2] btrfs: merge btrfs_read_dev_one_super() into btrfs_read_disk_super()

[Qu Wenruo]

We have two functions to read a super block from a block device:

- btrfs_read_dev_one_super()
  Exported from disk-io.c

- btrfs_read_disk_super()
  Local to volumes.c

And they have some minor differences:

- btrfs_read_dev_one_super() uses @copy_num
  Meanwhile btrfs_read_disk_super() relies on the physical and expected
  bytenr passed from the caller.

  To be hoest, the parameter list of btrfs_read_dev_one_super() is more
  user friendly.

- btrfs_read_disk_super() makes sure the label is NUL terminated

We do not need two different functions doing the same job, so merge the
behavior into btrfs_read_disk_super() by:

- Remove btrfs_read_dev_one_super()

- Export btrfs_read_disk_super()
  The name pairs with btrfs_release_disk_super() perfectly

- Change the parameter list of btrfs_read_disk_super() to mimic
  btrfs_read_dev_one_super()
  All existing callers are calculating the physical address and expected
  bytenr before calling btrfs_read_disk_super() already.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 53 +---------------------------
 fs/btrfs/disk-io.h |  2 --
 fs/btrfs/super.c   |  2 +-
 fs/btrfs/volumes.c | 86 +++++++++++++++++++++++-----------------------
 fs/btrfs/volumes.h |  2 ++
 5 files changed, 47 insertions(+), 98 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3d151d189510..df68796288a2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3702,57 +3702,6 @@ static void btrfs_end_super_write(struct bio *bio)
 	bio_put(bio);
 }
 
-struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
-						   int copy_num, bool drop_cache)
-{
-	struct btrfs_super_block *super;
-	struct page *page;
-	u64 bytenr, bytenr_orig;
-	struct address_space *mapping = bdev->bd_mapping;
-	int ret;
-
-	bytenr_orig = btrfs_sb_offset(copy_num);
-	ret = btrfs_sb_log_location_bdev(bdev, copy_num, READ, &bytenr);
-	if (ret == -ENOENT)
-		return ERR_PTR(-EINVAL);
-	else if (ret)
-		return ERR_PTR(ret);
-
-	if (bytenr + BTRFS_SUPER_INFO_SIZE >= bdev_nr_bytes(bdev))
-		return ERR_PTR(-EINVAL);
-
-	if (drop_cache) {
-		/* This should only be called with the primary sb. */
-		ASSERT(copy_num == 0);
-
-		/*
-		 * Drop the page of the primary superblock, so later read will
-		 * always read from the device.
-		 */
-		invalidate_inode_pages2_range(mapping,
-				bytenr >> PAGE_SHIFT,
-				(bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT);
-	}
-
-	page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);
-	if (IS_ERR(page))
-		return ERR_CAST(page);
-
-	super = page_address(page);
-	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
-		btrfs_release_disk_super(super);
-		return ERR_PTR(-ENODATA);
-	}
-
-	if (btrfs_super_bytenr(super) != bytenr_orig) {
-		btrfs_release_disk_super(super);
-		return ERR_PTR(-EINVAL);
-	}
-
-	return super;
-}
-
-
 struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev)
 {
 	struct btrfs_super_block *super, *latest = NULL;
@@ -3765,7 +3714,7 @@ struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev)
 	 * later supers, using BTRFS_SUPER_MIRROR_MAX instead
 	 */
 	for (i = 0; i < 1; i++) {
-		super = btrfs_read_dev_one_super(bdev, i, false);
+		super = btrfs_read_disk_super(bdev, i, false);
 		if (IS_ERR(super))
 			continue;
 
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index f87bbb7f8e7e..0ac9baf1215f 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -59,8 +59,6 @@ int btrfs_validate_super(const struct btrfs_fs_info *fs_info,
 int btrfs_check_features(struct btrfs_fs_info *fs_info, bool is_rw_mount);
 int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors);
 struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev);
-struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
-						   int copy_num, bool drop_cache);
 int btrfs_commit_super(struct btrfs_fs_info *fs_info);
 struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root,
 					const struct btrfs_key *key);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index eb92465536f3..bbaf99b118bb 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2292,7 +2292,7 @@ static int check_dev_super(struct btrfs_device *dev)
 		return 0;
 
 	/* Only need to check the primary super block. */
-	sb = btrfs_read_dev_one_super(dev->bdev, 0, true);
+	sb = btrfs_read_disk_super(dev->bdev, 0, true);
 	if (IS_ERR(sb))
 		return PTR_ERR(sb);
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ab8fccf65996..e751bc32e867 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1401,48 +1401,62 @@ void btrfs_release_disk_super(struct btrfs_super_block *super)
 	put_page(page);
 }
 
-static struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
-						       u64 bytenr, u64 bytenr_orig)
+struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
+						int copy_num, bool drop_cache)
 {
-	struct btrfs_super_block *disk_super;
+	struct btrfs_super_block *super;
 	struct page *page;
-	void *p;
-	pgoff_t index;
+	u64 bytenr, bytenr_orig;
+	struct address_space *mapping = bdev->bd_mapping;
+	int ret;
 
-	/* make sure our super fits in the device */
-	if (bytenr + PAGE_SIZE >= bdev_nr_bytes(bdev))
+	bytenr_orig = btrfs_sb_offset(copy_num);
+	ret = btrfs_sb_log_location_bdev(bdev, copy_num, READ, &bytenr);
+	if (ret == -ENOENT)
+		return ERR_PTR(-EINVAL);
+	else if (ret)
+		return ERR_PTR(ret);
+
+	if (bytenr + BTRFS_SUPER_INFO_SIZE >= bdev_nr_bytes(bdev))
 		return ERR_PTR(-EINVAL);
 
-	/* make sure our super fits in the page */
-	if (sizeof(*disk_super) > PAGE_SIZE)
-		return ERR_PTR(-EINVAL);
+	if (drop_cache) {
+		/* This should only be called with the primary sb. */
+		ASSERT(copy_num == 0);
 
-	/* make sure our super doesn't straddle pages on disk */
-	index = bytenr >> PAGE_SHIFT;
-	if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_SHIFT != index)
-		return ERR_PTR(-EINVAL);
-
-	/* pull in the page with our super */
-	page = read_cache_page_gfp(bdev->bd_mapping, index, GFP_KERNEL);
+		/*
+		 * Drop the page of the primary superblock, so later read will
+		 * always read from the device.
+		 */
+		invalidate_inode_pages2_range(mapping,
+				bytenr >> PAGE_SHIFT,
+				(bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT);
+	}
 
+	page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);
 	if (IS_ERR(page))
 		return ERR_CAST(page);
 
-	p = page_address(page);
+	super = page_address(page);
+	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
+		btrfs_release_disk_super(super);
+		return ERR_PTR(-ENODATA);
+	}
 
-	/* align our pointer to the offset of the super block */
-	disk_super = p + offset_in_page(bytenr);
-
-	if (btrfs_super_bytenr(disk_super) != bytenr_orig ||
-	    btrfs_super_magic(disk_super) != BTRFS_MAGIC) {
-		btrfs_release_disk_super(p);
+	if (btrfs_super_bytenr(super) != bytenr_orig) {
+		btrfs_release_disk_super(super);
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (disk_super->label[0] && disk_super->label[BTRFS_LABEL_SIZE - 1])
-		disk_super->label[BTRFS_LABEL_SIZE - 1] = 0;
+	/*
+	 * Make sure the last byte of label is properly NUL termiated.
+	 * We use '%s' to print the label, if not properly NUL termiated we
+	 * can access beyond the label.
+	 */
+	if (super->label[0] && super->label[BTRFS_LABEL_SIZE - 1])
+		super->label[BTRFS_LABEL_SIZE - 1] = 0;
 
-	return disk_super;
+	return super;
 }
 
 int btrfs_forget_devices(dev_t devt)
@@ -1514,7 +1528,6 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 	struct btrfs_device *device = NULL;
 	struct file *bdev_file;
 	char *canonical_path = NULL;
-	u64 bytenr;
 	dev_t devt;
 	int ret;
 
@@ -1544,20 +1557,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 	if (IS_ERR(bdev_file))
 		return ERR_CAST(bdev_file);
 
-	/*
-	 * We would like to check all the super blocks, but doing so would
-	 * allow a mount to succeed after a mkfs from a different filesystem.
-	 * Currently, recovery from a bad primary btrfs superblock is done
-	 * using the userspace command 'btrfs check --super'.
-	 */
-	ret = btrfs_sb_log_location_bdev(file_bdev(bdev_file), 0, READ, &bytenr);
-	if (ret) {
-		device = ERR_PTR(ret);
-		goto error_bdev_put;
-	}
-
-	disk_super = btrfs_read_disk_super(file_bdev(bdev_file), bytenr,
-					   btrfs_sb_offset(0));
+	disk_super = btrfs_read_disk_super(file_bdev(bdev_file), 0, false);
 	if (IS_ERR(disk_super)) {
 		device = ERR_CAST(disk_super);
 		goto error_bdev_put;
@@ -2223,7 +2223,7 @@ static void btrfs_scratch_superblock(struct btrfs_fs_info *fs_info,
 	const u64 bytenr = btrfs_sb_offset(copy_num);
 	int ret;
 
-	disk_super = btrfs_read_disk_super(bdev, bytenr, bytenr);
+	disk_super = btrfs_read_disk_super(bdev, copy_num, false);
 	if (IS_ERR(disk_super))
 		return;
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 72b8122938eb..37cfdd71e998 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -785,6 +785,8 @@ struct btrfs_chunk_map *btrfs_find_chunk_map_nolock(struct btrfs_fs_info *fs_inf
 struct btrfs_chunk_map *btrfs_get_chunk_map(struct btrfs_fs_info *fs_info,
 					    u64 logical, u64 length);
 void btrfs_remove_chunk_map(struct btrfs_fs_info *fs_info, struct btrfs_chunk_map *map);
+struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
+						int copy_num, bool drop_cache);
 void btrfs_release_disk_super(struct btrfs_super_block *super);
 
 static inline void btrfs_dev_stat_inc(struct btrfs_device *dev,
-- 
2.49.0

[PATCH 2/2] btrfs: get rid of btrfs_read_dev_super()

[Qu Wenruo]

The function is introduced by commit a512bbf855ff ("Btrfs: superblock
duplication") at the beginning of btrfs.

It leaves a comment saying we need a special mount option to read all
super blocks, but it's never implemented.

This means btrfs_read_dev_super() is always reading the first super
block, making all the code finding the latest super block unnecessary.

Just remove that function and replace all call sites with
btrfs_read_disk_super(bdev, 0, false).

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 30 +-----------------------------
 fs/btrfs/disk-io.h |  1 -
 fs/btrfs/volumes.c |  2 +-
 3 files changed, 2 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index df68796288a2..a69d33abf44c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3307,7 +3307,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	/*
 	 * Read super block and check the signature bytes only
 	 */
-	disk_super = btrfs_read_dev_super(fs_devices->latest_dev->bdev);
+	disk_super = btrfs_read_disk_super(fs_devices->latest_dev->bdev, 0, false);
 	if (IS_ERR(disk_super)) {
 		ret = PTR_ERR(disk_super);
 		goto fail_alloc;
@@ -3702,34 +3702,6 @@ static void btrfs_end_super_write(struct bio *bio)
 	bio_put(bio);
 }
 
-struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev)
-{
-	struct btrfs_super_block *super, *latest = NULL;
-	int i;
-	u64 transid = 0;
-
-	/* we would like to check all the supers, but that would make
-	 * a btrfs mount succeed after a mkfs from a different FS.
-	 * So, we need to add a special mount option to scan for
-	 * later supers, using BTRFS_SUPER_MIRROR_MAX instead
-	 */
-	for (i = 0; i < 1; i++) {
-		super = btrfs_read_disk_super(bdev, i, false);
-		if (IS_ERR(super))
-			continue;
-
-		if (!latest || btrfs_super_generation(super) > transid) {
-			if (latest)
-				btrfs_release_disk_super(super);
-
-			latest = super;
-			transid = btrfs_super_generation(super);
-		}
-	}
-
-	return super;
-}
-
 /*
  * Write superblock @sb to the @device. Do not wait for completion, all the
  * folios we use for writing are locked.
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 0ac9baf1215f..864a55a96226 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -58,7 +58,6 @@ int btrfs_validate_super(const struct btrfs_fs_info *fs_info,
 			 const struct btrfs_super_block *sb, int mirror_num);
 int btrfs_check_features(struct btrfs_fs_info *fs_info, bool is_rw_mount);
 int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors);
-struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev);
 int btrfs_commit_super(struct btrfs_fs_info *fs_info);
 struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root,
 					const struct btrfs_key *key);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e751bc32e867..57617020ee40 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -493,7 +493,7 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder,
 		}
 	}
 	invalidate_bdev(bdev);
-	*disk_super = btrfs_read_dev_super(bdev);
+	*disk_super = btrfs_read_disk_super(bdev, 0, false);
 	if (IS_ERR(*disk_super)) {
 		ret = PTR_ERR(*disk_super);
 		fput(*bdev_file);
-- 
2.49.0

Re: [PATCH 1/2] btrfs: merge btrfs_read_dev_one_super() into btrfs_read_disk_super()

[Johannes Thumshirn]

On 28.04.25 03:16, Qu Wenruo wrote:
> +	ret = btrfs_sb_log_location_bdev(bdev, copy_num, READ, &bytenr);
> +	if (ret == -ENOENT)
> +		return ERR_PTR(-EINVAL);
> +	else if (ret)
> +		return ERR_PTR(ret);

Nit: else after return is superfluous.

Maybe even:

	if (ret) {
		if (ret == -ENOENT)
			ret = -EINVAL
		return ERR_PTR(ret);
	}

Re: [PATCH 1/2] btrfs: merge btrfs_read_dev_one_super() into btrfs_read_disk_super()

[Qu Wenruo]

在 2025/4/28 23:51, Johannes Thumshirn 写道:
> On 28.04.25 03:16, Qu Wenruo wrote:
>> +	ret = btrfs_sb_log_location_bdev(bdev, copy_num, READ, &bytenr);
>> +	if (ret == -ENOENT)
>> +		return ERR_PTR(-EINVAL);
>> +	else if (ret)
>> +		return ERR_PTR(ret);
> 
> Nit: else after return is superfluous.
> 
> Maybe even:
> 
> 	if (ret) {
> 		if (ret == -ENOENT)
> 			ret = -EINVAL
> 		return ERR_PTR(ret);
> 	}


Sure, I'll modify it at the merge time.

Thanks,
Qu

Re: [PATCH 1/2] btrfs: merge btrfs_read_dev_one_super() into btrfs_read_disk_super()

[David Sterba]

On Mon, Apr 28, 2025 at 10:45:51AM +0930, Qu Wenruo wrote:
> We have two functions to read a super block from a block device:
> 
> - btrfs_read_dev_one_super()
>   Exported from disk-io.c
> 
> - btrfs_read_disk_super()
>   Local to volumes.c
> 
> And they have some minor differences:
> 
> - btrfs_read_dev_one_super() uses @copy_num
>   Meanwhile btrfs_read_disk_super() relies on the physical and expected
>   bytenr passed from the caller.
> 
>   To be hoest, the parameter list of btrfs_read_dev_one_super() is more
>   user friendly.
> 
> - btrfs_read_disk_super() makes sure the label is NUL terminated
> 
> We do not need two different functions doing the same job, so merge the
> behavior into btrfs_read_disk_super() by:
> 
> - Remove btrfs_read_dev_one_super()
> 
> - Export btrfs_read_disk_super()
>   The name pairs with btrfs_release_disk_super() perfectly
> 
> - Change the parameter list of btrfs_read_disk_super() to mimic
>   btrfs_read_dev_one_super()
>   All existing callers are calculating the physical address and expected
>   bytenr before calling btrfs_read_disk_super() already.

Nice, I've checked the history, the functions started as code factored
out of other functions and then updated (API and such) to the point when
they're almost the same.

Re: [PATCH 2/2] btrfs: get rid of btrfs_read_dev_super()

[David Sterba]

On Mon, Apr 28, 2025 at 10:45:52AM +0930, Qu Wenruo wrote:
> The function is introduced by commit a512bbf855ff ("Btrfs: superblock
> duplication") at the beginning of btrfs.
> 
> It leaves a comment saying we need a special mount option to read all
> super blocks, but it's never implemented.
> 
> This means btrfs_read_dev_super() is always reading the first super
> block, making all the code finding the latest super block unnecessary.
> 
> Just remove that function and replace all call sites with
> btrfs_read_disk_super(bdev, 0, false).

I think it's ok to remove it, overwriting the primary superblock is
pointing to a worse problem that may not be simply fixed by starting
from the copies. The rescue tools or check have the ability to use the
copy and start from there.

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 1/2] btrfs: merge btrfs_read_dev_one_super() into btrfs_read_disk_super()

[David Sterba]

On Mon, Apr 28, 2025 at 10:45:51AM +0930, Qu Wenruo wrote:
> -struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
> -						   int copy_num, bool drop_cache)
> -{

> -	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
> -		btrfs_release_disk_super(super);
> -		return ERR_PTR(-ENODATA);
> -	}
> -
> -	if (btrfs_super_bytenr(super) != bytenr_orig) {
> -		btrfs_release_disk_super(super);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
> -	return super;
> -}
> -
> -
>  struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev)
>  {
>  	struct btrfs_super_block *super, *latest = NULL;
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1401,48 +1401,62 @@ void btrfs_release_disk_super(struct btrfs_super_block *super)
>  	put_page(page);
>  }
>  
> -static struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
> -						       u64 bytenr, u64 bytenr_orig)
> +struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
> +						int copy_num, bool drop_cache)
>  {
> -	struct btrfs_super_block *disk_super;
> +	struct btrfs_super_block *super;
>  	struct page *page;
> -	void *p;
> -	pgoff_t index;
> +	u64 bytenr, bytenr_orig;
> +	struct address_space *mapping = bdev->bd_mapping;
> +	int ret;
>  
> -	/* make sure our super fits in the device */
> -	if (bytenr + PAGE_SIZE >= bdev_nr_bytes(bdev))
> +	bytenr_orig = btrfs_sb_offset(copy_num);
> +	ret = btrfs_sb_log_location_bdev(bdev, copy_num, READ, &bytenr);
> +	if (ret == -ENOENT)
> +		return ERR_PTR(-EINVAL);
> +	else if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (bytenr + BTRFS_SUPER_INFO_SIZE >= bdev_nr_bytes(bdev))
>  		return ERR_PTR(-EINVAL);
>  
> -	/* make sure our super fits in the page */
> -	if (sizeof(*disk_super) > PAGE_SIZE)
> -		return ERR_PTR(-EINVAL);
> +	if (drop_cache) {
> +		/* This should only be called with the primary sb. */
> +		ASSERT(copy_num == 0);
>  
> -	/* make sure our super doesn't straddle pages on disk */
> -	index = bytenr >> PAGE_SHIFT;
> -	if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_SHIFT != index)
> -		return ERR_PTR(-EINVAL);
> -
> -	/* pull in the page with our super */
> -	page = read_cache_page_gfp(bdev->bd_mapping, index, GFP_KERNEL);
> +		/*
> +		 * Drop the page of the primary superblock, so later read will
> +		 * always read from the device.
> +		 */
> +		invalidate_inode_pages2_range(mapping,
> +				bytenr >> PAGE_SHIFT,
> +				(bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT);
> +	}
>  
> +	page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);
>  	if (IS_ERR(page))
>  		return ERR_CAST(page);
>  
> -	p = page_address(page);
> +	super = page_address(page);
> +	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
> +		btrfs_release_disk_super(super);
> +		return ERR_PTR(-ENODATA);

This was in the other function but I think we should unify that to
-EINVAL, other filesystems do not distinguish between a failed magic
check and other errors.

Re: [PATCH 1/2] btrfs: merge btrfs_read_dev_one_super() into btrfs_read_disk_super()

[Qu Wenruo]

在 2025/4/29 16:52, David Sterba 写道:
> On Mon, Apr 28, 2025 at 10:45:51AM +0930, Qu Wenruo wrote:
>> -struct btrfs_super_block *btrfs_read_dev_one_super(struct block_device *bdev,
>> -						   int copy_num, bool drop_cache)
>> -{
> 
>> -	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
>> -		btrfs_release_disk_super(super);
>> -		return ERR_PTR(-ENODATA);
>> -	}
>> -
>> -	if (btrfs_super_bytenr(super) != bytenr_orig) {
>> -		btrfs_release_disk_super(super);
>> -		return ERR_PTR(-EINVAL);
>> -	}
>> -
>> -	return super;
>> -}
>> -
>> -
>>   struct btrfs_super_block *btrfs_read_dev_super(struct block_device *bdev)
>>   {
>>   	struct btrfs_super_block *super, *latest = NULL;
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1401,48 +1401,62 @@ void btrfs_release_disk_super(struct btrfs_super_block *super)
>>   	put_page(page);
>>   }
>>   
>> -static struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
>> -						       u64 bytenr, u64 bytenr_orig)
>> +struct btrfs_super_block *btrfs_read_disk_super(struct block_device *bdev,
>> +						int copy_num, bool drop_cache)
>>   {
>> -	struct btrfs_super_block *disk_super;
>> +	struct btrfs_super_block *super;
>>   	struct page *page;
>> -	void *p;
>> -	pgoff_t index;
>> +	u64 bytenr, bytenr_orig;
>> +	struct address_space *mapping = bdev->bd_mapping;
>> +	int ret;
>>   
>> -	/* make sure our super fits in the device */
>> -	if (bytenr + PAGE_SIZE >= bdev_nr_bytes(bdev))
>> +	bytenr_orig = btrfs_sb_offset(copy_num);
>> +	ret = btrfs_sb_log_location_bdev(bdev, copy_num, READ, &bytenr);
>> +	if (ret == -ENOENT)
>> +		return ERR_PTR(-EINVAL);
>> +	else if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	if (bytenr + BTRFS_SUPER_INFO_SIZE >= bdev_nr_bytes(bdev))
>>   		return ERR_PTR(-EINVAL);
>>   
>> -	/* make sure our super fits in the page */
>> -	if (sizeof(*disk_super) > PAGE_SIZE)
>> -		return ERR_PTR(-EINVAL);
>> +	if (drop_cache) {
>> +		/* This should only be called with the primary sb. */
>> +		ASSERT(copy_num == 0);
>>   
>> -	/* make sure our super doesn't straddle pages on disk */
>> -	index = bytenr >> PAGE_SHIFT;
>> -	if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_SHIFT != index)
>> -		return ERR_PTR(-EINVAL);
>> -
>> -	/* pull in the page with our super */
>> -	page = read_cache_page_gfp(bdev->bd_mapping, index, GFP_KERNEL);
>> +		/*
>> +		 * Drop the page of the primary superblock, so later read will
>> +		 * always read from the device.
>> +		 */
>> +		invalidate_inode_pages2_range(mapping,
>> +				bytenr >> PAGE_SHIFT,
>> +				(bytenr + BTRFS_SUPER_INFO_SIZE) >> PAGE_SHIFT);
>> +	}
>>   
>> +	page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, GFP_NOFS);
>>   	if (IS_ERR(page))
>>   		return ERR_CAST(page);
>>   
>> -	p = page_address(page);
>> +	super = page_address(page);
>> +	if (btrfs_super_magic(super) != BTRFS_MAGIC) {
>> +		btrfs_release_disk_super(super);
>> +		return ERR_PTR(-ENODATA);
> 
> This was in the other function but I think we should unify that to
> -EINVAL, other filesystems do not distinguish between a failed magic
> check and other errors.
> 

Sure, merged both super magic and super bytenr checks into the same if 
(), and return ERR_PTR(-EINVAL).

Will push the updated version (with Johannes and this one modified) to 
for-next after getting enough reviews.

Thanks,
Qu