Re: [PATCH 2/2] btrfs: add balance args info during start and resume

[Anand Jain]

 All comments accepted in v2 in ml, except for the one below.


  clear_bit(BTRFS_FS_BALANCE_RUNNING, _info->flags);
  if (bargs) {
@@ -3947,10 +4096,8 @@ static int balance_kthread(void *data)
  int ret = 0;
  mutex_lock(_info->balance_mutex);
-    if (fs_info->balance_ctl) {
-    btrfs_info(fs_info, "balance: resuming");
+    if (fs_info->balance_ctl)
  ret = btrfs_balance(fs_info->balance_ctl, NULL);
-    }

Unrelated change.


  Why?


 I believe this change is related to this patch, as this patch has
 added the resume log at btrfs_balance() and so we don't needed
 the resume log again in btrfs_kthread().
 OR.
 I don't know your reasoning. I am ignoring this comment as of now.

 V2 is in the ML.

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

Re: [PATCH 2/2] btrfs: add balance args info during start and resume

[Anand Jain]

On 04/27/2018 02:46 PM, Nikolay Borisov wrote:



On 26.04.2018 11:01, Anand Jain wrote:

Balance args info is an important information to be reviewed on the
system under audit. So this patch adds that.


Would have been good to add example output to the change log.


Here are the samples (will add in v2): (only for the illustrations, 
configs may not be ideal)


1. Find raid1 or single convert them to raid5 for both data and metadata:

btrfs bal start -dprofiles='raid1|single',convert=raid5 
-mprofiles='raid1|single',convert=raid5 /btrfs


 kernel: BTRFS info (device sdb): balance: start data 
profiles=raid1|single convert=raid5 metadata profiles=raid1|single 
convert=raid5 system profiles=raid1|single convert=raid5

::
 kernel: BTRFS info (device sdb): balance: ended with status: 0

2. Start, Pause and resume.

btrfs bal start -dprofiles=raid5,convert=single 
-mprofiles='raid1|single',convert=raid5 --background /btrfs && btrfs bal 
pause /btrfs && btrfs bal resume /btrfs


 kernel: BTRFS info (device sdb): balance: start data profiles=raid5 
convert=single metadata profiles=raid1|single convert=raid5 system 
profiles=raid1|single convert=raid5

 ::
 kernel: BTRFS info (device sdb): balance: paused  (<-- I am planning 
to add %age completed here at a later point of time).

 ::
 kernel: BTRFS info (device sdb): balance: resume data profiles=raid5 
convert=single metadata profiles=raid1|single convert=raid5 system 
profiles=raid1|single convert=raid5

 ::
 kernel: BTRFS info (device sdb): balance: ended with status: 0

::


@@ -3770,6 +3796,121 @@ static inline int validate_convert_profile(struct 
btrfs_balance_args *bctl_arg,
 (bctl_arg->target & ~allowed)));
  }
  
+static void get_balance_args(struct btrfs_balance_args *bargs, char *args)

