[PATCH v2 0/4] btrfs: sysfs: set / query btrfs stripe size

[PATCH v2 0/4] btrfs: sysfs: set / query btrfs stripe size

[Stefan Roesch]

Motivation:
The btrfs allocator is currently not ideal for all workloads. It tends
to suffer from overallocating data block groups and underallocating
metadata block groups. This results in filesystems becoming read-only
even though there is plenty of "free" space.

This is naturally confusing and distressing to users.

Patches:
1) Store the stripe and chunk size in the btrfs_space_info structure
2) Add a sysfs entry to expose the above information
3) Add a sysfs entry to force a space allocation
4) Increase the default size of the metadata chunk allocation to 5GB
   for volumes greater than 50GB.

Testing:
  A new test is being added to the xfstest suite. For reference the
  corresponding patch has the title:
    [PATCH] btrfs: Test chunk allocation with different sizes

  In addition also manual testing has been performed.
    - Run xfstests with the changes and the new test. It does not
      show new diffs.
    - Test with storage devices 10G, 20G, 30G, 50G, 60G
      - Default allocation
      - Increase of chunk size
      - If the stripe size is > the free space, it allocates
        free space - 1MB. The 1MB is left as free space.
      - If the device has a storage size > 50G, it uses a 5GB
        chunk size for new allocations.

Stefan Roesch (4):
  btrfs: store stripe size and chunk size in space-info struct.
  btrfs: expose stripe and chunk size in sysfs.
  btrfs: add force_chunk_alloc sysfs entry to force allocation
  btrfs: increase metadata alloc size to 5GB for volumes > 50GB

 fs/btrfs/space-info.c |  72 +++++++++++++++++++++
 fs/btrfs/space-info.h |   4 ++
 fs/btrfs/sysfs.c      | 145 ++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.c    |  28 +++-----
 4 files changed, 230 insertions(+), 19 deletions(-)

Signed-off-by: Stefan Roesch <shr@fb.com>
---
V2:
   - Split the patch in 4 patches
   - Added checks for zone volumes in sysfs.c
   - Replaced the BUG() with ASSERT()
   - Changed if with fallthrough
   - Removed comments in space-info.h
--
2.30.2

[PATCH v2 1/4] btrfs: store stripe size and chunk size in space-info struct.

[Stefan Roesch]

The stripe size and chunk size are stored in the btrfs_space_info
structure. They are initialized at the start and are then used.

Two api's are added to get the current value and one to update
the value.

These api's will be used to be able to expose the stripe_size
as a sysfs setting.

Signed-off-by: Stefan Roesch <shr@fb.com>
---
 fs/btrfs/space-info.c | 72 +++++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/space-info.h |  4 +++
 fs/btrfs/volumes.c    | 28 ++++++-----------
 3 files changed, 85 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 48d77f360a24..570acfebeae4 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -181,6 +181,74 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info *info)
 		found->full = 0;
 }
 
