[PATCH v2 0/3] btrfs: balance: improve kernel logs

[PATCH v2 0/3] btrfs: balance: improve kernel logs

[Anand Jain]

Kernel logs are very important for the forensic investigations of the
issues, these patchs make balance logs easy to review.

Anand Jain (3):
  btrfs: balance: prefix kernel logs
  btrfs: balance: add args info during start and resume
  btrfs: balance: add kernel log for end or paused

 fs/btrfs/volumes.c | 185 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 169 insertions(+), 16 deletions(-)

-- 
2.7.0

[PATCH v2 1/3] btrfs: balance: prefix kernel logs

[Anand Jain]

Kernel logs are very important for the forensic investigations of the
issues in general make it easy to use it. This patch adds 'balance:'
prefix so that it can be easily searched.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1-v2: Change log update.
 fs/btrfs/volumes.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9773bc143650..27da66c47ef2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3801,7 +3801,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		    !(bctl->flags & BTRFS_BALANCE_METADATA) ||
 		    memcmp(&bctl->data, &bctl->meta, sizeof(bctl->data))) {
 			btrfs_err(fs_info,
-				  "with mixed groups data and metadata balance options must be the same");
+				  "balance: mixed groups data and metadata options must be the same");
 			ret = -EINVAL;
 			goto out;
 		}
@@ -3823,23 +3823,26 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		allowed |= (BTRFS_BLOCK_GROUP_RAID10 |
 			    BTRFS_BLOCK_GROUP_RAID6);
 	if (validate_convert_profile(&bctl->data, allowed)) {
+		int index = btrfs_bg_flags_to_raid_index(bctl->data.target);
 		btrfs_err(fs_info,
-			  "unable to start balance with target data profile %llu",
-			  bctl->data.target);
+			  "balance: invalid convert data profile %s",
+			  get_raid_name(index));
 		ret = -EINVAL;
 		goto out;
 	}
 	if (validate_convert_profile(&bctl->meta, allowed)) {
+		int index = btrfs_bg_flags_to_raid_index(bctl->meta.target);
 		btrfs_err(fs_info,
-			  "unable to start balance with target metadata profile %llu",
-			  bctl->meta.target);
+			  "balance: invalid convert metadata profile %s",
+			  get_raid_name(index));
 		ret = -EINVAL;
 		goto out;
 	}
 	if (validate_convert_profile(&bctl->sys, allowed)) {
+		int index = btrfs_bg_flags_to_raid_index(bctl->sys.target);
 		btrfs_err(fs_info,
-			  "unable to start balance with target system profile %llu",
-			  bctl->sys.target);
+			  "balance: invalid convert system profile %s",
+			  get_raid_name(index));
 		ret = -EINVAL;
 		goto out;
 	}
@@ -3860,10 +3863,10 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		     !(bctl->meta.target & allowed))) {
 			if (bctl->flags & BTRFS_BALANCE_FORCE) {
 				btrfs_info(fs_info,
-					   "force reducing metadata integrity");
+					   "balance: force reducing metadata integrity");
 			} else {
 				btrfs_err(fs_info,
-					  "balance will reduce metadata integrity, use force if you want this");
+					  "balance: reduces metadata integrity, use force if you want this");
 				ret = -EINVAL;
 				goto out;
 			}
@@ -3877,9 +3880,11 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		bctl->data.target : fs_info->avail_data_alloc_bits;
 	if (btrfs_get_num_tolerated_disk_barrier_failures(meta_target) <
 		btrfs_get_num_tolerated_disk_barrier_failures(data_target)) {
+		int meta_index = btrfs_bg_flags_to_raid_index(meta_target);
+		int data_index = btrfs_bg_flags_to_raid_index(data_target);
 		btrfs_warn(fs_info,
-			   "metadata profile 0x%llx has lower redundancy than data profile 0x%llx",
-			   meta_target, data_target);
+			   "balance: metadata profile %s has lower redundancy than data profile %s",
+			   get_raid_name(meta_index), get_raid_name(data_index));
 	}
 
 	ret = insert_balance_item(fs_info, bctl);
@@ -3939,7 +3944,7 @@ static int balance_kthread(void *data)
 
 	mutex_lock(&fs_info->balance_mutex);
 	if (fs_info->balance_ctl) {
-		btrfs_info(fs_info, "continuing balance");
+		btrfs_info(fs_info, "balance: resuming");
 		ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL);
 	}
 	mutex_unlock(&fs_info->balance_mutex);
