[PATCH 0/2] improve balance logs

[PATCH 0/2] improve balance logs

[Anand Jain]

Anand Jain (2):
  btrfs: improve balance kernel logs
  btrfs: add balance args info during start and resume

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

-- 
2.7.0

[PATCH 1/2] btrfs: improve balance kernel logs

[Anand Jain]

Kernel logs are very important for the forensic investigations of the
issues in general. This patch adds 'balance:' prefix so that it can be
easily be searched and uses the block group names instead of its bitmap.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 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 14efa98b7183..e688c993197f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3805,7 +3805,7 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
 		    !(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;
 		}
@@ -3827,23 +3827,26 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
 		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;
 	}
@@ -3864,10 +3867,10 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
 		     !(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;
 			}
@@ -3881,9 +3884,11 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
 		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);
@@ -3943,7 +3948,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->balance_ctl, NULL);
 	}
 	mutex_unlock(&fs_info->balance_mutex);
@@ -3963,7 +3968,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;
 	}
 
@@ -4032,7 +4037,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);
@@ -4111,6 +4116,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

Re: [PATCH 1/2] btrfs: improve balance kernel logs

[Nikolay Borisov]

On 26.04.2018 11:01, Anand Jain wrote:
> Kernel logs are very important for the forensic investigations of the
> issues in general. This patch adds 'balance:' prefix so that it can be
> easily be searched and uses the block group names instead of its bitmap.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>


Code-wise it looks code. The subject however is somewhat inaccurate, I'd
rather have the subject be explicit about adding the prefix e.h :
"prefix balance related printouts" or "Add balance: prefix to certain
log items".
> ---
>  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 14efa98b7183..e688c993197f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3805,7 +3805,7 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>  		    !(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;
>  		}
> @@ -3827,23 +3827,26 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>  		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;
>  	}
> @@ -3864,10 +3867,10 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>  		     !(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;
>  			}
> @@ -3881,9 +3884,11 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>  		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);
> @@ -3943,7 +3948,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->balance_ctl, NULL);
>  	}
>  	mutex_unlock(&fs_info->balance_mutex);
> @@ -3963,7 +3968,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;
>  	}
>  
> @@ -4032,7 +4037,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);
> @@ -4111,6 +4116,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 1/2] btrfs: improve balance kernel logs

[Anand Jain]

On 04/26/2018 10:51 PM, Nikolay Borisov wrote:
> 
> 
> On 26.04.2018 11:01, Anand Jain wrote:
>> Kernel logs are very important for the forensic investigations of the
>> issues in general. This patch adds 'balance:' prefix so that it can be
>> easily be searched and uses the block group names instead of its bitmap.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> 
> Code-wise it looks code. The subject however is somewhat inaccurate, I'd
> rather have the subject be explicit about adding the prefix e.h :
> "prefix balance related printouts" or "Add balance: prefix to certain
> log items".

  As the patch also converts the bitmap to the flags, I had
  to reword the subject before sending. I am ok with the
  suggested title as above if it still makes sense. Thanks
  for the comments.

- Anand

>> ---
>>   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 14efa98b7183..e688c993197f 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -3805,7 +3805,7 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>>   		    !(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;
>>   		}
>> @@ -3827,23 +3827,26 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>>   		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;
>>   	}
>> @@ -3864,10 +3867,10 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>>   		     !(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;
>>   			}
>> @@ -3881,9 +3884,11 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>>   		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);
>> @@ -3943,7 +3948,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->balance_ctl, NULL);
>>   	}
>>   	mutex_unlock(&fs_info->balance_mutex);
>> @@ -3963,7 +3968,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;
>>   	}
>>   
>> @@ -4032,7 +4037,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);
>> @@ -4111,6 +4116,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");
>>   		}
>>   	}
>>   
>>
> --
> 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
> 

[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 <anand.jain@oracle.com>
---
 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 ");
+		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
  */
@@ -3910,11 +4051,19 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
 
 	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);
 
 	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) {
@@ -3947,10 +4096,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->balance_ctl, NULL);
-	}
 	mutex_unlock(&fs_info->balance_mutex);
 
 	return ret;
-- 
2.7.0

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!
⠈⠳⣄⠀⠀⠀⠀ 

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 <anand.jain@oracle.com>
> ---
>  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)
> +{
> +	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
>   */
> @@ -3910,11 +4051,19 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>  
>  	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);
>  
>  	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);
> +

Unrelated change

>  	clear_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>  
>  	if (bargs) {
> @@ -3947,10 +4096,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->balance_ctl, NULL);
> -	}
Unrelated change.

>  	mutex_unlock(&fs_info->balance_mutex);
>  
>  	return ret;
> 

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.

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

    kernel: BTRFS info (device sdb): balance: start data 
usage=386547056645 usage_min=5 usage_max=90 <-- we pick the usage with a 
wrong value. a bug.


>> @@ -3910,11 +4051,19 @@ int btrfs_balance(struct btrfs_balance_control *bctl,
>>   
>>   	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);
>>   
>>   	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);
>> +
> 
> Unrelated change

  As of now there is no logs when balance completes or paused.
  I can move this to a new patch as I didn't say it explicitly
  in the change log.

>>   	clear_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags);
>>   
>>   	if (bargs) {
>> @@ -3947,10 +4096,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->balance_ctl, NULL);
>> -	}
> Unrelated change.

  Why?

Thanks, Anand

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, &fs_info->flags);
>>>       if (bargs) {
>>> @@ -3947,10 +4096,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->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