+/*
+ * Compute stripe size depending on block type for regular volumes.
+ */
+static u64 compute_stripe_size_regular(struct btrfs_fs_info *info, u64 flags)
+{
+	ASSERT(flags & BTRFS_BLOCK_GROUP_TYPE_MASK);
+
+	if (flags & BTRFS_BLOCK_GROUP_DATA)
+		return SZ_1G;
+	else if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
+		return SZ_32M;
+
+	/* Handle BTRFS_BLOCK_GROUP_METADATA */
+	if (info->fs_devices->total_rw_bytes > 50ULL * SZ_1G)
+		return SZ_1G;
+
+	return SZ_256M;
+}
+
+/*
+ * Compute stripe size for zoned volumes.
+ */
+static u64 compute_stripe_size_zoned(struct btrfs_fs_info *info)
+{
+	return info->zone_size;
+}
+
+/*
+ * Compute stripe size depending on volume type.
+ */
+static u64 compute_stripe_size(struct btrfs_fs_info *info, u64 flags)
+{
+	if (btrfs_is_zoned(info))
+		return compute_stripe_size_zoned(info);
+
+	return compute_stripe_size_regular(info, flags);
+}
+
+/*
+ * Compute chunk size depending on block type and stripe size.
+ */
+static u64 compute_chunk_size(u64 flags, u64 max_stripe_size)
+{
+	ASSERT(flags & BTRFS_BLOCK_GROUP_TYPE_MASK);
+
+	if (flags & BTRFS_BLOCK_GROUP_DATA)
+		return BTRFS_MAX_DATA_CHUNK_SIZE;
+	else if (flags & BTRFS_BLOCK_GROUP_METADATA)
+		return max_stripe_size;
+
+	/* Handle BTRFS_BLOCK_GROUP_SYSTEM */
+	return 2 * max_stripe_size;
+}
+
+/*
+ * Update maximum stripe size and chunk size.
+ *
+ */
+void btrfs_update_space_info_max_alloc_sizes(struct btrfs_space_info *space_info,
+					     u64 flags, u64 max_stripe_size)
+{
+	spin_lock(&space_info->lock);
+	space_info->max_stripe_size = max_stripe_size;
+	space_info->max_chunk_size = compute_chunk_size(flags,
+						space_info->max_stripe_size);
+	spin_unlock(&space_info->lock);
+}
+
 static int create_space_info(struct btrfs_fs_info *info, u64 flags)
 {
 
@@ -203,6 +271,10 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
 	INIT_LIST_HEAD(&space_info->priority_tickets);
 	space_info->clamp = 1;
 
+	space_info->max_stripe_size = compute_stripe_size(info, flags);
+	space_info->max_chunk_size = compute_chunk_size(flags,
+						space_info->max_stripe_size);
+
 	ret = btrfs_sysfs_add_space_info_type(info, space_info);
 	if (ret)
 		return ret;
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index cb5056472e79..5ee3e381de38 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -23,6 +23,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. */
+	u64 max_chunk_size; /* maximum chunk size in bytes */
+	u64 max_stripe_size; /* maximum stripe size in bytes */
 
 	int clamp;		/* Used to scale our threshold for preemptive
 				   flushing. The value is >> clamp, so turns
@@ -115,6 +117,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_max_alloc_sizes(struct btrfs_space_info *space_info,
+			     u64 flags, u64 max_stripe_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 6031e2f4c6bc..6408592d4461 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5002,26 +5002,16 @@ static void init_alloc_chunk_ctl_policy_regular(
 				struct btrfs_fs_devices *fs_devices,
 				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;
-		ctl->max_chunk_size = BTRFS_MAX_DATA_CHUNK_SIZE;
-	} else if (type & BTRFS_BLOCK_GROUP_METADATA) {
-		/* For larger filesystems, use larger metadata chunks */
-		if (fs_devices->total_rw_bytes > 50ULL * SZ_1G)
-			ctl->max_stripe_size = SZ_1G;
-		else
-			ctl->max_stripe_size = SZ_256M;
-		ctl->max_chunk_size = ctl->max_stripe_size;
-	} else if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
-		ctl->max_stripe_size = SZ_32M;
-		ctl->max_chunk_size = 2 * ctl->max_stripe_size;
-		ctl->devs_max = min_t(int, ctl->devs_max,
-				      BTRFS_MAX_DEVS_SYS_CHUNK);
-	} else {
-		BUG();
-	}
+	space_info = btrfs_find_space_info(fs_devices->fs_info, ctl->type);
+	ASSERT(space_info);
+
+	ctl->max_stripe_size = space_info->max_stripe_size;
+	ctl->max_chunk_size = space_info->max_chunk_size;
+
+	if (ctl->type & BTRFS_BLOCK_GROUP_SYSTEM)
+		ctl->devs_max = min_t(int, ctl->devs_max, BTRFS_MAX_DEVS_SYS_CHUNK);
 
 	/* 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),
-- 
2.30.2

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

[Josef Bacik]

On Wed, Oct 27, 2021 at 01:14:38PM -0700, Stefan Roesch wrote:
> The stripe size and chunk size are stored in the btrfs_space_info
> structure. They are initialized at the start and are then used.
> 
> Two api's are added to get the current value and one to update
> the value.
> 
> These api's will be used to be able to expose the stripe_size
> as a sysfs setting.
> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> ---
>  fs/btrfs/space-info.c | 72 +++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/space-info.h |  4 +++
>  fs/btrfs/volumes.c    | 28 ++++++-----------
>  3 files changed, 85 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 48d77f360a24..570acfebeae4 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -181,6 +181,74 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info *info)
>  		found->full = 0;
>  }
>  
> +/*
> + * Compute stripe size depending on block type for regular volumes.
> + */
> +static u64 compute_stripe_size_regular(struct btrfs_fs_info *info, u64 flags)
> +{
> +	ASSERT(flags & BTRFS_BLOCK_GROUP_TYPE_MASK);
> +
> +	if (flags & BTRFS_BLOCK_GROUP_DATA)
> +		return SZ_1G;
> +	else if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
> +		return SZ_32M;
> +
> +	/* Handle BTRFS_BLOCK_GROUP_METADATA */
> +	if (info->fs_devices->total_rw_bytes > 50ULL * SZ_1G)
> +		return SZ_1G;
> +
> +	return SZ_256M;
> +}
> +
> +/*
> + * Compute stripe size for zoned volumes.
> + */
> +static u64 compute_stripe_size_zoned(struct btrfs_fs_info *info)
> +{
> +	return info->zone_size;
> +}
> +
> +/*
> + * Compute stripe size depending on volume type.
> + */
> +static u64 compute_stripe_size(struct btrfs_fs_info *info, u64 flags)
> +{
> +	if (btrfs_is_zoned(info))
> +		return compute_stripe_size_zoned(info);
> +
> +	return compute_stripe_size_regular(info, flags);
> +}
> +
> +/*
> + * Compute chunk size depending on block type and stripe size.
> + */
> +static u64 compute_chunk_size(u64 flags, u64 max_stripe_size)
> +{
> +	ASSERT(flags & BTRFS_BLOCK_GROUP_TYPE_MASK);
> +
> +	if (flags & BTRFS_BLOCK_GROUP_DATA)
> +		return BTRFS_MAX_DATA_CHUNK_SIZE;
> +	else if (flags & BTRFS_BLOCK_GROUP_METADATA)
> +		return max_stripe_size;
> +
> +	/* Handle BTRFS_BLOCK_GROUP_SYSTEM */
> +	return 2 * max_stripe_size;
> +}
> +
> +/*
> + * Update maximum stripe size and chunk size.
> + *
> + */
> +void btrfs_update_space_info_max_alloc_sizes(struct btrfs_space_info *space_info,
> +					     u64 flags, u64 max_stripe_size)
> +{
> +	spin_lock(&space_info->lock);
> +	space_info->max_stripe_size = max_stripe_size;
> +	space_info->max_chunk_size = compute_chunk_size(flags,
> +						space_info->max_stripe_size);
> +	spin_unlock(&space_info->lock);
> +}
> +
>  static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>  {
>  
> @@ -203,6 +271,10 @@ static int create_space_info(struct btrfs_fs_info *info, u64 flags)
>  	INIT_LIST_HEAD(&space_info->priority_tickets);
>  	space_info->clamp = 1;
>  
> +	space_info->max_stripe_size = compute_stripe_size(info, flags);
> +	space_info->max_chunk_size = compute_chunk_size(flags,
> +						space_info->max_stripe_size);
> +
>  	ret = btrfs_sysfs_add_space_info_type(info, space_info);
>  	if (ret)
>  		return ret;
> diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
> index cb5056472e79..5ee3e381de38 100644
> --- a/fs/btrfs/space-info.h
> +++ b/fs/btrfs/space-info.h
> @@ -23,6 +23,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. */
> +	u64 max_chunk_size; /* maximum chunk size in bytes */
> +	u64 max_stripe_size; /* maximum stripe size in bytes */

max_chunk_size isn't user set-able, and can be calculated from the
max_stripe_size.  You may change the names based on my previous emails, but
whatever it ends up being you only need one of these stored, the other can be
inferred.  Thanks,

Josef

[PATCH v2 2/4] btrfs: expose stripe and chunk size in sysfs.

[Stefan Roesch]

This adds a new sysfs entry in /sys/fs/btrfs/<uuid>/allocation/<block
type>/stripe_size. This allows to query the stripe size and also set the
stripe size.

Signed-off-by: Stefan Roesch <shr@fb.com>
---
 fs/btrfs/sysfs.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 25a6f587852b..bdfcd3d42bc4 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -21,6 +21,7 @@
 #include "space-info.h"
 #include "block-group.h"
 #include "qgroup.h"
+#include "misc.h"
 
 /*
  * Structure name                       Path
@@ -92,6 +93,7 @@ static struct btrfs_feature_attr btrfs_attr_features_##_name = {	     \
 
 static inline struct btrfs_fs_info *to_fs_info(struct kobject *kobj);
 static inline struct btrfs_fs_devices *to_fs_devs(struct kobject *kobj);
+static inline struct kobject *get_btrfs_kobj(struct kobject *kobj);
 
 static struct btrfs_feature_attr *to_btrfs_feature_attr(struct kobj_attribute *a)
 {
@@ -709,6 +711,67 @@ static ssize_t btrfs_space_info_show_##field(struct kobject *kobj,	\
 }									\
 BTRFS_ATTR(space_info, field, btrfs_space_info_show_##field)
 
+/*
+ * Return space info stripe size.
+ */
+static ssize_t btrfs_stripe_size_show(struct kobject *kobj,
+				      struct kobj_attribute *a, char *buf)
+{
+	struct btrfs_space_info *sinfo = to_space_info(kobj);
+
+	return btrfs_show_u64(&sinfo->max_stripe_size, &sinfo->lock, buf);
+}
+
+/*
+ * Store new user supplied stripe size in space info.
+ *
+ * Note: If the new stripe size value is larger than 10% of free space it is
+ *       reduced to match that limit.
+ */
+static ssize_t btrfs_stripe_size_store(struct kobject *kobj,
+				       struct kobj_attribute *a,
+				       const char *buf, size_t len)
+{
+	struct btrfs_space_info *space_info = to_space_info(kobj);
+	struct btrfs_fs_info *fs_info = to_fs_info(get_btrfs_kobj(kobj));
+	u64 val;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (!fs_info) {
+		pr_err("couldn't get fs_info\n");
+		return -EPERM;
+	}
+
+	if (sb_rdonly(fs_info->sb))
+		return -EROFS;
+
+	if (!fs_info->fs_devices)
+		return -EINVAL;
+
+	if (btrfs_is_zoned(fs_info))
+		return -EINVAL;
+
+	if (!space_info) {
+		btrfs_err(fs_info, "couldn't get space_info\n");
+		return -EPERM;
+	}
+
+	ret = kstrtoull(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * Limit stripe size to 10% of available space.
+	 */
+	val = min(div_factor(fs_info->fs_devices->total_rw_bytes, 1), val);
+	btrfs_update_space_info_max_alloc_sizes(space_info, space_info->flags, val);
+
+	return val;
+}
+
 SPACE_INFO_ATTR(flags);
 SPACE_INFO_ATTR(total_bytes);
 SPACE_INFO_ATTR(bytes_used);
@@ -719,6 +782,8 @@ SPACE_INFO_ATTR(bytes_readonly);
 SPACE_INFO_ATTR(bytes_zone_unusable);
 SPACE_INFO_ATTR(disk_used);
 SPACE_INFO_ATTR(disk_total);
+BTRFS_ATTR_RW(space_info, stripe_size, btrfs_stripe_size_show,
+	      btrfs_stripe_size_store);
 
 /*
  * Allocation information about block group types.
@@ -736,6 +801,7 @@ static struct attribute *space_info_attrs[] = {
 	BTRFS_ATTR_PTR(space_info, bytes_zone_unusable),
 	BTRFS_ATTR_PTR(space_info, disk_used),
 	BTRFS_ATTR_PTR(space_info, disk_total),
+	BTRFS_ATTR_PTR(space_info, stripe_size),
 	NULL,
 };
 ATTRIBUTE_GROUPS(space_info);
@@ -1103,6 +1169,20 @@ static inline struct btrfs_fs_info *to_fs_info(struct kobject *kobj)
 	return to_fs_devs(kobj)->fs_info;
 }
 
+/*
+ * Get btrfs sysfs kobject.
+ */
+static inline struct kobject *get_btrfs_kobj(struct kobject *kobj)
+{
+	while (kobj) {
+		if (kobj->ktype == &btrfs_ktype)
+			return kobj;
+		kobj = kobj->parent;
+	}
+
+	return NULL;
+}
+
 #define NUM_FEATURE_BITS 64
 #define BTRFS_FEATURE_NAME_MAX 13
 static char btrfs_unknown_feature_names[FEAT_MAX][NUM_FEATURE_BITS][BTRFS_FEATURE_NAME_MAX];
-- 
2.30.2

[PATCH v2 3/4] btrfs: add force_chunk_alloc sysfs entry to force allocation

[Stefan Roesch]

This adds the force_chunk_alloc sysfs entry to be able to force a
storage allocation. This is a debugging and test feature and is
enabled with the CONFIG_BTRFS_DEBUG configuration option.

It is stored at
/sys/fs/btrfs/<uuid>/allocation/<block_type>/force_chunk_alloc.

Signed-off-by: Stefan Roesch <shr@fb.com>
---
 fs/btrfs/sysfs.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index bdfcd3d42bc4..7c701931540c 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -772,6 +772,64 @@ static ssize_t btrfs_stripe_size_store(struct kobject *kobj,
 	return val;
 }
 
+#ifdef CONFIG_BTRFS_DEBUG
+/*
+ * Return if space info force allocation chunk flag is set.
+ */
+static ssize_t btrfs_force_chunk_alloc_show(struct kobject *kobj,
+					    struct kobj_attribute *a,
+					    char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0\n");
+}
+
+/*
+ * Request chunk allocation with current chunk size.
+ */
+static ssize_t btrfs_force_chunk_alloc_store(struct kobject *kobj,
+					     struct kobj_attribute *a,
+					     const char *buf, size_t len)
+{
+	struct btrfs_space_info *space_info = to_space_info(kobj);
+	struct btrfs_fs_info *fs_info = to_fs_info(get_btrfs_kobj(kobj));
+	struct btrfs_trans_handle *trans;
+	unsigned long val;
+	int ret;
+
+	if (!fs_info) {
+		pr_err("couldn't get fs_info\n");
+		return -EPERM;
+	}
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (sb_rdonly(fs_info->sb))
+		return -EROFS;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	if (val == 0)
+		return -EINVAL;
+
+	/*
+	 * Allocate new chunk.
+	 */
+	trans = btrfs_start_transaction(fs_info->extent_root, 0);
+	if (!trans)
+		return PTR_ERR(trans);
+	ret = btrfs_force_chunk_alloc(trans, space_info->flags);
+	btrfs_end_transaction(trans);
+
+	if (ret == 1)
+		return len;
+
+	return -ENOSPC;
+}
+#endif
+
 SPACE_INFO_ATTR(flags);
 SPACE_INFO_ATTR(total_bytes);
 SPACE_INFO_ATTR(bytes_used);
@@ -784,6 +842,10 @@ SPACE_INFO_ATTR(disk_used);
 SPACE_INFO_ATTR(disk_total);
 BTRFS_ATTR_RW(space_info, stripe_size, btrfs_stripe_size_show,
 	      btrfs_stripe_size_store);
+#ifdef CONFIG_BTRFS_DEBUG
+BTRFS_ATTR_RW(space_info, force_chunk_alloc, btrfs_force_chunk_alloc_show,
+	      btrfs_force_chunk_alloc_store);
+#endif
 
 /*
  * Allocation information about block group types.
@@ -802,6 +864,9 @@ static struct attribute *space_info_attrs[] = {
 	BTRFS_ATTR_PTR(space_info, disk_used),
 	BTRFS_ATTR_PTR(space_info, disk_total),
 	BTRFS_ATTR_PTR(space_info, stripe_size),
+#ifdef CONFIG_BTRFS_DEBUG
+	BTRFS_ATTR_PTR(space_info, force_chunk_alloc),
+#endif
 	NULL,
 };
 ATTRIBUTE_GROUPS(space_info);
-- 
2.30.2

[PATCH v2 4/4] btrfs: increase metadata alloc size to 5GB for volumes > 50GB

[Stefan Roesch]

This increases the metadata default allocation size from 1GB to 5GB for
volumes with a size greater than 50GB.

Signed-off-by: Stefan Roesch <shr@fb.com>
---
 fs/btrfs/space-info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 570acfebeae4..1314b0924512 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -195,7 +195,7 @@ static u64 compute_stripe_size_regular(struct btrfs_fs_info *info, u64 flags)
 
 	/* Handle BTRFS_BLOCK_GROUP_METADATA */
 	if (info->fs_devices->total_rw_bytes > 50ULL * SZ_1G)
-		return SZ_1G;
+		return 5ULL * SZ_1G;
 
 	return SZ_256M;
 }
-- 
2.30.2

Re: [PATCH v2 4/4] btrfs: increase metadata alloc size to 5GB for volumes > 50GB

[Wang Yugui]

Hi,

> This increases the metadata default allocation size from 1GB to 5GB for
> volumes with a size greater than 50GB.

If we change the metadata allocation size from 1GB to 5GB, then we have
less frequency that the unused metadata space is under 32M.

Could we begin to alloc metadata space with the size 1G after unused
metadata space is under 32M?  Is this a better way?

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/10/28


> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> ---
>  fs/btrfs/space-info.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 570acfebeae4..1314b0924512 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -195,7 +195,7 @@ static u64 compute_stripe_size_regular(struct btrfs_fs_info *info, u64 flags)
>  
>  	/* Handle BTRFS_BLOCK_GROUP_METADATA */
>  	if (info->fs_devices->total_rw_bytes > 50ULL * SZ_1G)
> -		return SZ_1G;
> +		return 5ULL * SZ_1G;
>  
>  	return SZ_256M;
>  }
> -- 
> 2.30.2

