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

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

[Anand Jain]

v3->v4:
 Pls ref to individual patches.

Based on misc-next.

v2->v3:
  Inspried by describe_relocation(), improves it, makes it a helper
  function and use it to log the balance operations.

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

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

 fs/btrfs/relocation.c |  30 ++---------
 fs/btrfs/volumes.c    | 146 ++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/btrfs/volumes.h    |   1 +
 3 files changed, 147 insertions(+), 30 deletions(-)

-- 
2.7.0

[PATCH v4 1/3] btrfs: add helper function describe_block_group()

[Anand Jain]

Improve on describe_relocation() add a common helper function to describe
the block groups.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3->v4: Just pass full flag name in the define DESCRIBE_FLAG(flag,..),
	 so that it can be used at couple of more places.
	Rename describe_block_groups() to btrfs_describe_block_groups().
	Drop useless return u32.
v3:	Born.

 fs/btrfs/relocation.c | 30 +++---------------------------
 fs/btrfs/volumes.c    | 36 ++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.h    |  1 +
 3 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 879b76fa881a..0434cebc2ea2 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4320,37 +4320,13 @@ static struct reloc_control *alloc_reloc_control(void)
 static void describe_relocation(struct btrfs_fs_info *fs_info,
 				struct btrfs_block_group_cache *block_group)
 {
-	char buf[128];		/* prefixed by a '|' that'll be dropped */
-	u64 flags = block_group->flags;
+	char buf[128];
 
-	/* Shouldn't happen */
-	if (!flags) {
-		strcpy(buf, "|NONE");
-	} else {
-		char *bp = buf;
-
-#define DESCRIBE_FLAG(f, d) \
-		if (flags & BTRFS_BLOCK_GROUP_##f) { \
-			bp += snprintf(bp, buf - bp + sizeof(buf), "|%s", d); \
-			flags &= ~BTRFS_BLOCK_GROUP_##f; \
-		}
-		DESCRIBE_FLAG(DATA,     "data");
-		DESCRIBE_FLAG(SYSTEM,   "system");
-		DESCRIBE_FLAG(METADATA, "metadata");
-		DESCRIBE_FLAG(RAID0,    "raid0");
-		DESCRIBE_FLAG(RAID1,    "raid1");
-		DESCRIBE_FLAG(DUP,      "dup");
-		DESCRIBE_FLAG(RAID10,   "raid10");
-		DESCRIBE_FLAG(RAID5,    "raid5");
-		DESCRIBE_FLAG(RAID6,    "raid6");
-		if (flags)
-			snprintf(bp, buf - bp + sizeof(buf), "|0x%llx", flags);
-#undef DESCRIBE_FLAG
-	}
+	btrfs_describe_block_groups(block_group->flags, buf, sizeof(buf));
 
 	btrfs_info(fs_info,
 		   "relocating block group %llu flags %s",
-		   block_group->key.objectid, buf + 1);
+		   block_group->key.objectid, buf);
 }
 
 /*
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b6757b53c297..44f4799bf06f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -126,6 +126,42 @@ const char *get_raid_name(enum btrfs_raid_types type)
 	return btrfs_raid_array[type].raid_name;
 }
 
+void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
+{
+	int i;
+	u32 ret;
+	char *bp = buf;
+	u64 flags = bg_flags;
+
+	if (!flags) {
+		snprintf(bp, size_buf, "%s", "NONE");
+		return;
+	}
+
+#define DESCRIBE_FLAG(f, d) \
+	do { \
+		if (flags & f) { \
+			bp += snprintf(bp, buf - bp + size_buf, "%s|", d); \
+			flags &= ~f; \
+		} \
+	} while (0)
+
+	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_DATA, "data");
+	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_SYSTEM, "system");
+	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_METADATA, "metadata");
+	DESCRIBE_FLAG(BTRFS_AVAIL_ALLOC_BIT_SINGLE, "single");
+	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++)
+		DESCRIBE_FLAG(btrfs_raid_array[i].bg_flag,
+			      btrfs_raid_array[i].raid_name);
+#undef DESCRIBE_FLAG
+
+	if (flags)
+		bp += snprintf(bp, buf - bp + size_buf, "0x%llx|", flags);
+
+	ret = bp - buf - 1;
+	buf[ret] = '\0'; /* remove last | */
+}
+
 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);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 5139ec8daf4c..5b9637a3ed48 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -433,6 +433,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
 int btrfs_balance(struct btrfs_fs_info *fs_info,
 		  struct btrfs_balance_control *bctl,
 		  struct btrfs_ioctl_balance_args *bargs);
+void btrfs_describe_block_groups(u64 flags, char *buf, u32 size_buf);
 int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info);
 int btrfs_recover_balance(struct btrfs_fs_info *fs_info);
 int btrfs_pause_balance(struct btrfs_fs_info *fs_info);
-- 
2.7.0

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

[Anand Jain]

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

Example:

->btrfs bal start --full-balance -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs

 kernel: BTRFS info (device sdc): balance: start -f -d usage=50,limit=10..20 -m soft,profiles=raid1,convert=single -s soft,profiles=raid1,convert=single

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3->v4: Change log updated with new example.
	Log format is changed to almost match with the cli.
	snprintf drop %s for inline string.
	Print range as =%u..%u instead of min=%umax=%u.
	soft is under the args now.
	force is printed as -f.

v2->v3: Change log updated.
        Make use of describe_block_group() added in 1/4
        Drop using strcat instead use snprintf.
        Logs string format updated as shown in the example above.

v1->v2: Change log update.
        Move adding the logs for balance complete and end to a new patch

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 44f4799bf06f..85f954f7f93e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3806,6 +3806,108 @@ static inline int validate_convert_profile(struct btrfs_balance_args *bctl_arg,
 		 (bctl_arg->target & ~allowed)));
 }
 
