[PATCH] btrfs-progs: Format change for btrfs fi df

[PATCH] btrfs-progs: Format change for btrfs fi df

[Qu Wenruo]

The GlobalReserve space in 'btrfs fi df' is always confusing for a lot
of users.
As it is not a special chunk type like DATA or METADATA, it's in fact a
sub-type of METADATA.

So change the output to skip GlobalReserve by default, and adding its
total to metadata used.
And add a new option '-r|--reserve' to show the GlobalReserve, but skip
the profile of GlobalReserve.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 Documentation/btrfs-filesystem.asciidoc |  8 ++++++
 cmds-filesystem.c                       | 51 ++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/Documentation/btrfs-filesystem.asciidoc b/Documentation/btrfs-filesystem.asciidoc
index 31cd51b..510c23f 100644
--- a/Documentation/btrfs-filesystem.asciidoc
+++ b/Documentation/btrfs-filesystem.asciidoc
@@ -22,6 +22,14 @@ Show space usage information for a mount point.
 +
 `Options`
 +
+-r|--reserve::::
+also show Global Reserve space info.
++
+Global Reserve space is reserved space from metadata. It's reserved for Btrfs
+metadata COW.
++
+It will be counted as 'used' space in metadata space info.
++
 -b|--raw::::
 raw numbers in bytes, without the 'B' suffix
 -h|--human-readable::::
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 25317fa..26e62e0 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -123,6 +123,8 @@ static const char * const filesystem_cmd_group_usage[] = {
 static const char * const cmd_filesystem_df_usage[] = {
 	"btrfs filesystem df [options] <path>",
 	"Show space usage information for a mount point",
+	"",
+	"-r|--reserve   show global reserve space info"
 	HELPINFO_UNITS_SHORT_LONG,
 	NULL
 };
@@ -175,12 +177,32 @@ static int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret)
 	return 0;
 }
 