Re: [PATCH v2 0/4] btrfs: sysfs: set / query btrfs stripe size

[Josef Bacik]

On Wed, Oct 27, 2021 at 01:14:37PM -0700, Stefan Roesch wrote:
> Motivation:
> The btrfs allocator is currently not ideal for all workloads. It tends
> to suffer from overallocating data block groups and underallocating
> metadata block groups. This results in filesystems becoming read-only
> even though there is plenty of "free" space.
> 
> This is naturally confusing and distressing to users.
> 
> Patches:
> 1) Store the stripe and chunk size in the btrfs_space_info structure
> 2) Add a sysfs entry to expose the above information
> 3) Add a sysfs entry to force a space allocation
> 4) Increase the default size of the metadata chunk allocation to 5GB
>    for volumes greater than 50GB.
> 
> Testing:
>   A new test is being added to the xfstest suite. For reference the
>   corresponding patch has the title:
>     [PATCH] btrfs: Test chunk allocation with different sizes
> 
>   In addition also manual testing has been performed.
>     - Run xfstests with the changes and the new test. It does not
>       show new diffs.
>     - Test with storage devices 10G, 20G, 30G, 50G, 60G
>       - Default allocation
>       - Increase of chunk size
>       - If the stripe size is > the free space, it allocates
>         free space - 1MB. The 1MB is left as free space.
>       - If the device has a storage size > 50G, it uses a 5GB
>         chunk size for new allocations.
> 
> Stefan Roesch (4):
>   btrfs: store stripe size and chunk size in space-info struct.
>   btrfs: expose stripe and chunk size in sysfs.
>   btrfs: add force_chunk_alloc sysfs entry to force allocation
>   btrfs: increase metadata alloc size to 5GB for volumes > 50GB
>