+static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf,
+				 u32 size_buf)
+{
+	char *bp = buf;
+	u64 flags = bargs->flags;
+	char tmp_buf[128];
+
+	if (!flags)
+		return 0;
+
+	if (flags & BTRFS_BALANCE_ARGS_SOFT)
+		bp += snprintf(bp, buf - bp + size_buf, "soft,");
+
+	if (flags & BTRFS_BALANCE_ARGS_PROFILES) {
+		btrfs_describe_block_groups(bargs->profiles, tmp_buf,
+					    sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "profiles=%s,",
+			       tmp_buf);
+	}
+
+	if (flags & BTRFS_BALANCE_ARGS_USAGE)
+		bp += snprintf(bp, buf - bp + size_buf, "usage=%llu,",
+			       bargs->usage);
+
+	if (flags & BTRFS_BALANCE_ARGS_USAGE_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "usage=%u..%u,",
+			       bargs->usage_min, bargs->usage_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_DEVID)
+		bp += snprintf(bp, buf - bp + size_buf, "devid=%llu,",
+			       bargs->devid);
+
+	if (flags & BTRFS_BALANCE_ARGS_DRANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "drange=%llu..%llu,",
+			       bargs->pstart, bargs->pend);
+
+	if (flags & BTRFS_BALANCE_ARGS_VRANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "vrange%llu..%llu,",
+			       bargs->vstart, bargs->vend);
+
+	if (flags & BTRFS_BALANCE_ARGS_LIMIT)
+		bp += snprintf(bp, buf - bp + size_buf, "limit=%llu,",
+			       bargs->limit);
+
+	if (flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "limit=%u..%u,",
+			       bargs->limit_min, bargs->limit_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "stripes=%u..%u,",
+			       bargs->stripes_min, bargs->stripes_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_CONVERT) {
+		int index = btrfs_bg_flags_to_raid_index(bargs->target);
+
+		bp += snprintf(bp, buf - bp + size_buf, "convert=%s,",
+			       get_raid_name(index));
+	}
+
+	buf[bp - buf - 1] = '\0'; /* remove last , */
+	return bp - buf - 1;
+}
+
+static void describe_balance_start_or_resume(struct btrfs_fs_info *fs_info)
+{
+	u32 sz = 1024;
+	char tmp_buf[64];
+	char *buf;
+	char *bp;
+	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
+
+	buf = kzalloc(sz, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	bp = buf;
+	if (bctl->flags & BTRFS_BALANCE_FORCE)
+		bp += snprintf(bp, buf - bp + sz, "-f ");
+
+	if (bctl->flags & BTRFS_BALANCE_DATA) {
+		describe_balance_args(&bctl->data, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + sz, "-d %s ", tmp_buf);
+	}
+
+	if (bctl->flags & BTRFS_BALANCE_METADATA) {
+		describe_balance_args(&bctl->meta, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + sz, "-m %s ", tmp_buf);
+	}
+
+	if (bctl->flags & BTRFS_BALANCE_SYSTEM) {
+		describe_balance_args(&bctl->sys, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + sz, "-s %s ", tmp_buf);
+	}
+
+	buf[bp - buf - 1] = '\0'; /* remove last " "*/
+	btrfs_info(fs_info, "%s %s",
+		   bctl->flags & BTRFS_BALANCE_RESUME ?
+		   "balance: resume":"balance: start", buf);
+
+	kfree(buf);
+}
+
 /*
  * Should be called with balance mutexe held
  */
@@ -3950,6 +4052,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);
+	describe_balance_start_or_resume(fs_info);
 	mutex_unlock(&fs_info->balance_mutex);
 
 	ret = __btrfs_balance(fs_info);
@@ -3987,10 +4090,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 v4 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.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3->v4: nothing.
v2->v3: nothing.
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 df884e4fb0a2..d07158491505 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4054,6 +4054,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 v4 1/3] btrfs: add helper function describe_block_group()

[David Sterba]