+{
+   char value[64];
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_PROFILES) {
+   strcat(args, "profiles=");
+   get_all_raid_names(bargs->profiles, value);
+   strcat(args, value);
+   strcat(args, " ");
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_USAGE) {
+   snprintf(value, 64, "usage=%llu ", bargs->usage);
+   strcat(args, value);

+   if (bg_flags & BTRFS_AVAIL_ALLOC_BIT_SINGLE) {
+   if (found) {


 Hmm.. I didn't get this part of your comments.


+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_USAGE_RANGE) {
+   snprintf(value, 64, "usage_min=%u usage_max=%u ",
+   bargs->usage_min, bargs->usage_max);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_DEVID) {
+   snprintf(value, 64, "devid=%llu ", bargs->devid);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_DRANGE) {
+   snprintf(value, 64, "pstart=%llu pend=%llu ",
+bargs->pstart, bargs->pend);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_VRANGE) {
+   snprintf(value, 64, "vstart=%llu vend %llu ",
+bargs->vstart, bargs->vend);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_LIMIT) {
+   snprintf(value, 64, "limit=%llu ", bargs->limit);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE) {
+   snprintf(value, 64, "limit_min=%u limit_max=%u ",
+bargs->limit_min, bargs->limit_max);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE) {
+   snprintf(value, 64, "stripes_min=%u stripes_max=%u ",
+bargs->stripes_min, bargs->stripes_max);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_CONVERT) {
+   int index = btrfs_bg_flags_to_raid_index(bargs->target);
+   snprintf(value, 64, "convert=%s ",
+get_raid_name(index));
+   strcat(args, value);
+   }
+


Having all those ifs as independent statements means we can potentially
have each and every value present, I haven't really counted how long the
string could potentially get. Is it not possible to convert some of them
into if/else if constructs or is it indeed possible to have all of them
present at once?


 It won't help much there are only -usage- and -limit- which are
 mutually exclusive with -usage_range- and -limit_range- respectively.
 Moreover its just one time when balance starts/resumes.

 Further as these are flags, I like to do the correct way, that is,
 depend on the flags to print its args, so that we know if there is
 anything wrong. And there is one..

 btrfs bal start -dusage="5..90",usage=100 /btrfs

   kernel: BTRFS info (device sdb): balance: start data usage=100 <-- 
we pick the -usage- which is correct.


 

Re: [PATCH 2/2] btrfs: add balance args info during start and resume

[Nikolay Borisov]

On 26.04.2018 11:01, Anand Jain wrote:
> Balance args info is an important information to be reviewed on the
> system under audit. So this patch adds that.

Would have been good to add example output to the change log.

> 
> Signed-off-by: Anand Jain 
> ---
>  fs/btrfs/volumes.c | 153 
> +++--
>  1 file changed, 150 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e688c993197f..3d47b36579b3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -138,6 +138,32 @@ const char *get_raid_name(enum btrfs_raid_types type)
>   return btrfs_raid_array[type].raid_name;
>  }
>  
> +static void get_all_raid_names(u64 bg_flags, char *raid_types)
> +{
> + int i;
> + bool found = false;
> +
> + for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) {
> + if (bg_flags & btrfs_raid_array[i].bg_flag) {
> + if (found) {
> + strcat(raid_types, "|");
> + strcat(raid_types, 
> btrfs_raid_array[i].raid_name);
> + } else {
> + found = true;
> + sprintf(raid_types, "%s", 
> btrfs_raid_array[i].raid_name);
> + }
> + }
> + }
> + if (bg_flags & BTRFS_AVAIL_ALLOC_BIT_SINGLE) {
> + if (found) {
> + strcat(raid_types, "|");
> + strcat(raid_types, "single");
> + } else {
> + sprintf(raid_types, "%s", "single");
> + }
> + }
> +}
> +
>  static int init_first_rw_device(struct btrfs_trans_handle *trans,
>   struct btrfs_fs_info *fs_info);
>  static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info);
> @@ -3770,6 +3796,121 @@ static inline int validate_convert_profile(struct 
> btrfs_balance_args *bctl_arg,
>(bctl_arg->target & ~allowed)));
>  }
>  
> +static void get_balance_args(struct btrfs_balance_args *bargs, char *args)
> +{
> + char value[64];
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_PROFILES) {
> + strcat(args, "profiles=");
> + get_all_raid_names(bargs->profiles, value);
> + strcat(args, value);
> + strcat(args, " ");
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_USAGE) {
> + snprintf(value, 64, "usage=%llu ", bargs->usage);
> + strcat(args, value);
+   if (bg_flags & BTRFS_AVAIL_ALLOC_BIT_SINGLE) {
+   if (found) {
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_USAGE_RANGE) {
> + snprintf(value, 64, "usage_min=%u usage_max=%u ",
> + bargs->usage_min, bargs->usage_max);
> + strcat(args, value);
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_DEVID) {
> + snprintf(value, 64, "devid=%llu ", bargs->devid);
> + strcat(args, value);
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_DRANGE) {
> + snprintf(value, 64, "pstart=%llu pend=%llu ",
> +  bargs->pstart, bargs->pend);
> + strcat(args, value);
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_VRANGE) {
> + snprintf(value, 64, "vstart=%llu vend %llu ",
> +  bargs->vstart, bargs->vend);
> + strcat(args, value);
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_LIMIT) {
> + snprintf(value, 64, "limit=%llu ", bargs->limit);
> + strcat(args, value);
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE) {
> + snprintf(value, 64, "limit_min=%u limit_max=%u ",
> +  bargs->limit_min, bargs->limit_max);
> + strcat(args, value);
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE) {
> + snprintf(value, 64, "stripes_min=%u stripes_max=%u ",
> +  bargs->stripes_min, bargs->stripes_max);
> + strcat(args, value);
> + }
> +
> + if (bargs->flags & BTRFS_BALANCE_ARGS_CONVERT) {
> + int index = btrfs_bg_flags_to_raid_index(bargs->target);
> + snprintf(value, 64, "convert=%s ",
> +  get_raid_name(index));
> + strcat(args, value);
> + }
> +

Having all those ifs as independent statements means we can potentially
have each and every value present, I haven't really counted how long the
string could potentially get. Is it not possible to convert some of them
into if/else if constructs or is it indeed possible to have all of them
present at once?

> + /* If space was the last char remove it */
> + if (strlen(args) && (args[strlen(args) - 1] == ' '))
> + args[strlen(args) - 1] = '\0';
> +}
> +
> +static void print_balance_start_or_resume(struct btrfs_fs_info *fs_info)
> 

Re: [PATCH 2/2] btrfs: add balance args info during start and resume

[Adam Borowski]

On Thu, Apr 26, 2018 at 04:01:29PM +0800, Anand Jain wrote:
> Balance args info is an important information to be reviewed on the
> system under audit. So this patch adds that.

This kept annoying me.  Thanks a lot!

-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ 
⢿⡄⠘⠷⠚⠋⠀ Certified airhead; got the CT scan to prove that!
⠈⠳⣄ 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] btrfs: add balance args info during start and resume