Sorry, I had this thought previously but it got lost when I started doing the
actual code review.

We have conflated stripe size and chunk size here, and unfortunately "stripe
size" means different things to different people.  What you are actually trying
to do here is to allow us to allocate a larger logical chunk size.

In terms of how this works out in the code you are changing the correct thing,
generally the stripe_size is what dictates the actual block group chunk size we
end up with at the end.

But this is sort of confusing when it comes to the interface, because people are
going to think it means something different.

Instead we should name the sysfs file chunk_size, and then keep the code you
have the way it is, just with the new name.  That way it's clear to the user
that they're changing how large of a chunk we're allocating at any given time.

Make that change, and I have a few other code comments, and then that should be
good.  Thanks,

Josef 

Re: [PATCH v2 0/4] btrfs: sysfs: set / query btrfs stripe size

[Josef Bacik]

On Thu, Oct 28, 2021 at 09:43:51AM -0400, Josef Bacik wrote:
> On Wed, Oct 27, 2021 at 01:14:37PM -0700, Stefan Roesch wrote:
> > Motivation:
> > The btrfs allocator is currently not ideal for all workloads. It tends
> > to suffer from overallocating data block groups and underallocating
> > metadata block groups. This results in filesystems becoming read-only
> > even though there is plenty of "free" space.
> > 
> > This is naturally confusing and distressing to users.
> > 
> > Patches:
> > 1) Store the stripe and chunk size in the btrfs_space_info structure
> > 2) Add a sysfs entry to expose the above information
> > 3) Add a sysfs entry to force a space allocation
> > 4) Increase the default size of the metadata chunk allocation to 5GB
> >    for volumes greater than 50GB.
> > 
> > Testing:
> >   A new test is being added to the xfstest suite. For reference the
> >   corresponding patch has the title:
> >     [PATCH] btrfs: Test chunk allocation with different sizes
> > 
> >   In addition also manual testing has been performed.
> >     - Run xfstests with the changes and the new test. It does not
> >       show new diffs.
> >     - Test with storage devices 10G, 20G, 30G, 50G, 60G
> >       - Default allocation
> >       - Increase of chunk size
> >       - If the stripe size is > the free space, it allocates
> >         free space - 1MB. The 1MB is left as free space.
> >       - If the device has a storage size > 50G, it uses a 5GB
> >         chunk size for new allocations.
> > 
> > Stefan Roesch (4):
> >   btrfs: store stripe size and chunk size in space-info struct.
> >   btrfs: expose stripe and chunk size in sysfs.
> >   btrfs: add force_chunk_alloc sysfs entry to force allocation
> >   btrfs: increase metadata alloc size to 5GB for volumes > 50GB
> >
> 
> Sorry, I had this thought previously but it got lost when I started doing the
> actual code review.
> 
> We have conflated stripe size and chunk size here, and unfortunately "stripe
> size" means different things to different people.  What you are actually trying
> to do here is to allow us to allocate a larger logical chunk size.
> 
> In terms of how this works out in the code you are changing the correct thing,
> generally the stripe_size is what dictates the actual block group chunk size we
> end up with at the end.
> 
> But this is sort of confusing when it comes to the interface, because people are
> going to think it means something different.
> 
> Instead we should name the sysfs file chunk_size, and then keep the code you
> have the way it is, just with the new name.  That way it's clear to the user
> that they're changing how large of a chunk we're allocating at any given time.
> 
> Make that change, and I have a few other code comments, and then that should be
> good.  Thanks,
> 