@@ -3959,7 +3964,7 @@ int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info)
 	mutex_unlock(&fs_info->balance_mutex);
 
 	if (btrfs_test_opt(fs_info, SKIP_BALANCE)) {
-		btrfs_info(fs_info, "force skipping balance");
+		btrfs_info(fs_info, "balance: resume skipped");
 		return 0;
 	}
 
@@ -4024,7 +4029,7 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
 	 */
 	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags))
 		btrfs_warn(fs_info,
-	"cannot set exclusive op status to balance, resume manually");
+	"balance: cannot set exclusive op status, resume manually");
 
 	mutex_lock(&fs_info->balance_mutex);
 	BUG_ON(fs_info->balance_ctl);
@@ -4105,6 +4110,7 @@ int btrfs_cancel_balance(struct btrfs_fs_info *fs_info)
 		if (fs_info->balance_ctl) {
 			reset_balance_state(fs_info);
 			clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+			btrfs_info(fs_info, "balance: canceled");
 		}
 	}
 
-- 
2.7.0

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

[Anand Jain]

Balance args info is an important information to be reviewed for the
system audit. So this patch adds it to the kernel log.

Example:

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

-> btrfs bal start -dprofiles=raid5,convert=single -mprofiles='raid1|single',convert=raid5 --background /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

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: Change log update.
        Move adding the logs for balance complete and end to a new patch

 fs/btrfs/volumes.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 143 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 27da66c47ef2..ce68c4f42f94 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -126,6 +126,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);
@@ -3766,6 +3792,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 ");
+		get_balance_args(&bctl->data, args);
+	}
+
+	if (bctl->flags & BTRFS_BALANCE_METADATA) {
+		if (strlen(args) > 0)
+			strcat(args, " ");
+		strcat(args, "metadata ");
+		get_balance_args(&bctl->meta, args);
+	}
+
+	if (bctl->flags & BTRFS_BALANCE_SYSTEM) {
+		if (strlen(args) > 0)
+			strcat(args, " ");
+		strcat(args, "system ");
+		get_balance_args(&bctl->sys, args);
+	}
+
+	BUG_ON(strlen(args) > log_size);
+	btrfs_info(fs_info, "%s %s",
+		   bctl->flags & BTRFS_BALANCE_RESUME ?\
+		   "balance: resume":"balance: start", args);
+
+	kfree(args);
+}
+
 /*
  * Should be called with balance mutexe held
  */
@@ -3906,6 +4047,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 
 	ASSERT(!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags));
 	set_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
+	print_balance_start_or_resume(fs_info);
 	mutex_unlock(&fs_info->balance_mutex);
 
 	ret = __btrfs_balance(fs_info);
@@ -3943,10 +4085,8 @@ static int balance_kthread(void *data)
 	int ret = 0;
 
 	mutex_lock(&fs_info->balance_mutex);
-	if (fs_info->balance_ctl) {
-		btrfs_info(fs_info, "balance: resuming");
+	if (fs_info->balance_ctl)
 		ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL);
-	}
 	mutex_unlock(&fs_info->balance_mutex);
 
 	return ret;
-- 
2.7.0

[PATCH v2 3/3] btrfs: balance: add kernel log for end or paused

[Anand Jain]

Add a kernel log when the balance ends, either for cancel or completed
or if it is paused.
---
v1->v2: Moved from 2/3 to 3/3

 fs/btrfs/volumes.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ce68c4f42f94..a4e243a29f5c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4053,6 +4053,13 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 	ret = __btrfs_balance(fs_info);
 
 	mutex_lock(&fs_info->balance_mutex);