On Wed, May 23, 2018 at 02:35:06PM +0800, Anand Jain wrote:
> Improve on describe_relocation() add a common helper function to describe
> the block groups.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v3->v4: Just pass full flag name in the define DESCRIBE_FLAG(flag,..),
> 	 so that it can be used at couple of more places.
> 	Rename describe_block_groups() to btrfs_describe_block_groups().
> 	Drop useless return u32.
> v3:	Born.
> 
>  fs/btrfs/relocation.c | 30 +++---------------------------
>  fs/btrfs/volumes.c    | 36 ++++++++++++++++++++++++++++++++++++
>  fs/btrfs/volumes.h    |  1 +
>  3 files changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 879b76fa881a..0434cebc2ea2 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4320,37 +4320,13 @@ static struct reloc_control *alloc_reloc_control(void)
>  static void describe_relocation(struct btrfs_fs_info *fs_info,
>  				struct btrfs_block_group_cache *block_group)
>  {
> -	char buf[128];		/* prefixed by a '|' that'll be dropped */
> -	u64 flags = block_group->flags;
> +	char buf[128];
>  
> -	/* Shouldn't happen */
> -	if (!flags) {
> -		strcpy(buf, "|NONE");
> -	} else {
> -		char *bp = buf;
> -
> -#define DESCRIBE_FLAG(f, d) \
> -		if (flags & BTRFS_BLOCK_GROUP_##f) { \
> -			bp += snprintf(bp, buf - bp + sizeof(buf), "|%s", d); \
> -			flags &= ~BTRFS_BLOCK_GROUP_##f; \
> -		}
> -		DESCRIBE_FLAG(DATA,     "data");
> -		DESCRIBE_FLAG(SYSTEM,   "system");
> -		DESCRIBE_FLAG(METADATA, "metadata");
> -		DESCRIBE_FLAG(RAID0,    "raid0");
> -		DESCRIBE_FLAG(RAID1,    "raid1");
> -		DESCRIBE_FLAG(DUP,      "dup");
> -		DESCRIBE_FLAG(RAID10,   "raid10");
> -		DESCRIBE_FLAG(RAID5,    "raid5");
> -		DESCRIBE_FLAG(RAID6,    "raid6");
> -		if (flags)
> -			snprintf(bp, buf - bp + sizeof(buf), "|0x%llx", flags);
> -#undef DESCRIBE_FLAG
> -	}
> +	btrfs_describe_block_groups(block_group->flags, buf, sizeof(buf));
>  
>  	btrfs_info(fs_info,
>  		   "relocating block group %llu flags %s",
> -		   block_group->key.objectid, buf + 1);
> +		   block_group->key.objectid, buf);
>  }
>  
>  /*
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index b6757b53c297..44f4799bf06f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -126,6 +126,42 @@ const char *get_raid_name(enum btrfs_raid_types type)
>  	return btrfs_raid_array[type].raid_name;
>  }
>  
> +void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
> +{
> +	int i;
> +	u32 ret;
> +	char *bp = buf;
> +	u64 flags = bg_flags;
> +
> +	if (!flags) {
> +		snprintf(bp, size_buf, "%s", "NONE");

This could be strcpy as the original code did, but that's a minor issue.

> +		return;
> +	}
> +
> +#define DESCRIBE_FLAG(f, d) \
> +	do { \
> +		if (flags & f) { \
> +			bp += snprintf(bp, buf - bp + size_buf, "%s|", d); \
> +			flags &= ~f; \
> +		} \
> +	} while (0)
> +
> +	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_DATA, "data");
> +	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_SYSTEM, "system");
> +	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_METADATA, "metadata");
> +	DESCRIBE_FLAG(BTRFS_AVAIL_ALLOC_BIT_SINGLE, "single");
> +	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++)
> +		DESCRIBE_FLAG(btrfs_raid_array[i].bg_flag,
> +			      btrfs_raid_array[i].raid_name);
> +#undef DESCRIBE_FLAG

Yeah, that's it, thanks.

Reviewed-by: David Sterba <dsterba@suse.com>

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

[David Sterba]

On Wed, May 23, 2018 at 02:35:07PM +0800, Anand Jain wrote:
> Balance arg info is an important information to be reviewed for the
> system audit. So this patch adds them to the kernel log.
> 
> Example:
> 
> ->btrfs bal start --full-balance -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs

--full-balance should not be here as it's for the case when no filter is
used.

>  kernel: BTRFS info (device sdc): balance: start -f -d usage=50,limit=10..20 -m soft,profiles=raid1,convert=single -s soft,profiles=raid1,convert=single

While this looks good, it would not work as is because of the dual
syntax of -d and -m, the argument is optional and with the space between
it'll fail with "btrfs filesystem balance start: too many arguments" .

At least the main parts of the string are copy&paste ready, so the
format can be like

balance: start force D:usage=50,limit=10..20 M:soft,profiles=raid1,convert=single S:soft,profiles=raid1,convert=single

or with "DATA:" "METADATA:"

> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v3->v4: Change log updated with new example.
> 	Log format is changed to almost match with the cli.
> 	snprintf drop %s for inline string.
> 	Print range as =%u..%u instead of min=%umax=%u.
> 	soft is under the args now.
> 	force is printed as -f.
> 
> v2->v3: Change log updated.
>         Make use of describe_block_group() added in 1/4
>         Drop using strcat instead use snprintf.
>         Logs string format updated as shown in the example above.
> 
> v1->v2: Change log update.
>         Move adding the logs for balance complete and end to a new patch
> 
>  fs/btrfs/volumes.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 104 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 44f4799bf06f..85f954f7f93e 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3806,6 +3806,108 @@ static inline int validate_convert_profile(struct btrfs_balance_args *bctl_arg,
>  		 (bctl_arg->target & ~allowed)));
>  }
>  
> +static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf,
> +				 u32 size_buf)
> +{
> +	char *bp = buf;
> +	u64 flags = bargs->flags;
> +	char tmp_buf[128];
> +
> +	if (!flags)
> +		return 0;
> +
> +	if (flags & BTRFS_BALANCE_ARGS_SOFT)
> +		bp += snprintf(bp, buf - bp + size_buf, "soft,");
> +
> +	if (flags & BTRFS_BALANCE_ARGS_PROFILES) {
> +		btrfs_describe_block_groups(bargs->profiles, tmp_buf,
> +					    sizeof(tmp_buf));
> +		bp += snprintf(bp, buf - bp + size_buf, "profiles=%s,",
> +			       tmp_buf);
> +	}
> +
> +	if (flags & BTRFS_BALANCE_ARGS_USAGE)
> +		bp += snprintf(bp, buf - bp + size_buf, "usage=%llu,",
> +			       bargs->usage);
> +
> +	if (flags & BTRFS_BALANCE_ARGS_USAGE_RANGE)
> +		bp += snprintf(bp, buf - bp + size_buf, "usage=%u..%u,",
> +			       bargs->usage_min, bargs->usage_max);
> +
> +	if (flags & BTRFS_BALANCE_ARGS_DEVID)
> +		bp += snprintf(bp, buf - bp + size_buf, "devid=%llu,",
> +			       bargs->devid);
> +
> +	if (flags & BTRFS_BALANCE_ARGS_DRANGE)
> +		bp += snprintf(bp, buf - bp + size_buf, "drange=%llu..%llu,",
> +			       bargs->pstart, bargs->pend);
> +
> +	if (flags & BTRFS_BALANCE_ARGS_VRANGE)
> +		bp += snprintf(bp, buf - bp + size_buf, "vrange%llu..%llu,",
> +			       bargs->vstart, bargs->vend);
> +
> +	if (flags & BTRFS_BALANCE_ARGS_LIMIT)
> +		bp += snprintf(bp, buf - bp + size_buf, "limit=%llu,",
> +			       bargs->limit);
> +
> +	if (flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE)
> +		bp += snprintf(bp, buf - bp + size_buf, "limit=%u..%u,",
> +			       bargs->limit_min, bargs->limit_max);
> +
> +	if (flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE)
> +		bp += snprintf(bp, buf - bp + size_buf, "stripes=%u..%u,",
> +			       bargs->stripes_min, bargs->stripes_max);
> +
> +	if (flags & BTRFS_BALANCE_ARGS_CONVERT) {
> +		int index = btrfs_bg_flags_to_raid_index(bargs->target);
> +
> +		bp += snprintf(bp, buf - bp + size_buf, "convert=%s,",
> +			       get_raid_name(index));
> +	}
> +
> +	buf[bp - buf - 1] = '\0'; /* remove last , */
> +	return bp - buf - 1;
> +}
> +
> +static void describe_balance_start_or_resume(struct btrfs_fs_info *fs_info)
> +{
> +	u32 sz = 1024;
> +	char tmp_buf[64];

This is too short, the profiles= can contain all profile names that
itself has 50 bytes, plus any filter, this will not fit the buffer.

Other than that, it looks good and I'll add it to for-next. We may still
want tweak the exact output format and buffer sizes, there's a bit of time.

Re: [PATCH v4 1/3] btrfs: add helper function describe_block_group()

[Anand Jain]

On 05/24/2018 08:43 PM, David Sterba wrote:
> On Wed, May 23, 2018 at 02:35:06PM +0800, Anand Jain wrote:
>> Improve on describe_relocation() add a common helper function to describe
>> the block groups.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v3->v4: Just pass full flag name in the define DESCRIBE_FLAG(flag,..),
>> 	 so that it can be used at couple of more places.
>> 	Rename describe_block_groups() to btrfs_describe_block_groups().
>> 	Drop useless return u32.
>> v3:	Born.
>>
>>   fs/btrfs/relocation.c | 30 +++---------------------------
>>   fs/btrfs/volumes.c    | 36 ++++++++++++++++++++++++++++++++++++
>>   fs/btrfs/volumes.h    |  1 +
>>   3 files changed, 40 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 879b76fa881a..0434cebc2ea2 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -4320,37 +4320,13 @@ static struct reloc_control *alloc_reloc_control(void)
>>   static void describe_relocation(struct btrfs_fs_info *fs_info,
>>   				struct btrfs_block_group_cache *block_group)
>>   {
>> -	char buf[128];		/* prefixed by a '|' that'll be dropped */
>> -	u64 flags = block_group->flags;
>> +	char buf[128];
>>   
>> -	/* Shouldn't happen */
>> -	if (!flags) {
>> -		strcpy(buf, "|NONE");
>> -	} else {
>> -		char *bp = buf;
>> -
>> -#define DESCRIBE_FLAG(f, d) \
>> -		if (flags & BTRFS_BLOCK_GROUP_##f) { \
>> -			bp += snprintf(bp, buf - bp + sizeof(buf), "|%s", d); \
>> -			flags &= ~BTRFS_BLOCK_GROUP_##f; \
>> -		}
>> -		DESCRIBE_FLAG(DATA,     "data");
>> -		DESCRIBE_FLAG(SYSTEM,   "system");
>> -		DESCRIBE_FLAG(METADATA, "metadata");
>> -		DESCRIBE_FLAG(RAID0,    "raid0");
>> -		DESCRIBE_FLAG(RAID1,    "raid1");
>> -		DESCRIBE_FLAG(DUP,      "dup");
>> -		DESCRIBE_FLAG(RAID10,   "raid10");
>> -		DESCRIBE_FLAG(RAID5,    "raid5");
>> -		DESCRIBE_FLAG(RAID6,    "raid6");
>> -		if (flags)
>> -			snprintf(bp, buf - bp + sizeof(buf), "|0x%llx", flags);
>> -#undef DESCRIBE_FLAG
>> -	}
>> +	btrfs_describe_block_groups(block_group->flags, buf, sizeof(buf));
>>   
>>   	btrfs_info(fs_info,
>>   		   "relocating block group %llu flags %s",
>> -		   block_group->key.objectid, buf + 1);
>> +		   block_group->key.objectid, buf);
>>   }
>>   
>>   /*
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index b6757b53c297..44f4799bf06f 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -126,6 +126,42 @@ const char *get_raid_name(enum btrfs_raid_types type)
>>   	return btrfs_raid_array[type].raid_name;
>>   }
>>   
>> +void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
>> +{
>> +	int i;
>> +	u32 ret;
>> +	char *bp = buf;
>> +	u64 flags = bg_flags;
>> +
>> +	if (!flags) {
>> +		snprintf(bp, size_buf, "%s", "NONE");
> 
> This could be strcpy as the original code did, but that's a minor issue.

  I am right at it. Will fix it anyway. Pls find v4.1.

Thanks, Anand

>> +		return;
>> +	}
>> +
>> +#define DESCRIBE_FLAG(f, d) \
>> +	do { \
>> +		if (flags & f) { \
>> +			bp += snprintf(bp, buf - bp + size_buf, "%s|", d); \
>> +			flags &= ~f; \
>> +		} \
>> +	} while (0)
>> +
>> +	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_DATA, "data");
>> +	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_SYSTEM, "system");
>> +	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_METADATA, "metadata");
>> +	DESCRIBE_FLAG(BTRFS_AVAIL_ALLOC_BIT_SINGLE, "single");
>> +	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++)
>> +		DESCRIBE_FLAG(btrfs_raid_array[i].bg_flag,
>> +			      btrfs_raid_array[i].raid_name);
>> +#undef DESCRIBE_FLAG
> 
> Yeah, that's it, thanks.
> 
> Reviewed-by: David Sterba <dsterba@suse.com>
> --
> 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 v4.1 1/3] btrfs: add helper function describe_block_group()

[Anand Jain]

Improve on describe_relocation() add a common helper function to describe
the block groups.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.com>
---
v4->v4.1: Use strcpy(buf, "|NONE"); as in the original
v3->v4: Just pass full flag name in the define DESCRIBE_FLAG(flag,..),
	 so that it can be used at couple of more places.
	Rename describe_block_groups() to btrfs_describe_block_groups().
	Drop useless return u32.
v3:	Born.

 fs/btrfs/relocation.c | 30 +++---------------------------
 fs/btrfs/volumes.c    | 36 ++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.h    |  1 +
 3 files changed, 40 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 879b76fa881a..0434cebc2ea2 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4320,37 +4320,13 @@ static struct reloc_control *alloc_reloc_control(void)
 static void describe_relocation(struct btrfs_fs_info *fs_info,
 				struct btrfs_block_group_cache *block_group)
 {
-	char buf[128];		/* prefixed by a '|' that'll be dropped */
-	u64 flags = block_group->flags;
+	char buf[128];
 