In fact I talked about this with Johannes just now.  We sort of conflate the two
things, max_chunk_size and max_stripe_size, to get the answer we want.  But
these aren't well named and don't really behave in a way you'd expect.

Currently, we set max_stripe_size to make sure we clamp down on any dev extents
we find.  So if the whole disk is free we clearly don't want to allocate the
whole thing, so we clamp it down to max_stripe_size.  This, in effect, ends up
being our actual chunk_size.  We have this max_chunk_size thing but it doesn't
really do anything in practice because our stripe_size is already clamped down
so it'll be <= max_chunk_size.

All this is to say we should simply set max_stripe_size = max_chunk_size, but
call max_chunk_size default_chunk_size, because that's really what it is.  So
you should

1) Change the sysfs file to be chunk_size or something similar.
2) Don't expose stripe_size via sysfs, it's just a function of chunk_size.
3) Set stripe_size == chunk_size.
4) Get rid of the max_chunk_size logic, it's unneeded.

I think that's the proper way to deal with everything, if there are any corners
I'm missing then feel free to point them out, but I'm pretty sure 1-3 are
correct.  Thanks,

Josef

Re: [PATCH v2 0/4] btrfs: sysfs: set / query btrfs stripe size

[Johannes Thumshirn]

On 28/10/2021 16:27, Josef Bacik wrote:
> On Thu, Oct 28, 2021 at 09:43:51AM -0400, Josef Bacik wrote:
>> On Wed, Oct 27, 2021 at 01:14:37PM -0700, Stefan Roesch wrote:
>>> Motivation:
>>> The btrfs allocator is currently not ideal for all workloads. It tends
>>> to suffer from overallocating data block groups and underallocating
>>> metadata block groups. This results in filesystems becoming read-only
>>> even though there is plenty of "free" space.
>>>
>>> This is naturally confusing and distressing to users.
>>>
>>> Patches:
>>> 1) Store the stripe and chunk size in the btrfs_space_info structure
>>> 2) Add a sysfs entry to expose the above information
>>> 3) Add a sysfs entry to force a space allocation
>>> 4) Increase the default size of the metadata chunk allocation to 5GB
>>>    for volumes greater than 50GB.
>>>
>>> Testing:
>>>   A new test is being added to the xfstest suite. For reference the
>>>   corresponding patch has the title:
>>>     [PATCH] btrfs: Test chunk allocation with different sizes
>>>
>>>   In addition also manual testing has been performed.
>>>     - Run xfstests with the changes and the new test. It does not
>>>       show new diffs.
>>>     - Test with storage devices 10G, 20G, 30G, 50G, 60G
>>>       - Default allocation
>>>       - Increase of chunk size
>>>       - If the stripe size is > the free space, it allocates
>>>         free space - 1MB. The 1MB is left as free space.
>>>       - If the device has a storage size > 50G, it uses a 5GB
>>>         chunk size for new allocations.
>>>
>>> Stefan Roesch (4):
>>>   btrfs: store stripe size and chunk size in space-info struct.
>>>   btrfs: expose stripe and chunk size in sysfs.
>>>   btrfs: add force_chunk_alloc sysfs entry to force allocation
>>>   btrfs: increase metadata alloc size to 5GB for volumes > 50GB
>>>
>>
>> Sorry, I had this thought previously but it got lost when I started doing the
>> actual code review.
>>
>> We have conflated stripe size and chunk size here, and unfortunately "stripe
>> size" means different things to different people.  What you are actually trying
>> to do here is to allow us to allocate a larger logical chunk size.
>>
>> In terms of how this works out in the code you are changing the correct thing,
>> generally the stripe_size is what dictates the actual block group chunk size we
>> end up with at the end.
>>
>> But this is sort of confusing when it comes to the interface, because people are
>> going to think it means something different.
>>
>> Instead we should name the sysfs file chunk_size, and then keep the code you
>> have the way it is, just with the new name.  That way it's clear to the user
>> that they're changing how large of a chunk we're allocating at any given time.
>>
>> Make that change, and I have a few other code comments, and then that should be
>> good.  Thanks,
>>
> 
> In fact I talked about this with Johannes just now.  We sort of conflate the two
> things, max_chunk_size and max_stripe_size, to get the answer we want.  But
> these aren't well named and don't really behave in a way you'd expect.
> 
> Currently, we set max_stripe_size to make sure we clamp down on any dev extents
> we find.  So if the whole disk is free we clearly don't want to allocate the
> whole thing, so we clamp it down to max_stripe_size.  This, in effect, ends up
> being our actual chunk_size.  We have this max_chunk_size thing but it doesn't
> really do anything in practice because our stripe_size is already clamped down
> so it'll be <= max_chunk_size.