+	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
+		btrfs_info(fs_info, "balance: paused");
+	else if (ret == -ECANCELED && atomic_read(&fs_info->balance_cancel_req))
+		btrfs_info(fs_info, "balance: canceled");
+	else
+		btrfs_info(fs_info, "balance: ended with status: %d", ret);
+
 	clear_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
 
 	if (bargs) {
-- 
2.7.0

Re: [PATCH v2 1/3] btrfs: balance: prefix kernel logs

[Nikolay Borisov]

On 16.05.2018 05:51, Anand Jain wrote:
> Kernel logs are very important for the forensic investigations of the
> issues in general make it easy to use it. This patch adds 'balance:'
> prefix so that it can be easily searched.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Straightforward:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> v1-v2: Change log update.
>  fs/btrfs/volumes.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 9773bc143650..27da66c47ef2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3801,7 +3801,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		    !(bctl->flags & BTRFS_BALANCE_METADATA) ||
>  		    memcmp(&bctl->data, &bctl->meta, sizeof(bctl->data))) {
>  			btrfs_err(fs_info,
> -				  "with mixed groups data and metadata balance options must be the same");
> +				  "balance: mixed groups data and metadata options must be the same");
>  			ret = -EINVAL;
>  			goto out;
>  		}
> @@ -3823,23 +3823,26 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		allowed |= (BTRFS_BLOCK_GROUP_RAID10 |
>  			    BTRFS_BLOCK_GROUP_RAID6);
>  	if (validate_convert_profile(&bctl->data, allowed)) {
> +		int index = btrfs_bg_flags_to_raid_index(bctl->data.target);
>  		btrfs_err(fs_info,
> -			  "unable to start balance with target data profile %llu",
> -			  bctl->data.target);
> +			  "balance: invalid convert data profile %s",
> +			  get_raid_name(index));
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  	if (validate_convert_profile(&bctl->meta, allowed)) {
> +		int index = btrfs_bg_flags_to_raid_index(bctl->meta.target);
>  		btrfs_err(fs_info,
> -			  "unable to start balance with target metadata profile %llu",
> -			  bctl->meta.target);
> +			  "balance: invalid convert metadata profile %s",
> +			  get_raid_name(index));
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  	if (validate_convert_profile(&bctl->sys, allowed)) {
> +		int index = btrfs_bg_flags_to_raid_index(bctl->sys.target);
>  		btrfs_err(fs_info,
> -			  "unable to start balance with target system profile %llu",
> -			  bctl->sys.target);
> +			  "balance: invalid convert system profile %s",
> +			  get_raid_name(index));
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -3860,10 +3863,10 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		     !(bctl->meta.target & allowed))) {
>  			if (bctl->flags & BTRFS_BALANCE_FORCE) {
>  				btrfs_info(fs_info,
> -					   "force reducing metadata integrity");
> +					   "balance: force reducing metadata integrity");
>  			} else {
>  				btrfs_err(fs_info,
> -					  "balance will reduce metadata integrity, use force if you want this");
> +					  "balance: reduces metadata integrity, use force if you want this");
>  				ret = -EINVAL;
>  				goto out;
>  			}
> @@ -3877,9 +3880,11 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		bctl->data.target : fs_info->avail_data_alloc_bits;
>  	if (btrfs_get_num_tolerated_disk_barrier_failures(meta_target) <
>  		btrfs_get_num_tolerated_disk_barrier_failures(data_target)) {
> +		int meta_index = btrfs_bg_flags_to_raid_index(meta_target);
> +		int data_index = btrfs_bg_flags_to_raid_index(data_target);
>  		btrfs_warn(fs_info,
> -			   "metadata profile 0x%llx has lower redundancy than data profile 0x%llx",
> -			   meta_target, data_target);
> +			   "balance: metadata profile %s has lower redundancy than data profile %s",
> +			   get_raid_name(meta_index), get_raid_name(data_index));
>  	}
>  
>  	ret = insert_balance_item(fs_info, bctl);
> @@ -3939,7 +3944,7 @@ static int balance_kthread(void *data)
>  
>  	mutex_lock(&fs_info->balance_mutex);
>  	if (fs_info->balance_ctl) {
> -		btrfs_info(fs_info, "continuing balance");
> +		btrfs_info(fs_info, "balance: resuming");
>  		ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL);
>  	}
>  	mutex_unlock(&fs_info->balance_mutex);
> @@ -3959,7 +3964,7 @@ int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info)
>  	mutex_unlock(&fs_info->balance_mutex);
>  
>  	if (btrfs_test_opt(fs_info, SKIP_BALANCE)) {
> -		btrfs_info(fs_info, "force skipping balance");
> +		btrfs_info(fs_info, "balance: resume skipped");
>  		return 0;
>  	}
>  
> @@ -4024,7 +4029,7 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
>  	 */
>  	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags))
>  		btrfs_warn(fs_info,
> -	"cannot set exclusive op status to balance, resume manually");
> +	"balance: cannot set exclusive op status, resume manually");
>  
>  	mutex_lock(&fs_info->balance_mutex);
>  	BUG_ON(fs_info->balance_ctl);
> @@ -4105,6 +4110,7 @@ int btrfs_cancel_balance(struct btrfs_fs_info *fs_info)
>  		if (fs_info->balance_ctl) {
>  			reset_balance_state(fs_info);
>  			clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
> +			btrfs_info(fs_info, "balance: canceled");
>  		}
>  	}
>  
> 