-	/* Shouldn't happen */
-	if (!flags) {
-		strcpy(buf, "|NONE");
-	} else {
-		char *bp = buf;
-
-#define DESCRIBE_FLAG(f, d) \
-		if (flags & BTRFS_BLOCK_GROUP_##f) { \
-			bp += snprintf(bp, buf - bp + sizeof(buf), "|%s", d); \
-			flags &= ~BTRFS_BLOCK_GROUP_##f; \
-		}
-		DESCRIBE_FLAG(DATA,     "data");
-		DESCRIBE_FLAG(SYSTEM,   "system");
-		DESCRIBE_FLAG(METADATA, "metadata");
-		DESCRIBE_FLAG(RAID0,    "raid0");
-		DESCRIBE_FLAG(RAID1,    "raid1");
-		DESCRIBE_FLAG(DUP,      "dup");
-		DESCRIBE_FLAG(RAID10,   "raid10");
-		DESCRIBE_FLAG(RAID5,    "raid5");
-		DESCRIBE_FLAG(RAID6,    "raid6");
-		if (flags)
-			snprintf(bp, buf - bp + sizeof(buf), "|0x%llx", flags);
-#undef DESCRIBE_FLAG
-	}
+	btrfs_describe_block_groups(block_group->flags, buf, sizeof(buf));
 
 	btrfs_info(fs_info,
 		   "relocating block group %llu flags %s",
-		   block_group->key.objectid, buf + 1);
+		   block_group->key.objectid, buf);
 }
 
 /*
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ddabea6a8e8f..e858882a98eb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -126,6 +126,42 @@ const char *get_raid_name(enum btrfs_raid_types type)
 	return btrfs_raid_array[type].raid_name;
 }
 
+void btrfs_describe_block_groups(u64 bg_flags, char *buf, u32 size_buf)
+{
+	int i;
+	u32 ret;
+	char *bp = buf;
+	u64 flags = bg_flags;
+
+	if (!flags) {
+		strcpy(bp, "NONE");
+		return;
+	}
+
+#define DESCRIBE_FLAG(f, d) \
+	do { \
+		if (flags & f) { \
+			bp += snprintf(bp, buf - bp + size_buf, "%s|", d); \
+			flags &= ~f; \
+		} \
+	} while (0)
+
+	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_DATA, "data");
+	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_SYSTEM, "system");
+	DESCRIBE_FLAG(BTRFS_BLOCK_GROUP_METADATA, "metadata");
+	DESCRIBE_FLAG(BTRFS_AVAIL_ALLOC_BIT_SINGLE, "single");
+	for (i = 0; i < BTRFS_NR_RAID_TYPES; i++)
+		DESCRIBE_FLAG(btrfs_raid_array[i].bg_flag,
+			      btrfs_raid_array[i].raid_name);
+#undef DESCRIBE_FLAG
+
+	if (flags)
+		bp += snprintf(bp, buf - bp + size_buf, "0x%llx|", flags);
+
+	ret = bp - buf - 1;
+	buf[ret] = '\0'; /* remove last | */
+}
+
 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);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 5139ec8daf4c..5b9637a3ed48 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -433,6 +433,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
 int btrfs_balance(struct btrfs_fs_info *fs_info,
 		  struct btrfs_balance_control *bctl,
 		  struct btrfs_ioctl_balance_args *bargs);
