Skip to contentSkip to conversation

btrfs: introduce BTRFS_BLOCK_SIZE to replace number 4096

PATCH In review [email protected] 3 participants 11 messages Apr 30, 2015 – May 11, 2015 Btrfs file system

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/3 btrfs: introduce BTRFS_BLOCK_SIZE to replace number 4096

From: George Wang <[email protected]>

Signed-off-by George Wang <[email protected]>
---
 fs/btrfs/disk-io.c | 28 ++++++++++++++--------------
 fs/btrfs/disk-io.h |  3 ++-
 2 files changed, 16 insertions(+), 15 deletions(-)
fs/btrfs/disk-io.c +14 -14
index 2ef9a4b..342d4fc 100644
@@ -2548,8 +2548,8 @@ int open_ctree(struct super_block *sb,
 	btrfs_init_balance(fs_info);
 	btrfs_init_async_reclaim_work(&fs_info->async_reclaim_work);
 
-	sb->s_blocksize = 4096;
-	sb->s_blocksize_bits = blksize_bits(4096);
+	sb->s_blocksize = BTRFS_BLOCK_SIZE;
+	sb->s_blocksize_bits = blksize_bits(BTRFS_BLOCK_SIZE);
 	sb->s_bdi = &fs_info->bdi;
 
 	btrfs_init_btree_inode(fs_info, tree_root);

@@ -3144,7 +3144,7 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
 		if (bytenr + BTRFS_SUPER_INFO_SIZE >=
 					i_size_read(bdev->bd_inode))
 			break;
-		bh = __bread(bdev, bytenr / 4096,
+		bh = __bread(bdev, bytenr / BTRFS_BLOCK_SIZE,
 					BTRFS_SUPER_INFO_SIZE);
 		if (!bh)
 			continue;

@@ -3199,7 +3199,7 @@ static int write_dev_supers(struct btrfs_device *device,
 			break;
 
 		if (wait) {
-			bh = __find_get_block(device->bdev, bytenr / 4096,
+			bh = __find_get_block(device->bdev, bytenr / BTRFS_BLOCK_SIZE,
 					      BTRFS_SUPER_INFO_SIZE);
 			if (!bh) {
 				errors++;

@@ -3229,7 +3229,7 @@ static int write_dev_supers(struct btrfs_device *device,
 			 * one reference for us, and we leave it for the
 			 * caller
 			 */
-			bh = __getblk(device->bdev, bytenr / 4096,
+			bh = __getblk(device->bdev, bytenr / BTRFS_BLOCK_SIZE,
 				      BTRFS_SUPER_INFO_SIZE);
 			if (!bh) {
 				printk(KERN_ERR "BTRFS: couldn't get super "

@@ -3904,13 +3904,13 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
 	 * The common minimum, we don't know if we can trust the nodesize/sectorsize
 	 * items yet, they'll be verified later. Issue just a warning.
 	 */
-	if (!IS_ALIGNED(btrfs_super_root(sb), 4096))
+	if (!IS_ALIGNED(btrfs_super_root(sb), BTRFS_BLOCK_SIZE))
 		printk(KERN_WARNING "BTRFS: tree_root block unaligned: %llu\n",
 				btrfs_super_root(sb));
-	if (!IS_ALIGNED(btrfs_super_chunk_root(sb), 4096))
+	if (!IS_ALIGNED(btrfs_super_chunk_root(sb), BTRFS_BLOCK_SIZE))
 		printk(KERN_WARNING "BTRFS: chunk_root block unaligned: %llu\n",
 				btrfs_super_chunk_root(sb));
-	if (!IS_ALIGNED(btrfs_super_log_root(sb), 4096))
+	if (!IS_ALIGNED(btrfs_super_log_root(sb), BTRFS_BLOCK_SIZE))
 		printk(KERN_WARNING "BTRFS: log_root block unaligned: %llu\n",
 				btrfs_super_log_root(sb));
 

@@ -3918,14 +3918,14 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
 	 * Check the lower bound, the alignment and other constraints are
 	 * checked later.
 	 */
-	if (btrfs_super_nodesize(sb) < 4096) {
-		printk(KERN_ERR "BTRFS: nodesize too small: %u < 4096\n",
-				btrfs_super_nodesize(sb));
+	if (btrfs_super_nodesize(sb) < BTRFS_BLOCK_SIZE) {
+		printk(KERN_ERR "BTRFS: nodesize too small: %u < %d\n",
+				btrfs_super_nodesize(sb), BTRFS_BLOCK_SIZE);
 		ret = -EINVAL;
 	}
-	if (btrfs_super_sectorsize(sb) < 4096) {
-		printk(KERN_ERR "BTRFS: sectorsize too small: %u < 4096\n",
-				btrfs_super_sectorsize(sb));
+	if (btrfs_super_sectorsize(sb) < BTRFS_BLOCK_SIZE) {
+		printk(KERN_ERR "BTRFS: sectorsize too small: %u < %d\n",
+				btrfs_super_sectorsize(sb), BTRFS_BLOCK_SIZE);
 		ret = -EINVAL;
 	}
fs/btrfs/disk-io.h +2 -1
index d4cbfee..4d246ff 100644
@@ -19,8 +19,9 @@
 #ifndef __DISKIO__
 #define __DISKIO__
 
+#define BTRFS_BLOCK_SIZE 4096
 #define BTRFS_SUPER_INFO_OFFSET (64 * 1024)
-#define BTRFS_SUPER_INFO_SIZE 4096
+#define BTRFS_SUPER_INFO_SIZE BTRFS_BLOCK_SIZE
 
 #define BTRFS_SUPER_MIRROR_MAX	 3
 #define BTRFS_SUPER_MIRROR_SHIFT 12
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH 2/3 btrfs: support to find missing device by path

From: George Wang <[email protected]>

First try to find the device matches specified device path, if nothing, then find the device by (devid, dev_uuid). This can fix the regression for replacing an offline device which path is held in btrfs_device.

Signed-off-by George Wang <[email protected]>
---
 fs/btrfs/volumes.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)
fs/btrfs/volumes.c +44 -2
index 8bcd2a0..c8ece13 100644
@@ -1888,8 +1888,37 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 	mutex_unlock(&uuid_mutex);
 }
 
-static int btrfs_find_device_by_path(struct btrfs_root *root, char *device_path,
-				     struct btrfs_device **device)
+
+/*
+ * Find specified device in fs_devices, caller must hold volume_mutex.
+ * Use @device_path as the key, do not check the devid and dev_uuid. Depends
+ * on usage, the caller may do more checking for ret device.
+ */
+static int __fast_find_device_by_path(struct btrfs_root *root, char *device_path,
+                                       struct btrfs_device **device)
+{
+	struct list_head *devices;
+	struct btrfs_device *tmp;
+	char *name;
+
+	devices = &root->fs_info->fs_devices->devices;
+
+	list_for_each_entry(tmp, devices, dev_list) {
+		name = rcu_str_deref(tmp->name);
+		if (tmp && 0 == strcmp(name, device_path)) {
+			*device = tmp;
+			return 0;
+		}
+	}
+
+       return -ENOENT;
+}
+
+/*
+ * Real read (devid, dev_uuid) from device, then find it in fs_devices
+ */
+static int __slow_find_device_by_path(struct btrfs_root *root, char *device_path,
+				struct btrfs_device **device)
 {
 	int ret = 0;
 	struct btrfs_super_block *disk_super;

@@ -1915,6 +1944,19 @@ static int btrfs_find_device_by_path(struct btrfs_root *root, char *device_path,
 	return ret;
 }
 
+ 
+static int btrfs_find_device_by_path(struct btrfs_root *root, char *device_path,
+			struct btrfs_device **device)
+{
+	int ret;
+
+	ret = __fast_find_device_by_path(root, device_path, device);
+	if (ret)
+		ret = __slow_find_device_by_path(root, device_path, device);
+
+	return ret;
+}
+
 int btrfs_find_device_missing_or_by_path(struct btrfs_root *root,
 					 char *device_path,
 					 struct btrfs_device **device)
Signature · 1 line
1.9.3
#2 lore raw

Re: [PATCH 2/3] btrfs: support to find missing device by path

Quoted 5 lines · On Thu, Apr 30, 2015 at 05:07:24PM +0800, [email protected] wrote:

From: George Wang <[email protected]>

First try to find the device matches specified device path, if nothing, then find the device by (devid, dev_uuid). This can fix the regression for replacing an offline device which path is held in btrfs_device.

I vaguely remember similar patches sent by Anand, CCed.

#3 lore raw

Re: [PATCH 2/3] btrfs: support to find missing device by path

Quoted 8 lines · David Sterba
Quoted 5 lines · On Thu, Apr 30, 2015 at 05:07:24PM +0800, [email protected] wrote:

From: George Wang <[email protected]>

First try to find the device matches specified device path, if nothing, then find the device by (devid, dev_uuid). This can fix the regression for replacing an offline device which path is held in btrfs_device.

I vaguely remember similar patches sent by Anand, CCed.

George,

Critically we don't need this patch. right ?
Anyway user of replace cli can use devid if device read fails.

I think David is talking about:
[PATCH] device delete by devid

it was critical for device delete. since there was device delete by devid. I used device delete by devid instead of device path strcmp mainly because to maintain consistency between device replace and delete.
the above patch set also provides code cleanups between device replace and delete codes.

Thanks, Anand

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw

Re: [PATCH 2/3] btrfs: support to find missing device by path

fix typo. (I have to blame thunderbird's bug which vanishes some of the words as I scroll up and down in the 'write' window.).

Quoted 39 lines · Anand Jain
Quoted 9 lines · David Sterba
Quoted 6 lines · On Thu, Apr 30, 2015 at 05:07:24PM +0800, [email protected] wrote:

From: George Wang <[email protected]>

First try to find the device matches specified device path, if nothing, then
find the device by (devid, dev_uuid). This can fix the regression for replacing an offline device which path is held in btrfs_device.

I vaguely remember similar patches sent by Anand, CCed.

George,

Critically we don't need this patch. right ?
Anyway user of replace cli can use devid if device read fails.

I think David is talking about:
[PATCH] device delete by devid

it was critical for device delete. since there wasn't device delete by devid. I used device delete by devid instead of device path strcmp mainly because to maintain consistency between device replace and delete.
the above patch set also provides code cleanups between device replace and delete codes.

Thanks, Anand

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#5 lore raw

Re: [PATCH 2/3] btrfs: support to find missing device by path

Thanks for reply.

Quoted 27 lines · Anand Jain

fix typo. (I have to blame thunderbird's bug which vanishes some of the words as I scroll up and down in the 'write' window.).

Quoted 21 lines · Anand Jain
Quoted 5 lines · David Sterba
Quoted 3 lines · On Thu, Apr 30, 2015 at 05:07:24PM +0800, [email protected] wrote:

From: George Wang <[email protected]>

Quoted 7 lines
Quoted 4 lines

First try to find the device matches specified device path, if nothing, then
find the device by (devid, dev_uuid). This can fix the regression for replacing an offline device which path is held in btrfs_device.

I vaguely remember similar patches sent by Anand, CCed.

George,

Critically we don't need this patch. right ?

Yes, I agree it.

Quoted 4 lines
Quoted 4 lines

Anyway user of replace cli can use devid if device read fails.

I think David is talking about:
[PATCH] device delete by devid

We can delete the device by devid on behalf of "btrfs_find_device". In my opinion, the dev path is easier and humanized to use. This was OK before, but now I can not replace offline device by path. So I consider it as a regression.

Anyway, maybe we should support replace offline device by path in the future.

Thanks,

George

Quoted 9 lines
Quoted 9 lines

it was critical for device delete. since there wasn't device delete by devid. I used device delete by devid instead of device path strcmp mainly because to maintain consistency between device replace and delete.
the above patch set also provides code cleanups between device replace and delete codes.

Thanks, Anand

#6 lore raw

Re: [PATCH 2/3] btrfs: support to find missing device by path

Quoted 10 lines
Quoted 4 lines
Quoted 4 lines

Critically we don't need this patch. right ?
Anyway user of replace cli can use devid if device read fails.

Yes, I agree it.

Quoted 3 lines
Quoted 3 lines

I think David is talking about:
[PATCH] device delete by devid

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines

it was critical for device delete. since there wasn't device delete by devid. I used device delete by devid instead of device path strcmp mainly because to maintain consistency between device replace and delete.
the above patch set also provides code cleanups between device replace and delete codes.

We can delete the device by devid on behalf of "btrfs_find_device".

yes. patch-set (above) uses btrfs_find_device for device delete now. replace was already using it.

Quoted 1 line

In my opinion, the dev path is easier and humanized to use.

yes. good to have. in the long run. But not a regression/critical. this will conflict with my patch, can you rebase on top of above path which has some cleanups in this area as well.

Quoted 3 lines

This was OK before, but now I can not replace offline device by path.
So I consider it as a regression.

You mean to say you could replace the offline device using the device path before (not devid) and now you can't ?

Then what patch introduced the regression ? Do you see any older version replace working with offline device using the device path ?

Thanks, Anand

author
#7 lore raw

Re: [PATCH 2/3] btrfs: support to find missing device by path

Quoted 21 lines · Anand Jain
Quoted 19 lines
Quoted 4 lines
Quoted 4 lines

Critically we don't need this patch. right ?
Anyway user of replace cli can use devid if device read fails.

Yes, I agree it.

Quoted 9 lines
Quoted 9 lines

I think David is talking about:
[PATCH] device delete by devid

it was critical for device delete. since there wasn't device delete by devid. I used device delete by devid instead of device path strcmp mainly because to maintain consistency between device replace and delete.
the above patch set also provides code cleanups between device replace and delete codes.

Quoted 1 line

We can delete the device by devid on behalf of "btrfs_find_device".

Quoted 12 lines

yes. patch-set (above) uses btrfs_find_device for device delete now. replace was already using it.

Quoted 1 line

In my opinion, the dev path is easier and humanized to use.

yes. good to have. in the long run. But not a regression/critical. this will conflict with my patch, can you rebase on top of above path which has some cleanups in this area as well.

Quoted 3 lines

This was OK before, but now I can not replace offline device by path.
So I consider it as a regression.

Oh, sorry, my fault. NOT regression, It should always be the srcdevid.
I confused with another stuff.

Thanks,

George

Quoted 9 lines

You mean to say you could replace the offline device using the device path before (not devid) and now you can't ?

Then what patch introduced the regression ? Do you see any older version replace working with offline device using the device path ?

Thanks, Anand

author
#8 lore raw

PATCH 3/3 btrfs: do not allow device path updated by the stale one

In reply to [email protected]

From: George Wang <[email protected]>

Use the btrfs_device->generation to identify which device(with same devid and dev_uuid) is newer, and never allow the path updated by the stale one. The btrfs_device->generation is corresponded with transaction id, which is increased from old to new. Whether the fs_devices opened or not, stale device can never get effective.

Signed-off-by George Wang <[email protected]>
---
 fs/btrfs/disk-io.c | 3 +++
 fs/btrfs/volumes.c | 5 ++---
 2 files changed, 5 insertions(+), 3 deletions(-)
fs/btrfs/disk-io.c +3 -0
index 342d4fc..25faa53 100644
@@ -3508,6 +3508,9 @@ static int write_all_supers(struct btrfs_root *root, int max_mirrors)
 		if (!dev->in_fs_metadata || !dev->writeable)
 			continue;
 
+		/* update device generation according to disk super */
+		dev->generation = sb->generation;
+
 		btrfs_set_stack_device_generation(dev_item, 0);
 		btrfs_set_stack_device_type(dev_item, dev->type);
 		btrfs_set_stack_device_id(dev_item, dev->devid);
fs/btrfs/volumes.c +2 -3
index c8ece13..de6968d 100644
@@ -525,10 +525,9 @@ static noinline int device_list_add(const char *path,
 		 * tracking a problem where systems fail mount by subvolume id
 		 * when we reject replacement on a mounted FS.
 		 */
-		if (!fs_devices->opened && found_transid < device->generation) {
+		if (found_transid < device->generation) {
 			/*
-			 * That is if the FS is _not_ mounted and if you
-			 * are here, that means there is more than one
+			 * If you are here, that means there is more than one
 			 * disk with same uuid and devid.We keep the one
 			 * with larger generation number or the last-in if
 			 * generation are equal.
Signature · 1 line
1.9.3
#9 lore raw
In reply to [email protected]

General comment: everywhere the superblock "4096" is used, the right replacement is BTRFS_SUPER_INFO_SIZE

Quoted 14 lines · On Thu, Apr 30, 2015 at 05:07:23PM +0800, [email protected] wrote:
fs/btrfs/disk-io.c +2 -2
@@ -2548,8 +2548,8 @@ int open_ctree(struct super_block *sb,
 	btrfs_init_balance(fs_info);
 	btrfs_init_async_reclaim_work(&fs_info->async_reclaim_work);
 
-	sb->s_blocksize = 4096;
-	sb->s_blocksize_bits = blksize_bits(4096);
+	sb->s_blocksize = BTRFS_BLOCK_SIZE;
+	sb->s_blocksize_bits = blksize_bits(BTRFS_BLOCK_SIZE);
 	sb->s_bdi = &fs_info->bdi;
 
 	btrfs_init_btree_inode(fs_info, tree_root);

fs/btrfs/disk-io.c struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) · line 3147

Quoted 5 lines
fs/btrfs/disk-io.c +1 -1
@@ -3144,7 +3144,7 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
 		if (bytenr + BTRFS_SUPER_INFO_SIZE >=
 					i_size_read(bdev->bd_inode))
 			break;
-		bh = __bread(bdev, bytenr / 4096,
+		bh = __bread(bdev, bytenr / BTRFS_BLOCK_SIZE,

That one is really BTRFS_SUPER_INFO_SIZE

fs/btrfs/disk-io.c static int write_dev_supers(struct btrfs_device *device, · line 3202

Quoted 9 lines
fs/btrfs/disk-io.c +1 -1
 					BTRFS_SUPER_INFO_SIZE);
 		if (!bh)
 			continue;

@@ -3199,7 +3199,7 @@ static int write_dev_supers(struct btrfs_device *device,
 			break;
 
 		if (wait) {
-			bh = __find_get_block(device->bdev, bytenr / 4096,
+			bh = __find_get_block(device->bdev, bytenr / BTRFS_BLOCK_SIZE,

same here

fs/btrfs/disk-io.c static int write_dev_supers(struct btrfs_device *device, · line 3232

Quoted 9 lines
fs/btrfs/disk-io.c +1 -1
 					      BTRFS_SUPER_INFO_SIZE);
 			if (!bh) {
 				errors++;

@@ -3229,7 +3229,7 @@ static int write_dev_supers(struct btrfs_device *device,
 			 * one reference for us, and we leave it for the
 			 * caller
 			 */
-			bh = __getblk(device->bdev, bytenr / 4096,
+			bh = __getblk(device->bdev, bytenr / BTRFS_BLOCK_SIZE,

same here

Quoted 9 lines
fs/btrfs/disk-io.c +1 -1
 				      BTRFS_SUPER_INFO_SIZE);
 			if (!bh) {
 				printk(KERN_ERR "BTRFS: couldn't get super "

@@ -3904,13 +3904,13 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
 	 * The common minimum, we don't know if we can trust the nodesize/sectorsize
 	 * items yet, they'll be verified later. Issue just a warning.
 	 */
-	if (!IS_ALIGNED(btrfs_super_root(sb), 4096))
+	if (!IS_ALIGNED(btrfs_super_root(sb), BTRFS_BLOCK_SIZE))

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, · line 3915

Quoted 10 lines
fs/btrfs/disk-io.c +2 -2
 		printk(KERN_WARNING "BTRFS: tree_root block unaligned: %llu\n",
 				btrfs_super_root(sb));
-	if (!IS_ALIGNED(btrfs_super_chunk_root(sb), 4096))
+	if (!IS_ALIGNED(btrfs_super_chunk_root(sb), BTRFS_BLOCK_SIZE))
 		printk(KERN_WARNING "BTRFS: chunk_root block unaligned: %llu\n",
 				btrfs_super_chunk_root(sb));
-	if (!IS_ALIGNED(btrfs_super_log_root(sb), 4096))
+	if (!IS_ALIGNED(btrfs_super_log_root(sb), BTRFS_BLOCK_SIZE))
 		printk(KERN_WARNING "BTRFS: log_root block unaligned: %llu\n",
 				btrfs_super_log_root(sb));

Throughout btrfs_check_super_valid 4096 is used as a minimum block size and the macro should be more descriptive.

Quoted 27 lines
fs/btrfs/disk-io.c +6 -6
@@ -3918,14 +3918,14 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
 	 * Check the lower bound, the alignment and other constraints are
 	 * checked later.
 	 */
-	if (btrfs_super_nodesize(sb) < 4096) {
-		printk(KERN_ERR "BTRFS: nodesize too small: %u < 4096\n",
-				btrfs_super_nodesize(sb));
+	if (btrfs_super_nodesize(sb) < BTRFS_BLOCK_SIZE) {
+		printk(KERN_ERR "BTRFS: nodesize too small: %u < %d\n",
+				btrfs_super_nodesize(sb), BTRFS_BLOCK_SIZE);
 		ret = -EINVAL;
 	}
-	if (btrfs_super_sectorsize(sb) < 4096) {
-		printk(KERN_ERR "BTRFS: sectorsize too small: %u < 4096\n",
-				btrfs_super_sectorsize(sb));
+	if (btrfs_super_sectorsize(sb) < BTRFS_BLOCK_SIZE) {
+		printk(KERN_ERR "BTRFS: sectorsize too small: %u < %d\n",
+				btrfs_super_sectorsize(sb), BTRFS_BLOCK_SIZE);
 		ret = -EINVAL;
 	}
fs/btrfs/disk-io.h +0 -0
index d4cbfee..4d246ff 100644

fs/btrfs/disk-io.h line 24

Quoted 7 lines
fs/btrfs/disk-io.h +2 -1
@@ -19,8 +19,9 @@
 #ifndef __DISKIO__
 #define __DISKIO__
 
+#define BTRFS_BLOCK_SIZE 4096
 #define BTRFS_SUPER_INFO_OFFSET (64 * 1024)
-#define BTRFS_SUPER_INFO_SIZE 4096
+#define BTRFS_SUPER_INFO_SIZE BTRFS_BLOCK_SIZE

This should stay 4096, super info size is defined as 4096, block size can vary according to page size.

IOW, I don't see the point to introduce BTRFS_BLOCK_SIZE, it should be something like BTRFS_MIN_BLOCK_SIZE and used in the validation functions.

author
#10 lore raw

Thanks for review.

Quoted 3 lines · David Sterba

General comment: everywhere the superblock "4096" is used, the right replacement is BTRFS_SUPER_INFO_SIZE

I think it depends on the context. BTRFS_SUPER_INFO_SIZE means the super block size for btrfs, which may not be considered as block size logically.

Quoted 13 lines · On Thu, Apr 30, 2015 at 05:07:23PM +0800, [email protected] wrote:
Quoted 12 lines
fs/btrfs/disk-io.c +2 -2
@@ -2548,8 +2548,8 @@ int open_ctree(struct super_block *sb,
      btrfs_init_balance(fs_info);
      btrfs_init_async_reclaim_work(&fs_info->async_reclaim_work);
 
-     sb->s_blocksize = 4096;
-     sb->s_blocksize_bits = blksize_bits(4096);
+     sb->s_blocksize = BTRFS_BLOCK_SIZE;
+     sb->s_blocksize_bits = blksize_bits(BTRFS_BLOCK_SIZE);
      sb->s_bdi = &fs_info->bdi;
Quoted 9 lines

fs/btrfs/disk-io.c struct buffer_head *btrfs_read_dev_super(struct block_device *bdev) · line 3147

Quoted 7 lines
fs/btrfs/disk-io.c +1 -1
      btrfs_init_btree_inode(fs_info, tree_root);

@@ -3144,7 +3144,7 @@ struct buffer_head *btrfs_read_dev_super(struct block_device *bdev)
              if (bytenr + BTRFS_SUPER_INFO_SIZE >=
                                      i_size_read(bdev->bd_inode))
                      break;
-             bh = __bread(bdev, bytenr / 4096,
+             bh = __bread(bdev, bytenr / BTRFS_BLOCK_SIZE,

That one is really BTRFS_SUPER_INFO_SIZE

Here we call the __bread->__bread_gfp->__getblk_gfp, and the block is based on bdev->bd_inode->i_blkbits, which is initialized by set_blocksize in volumes.c of btrfs codes. This means the param block is the block nr based on the bdev, not the BTRFS_SUPER_INFO_SIZE.(I mean logically, because not all of them are 4096)

So I think "bytenr / BTRFS_MIN_BLOCK_SIZE" or "bytenr / block_size(bdev)" may be more accurate.

Quoted 7 lines
Quoted 7 lines
fs/btrfs/disk-io.c +0 -0
                                      BTRFS_SUPER_INFO_SIZE);
              if (!bh)
                      continue;

@@ -3199,7 +3199,7 @@ static int write_dev_supers(struct btrfs_device *device,
                      break;
Quoted 5 lines

fs/btrfs/disk-io.c static int write_dev_supers(struct btrfs_device *device, · line 3202

Quoted 3 lines
fs/btrfs/disk-io.c +1 -1
              if (wait) {
-                     bh = __find_get_block(device->bdev, bytenr / 4096,
+                     bh = __find_get_block(device->bdev, bytenr / BTRFS_BLOCK_SIZE,

same here

The __find_get_block also in the same context, the 2nd param block nr is based on
the blocksize of bdev.

Quoted 12 lines

fs/btrfs/disk-io.c static int write_dev_supers(struct btrfs_device *device, · line 3232

Quoted 10 lines
fs/btrfs/disk-io.c +1 -1
                                            BTRFS_SUPER_INFO_SIZE);
                      if (!bh) {
                              errors++;

@@ -3229,7 +3229,7 @@ static int write_dev_supers(struct btrfs_device *device,
                       * one reference for us, and we leave it for the
                       * caller
                       */
-                     bh = __getblk(device->bdev, bytenr / 4096,
+                     bh = __getblk(device->bdev, bytenr / BTRFS_BLOCK_SIZE,

same here

I think here is the same.

Quoted 15 lines
Quoted 15 lines
fs/btrfs/disk-io.c +2 -2
                                    BTRFS_SUPER_INFO_SIZE);
                      if (!bh) {
                              printk(KERN_ERR "BTRFS: couldn't get super "

@@ -3904,13 +3904,13 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
       * The common minimum, we don't know if we can trust the nodesize/sectorsize
       * items yet, they'll be verified later. Issue just a warning.
       */
-     if (!IS_ALIGNED(btrfs_super_root(sb), 4096))
+     if (!IS_ALIGNED(btrfs_super_root(sb), BTRFS_BLOCK_SIZE))
              printk(KERN_WARNING "BTRFS: tree_root block unaligned: %llu\n",
                              btrfs_super_root(sb));
-     if (!IS_ALIGNED(btrfs_super_chunk_root(sb), 4096))
+     if (!IS_ALIGNED(btrfs_super_chunk_root(sb), BTRFS_BLOCK_SIZE))
              printk(KERN_WARNING "BTRFS: chunk_root block unaligned: %llu\n",
Quoted 8 lines

fs/btrfs/disk-io.c static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info, · line 3915

Quoted 5 lines
fs/btrfs/disk-io.c +1 -1
                              btrfs_super_chunk_root(sb));
-     if (!IS_ALIGNED(btrfs_super_log_root(sb), 4096))
+     if (!IS_ALIGNED(btrfs_super_log_root(sb), BTRFS_BLOCK_SIZE))
              printk(KERN_WARNING "BTRFS: log_root block unaligned: %llu\n",
                              btrfs_super_log_root(sb));

Throughout btrfs_check_super_valid 4096 is used as a minimum block size and the macro should be more descriptive.

Yes, I will convert the BTRFS_BLOCK_SIZE to BTRFS_MIN_BLOCK_SIZE.

Quoted 30 lines
Quoted 30 lines
fs/btrfs/disk-io.c +6 -6
@@ -3918,14 +3918,14 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
       * Check the lower bound, the alignment and other constraints are
       * checked later.
       */
-     if (btrfs_super_nodesize(sb) < 4096) {
-             printk(KERN_ERR "BTRFS: nodesize too small: %u < 4096\n",
-                             btrfs_super_nodesize(sb));
+     if (btrfs_super_nodesize(sb) < BTRFS_BLOCK_SIZE) {
+             printk(KERN_ERR "BTRFS: nodesize too small: %u < %d\n",
+                             btrfs_super_nodesize(sb), BTRFS_BLOCK_SIZE);
              ret = -EINVAL;
      }
-     if (btrfs_super_sectorsize(sb) < 4096) {
-             printk(KERN_ERR "BTRFS: sectorsize too small: %u < 4096\n",
-                             btrfs_super_sectorsize(sb));
+     if (btrfs_super_sectorsize(sb) < BTRFS_BLOCK_SIZE) {
+             printk(KERN_ERR "BTRFS: sectorsize too small: %u < %d\n",
+                             btrfs_super_sectorsize(sb), BTRFS_BLOCK_SIZE);
              ret = -EINVAL;
      }
fs/btrfs/disk-io.h +0 -0
index d4cbfee..4d246ff 100644
@@ -19,8 +19,9 @@
 #ifndef __DISKIO__
 #define __DISKIO__
Quoted 8 lines

fs/btrfs/disk-io.h line 24

Quoted 4 lines
fs/btrfs/disk-io.h +2 -1
+#define BTRFS_BLOCK_SIZE 4096
 #define BTRFS_SUPER_INFO_OFFSET (64 * 1024)
-#define BTRFS_SUPER_INFO_SIZE 4096
+#define BTRFS_SUPER_INFO_SIZE BTRFS_BLOCK_SIZE

This should stay 4096, super info size is defined as 4096, block size can vary according to page size.

Yes, the BTRFS_SUPER_INFO_SIZE should irrelevant to the BTRFS_BLOCK_SIZE.

Quoted 3 lines

IOW, I don't see the point to introduce BTRFS_BLOCK_SIZE, it should be something like BTRFS_MIN_BLOCK_SIZE and used in the validation functions.

Yes, I will use the BTRFS_MIN_BLOCK_SIZE.