Re: [PATCH v2 3/3] btrfs: balance: add kernel log for end or paused

[Nikolay Borisov]

On 16.05.2018 05:51, Anand Jain wrote:
> Add a kernel log when the balance ends, either for cancel or completed
> or if it is paused.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> v1->v2: Moved from 2/3 to 3/3
> 
>  fs/btrfs/volumes.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ce68c4f42f94..a4e243a29f5c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4053,6 +4053,13 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  	ret = __btrfs_balance(fs_info);
>  
>  	mutex_lock(&fs_info->balance_mutex);
> +	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
> +		btrfs_info(fs_info, "balance: paused");
> +	else if (ret == -ECANCELED && atomic_read(&fs_info->balance_cancel_req))
> +		btrfs_info(fs_info, "balance: canceled");
> +	else
> +		btrfs_info(fs_info, "balance: ended with status: %d", ret);
> +
>  	clear_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>  
>  	if (bargs) {
> 

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

[Nikolay Borisov]

On 16.05.2018 05:51, Anand Jain wrote:
> Balance args info is an important information to be reviewed for the
> system audit. So this patch adds it to the kernel log.
> 
> Example:
> 
> -> 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
> 
> -> btrfs bal start -dprofiles=raid5,convert=single -mprofiles='raid1|single',convert=raid5 --background /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
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Why can't this code be part of progs, the bctl which you are parsing is
constructed from the arguments passed from users space? I think you are
adding way too much string parsing code to the kernel and this is never
a good sign, since it's very easy to trip.

> ---
> v1->v2: Change log update.
>         Move adding the logs for balance complete and end to a new patch
> 
>  fs/btrfs/volumes.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 143 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 27da66c47ef2..ce68c4f42f94 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -126,6 +126,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);
> @@ -3766,6 +3792,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 ");
> +		get_balance_args(&bctl->data, args);
> +	}
> +
> +	if (bctl->flags & BTRFS_BALANCE_METADATA) {
> +		if (strlen(args) > 0)
> +			strcat(args, " ");
> +		strcat(args, "metadata ");
> +		get_balance_args(&bctl->meta, args);
> +	}
> +
> +	if (bctl->flags & BTRFS_BALANCE_SYSTEM) {
> +		if (strlen(args) > 0)
> +			strcat(args, " ");
> +		strcat(args, "system ");
> +		get_balance_args(&bctl->sys, args);
> +	}
> +
> +	BUG_ON(strlen(args) > log_size);
> +	btrfs_info(fs_info, "%s %s",
> +		   bctl->flags & BTRFS_BALANCE_RESUME ?\
> +		   "balance: resume":"balance: start", args);
> +
> +	kfree(args);
> +}
> +
>  /*
>   * Should be called with balance mutexe held
>   */
> @@ -3906,6 +4047,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  
>  	ASSERT(!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags));
>  	set_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
> +	print_balance_start_or_resume(fs_info);
>  	mutex_unlock(&fs_info->balance_mutex);
>  
>  	ret = __btrfs_balance(fs_info);
> @@ -3943,10 +4085,8 @@ static int balance_kthread(void *data)
>  	int ret = 0;
>  
>  	mutex_lock(&fs_info->balance_mutex);
> -	if (fs_info->balance_ctl) {
> -		btrfs_info(fs_info, "balance: resuming");
> +	if (fs_info->balance_ctl)
>  		ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL);
> -	}
>  	mutex_unlock(&fs_info->balance_mutex);
>  
>  	return ret;
> 

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

[Anand Jain]