+void btrfs_describe_block_groups(u64 flags, char *buf, u32 size_buf);
 int btrfs_resume_balance_async(struct btrfs_fs_info *fs_info);
 int btrfs_recover_balance(struct btrfs_fs_info *fs_info);
 int btrfs_pause_balance(struct btrfs_fs_info *fs_info);
-- 
2.7.0

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

[Anand Jain]

On 05/24/2018 09:03 PM, David Sterba wrote:
> On Wed, May 23, 2018 at 02:35:07PM +0800, Anand Jain wrote:
>> Balance arg info is an important information to be reviewed for the
>> system audit. So this patch adds them to the kernel log.
>>
>> Example:
>>
>> ->btrfs bal start --full-balance -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs
> 
> --full-balance should not be here as it's for the case when no filter is
> used.

  Oh yep. I should remove that.

>>   kernel: BTRFS info (device sdc): balance: start -f -d usage=50,limit=10..20 -m soft,profiles=raid1,convert=single -s soft,profiles=raid1,convert=single
> 
> While this looks good, it would not work as is because of the dual
> syntax of -d and -m, the argument is optional and with the space between
> it'll fail with "btrfs filesystem balance start: too many arguments" .

  I have dropped the space after -d -m -s and cut and paste works.
  I have it in the version v4.1a.

> At least the main parts of the string are copy&paste ready, so the
> format can be like
> 
> balance: start force D:usage=50,limit=10..20 M:soft,profiles=raid1,convert=single S:soft,profiles=raid1,convert=single

  This not bad either. But I liked the idea of just plain of cut and
  paste and I am fine with either format. And I have this in the version
  v4.1b. So can you pls pick either v4.1a or v4.1b for the integration.

more below..

> or with "DATA:" "METADATA:"