[Anand Jain]

Balance args info is an important information to be reviewed on the
system under audit. So this patch adds that.

Signed-off-by: Anand Jain 
---
 fs/btrfs/volumes.c | 153 +++--
 1 file changed, 150 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e688c993197f..3d47b36579b3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -138,6 +138,32 @@ const char *get_raid_name(enum btrfs_raid_types type)
return btrfs_raid_array[type].raid_name;
 }
 
+static void get_all_raid_names(u64 bg_flags, char *raid_types)
+{
+   int i;
+   bool found = false;
+
+   for (i = 0; i < BTRFS_NR_RAID_TYPES; i++) {
+   if (bg_flags & btrfs_raid_array[i].bg_flag) {
+   if (found) {
+   strcat(raid_types, "|");
+   strcat(raid_types, 
btrfs_raid_array[i].raid_name);
+   } else {
+   found = true;
+   sprintf(raid_types, "%s", 
btrfs_raid_array[i].raid_name);
+   }
+   }
+   }
+   if (bg_flags & BTRFS_AVAIL_ALLOC_BIT_SINGLE) {
+   if (found) {
+   strcat(raid_types, "|");
+   strcat(raid_types, "single");
+   } else {
+   sprintf(raid_types, "%s", "single");
+   }
+   }
+}
+
 static int init_first_rw_device(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info);
 static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info);
@@ -3770,6 +3796,121 @@ static inline int validate_convert_profile(struct 
btrfs_balance_args *bctl_arg,
 (bctl_arg->target & ~allowed)));
 }
 
+static void get_balance_args(struct btrfs_balance_args *bargs, char *args)
+{
+   char value[64];
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_PROFILES) {
+   strcat(args, "profiles=");
+   get_all_raid_names(bargs->profiles, value);
+   strcat(args, value);
+   strcat(args, " ");
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_USAGE) {
+   snprintf(value, 64, "usage=%llu ", bargs->usage);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_USAGE_RANGE) {
+   snprintf(value, 64, "usage_min=%u usage_max=%u ",
+   bargs->usage_min, bargs->usage_max);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_DEVID) {
+   snprintf(value, 64, "devid=%llu ", bargs->devid);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_DRANGE) {
+   snprintf(value, 64, "pstart=%llu pend=%llu ",
+bargs->pstart, bargs->pend);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_VRANGE) {
+   snprintf(value, 64, "vstart=%llu vend %llu ",
+bargs->vstart, bargs->vend);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_LIMIT) {
+   snprintf(value, 64, "limit=%llu ", bargs->limit);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE) {
+   snprintf(value, 64, "limit_min=%u limit_max=%u ",
+bargs->limit_min, bargs->limit_max);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE) {
+   snprintf(value, 64, "stripes_min=%u stripes_max=%u ",
+bargs->stripes_min, bargs->stripes_max);
+   strcat(args, value);
+   }
+
+   if (bargs->flags & BTRFS_BALANCE_ARGS_CONVERT) {
+   int index = btrfs_bg_flags_to_raid_index(bargs->target);
+   snprintf(value, 64, "convert=%s ",
+get_raid_name(index));
+   strcat(args, value);
+   }
+
+   /* If space was the last char remove it */
+   if (strlen(args) && (args[strlen(args) - 1] == ' '))
+   args[strlen(args) - 1] = '\0';
+}
+
+static void print_balance_start_or_resume(struct btrfs_fs_info *fs_info)
+{
+   struct btrfs_balance_control *bctl = fs_info->balance_ctl;
+   int log_size = 1024;
+   char *args;
+
+   args = kzalloc(log_size, GFP_KERNEL);
+   if (!args) {
+   btrfs_warn(fs_info, "balance: failed to log: ENOMEM");
+   return;
+   }
+
+   if (bctl->flags & BTRFS_BALANCE_ARGS_SOFT) {
+   strcat(args, "soft ");
+   }
+
+   if (bctl->flags & BTRFS_BALANCE_FORCE) {
+   strcat(args, "force ");
+   }
+
+   if (bctl->flags & BTRFS_BALANCE_DATA) {
+   strcat(args, "data ");
+