On 05/16/2018 03:57 PM, Nikolay Borisov wrote:
> 
> 
> On 16.05.2018 05:51, Anand Jain wrote:
>> Balance args info is an important information to be reviewed for the
>> system audit. So this patch adds it to the kernel log.
>>
>> Example:
>>
>> -> 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
>>
>> -> btrfs bal start -dprofiles=raid5,convert=single -mprofiles='raid1|single',convert=raid5 --background /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
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Why can't this code be part of progs, the bctl which you are parsing is
> constructed from the arguments passed from users space? I think you are
> adding way too much string parsing code to the kernel and this is never
> a good sign, since it's very easy to trip.
Theoretically progs is a wrong place for this.
Unfortunately our bctl which is in the kernel, matches with the user
land balance ioctl args so it may feel like progs do it, but its wrong.
If kernel is modifying the chunks there must be proper logs for it,
which this patch does. Next this log is only called at the time of the 
balance setup and resume. And string parsing is behind the flag checks, 
so they aren't that intensive at run time.

Thanks, Anand

>> ---
>> v1->v2: Change log update.
>>          Move adding the logs for balance complete and end to a new patch
>>
>>   fs/btrfs/volumes.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 143 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 27da66c47ef2..ce68c4f42f94 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -126,6 +126,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);
>> @@ -3766,6 +3792,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 ");
>> +		get_balance_args(&bctl->data, args);
>> +	}
>> +
>> +	if (bctl->flags & BTRFS_BALANCE_METADATA) {
>> +		if (strlen(args) > 0)
>> +			strcat(args, " ");
>> +		strcat(args, "metadata ");
>> +		get_balance_args(&bctl->meta, args);
>> +	}
>> +
>> +	if (bctl->flags & BTRFS_BALANCE_SYSTEM) {
>> +		if (strlen(args) > 0)
>> +			strcat(args, " ");
>> +		strcat(args, "system ");
>> +		get_balance_args(&bctl->sys, args);
>> +	}
>> +
>> +	BUG_ON(strlen(args) > log_size);
>> +	btrfs_info(fs_info, "%s %s",
>> +		   bctl->flags & BTRFS_BALANCE_RESUME ?\
>> +		   "balance: resume":"balance: start", args);
>> +
>> +	kfree(args);
>> +}
>> +
>>   /*
>>    * Should be called with balance mutexe held
>>    */
>> @@ -3906,6 +4047,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>   
>>   	ASSERT(!test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags));
>>   	set_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>> +	print_balance_start_or_resume(fs_info);
>>   	mutex_unlock(&fs_info->balance_mutex);
>>   
>>   	ret = __btrfs_balance(fs_info);
>> @@ -3943,10 +4085,8 @@ static int balance_kthread(void *data)
>>   	int ret = 0;
>>   
>>   	mutex_lock(&fs_info->balance_mutex);
>> -	if (fs_info->balance_ctl) {
>> -		btrfs_info(fs_info, "balance: resuming");
>> +	if (fs_info->balance_ctl)
>>   		ret = btrfs_balance(fs_info, fs_info->balance_ctl, NULL);
>> -	}
>>   	mutex_unlock(&fs_info->balance_mutex);
>>   
>>   	return ret;
>>
> --
> 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 v2 3/3] btrfs: balance: add kernel log for end or paused

[Austin S. Hemmelgarn]

