| Message ID | 1450070396-20744-1-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)); > -- 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
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. -- 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
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.
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 -- 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
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 >
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'. -- 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
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 > > -- 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
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 >> > > -- 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
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));
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(-)