>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v3->v4: Change log updated with new example.
>> 	Log format is changed to almost match with the cli.
>> 	snprintf drop %s for inline string.
>> 	Print range as =%u..%u instead of min=%umax=%u.
>> 	soft is under the args now.
>> 	force is printed as -f.
>>
>> v2->v3: Change log updated.
>>          Make use of describe_block_group() added in 1/4
>>          Drop using strcat instead use snprintf.
>>          Logs string format updated as shown in the example above.
>>
>> v1->v2: Change log update.
>>          Move adding the logs for balance complete and end to a new patch
>>
>>   fs/btrfs/volumes.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 104 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 44f4799bf06f..85f954f7f93e 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -3806,6 +3806,108 @@ static inline int validate_convert_profile(struct btrfs_balance_args *bctl_arg,
>>   		 (bctl_arg->target & ~allowed)));
>>   }
>>   
>> +static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf,
>> +				 u32 size_buf)
>> +{
>> +	char *bp = buf;
>> +	u64 flags = bargs->flags;
>> +	char tmp_buf[128];
>> +
>> +	if (!flags)
>> +		return 0;
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_SOFT)
>> +		bp += snprintf(bp, buf - bp + size_buf, "soft,");
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_PROFILES) {
>> +		btrfs_describe_block_groups(bargs->profiles, tmp_buf,
>> +					    sizeof(tmp_buf));
>> +		bp += snprintf(bp, buf - bp + size_buf, "profiles=%s,",
>> +			       tmp_buf);
>> +	}
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_USAGE)
>> +		bp += snprintf(bp, buf - bp + size_buf, "usage=%llu,",
>> +			       bargs->usage);
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_USAGE_RANGE)
>> +		bp += snprintf(bp, buf - bp + size_buf, "usage=%u..%u,",
>> +			       bargs->usage_min, bargs->usage_max);
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_DEVID)
>> +		bp += snprintf(bp, buf - bp + size_buf, "devid=%llu,",
>> +			       bargs->devid);
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_DRANGE)
>> +		bp += snprintf(bp, buf - bp + size_buf, "drange=%llu..%llu,",
>> +			       bargs->pstart, bargs->pend);
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_VRANGE)
>> +		bp += snprintf(bp, buf - bp + size_buf, "vrange%llu..%llu,",
>> +			       bargs->vstart, bargs->vend);
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_LIMIT)
>> +		bp += snprintf(bp, buf - bp + size_buf, "limit=%llu,",
>> +			       bargs->limit);
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE)
>> +		bp += snprintf(bp, buf - bp + size_buf, "limit=%u..%u,",
>> +			       bargs->limit_min, bargs->limit_max);
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE)
>> +		bp += snprintf(bp, buf - bp + size_buf, "stripes=%u..%u,",
>> +			       bargs->stripes_min, bargs->stripes_max);
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_CONVERT) {
>> +		int index = btrfs_bg_flags_to_raid_index(bargs->target);
>> +
>> +		bp += snprintf(bp, buf - bp + size_buf, "convert=%s,",
>> +			       get_raid_name(index));
>> +	}
>> +
>> +	buf[bp - buf - 1] = '\0'; /* remove last , */
>> +	return bp - buf - 1;
>> +}
>> +
>> +static void describe_balance_start_or_resume(struct btrfs_fs_info *fs_info)
>> +{
>> +	u32 sz = 1024;

  I have taken this opportunity to rename this to size_buf.

>> +	char tmp_buf[64];
> 
> This is too short, the profiles= can contain all profile names that
> itself has 50 bytes, plus any filter, this will not fit the buffer.

  oh yes. Theoretically it should be more than the buffer in the
  describe_balance_args() + btrfs_describe_block_groups (128). Given
  that the user might specify all the options with the lengthy stripe
  range in describe_balance_args(), I have made it 192 bytes.

> Other than that, it looks good and I'll add it to for-next. We may still
> want tweak the exact output format and buffer sizes, there's a bit of time.

  I have both types sent (v4.1a: no space v4.1b: cap abbreviates) pls
  pick one. I would go by the method which ever is more easier to cut
  and paste.

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
> 

[PATCH v4.1a 2/3] btrfs: balance: add args info during start and resume

[Anand Jain]

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

Example:
->btrfs bal start -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs

kernel: BTRFS info (device sdb): balance: start force D:usage=50,limit=10..20 M:soft,profiles=raid1,convert=single S:soft,profiles=raid1,convert=single

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4->v4.1a: Rename last missed sz to size_buf.
	   Allocate at least (64*3) 192 bytes for tmp_buf.
	   Log format change: Use CAPS abbreviate for group type,
	    as in the above Example.
v3->v4: Change log updated with new example.
	Log format is changed to almost match with the cli.
	snprintf drop %s for inline string.
	Print range as =%u..%u instead of min=%umax=%u.
	soft is under the args now.
	force is printed as -f.

v2->v3: Change log updated.
        Make use of describe_block_group() added in 1/4
        Drop using strcat instead use snprintf.
        Logs string format updated as shown in the example above.

v1->v2: Change log update.
        Move adding the logs for balance complete and end to a new patch

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e858882a98eb..833dde6168de 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3809,6 +3809,108 @@ static inline int validate_convert_profile(struct btrfs_balance_args *bctl_arg,
 		 (bctl_arg->target & ~allowed)));
 }
 