On 2018-05-15 22:51, Anand Jain wrote:
> Add a kernel log when the balance ends, either for cancel or completed
> or if it is paused.
> ---
> v1->v2: Moved from 2/3 to 3/3
> 
>   fs/btrfs/volumes.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ce68c4f42f94..a4e243a29f5c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4053,6 +4053,13 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>   	ret = __btrfs_balance(fs_info);
>   
>   	mutex_lock(&fs_info->balance_mutex);
> +	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
> +		btrfs_info(fs_info, "balance: paused");
> +	else if (ret == -ECANCELED && atomic_read(&fs_info->balance_cancel_req))
> +		btrfs_info(fs_info, "balance: canceled");
> +	else
> +		btrfs_info(fs_info, "balance: ended with status: %d", ret);
> +
>   	clear_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>   
>   	if (bargs) {
> 
Is there some way that these messages could be extended to include info 
about which volume the balance in question was on.  Ideally, something 
that matches up with what's listed in the message from the previous 
patch.  There's nothi9ng that prevents you from running balances on 
separate BTRFS volumes in parallel, so this message won't necessarily be 
for the most recent balance start message.

Re: [PATCH v2 3/3] btrfs: balance: add kernel log for end or paused

[Anand Jain]

On 05/16/2018 07:25 PM, Austin S. Hemmelgarn wrote:
> On 2018-05-15 22:51, Anand Jain wrote:
>> Add a kernel log when the balance ends, either for cancel or completed
>> or if it is paused.
>> ---
>> v1->v2: Moved from 2/3 to 3/3
>>
>>   fs/btrfs/volumes.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index ce68c4f42f94..a4e243a29f5c 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -4053,6 +4053,13 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>       ret = __btrfs_balance(fs_info);
>>       mutex_lock(&fs_info->balance_mutex);
>> +    if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
>> +        btrfs_info(fs_info, "balance: paused");
>> +    else if (ret == -ECANCELED && 
>> atomic_read(&fs_info->balance_cancel_req))
>> +        btrfs_info(fs_info, "balance: canceled");
>> +    else
>> +        btrfs_info(fs_info, "balance: ended with status: %d", ret);
>> +
>>       clear_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>>       if (bargs) {
>>
> Is there some way that these messages could be extended to include info 
> about which volume the balance in question was on.  Ideally, something 
> that matches up with what's listed in the message from the previous 
> patch.  There's nothi9ng that prevents you from running balances on 
> separate BTRFS volumes in parallel, so this message won't necessarily be 
> for the most recent balance start message.

  Hm. That's not true, btrfs_info(fs_info,,) adds the fsid.
  So its already drilled down to the lowest granular possible.

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 v2 3/3] btrfs: balance: add kernel log for end or paused

[Austin S. Hemmelgarn]

On 2018-05-16 09:23, Anand Jain wrote:
> 
> 
> On 05/16/2018 07:25 PM, Austin S. Hemmelgarn wrote:
>> On 2018-05-15 22:51, Anand Jain wrote:
>>> Add a kernel log when the balance ends, either for cancel or completed
>>> or if it is paused.
>>> ---
>>> v1->v2: Moved from 2/3 to 3/3
>>>
>>>   fs/btrfs/volumes.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index ce68c4f42f94..a4e243a29f5c 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -4053,6 +4053,13 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>>       ret = __btrfs_balance(fs_info);
>>>       mutex_lock(&fs_info->balance_mutex);
>>> +    if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
>>> +        btrfs_info(fs_info, "balance: paused");
>>> +    else if (ret == -ECANCELED && 
>>> atomic_read(&fs_info->balance_cancel_req))
>>> +        btrfs_info(fs_info, "balance: canceled");
>>> +    else
>>> +        btrfs_info(fs_info, "balance: ended with status: %d", ret);
>>> +
>>>       clear_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>>>       if (bargs) {
>>>
>> Is there some way that these messages could be extended to include 
>> info about which volume the balance in question was on.  Ideally, 
>> something that matches up with what's listed in the message from the 
>> previous patch.  There's nothi9ng that prevents you from running 
>> balances on separate BTRFS volumes in parallel, so this message won't 
>> necessarily be for the most recent balance start message.
> 
>   Hm. That's not true, btrfs_info(fs_info,,) adds the fsid.
>   So its already drilled down to the lowest granular possible.
> 
Ah, you're right, it does.  Sorry about the noise.

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

[Adam Borowski]

On Wed, May 16, 2018 at 10:57:57AM +0300, Nikolay Borisov wrote:
> On 16.05.2018 05:51, Anand Jain wrote:
> > Balance args info is an important information to be reviewed for the
> > system audit. So this patch adds it to the kernel log.
> > 
> > Example:
> > 
> > -> 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
> > 
> > -> btrfs bal start -dprofiles=raid5,convert=single -mprofiles='raid1|single',convert=raid5 --background /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
> > 
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Why can't this code be part of progs, the bctl which you are parsing is
> constructed from the arguments passed from users space? I think you are
> adding way too much string parsing code to the kernel and this is never
> a good sign, since it's very easy to trip.

progs are not the only way to start a balance, they're merely the most
widespread one.  For example, Hans van Kranenburg has some smarter scripts
among his tools -- currently only of "example" quality, but quite useful
already.  "balance_least_used" works in greedy order (least used first) with
nice verbose output.  It's not unlikely he or someone else improves this
further.  Thus, I really think the logging should be kernel side.

On the other hand, the string producing (not parsing) code is so repetitive
that it indeed could use some refactoring.


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ 
⣾⠁⢰⠒⠀⣿⡁ 
⢿⡄⠘⠷⠚⠋⠀ Certified airhead; got the CT scan to prove that!
⠈⠳⣄⠀⠀⠀⠀ 

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

[David Sterba]

On Wed, May 16, 2018 at 10:51:27AM +0800, Anand Jain wrote:
> Balance args info is an important information to be reviewed for the
> system audit. So this patch adds it to the kernel log.
> 
> Example:
> 
> -> 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
> 
> -> btrfs bal start -dprofiles=raid5,convert=single -mprofiles='raid1|single',convert=raid5 --background /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
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

The log message can be useful, however please avoid using unchecked
strcat and rewrite it without it if possible. You can find some
inspiration how to join several strings in describe_relocation().

Re: [PATCH v2 1/3] btrfs: balance: prefix kernel logs

[David Sterba]

On Wed, May 16, 2018 at 10:51:26AM +0800, Anand Jain wrote:
> Kernel logs are very important for the forensic investigations of the
> issues in general make it easy to use it. This patch adds 'balance:'
> prefix so that it can be easily searched.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Added to next, thanks. Please stick to the coding style and add an empty
newlines between declarations and statements and un-indent long printk
strings so they fit 80 columns. I've fixed that for now.

Re: [PATCH v2 3/3] btrfs: balance: add kernel log for end or paused

[David Sterba]

On Wed, May 16, 2018 at 10:51:28AM +0800, Anand Jain wrote:
> Add a kernel log when the balance ends, either for cancel or completed
> or if it is paused.

Missing S-O-B.

> ---
> v1->v2: Moved from 2/3 to 3/3
> 
>  fs/btrfs/volumes.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ce68c4f42f94..a4e243a29f5c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4053,6 +4053,13 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  	ret = __btrfs_balance(fs_info);
>  
>  	mutex_lock(&fs_info->balance_mutex);
> +	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
> +		btrfs_info(fs_info, "balance: paused");
> +	else if (ret == -ECANCELED && atomic_read(&fs_info->balance_cancel_req))
> +		btrfs_info(fs_info, "balance: canceled");
> +	else
> +		btrfs_info(fs_info, "balance: ended with status: %d", ret);

Wouldn't that repeat the same once again? There are status messages
printed already, when eg. balance is cancelled or paused.

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

[Anand Jain]

On 05/17/2018 07:59 PM, David Sterba wrote:
> On Wed, May 16, 2018 at 10:51:27AM +0800, Anand Jain wrote:
>> Balance args info is an important information to be reviewed for the
>> system audit. So this patch adds it to the kernel log.
>>
>> Example:
>>
>> -> 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
>>
>> -> btrfs bal start -dprofiles=raid5,convert=single -mprofiles='raid1|single',convert=raid5 --background /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
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> The log message can be useful, however please avoid using unchecked
> strcat and rewrite it without it if possible. You can find some
> inspiration how to join several strings in describe_relocation().

  Indeed describe_relocation() is nicer. I have updated it and made it
  a helper function in V3-set, so that balance can use it.

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 v2 3/3] btrfs: balance: add kernel log for end or paused

[Anand Jain]

On 05/17/2018 08:06 PM, David Sterba wrote:
> On Wed, May 16, 2018 at 10:51:28AM +0800, Anand Jain wrote:
>> Add a kernel log when the balance ends, either for cancel or completed
>> or if it is paused.
> 
> Missing S-O-B.

  oops. Fixed in v3.

>> ---
>> v1->v2: Moved from 2/3 to 3/3
>>
>>   fs/btrfs/volumes.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index ce68c4f42f94..a4e243a29f5c 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -4053,6 +4053,13 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>   	ret = __btrfs_balance(fs_info);
>>   
>>   	mutex_lock(&fs_info->balance_mutex);
>> +	if (ret == -ECANCELED && atomic_read(&fs_info->balance_pause_req))
>> +		btrfs_info(fs_info, "balance: paused");
>> +	else if (ret == -ECANCELED && atomic_read(&fs_info->balance_cancel_req))
>> +		btrfs_info(fs_info, "balance: canceled");
>> +	else
>> +		btrfs_info(fs_info, "balance: ended with status: %d", ret);
> 
> Wouldn't that repeat the same once again? There are status messages
> printed already, when eg. balance is cancelled or paused.

  No it won't. They are under if - else if- else. The __btrfs_balance()
  returns ECANCELED for both paused and canceled so bifurcating them
  here is useful.

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
>