We should also add an ASSERT() to verify we're really never ever going
beyond max_chunk_size.
 
> All this is to say we should simply set max_stripe_size = max_chunk_size, but
> call max_chunk_size default_chunk_size, because that's really what it is.  So
> you should
> 
> 1) Change the sysfs file to be chunk_size or something similar.
> 2) Don't expose stripe_size via sysfs, it's just a function of chunk_size.
> 3) Set stripe_size == chunk_size.
> 4) Get rid of the max_chunk_size logic, it's unneeded.
> 
> I think that's the proper way to deal with everything, if there are any corners
> I'm missing then feel free to point them out, but I'm pretty sure 1-3 are
> correct.  Thanks,
> 
> Josef
> 

Re: [PATCH v2 0/4] btrfs: sysfs: set / query btrfs stripe size

[Stefan Roesch]

On 10/28/21 8:00 AM, Johannes Thumshirn wrote:
> On 28/10/2021 16:27, Josef Bacik wrote:
>> On Thu, Oct 28, 2021 at 09:43:51AM -0400, Josef Bacik wrote:
>>> On Wed, Oct 27, 2021 at 01:14:37PM -0700, Stefan Roesch wrote:
>>>> Motivation:
>>>> The btrfs allocator is currently not ideal for all workloads. It tends
>>>> to suffer from overallocating data block groups and underallocating
>>>> metadata block groups. This results in filesystems becoming read-only
>>>> even though there is plenty of "free" space.
>>>>
>>>> This is naturally confusing and distressing to users.
>>>>
>>>> Patches:
>>>> 1) Store the stripe and chunk size in the btrfs_space_info structure
>>>> 2) Add a sysfs entry to expose the above information
>>>> 3) Add a sysfs entry to force a space allocation
>>>> 4) Increase the default size of the metadata chunk allocation to 5GB
>>>>    for volumes greater than 50GB.
>>>>
>>>> Testing:
>>>>   A new test is being added to the xfstest suite. For reference the
>>>>   corresponding patch has the title:
>>>>     [PATCH] btrfs: Test chunk allocation with different sizes
>>>>
>>>>   In addition also manual testing has been performed.
>>>>     - Run xfstests with the changes and the new test. It does not
>>>>       show new diffs.
>>>>     - Test with storage devices 10G, 20G, 30G, 50G, 60G
>>>>       - Default allocation
>>>>       - Increase of chunk size
>>>>       - If the stripe size is > the free space, it allocates
>>>>         free space - 1MB. The 1MB is left as free space.
>>>>       - If the device has a storage size > 50G, it uses a 5GB
>>>>         chunk size for new allocations.
>>>>
>>>> Stefan Roesch (4):
>>>>   btrfs: store stripe size and chunk size in space-info struct.
>>>>   btrfs: expose stripe and chunk size in sysfs.
>>>>   btrfs: add force_chunk_alloc sysfs entry to force allocation
>>>>   btrfs: increase metadata alloc size to 5GB for volumes > 50GB
>>>>
>>>
>>> Sorry, I had this thought previously but it got lost when I started doing the
>>> actual code review.
>>>
>>> We have conflated stripe size and chunk size here, and unfortunately "stripe
>>> size" means different things to different people.  What you are actually trying
>>> to do here is to allow us to allocate a larger logical chunk size.
>>>
>>> In terms of how this works out in the code you are changing the correct thing,
>>> generally the stripe_size is what dictates the actual block group chunk size we
>>> end up with at the end.
>>>
>>> But this is sort of confusing when it comes to the interface, because people are
>>> going to think it means something different.
>>>
>>> Instead we should name the sysfs file chunk_size, and then keep the code you
>>> have the way it is, just with the new name.  That way it's clear to the user
>>> that they're changing how large of a chunk we're allocating at any given time.
>>>
>>> Make that change, and I have a few other code comments, and then that should be
>>> good.  Thanks,
>>>
>>
>> In fact I talked about this with Johannes just now.  We sort of conflate the two
>> things, max_chunk_size and max_stripe_size, to get the answer we want.  But
>> these aren't well named and don't really behave in a way you'd expect.
>>
>> Currently, we set max_stripe_size to make sure we clamp down on any dev extents
>> we find.  So if the whole disk is free we clearly don't want to allocate the
>> whole thing, so we clamp it down to max_stripe_size.  This, in effect, ends up
>> being our actual chunk_size.  We have this max_chunk_size thing but it doesn't
>> really do anything in practice because our stripe_size is already clamped down
>> so it'll be <= max_chunk_size.
> 
> We should also add an ASSERT() to verify we're really never ever going
> beyond max_chunk_size.
>  