+static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf,
+				 u32 size_buf)
+{
+	char *bp = buf;
+	u64 flags = bargs->flags;
+	char tmp_buf[128];
+
+	if (!flags)
+		return 0;
+
+	if (flags & BTRFS_BALANCE_ARGS_SOFT)
+		bp += snprintf(bp, buf - bp + size_buf, "soft,");
+
+	if (flags & BTRFS_BALANCE_ARGS_PROFILES) {
+		btrfs_describe_block_groups(bargs->profiles, tmp_buf,
+					    sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "profiles=%s,",
+			       tmp_buf);
+	}
+
+	if (flags & BTRFS_BALANCE_ARGS_USAGE)
+		bp += snprintf(bp, buf - bp + size_buf, "usage=%llu,",
+			       bargs->usage);
+
+	if (flags & BTRFS_BALANCE_ARGS_USAGE_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "usage=%u..%u,",
+			       bargs->usage_min, bargs->usage_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_DEVID)
+		bp += snprintf(bp, buf - bp + size_buf, "devid=%llu,",
+			       bargs->devid);
+
+	if (flags & BTRFS_BALANCE_ARGS_DRANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "drange=%llu..%llu,",
+			       bargs->pstart, bargs->pend);
+
+	if (flags & BTRFS_BALANCE_ARGS_VRANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "vrange%llu..%llu,",
+			       bargs->vstart, bargs->vend);
+
+	if (flags & BTRFS_BALANCE_ARGS_LIMIT)
+		bp += snprintf(bp, buf - bp + size_buf, "limit=%llu,",
+			       bargs->limit);
+
+	if (flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "limit=%u..%u,",
+			       bargs->limit_min, bargs->limit_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "stripes=%u..%u,",
+			       bargs->stripes_min, bargs->stripes_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_CONVERT) {
+		int index = btrfs_bg_flags_to_raid_index(bargs->target);
+
+		bp += snprintf(bp, buf - bp + size_buf, "convert=%s,",
+			       get_raid_name(index));
+	}
+
+	buf[bp - buf - 1] = '\0'; /* remove last , */
+	return bp - buf - 1;
+}
+
+static void describe_balance_start_or_resume(struct btrfs_fs_info *fs_info)
+{
+	u32 size_buf = 1024;
+	char tmp_buf[192];
+	char *buf;
+	char *bp;
+	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
+
+	buf = kzalloc(size_buf, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	bp = buf;
+	if (bctl->flags & BTRFS_BALANCE_FORCE)
+		bp += snprintf(bp, buf - bp + size_buf, "force ");
+
+	if (bctl->flags & BTRFS_BALANCE_DATA) {
+		describe_balance_args(&bctl->data, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "D:%s ", tmp_buf);
+	}
+
+	if (bctl->flags & BTRFS_BALANCE_METADATA) {
+		describe_balance_args(&bctl->meta, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "M:%s ", tmp_buf);
+	}
+
+	if (bctl->flags & BTRFS_BALANCE_SYSTEM) {
+		describe_balance_args(&bctl->sys, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "S:%s ", tmp_buf);
+	}
+
+	buf[bp - buf - 1] = '\0'; /* remove last " "*/
+	btrfs_info(fs_info, "%s %s",
+		   bctl->flags & BTRFS_BALANCE_RESUME ?
+		   "balance: resume":"balance: start", buf);
+
+	kfree(buf);
+}
+
 /*
  * Should be called with balance mutexe held
  */
@@ -3953,6 +4055,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);
+	describe_balance_start_or_resume(fs_info);
 	mutex_unlock(&fs_info->balance_mutex);
 
 	ret = __btrfs_balance(fs_info);
@@ -3990,10 +4093,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 v4.1b 2/3] btrfs: balance: add args info during start and resume

[Anand Jain]

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

Example:
->btrfs bal start -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs

kernel: BTRFS info (device sdb): balance: start -f -dusage=50,limit=10..20 -msoft,profiles=raid1,convert=single -ssoft,profiles=raid1,convert=single

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4->v4.1b: Rename last missed sz to size_buf.
	   Allocate at least (64*3) 192 bytes for tmp_buf.
	   Log format change: No space after group type,
	    as in the above Example.
v3->v4: Change log updated with new example.
	Log format is changed to almost match with the cli.
	snprintf drop %s for inline string.
	Print range as =%u..%u instead of min=%umax=%u.
	soft is under the args now.
	force is printed as -f.

v2->v3: Change log updated.
        Make use of describe_block_group() added in 1/4
        Drop using strcat instead use snprintf.
        Logs string format updated as shown in the example above.

v1->v2: Change log update.
        Move adding the logs for balance complete and end to a new patch

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e858882a98eb..c02a4fb8ae7e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3809,6 +3809,108 @@ static inline int validate_convert_profile(struct btrfs_balance_args *bctl_arg,
 		 (bctl_arg->target & ~allowed)));
 }
 
+static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf,
+				 u32 size_buf)
+{
+	char *bp = buf;
+	u64 flags = bargs->flags;
+	char tmp_buf[128];
+
+	if (!flags)
+		return 0;
+
+	if (flags & BTRFS_BALANCE_ARGS_SOFT)
+		bp += snprintf(bp, buf - bp + size_buf, "soft,");
+
+	if (flags & BTRFS_BALANCE_ARGS_PROFILES) {
+		btrfs_describe_block_groups(bargs->profiles, tmp_buf,
+					    sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "profiles=%s,",
+			       tmp_buf);
+	}
+
+	if (flags & BTRFS_BALANCE_ARGS_USAGE)
+		bp += snprintf(bp, buf - bp + size_buf, "usage=%llu,",
+			       bargs->usage);
+
+	if (flags & BTRFS_BALANCE_ARGS_USAGE_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "usage=%u..%u,",
+			       bargs->usage_min, bargs->usage_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_DEVID)
+		bp += snprintf(bp, buf - bp + size_buf, "devid=%llu,",
+			       bargs->devid);
+
+	if (flags & BTRFS_BALANCE_ARGS_DRANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "drange=%llu..%llu,",
+			       bargs->pstart, bargs->pend);
+
+	if (flags & BTRFS_BALANCE_ARGS_VRANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "vrange%llu..%llu,",
+			       bargs->vstart, bargs->vend);
+
+	if (flags & BTRFS_BALANCE_ARGS_LIMIT)
+		bp += snprintf(bp, buf - bp + size_buf, "limit=%llu,",
+			       bargs->limit);
+
+	if (flags & BTRFS_BALANCE_ARGS_LIMIT_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "limit=%u..%u,",
+			       bargs->limit_min, bargs->limit_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_STRIPES_RANGE)
+		bp += snprintf(bp, buf - bp + size_buf, "stripes=%u..%u,",
+			       bargs->stripes_min, bargs->stripes_max);
+
+	if (flags & BTRFS_BALANCE_ARGS_CONVERT) {
+		int index = btrfs_bg_flags_to_raid_index(bargs->target);
+
+		bp += snprintf(bp, buf - bp + size_buf, "convert=%s,",
+			       get_raid_name(index));
+	}
+
+	buf[bp - buf - 1] = '\0'; /* remove last , */
+	return bp - buf - 1;
+}
+
+static void describe_balance_start_or_resume(struct btrfs_fs_info *fs_info)
+{
+	u32 size_buf = 1024;
+	char tmp_buf[192];
+	char *buf;
+	char *bp;
+	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
+
+	buf = kzalloc(size_buf, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	bp = buf;
+	if (bctl->flags & BTRFS_BALANCE_FORCE)
+		bp += snprintf(bp, buf - bp + size_buf, "-f ");
+
+	if (bctl->flags & BTRFS_BALANCE_DATA) {
+		describe_balance_args(&bctl->data, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "-d%s ", tmp_buf);
+	}
+
+	if (bctl->flags & BTRFS_BALANCE_METADATA) {
+		describe_balance_args(&bctl->meta, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "-m%s ", tmp_buf);
+	}
+
+	if (bctl->flags & BTRFS_BALANCE_SYSTEM) {
+		describe_balance_args(&bctl->sys, tmp_buf, sizeof(tmp_buf));
+		bp += snprintf(bp, buf - bp + size_buf, "-s%s ", tmp_buf);
+	}
+
+	buf[bp - buf - 1] = '\0'; /* remove last " "*/
+	btrfs_info(fs_info, "%s %s",
+		   bctl->flags & BTRFS_BALANCE_RESUME ?
+		   "balance: resume":"balance: start", buf);
+
+	kfree(buf);
+}
+
 /*
  * Should be called with balance mutexe held
  */
@@ -3953,6 +4055,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);
+	describe_balance_start_or_resume(fs_info);
 	mutex_unlock(&fs_info->balance_mutex);
 
 	ret = __btrfs_balance(fs_info);