-static void print_df(struct btrfs_ioctl_space_args *sargs, unsigned unit_mode)
+static void print_df(struct btrfs_ioctl_space_args *sargs, unsigned unit_mode,
+		     int show_reserve)
 {
 	u64 i;
+	u64 global_reserve = 0;
 	struct btrfs_ioctl_space_info *sp = sargs->spaces;
 
+	/* First iterate to get global reserve space size */
 	for (i = 0; i < sargs->total_spaces; i++, sp++) {
+		if (sp->flags & BTRFS_SPACE_INFO_GLOBAL_RSV)
+			global_reserve = sp->total_bytes;
+	}
+
+	for (i = 0, sp = sargs->spaces; i < sargs->total_spaces; i++, sp++) {
+		if (sp->flags & BTRFS_SPACE_INFO_GLOBAL_RSV) {
+			if (!show_reserve)
+				continue;
+			printf(" \\- %s: reserved=%s, used=%s\n",
+				btrfs_group_type_str(sp->flags),
+				pretty_size_mode(sp->total_bytes, unit_mode),
+				pretty_size_mode(sp->used_bytes, unit_mode));
+			continue;
+		}
+
+		if (sp->flags & BTRFS_BLOCK_GROUP_METADATA)
+			sp->used_bytes += global_reserve;
 		printf("%s, %s: total=%s, used=%s\n",
 			btrfs_group_type_str(sp->flags),
 			btrfs_group_profile_str(sp->flags),
@@ -196,14 +218,35 @@ static int cmd_filesystem_df(int argc, char **argv)
 	int fd;
 	char *path;
 	DIR *dirstream = NULL;
+	int show_reserve = 0;
 	unsigned unit_mode;
 
 	unit_mode = get_unit_mode_from_arg(&argc, argv, 1);
 
-	if (argc != 2 || argv[1][0] == '-')
+	while (1) {
+		int c;
+		static const struct option long_options[] = {
+			{ "reserve", no_argument, NULL, 'r'},
+			{ NULL, 0, NULL, 0}
+		};
+
+		c = getopt_long(argc, argv, "r", long_options, NULL);
+		if (c < 0)
+			break;
+		switch (c) {
+		case 'r':
+			show_reserve = 1;
+			break;
+		default:
+			usage(cmd_filesystem_df_usage);
+		}
+	}
+
+	argc = argc - optind;
+	if (check_argc_exact(argc, 1))
 		usage(cmd_filesystem_df_usage);
 
-	path = argv[1];
+	path = argv[optind];
 
 	fd = btrfs_open_dir(path, &dirstream, 1);
 	if (fd < 0)
@@ -212,7 +255,7 @@ static int cmd_filesystem_df(int argc, char **argv)
 	ret = get_df(fd, &sargs);
 
 	if (ret == 0) {
-		print_df(sargs, unit_mode);
+		print_df(sargs, unit_mode, show_reserve);
 		free(sargs);
 	} else {
 		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(-ret));
-- 
2.6.3

Re: [PATCH] btrfs-progs: Format change for btrfs fi df

[Qu Wenruo]

Hi David,

Would you please consider merge this patch?

As current output is already quite confusing for a lot of users who are 
not familiar with btrfs.

Thanks,
Qu

Qu Wenruo wrote on 2015/12/14 13:19 +0800:
> The GlobalReserve space in 'btrfs fi df' is always confusing for a lot
> of users.
> As it is not a special chunk type like DATA or METADATA, it's in fact a
> sub-type of METADATA.
>
> So change the output to skip GlobalReserve by default, and adding its
> total to metadata used.
> And add a new option '-r|--reserve' to show the GlobalReserve, but skip
> the profile of GlobalReserve.
>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>   Documentation/btrfs-filesystem.asciidoc |  8 ++++++
>   cmds-filesystem.c                       | 51 ++++++++++++++++++++++++++++++---
>   2 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/btrfs-filesystem.asciidoc b/Documentation/btrfs-filesystem.asciidoc
> index 31cd51b..510c23f 100644
> --- a/Documentation/btrfs-filesystem.asciidoc
> +++ b/Documentation/btrfs-filesystem.asciidoc
> @@ -22,6 +22,14 @@ Show space usage information for a mount point.
>   +
>   `Options`
>   +
> +-r|--reserve::::
> +also show Global Reserve space info.
> ++
> +Global Reserve space is reserved space from metadata. It's reserved for Btrfs
> +metadata COW.
> ++
> +It will be counted as 'used' space in metadata space info.
> ++
>   -b|--raw::::
>   raw numbers in bytes, without the 'B' suffix
>   -h|--human-readable::::
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 25317fa..26e62e0 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -123,6 +123,8 @@ static const char * const filesystem_cmd_group_usage[] = {
>   static const char * const cmd_filesystem_df_usage[] = {
>   	"btrfs filesystem df [options] <path>",
>   	"Show space usage information for a mount point",
> +	"",
> +	"-r|--reserve   show global reserve space info"
>   	HELPINFO_UNITS_SHORT_LONG,
>   	NULL
>   };
> @@ -175,12 +177,32 @@ static int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret)
>   	return 0;
>   }
>
> -static void print_df(struct btrfs_ioctl_space_args *sargs, unsigned unit_mode)
> +static void print_df(struct btrfs_ioctl_space_args *sargs, unsigned unit_mode,
> +		     int show_reserve)
>   {
>   	u64 i;
> +	u64 global_reserve = 0;
>   	struct btrfs_ioctl_space_info *sp = sargs->spaces;
>
> +	/* First iterate to get global reserve space size */
>   	for (i = 0; i < sargs->total_spaces; i++, sp++) {
> +		if (sp->flags & BTRFS_SPACE_INFO_GLOBAL_RSV)
> +			global_reserve = sp->total_bytes;
> +	}
> +
> +	for (i = 0, sp = sargs->spaces; i < sargs->total_spaces; i++, sp++) {
> +		if (sp->flags & BTRFS_SPACE_INFO_GLOBAL_RSV) {
> +			if (!show_reserve)
> +				continue;
> +			printf(" \\- %s: reserved=%s, used=%s\n",
> +				btrfs_group_type_str(sp->flags),
> +				pretty_size_mode(sp->total_bytes, unit_mode),
> +				pretty_size_mode(sp->used_bytes, unit_mode));
> +			continue;
> +		}
> +
> +		if (sp->flags & BTRFS_BLOCK_GROUP_METADATA)
> +			sp->used_bytes += global_reserve;
>   		printf("%s, %s: total=%s, used=%s\n",
>   			btrfs_group_type_str(sp->flags),
>   			btrfs_group_profile_str(sp->flags),
> @@ -196,14 +218,35 @@ static int cmd_filesystem_df(int argc, char **argv)
>   	int fd;
>   	char *path;
>   	DIR *dirstream = NULL;
> +	int show_reserve = 0;
>   	unsigned unit_mode;
>
>   	unit_mode = get_unit_mode_from_arg(&argc, argv, 1);
>
> -	if (argc != 2 || argv[1][0] == '-')
> +	while (1) {
> +		int c;
> +		static const struct option long_options[] = {
> +			{ "reserve", no_argument, NULL, 'r'},
> +			{ NULL, 0, NULL, 0}
> +		};
> +
> +		c = getopt_long(argc, argv, "r", long_options, NULL);
> +		if (c < 0)
> +			break;
> +		switch (c) {
> +		case 'r':
> +			show_reserve = 1;
> +			break;
> +		default:
> +			usage(cmd_filesystem_df_usage);
> +		}
> +	}
> +
> +	argc = argc - optind;
> +	if (check_argc_exact(argc, 1))
>   		usage(cmd_filesystem_df_usage);
>
> -	path = argv[1];
> +	path = argv[optind];
>
>   	fd = btrfs_open_dir(path, &dirstream, 1);
>   	if (fd < 0)
> @@ -212,7 +255,7 @@ static int cmd_filesystem_df(int argc, char **argv)
>   	ret = get_df(fd, &sargs);
>
>   	if (ret == 0) {
> -		print_df(sargs, unit_mode);
> +		print_df(sargs, unit_mode, show_reserve);
>   		free(sargs);
>   	} else {
>   		fprintf(stderr, "ERROR: get_df failed %s\n", strerror(-ret));
>

Re: [PATCH] btrfs-progs: Format change for btrfs fi df

[David Sterba]

Hi

On Tue, Jan 05, 2016 at 10:01:08AM +0800, Qu Wenruo wrote:
> Would you please consider merge this patch?
> 
> As current output is already quite confusing for a lot of users who are 
> not familiar with btrfs.

I understand it's confusing but as there's some history behind this
option I need to think about the consequences. I'd rather keep it in the
output, removing stuff is likely to cause breakage in scripts etc. OTOH,
the 'fi df' was supposed to be a debugging helper but not everybody is
used to 'fi usage' yet.

Re: [PATCH] btrfs-progs: Format change for btrfs fi df

[Duncan]

David Sterba posted on Mon, 11 Jan 2016 16:51:08 +0100 as excerpted:

> Hi
> 
> On Tue, Jan 05, 2016 at 10:01:08AM +0800, Qu Wenruo wrote:
>> Would you please consider merge this patch?
>> 
>> As current output is already quite confusing for a lot of users who are
>> not familiar with btrfs.
> 
> I understand it's confusing but as there's some history behind this
> option I need to think about the consequences. I'd rather keep it in the
> output, removing stuff is likely to cause breakage in scripts etc. OTOH,
> the 'fi df' was supposed to be a debugging helper but not everybody is
> used to 'fi usage' yet.

Not only is not everybody used to btrfs fi usage yet, but because it's 
still broken in various cases as well as relatively new, btrfs fi df 
along with btrfs fi sh remains the recommended way of getting that 
information.

Of course we can't go back in time to introduce btrfs fi usage earlier, 
but the sooner it works on all btrfs, including mixed-mode, which I know 
it doesn't properly support yet, and raid56, which IIRC it didn't support 
but I can't directly test, without producing weird EiB readings, the 
sooner we'll be able to generally recommend it except for people using 
what will eventually be very old tools, where the first recommendation is 
to upgrade in any case.

But for now, because fi usage doesn't work in a significant number of 
cases, recommending that people use and post fi df and fi sh remains the 
easiest way to dependably get the needed information, without going off 
into paragraphs of exception explanation.

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [PATCH] btrfs-progs: Format change for btrfs fi df

[David Sterba]

On Mon, Dec 14, 2015 at 01:19:56PM +0800, Qu Wenruo wrote:
> The GlobalReserve space in 'btrfs fi df' is always confusing for a lot
> of users.
> As it is not a special chunk type like DATA or METADATA, it's in fact a
> sub-type of METADATA.
> 
> So change the output to skip GlobalReserve by default, and adding its
> total to metadata used.
> And add a new option '-r|--reserve' to show the GlobalReserve, but skip
> the profile of GlobalReserve.

How about this:

- keep the global reserve in the output, separate from metadata and not
  accounted
- add new option to account the global reserve into metadata and then
  drop it from the output

Re: [PATCH] btrfs-progs: Format change for btrfs fi df

[Goffredo Baroncelli]

On 2016-01-12 15:28, David Sterba wrote:
[...]
> How about this:
> 
> - keep the global reserve in the output, separate from metadata and not
>    accounted
> - add new option to account the global reserve into metadata and then
>    drop it from the output


Instead of dropping it, what about prefixing it with '\_' in order to show that
it is a subtype of metadata (it is not a my idea, I read this sometime ago in 
this mailing list) ?

$ btrfs fi df /
Data, single: total=29.00GiB, used=21.01GiB
System, single: total=32.00MiB, used=16.00KiB
Metadata, single: total=2.00GiB, used=716.59MiB
\_GlobalReserve, single: total=240.00MiB, used=0.00B

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


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH] btrfs-progs: Format change for btrfs fi df

[Qu Wenruo]

Goffredo Baroncelli wrote on 2016/01/12 20:55 +0100:
> On 2016-01-12 15:28, David Sterba wrote:
> [...]
>> How about this:
>>
>> - keep the global reserve in the output, separate from metadata and not
>>     accounted
>> - add new option to account the global reserve into metadata and then
>>     drop it from the output
>
>
> Instead of dropping it, what about prefixing it with '\_' in order to show that
> it is a subtype of metadata (it is not a my idea, I read this sometime ago in
> this mailing list) ?
>
> $ btrfs fi df /
> Data, single: total=29.00GiB, used=21.01GiB
> System, single: total=32.00MiB, used=16.00KiB
> Metadata, single: total=2.00GiB, used=716.59MiB
> \_GlobalReserve, single: total=240.00MiB, used=0.00B

Yes, that's what my new patch does:

See the code:
+			printf(" \\- %s: reserved=%s, used=%s\n",
+				btrfs_group_type_str(sp->flags),
+				pretty_size_mode(sp->total_bytes, unit_mode),
+				pretty_size_mode(sp->used_bytes, unit_mode));

Although I'm using "\-" other than "\_".

Thanks,
Qu

>
>> --
>> 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] btrfs-progs: Format change for btrfs fi df

[Henk Slager]

On Tue, Jan 12, 2016 at 3:28 PM, David Sterba <dsterba@suse.cz> wrote:
> On Mon, Dec 14, 2015 at 01:19:56PM +0800, Qu Wenruo wrote:
>> The GlobalReserve space in 'btrfs fi df' is always confusing for a lot
>> of users.
>> As it is not a special chunk type like DATA or METADATA, it's in fact a
>> sub-type of METADATA.
>>
>> So change the output to skip GlobalReserve by default, and adding its
>> total to metadata used.
>> And add a new option '-r|--reserve' to show the GlobalReserve, but skip
>> the profile of GlobalReserve.
>
> How about this:
>
> - keep the global reserve in the output, separate from metadata and not
>   accounted
> - add new option to account the global reserve into metadata and then
>   drop it from the output

I think this alternative does not remove the confusion for people,
maybe even keeps it and/or increases it. The default 'btrfs fi df'
would then still include a confusing term and numbers belonging to it
that still don't make sense.

Maybe there are scripts around that try to do something with the
number belonging to GlobalReserve, but I think such would already be
part of the problem. Such calculations should be in progs itself by
default. If people want more detail or more output, the -r can be
used. If the used= for metadata is smaller by default and there is
some 'free space hidden', the risk is higher that scripts/people let
the fs grow to even close to full with all the trouble we see so often
in this list and elsewhere.

So I think the original patch is the way forward; It should also be
applied to 'btrfs fi us' I think.

Additionally, when 'btrfs fi df' is default 3 lines, those 3 lines
could exactly match lines that 'btrfs fi us' outputs for
Data/System/Metadata, so aligning Size: with total=  etc. Hopefully
this gives options for making the code more compact for 'btrfs fi df'
+ 'btrfs fi sh' + 'btrfs fi us' + 'btrfs de us'.

Re: [PATCH] btrfs-progs: Format change for btrfs fi df

[Qu Wenruo]

David Sterba wrote on 2016/01/12 15:28 +0100:
> On Mon, Dec 14, 2015 at 01:19:56PM +0800, Qu Wenruo wrote:
>> The GlobalReserve space in 'btrfs fi df' is always confusing for a lot
>> of users.
>> As it is not a special chunk type like DATA or METADATA, it's in fact a
>> sub-type of METADATA.
>>
>> So change the output to skip GlobalReserve by default, and adding its
>> total to metadata used.
>> And add a new option '-r|--reserve' to show the GlobalReserve, but skip
>> the profile of GlobalReserve.
>
> How about this:
>
> - keep the global reserve in the output, separate from metadata and not
>    accounted
> - add new option to account the global reserve into metadata and then
>    drop it from the output

I'm OK about it, although that would just make '-r' option default.

But anyway, the profile of GlobalReserve should not be output.

Thanks,
Qu

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