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

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

[xuw2015]

From: George Wang <xuw2015@gmail.com>

Signed-off-by: George Wang <xuw2015@gmail.com>
---
 fs/btrfs/disk-io.c | 28 ++++++++++++++--------------
 fs/btrfs/disk-io.h |  3 ++-
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2ef9a4b..342d4fc 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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;
 	}
 
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index d4cbfee..4d246ff 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -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
-- 
1.9.3

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

[xuw2015]

From: George Wang <xuw2015@gmail.com>

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 <xuw2015@gmail.com>
---
 fs/btrfs/volumes.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 8bcd2a0..c8ece13 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -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)
-- 
1.9.3

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

[David Sterba]

On Thu, Apr 30, 2015 at 05:07:24PM +0800, xuw2015@gmail.com wrote:
> From: George Wang <xuw2015@gmail.com>
> 
> 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.

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

[Anand Jain]

On 05/05/2015 11:38 PM, David Sterba wrote:
> On Thu, Apr 30, 2015 at 05:07:24PM +0800, xuw2015@gmail.com wrote:
>> From: George Wang <xuw2015@gmail.com>
>>
>> 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

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

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

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

On 05/06/2015 05:15 PM, Anand Jain wrote:
>
>
> On 05/05/2015 11:38 PM, David Sterba wrote:
>> On Thu, Apr 30, 2015 at 05:07:24PM +0800, xuw2015@gmail.com wrote:
>>> From: George Wang <xuw2015@gmail.com>
>>>
>>> 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
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[George Wang]

Thanks for reply.

On Wed, May 6, 2015 at 5:22 PM, Anand Jain <Anand.Jain@oracle.com> wrote:
>
>
> 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.).
>
> On 05/06/2015 05:15 PM, Anand Jain wrote:
>>
>>
>>
>> On 05/05/2015 11:38 PM, David Sterba wrote:
>>>
>>> On Thu, Apr 30, 2015 at 05:07:24PM +0800, xuw2015@gmail.com wrote:
>>>>
>>>> From: George Wang <xuw2015@gmail.com>
>>>>
>>>> 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.

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

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

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

[Anand Jain]

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

 > Yes, I agree it.

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

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

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

 > 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

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

[George Wang]

On Thu, May 7, 2015 at 11:17 AM, Anand Jain <Anand.Jain@oracle.com> wrote:
>
>
>>>>    Critically we don't need this patch. right ?
>>>>    Anyway user of replace cli can use devid if device read fails.
>
>
>> Yes, I agree it.
>
>>>>    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.
>
>
>> 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.
>
>> 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.
>
>> 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
>
>  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

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

[xuw2015]

From: George Wang <xuw2015@gmail.com>

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 <xuw2015@gmail.com>
---
 fs/btrfs/disk-io.c | 3 +++
 fs/btrfs/volumes.c | 5 ++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 342d4fc..25faa53 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c8ece13..de6968d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -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.
-- 
1.9.3

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

[David Sterba]

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

On Thu, Apr 30, 2015 at 05:07:23PM +0800, xuw2015@gmail.com wrote:
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -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,

That one is really BTRFS_SUPER_INFO_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,

same here

>  					      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

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

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

>  
> @@ -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;
>  	}
>  
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index d4cbfee..4d246ff 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -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.

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

[George Wang]

Thanks for review.

On Tue, May 5, 2015 at 11:34 PM, David Sterba <dsterba@suse.cz> wrote:
> 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.

> On Thu, Apr 30, 2015 at 05:07:23PM +0800, xuw2015@gmail.com wrote:
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -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,
>
> 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.

>
>>                                       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
The __find_get_block also in the same context, the 2nd param block nr
is based on
the blocksize of bdev.

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

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

>>
>> @@ -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;
>>       }
>>
>> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
>> index d4cbfee..4d246ff 100644
>> --- a/fs/btrfs/disk-io.h
>> +++ b/fs/btrfs/disk-io.h
>> @@ -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.
>
Yes, the BTRFS_SUPER_INFO_SIZE should irrelevant to the
BTRFS_BLOCK_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.
Yes, I will use the BTRFS_MIN_BLOCK_SIZE.