@@ -3990,10 +4093,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

Re: [PATCH v4.1b 2/3] btrfs: balance: add args info during start and resume

[David Sterba]

On Fri, May 25, 2018 at 11:05:47AM +0800, Anand Jain wrote:
> Balance arg info is an important information to be reviewed for the
> system audit. So this patch adds them to the kernel log.
> 
> Example:
> ->btrfs bal start -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs
> 
> kernel: BTRFS info (device sdb): balance: start -f -dusage=50,limit=10..20 -msoft,profiles=raid1,convert=single -ssoft,profiles=raid1,convert=single
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v4->v4.1b: Rename last missed sz to size_buf.

Please send a full version instead of the incremental bumps to
individual patches. I try to take the last version but am never sure
what exactly gets merged.

> +static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf,
> +				 u32 size_buf)
> +{
> +	char *bp = buf;
> +	u64 flags = bargs->flags;
> +	char tmp_buf[128];
> +
> +	if (!flags)
> +		return 0;
> +
> +	if (flags & BTRFS_BALANCE_ARGS_SOFT)
> +		bp += snprintf(bp, buf - bp + size_buf, "soft,");

I have some suspicion that this snprintf conscturct is not safe when the
value of 'buf - bp + size_buf' becomes accidentally negative. A quick
test showed that a sequence of snprintf and incremented bp does not stop
writing when the size_buf limit is hit and happily overwrites the
memory.

We'd have to check that the negative value is never passed to snprintf.
The patches are potponed until the issue is resolved, either to verify
that it's a false alert or the bounds are correctly checked.

The bug should not be in the original code as the buffer is known to be
large enough for all the printed bg flags.

I'm sorry to delay the patches, the log messages are useful, but I want
to be sure that there are no random memory overwrites introduced.

Re: [PATCH v4.1b 2/3] btrfs: balance: add args info during start and resume

[Anand Jain]

On 05/31/2018 05:47 PM, David Sterba wrote:
> On Fri, May 25, 2018 at 11:05:47AM +0800, Anand Jain wrote:
>> Balance arg info is an important information to be reviewed for the
>> system audit. So this patch adds them to the kernel log.
>>
>> Example:
>> ->btrfs bal start -f -mprofiles=raid1,convert=single,soft -dlimit=10..20,usage=50 /btrfs
>>
>> kernel: BTRFS info (device sdb): balance: start -f -dusage=50,limit=10..20 -msoft,profiles=raid1,convert=single -ssoft,profiles=raid1,convert=single
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v4->v4.1b: Rename last missed sz to size_buf.
> 
> Please send a full version instead of the incremental bumps to
> individual patches. I try to take the last version but am never sure
> what exactly gets merged.
> 
>> +static u32 describe_balance_args(struct btrfs_balance_args *bargs, char *buf,
>> +				 u32 size_buf)
>> +{
>> +	char *bp = buf;
>> +	u64 flags = bargs->flags;
>> +	char tmp_buf[128];
>> +
>> +	if (!flags)
>> +		return 0;
>> +
>> +	if (flags & BTRFS_BALANCE_ARGS_SOFT)
>> +		bp += snprintf(bp, buf - bp + size_buf, "soft,");
> 
> I have some suspicion that this snprintf conscturct is not safe when the
> value of 'buf - bp + size_buf' becomes accidentally negative. A quick
> test showed that a sequence of snprintf and incremented bp does not stop
> writing when the size_buf limit is hit and happily overwrites the
> memory.
> 
> We'd have to check that the negative value is never passed to snprintf.
> The patches are potponed until the issue is resolved, either to verify
> that it's a false alert or the bounds are correctly checked.
> 
> The bug should not be in the original code as the buffer is known to be
> large enough for all the printed bg flags.

  Agreed, possibilities of accidental -ve value for snprintf.
  It followed the original code, but as you said the data length
  is more deterministic there.
  I am fixing this in both new and old original code. Pls find v5.
  Sorry for the delay, I took some time to get a fresh look at it.

Thanks, Anand


> I'm sorry to delay the patches, the log messages are useful, but I want
> to be sure that there are no random memory overwrites introduced.
> --
> 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
>