Re: [PATCH v6 1/4] btrfs: store chunk size in space-info struct.

[Wang Yugui <wangyugui@e16-tech.com>]

[-- Attachment #1: Type: text/plain, Size: 3141 bytes --]

Hi,

attachement file(folded-v2.patch):

changes:
   move ASSERT(space_info) into btrfs_update_space_info_chunk_size();

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2022/07/29

> Hi,
> 
> I tried to fold my fix/comment into the attachement file(folded.patch).
> 
> Just the following is new, others are just orig feature or refactor.
> 
> +	if(ctl->max_stripe_size > ctl->max_chunk_size)
> +		ctl->max_stripe_size = ctl->max_chunk_size;
> 
> Best Regards
> Wang Yugui (wangyugui@e16-tech.com)
> 2022/07/29
> 
> > On Tue, Jul 26, 2022 at 06:25:38AM +0800, Wang Yugui wrote:
> > > > On Sat, Jul 23, 2022 at 07:49:37AM +0800, Wang Yugui wrote:
> > > > > In this patch, the max chunk size is changed from 
> > > > > BTRFS_MAX_DATA_CHUNK_SIZE(10G) to SZ_1G without any comment ?
> > > > 
> > > > The patch hasn't been merged, the change from 1G to 10G without proper
> > > > evaluation won't happen. The sysfs knob is available for users who want
> > > > to test it or know that the non-default value works in their
> > > > environment.
> > > 
> > > this patch is in misc-next( 5.19.0-rc8 based, 5.19.0-rc7 based) now.
> > > 
> > > 5.19.0-rc8 based:
> > > f6fca3917b4d btrfs: store chunk size in space-info struct
> > >
> > > The sysfs knob show that current default chunk size is 1G, not 10G as
> > > older version.
> > 
> > So there are two things regarding chunk size, the default size and that
> > it's settable by user (with some limitations). I was replying to the
> > default size change while you are concerned about the max_chunk_size.
> > 
> > You're right that the value changed in the patch, but as I'm reading the
> > code it should not have any effect. When user sets a value in
> > btrfs_chunk_size_store() it's limited inside the sysfs handler to the
> > 10G. Also there are various adjustments when the chunk size is
> > initialized (init_alloc_chunk_ctl_policy_regular).
> > 
> > The only difference I can see comparing master and misc-next is in
> > decide_stripe_size_regular()
> > 
> > 5259         /*
> > 5260          * Use the number of data stripes to figure out how big this chunk is
> > 5261          * really going to be in terms of logical address space, and compare
> > 5262          * that answer with the max chunk size. If it's higher, we try to
> > 5263          * reduce stripe_size.
> > 5264          */
> > 5265         if (ctl->stripe_size * data_stripes > ctl->max_chunk_size) {
> > ^^^^
> > 5266                 /*
> > 5267                  * Reduce stripe_size, round it up to a 16MB boundary again and
> > 5268                  * then use it, unless it ends up being even bigger than the
> > 5269                  * previous value we had already.
> > 5270                  */
> > 5271                 ctl->stripe_size = min(round_up(div_u64(ctl->max_chunk_size,
> > 5272                                                         data_stripes), SZ_16M),
> > 5273                                        ctl->stripe_size);
> > 5274         }
> > 
> > Here it could lead to a different stripe_size when max_chunk_size would
> > be 1G vs 10G, though the other adjustments could change the upper value.
> 


[-- Attachment #2: folded-v2.patch --]
[-- Type: application/octet-stream, Size: 3406 bytes --]

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 2cf8da1116eb..bfe884f13934 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -187,6 +187,16 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info *info)
  */
 #define BTRFS_DEFAULT_ZONED_RECLAIM_THRESH			(75)
 
+/*
+ * Update default chunk size.
+ */
+void btrfs_update_space_info_chunk_size(struct btrfs_space_info *space_info,
+					u64 chunk_size)
+{
+	ASSERT(space_info);
+	WRITE_ONCE(space_info->chunk_size, chunk_size);
+}
+
 static int create_space_info(struct btrfs_fs_info *info, u64 flags)
 {
 
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index c096695598c1..e7de24a529cf 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -25,6 +25,8 @@ struct btrfs_space_info {
 	u64 max_extent_size;	/* This will hold the maximum extent size of
 				   the space info if we had an ENOSPC in the
 				   allocator. */
+	/* Chunk size in bytes */
+	u64 chunk_size;
 
 	/*
 	 * Once a block group drops below this threshold (percents) we'll
@@ -123,6 +125,8 @@ void btrfs_update_space_info(struct btrfs_fs_info *info, u64 flags,
 			     u64 total_bytes, u64 bytes_used,
 			     u64 bytes_readonly, u64 bytes_zone_unusable,
 			     struct btrfs_space_info **space_info);
+void btrfs_update_space_info_chunk_size(struct btrfs_space_info *space_info,
+					u64 chunk_size);
 struct btrfs_space_info *btrfs_find_space_info(struct btrfs_fs_info *info,
 					       u64 flags);
 u64 __pure btrfs_space_info_used(struct btrfs_space_info *s_info,
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a2bb0928dc06..b7b7d254ecf0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5072,6 +5072,7 @@ static void init_alloc_chunk_ctl_policy_regular(
 				struct alloc_chunk_ctl *ctl)
 {
 	u64 type = ctl->type;
+	struct btrfs_space_info *space_info;
 
 	if (type & BTRFS_BLOCK_GROUP_DATA) {
 		ctl->max_stripe_size = SZ_1G;
@@ -5095,7 +5096,16 @@ static void init_alloc_chunk_ctl_policy_regular(
 	/* We don't want a chunk larger than 10% of writable space */
 	ctl->max_chunk_size = min(div_factor(fs_devices->total_rw_bytes, 1),
 				  ctl->max_chunk_size);
+	if(ctl->max_stripe_size > ctl->max_chunk_size)
+		ctl->max_stripe_size = ctl->max_chunk_size;
+
 	ctl->dev_extent_min = BTRFS_STRIPE_LEN * ctl->dev_stripes;
+
+	if (ctl->type & BTRFS_BLOCK_GROUP_SYSTEM)
+		ctl->devs_max = min_t(int, ctl->devs_max, BTRFS_MAX_DEVS_SYS_CHUNK);
+
+	space_info = btrfs_find_space_info(fs_devices->fs_info, ctl->type);
+	btrfs_update_space_info_chunk_size(space_info, ctl->max_chunk_size);
 }
 
 static void init_alloc_chunk_ctl_policy_zoned(
@@ -5108,6 +5119,7 @@ static void init_alloc_chunk_ctl_policy_zoned(
 	int min_data_stripes = (min_num_stripes - ctl->nparity) / ctl->ncopies;
 	u64 min_chunk_size = min_data_stripes * zone_size;
 	u64 type = ctl->type;
+	struct btrfs_space_info *space_info;
 
 	ctl->max_stripe_size = zone_size;
 	if (type & BTRFS_BLOCK_GROUP_DATA) {
@@ -5129,6 +5141,9 @@ static void init_alloc_chunk_ctl_policy_zoned(
 		    min_chunk_size);
 	ctl->max_chunk_size = min(limit, ctl->max_chunk_size);
 	ctl->dev_extent_min = zone_size * ctl->dev_stripes;
+
+	space_info = btrfs_find_space_info(fs_devices->fs_info, ctl->type);
+	btrfs_update_space_info_chunk_size(space_info, ctl->max_chunk_size);
 }
 
 static void init_alloc_chunk_ctl(struct btrfs_fs_devices *fs_devices,