Do you want an ASSERT() against BTRFS_MAX_DATA_CHUNK_SIZE?

>> All this is to say we should simply set max_stripe_size = max_chunk_size, but
>> call max_chunk_size default_chunk_size, because that's really what it is.  So
>> you should
>>
>> 1) Change the sysfs file to be chunk_size or something similar.
>> 2) Don't expose stripe_size via sysfs, it's just a function of chunk_size.
>> 3) Set stripe_size == chunk_size.
>> 4) Get rid of the max_chunk_size logic, it's unneeded.
>>
>> I think that's the proper way to deal with everything, if there are any corners
>> I'm missing then feel free to point them out, but I'm pretty sure 1-3 are
>> correct.  Thanks,
>>
>> Josef
>>
> 

Re: [PATCH v2 0/4] btrfs: sysfs: set / query btrfs stripe size

[Johannes Thumshirn]

On 29/10/2021 05:11, Stefan Roesch wrote:
>>> In fact I talked about this with Johannes just now.  We sort of conflate the two
>>> things, max_chunk_size and max_stripe_size, to get the answer we want.  But
>>> these aren't well named and don't really behave in a way you'd expect.
>>>
>>> Currently, we set max_stripe_size to make sure we clamp down on any dev extents
>>> we find.  So if the whole disk is free we clearly don't want to allocate the
>>> whole thing, so we clamp it down to max_stripe_size.  This, in effect, ends up
>>> being our actual chunk_size.  We have this max_chunk_size thing but it doesn't
>>> really do anything in practice because our stripe_size is already clamped down
>>> so it'll be <= max_chunk_size.
>>
>> We should also add an ASSERT() to verify we're really never ever going
>> beyond max_chunk_size.
>>  
> 
> Do you want an ASSERT() against BTRFS_MAX_DATA_CHUNK_SIZE?
> 

Nope, I thought of an ASSERT(stripe_size <= max_chunk_size), but now I'm not sure
anymore if this really makes sense or not.