* [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
@ 2012-10-03 11:43 Goffredo Baroncelli
2012-10-03 11:43 ` [PATCH 1/2] Update btrfs filesystem df command Goffredo Baroncelli
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 11:43 UTC (permalink / raw)
To: Chris Mason; +Cc: linux-btrfs, Goffredo Baroncelli
This serie of patches updated the command "btrfs filesystem
df". I update this command because it is not so easy to get
the information about the disk usage from the command "fi df" and "fi show".
>From the man page (see 2nd patch):
[...]
The command btrfs filesystem df is used to query the
status of the chunks, how many space on the disk(s) are used by
the chunks, how many space are available in the chunks, and an
estimation of the free space of the filesystem.
[...]
$ ./btrfs filesystem df --help
usage: btrfs filesystem disk-usage [-d][-s][-k] <path> [<path>..]
Show space usage information for a mount point(s).
-k Set KB (1024 bytes) as unit
-s Don't show the summary section
-d Don't show the detail section
$ ./btrfs filesystem df /
Path: /
Summary:
Disk_size: 72.57GB
Disk_allocated: 25.10GB
Disk_unallocated: 47.48GB
Logical_size: 23.06GB
Used: 11.01GB
Free_(Estimated): 55.66GB (Max: 59.52GB, Min: 35.78GB)
Data_to_disk_ratio: 92 %
Details:
Chunk-type Mode Chunk-size Logical-size Used
Data Single 21.01GB 21.01GB 10.34GB
System DUP 80.00MB 40.00MB 4.00KB
System Single 4.00MB 4.00MB 0.00
Metadata DUP 4.00GB 2.00GB 686.93MB
Metadata Single 8.00MB 8.00MB 0.00
Where:
Disk_size -> sum of sizes of teh disks
Disk_allocated -> sum of chunk sizes
Disk_unallocated -> Disk_size - Disk_allocated
Logical_size -> sum of logical area sizes
Used -> logical area used
Free_(Estimated) -> on the basis of allocated
chunk, an estrapolation of
the free space
Data_to_disk_ratio -> ration between the space occuped
by a chunk and the real space
available ( due to duplication
and/or RAID level)
Chunk-type -> kind of chunk
Mode -> allocation policy of a chunk
Chunk-size -> area of disk(s) occuped by the
chunk (see it as raw space used)
Logical-size -> logical area size of the chunk
Used -> portion of the logical area
used by the filesystem
You can pull this change from
http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
branch
disk_free
Comments are welcome.
BR
Goffredo Baroncelli
Changelog:
V0->V1 Change the name of command from disk-usage to df
Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/2] Update btrfs filesystem df command
2012-10-03 11:43 [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Goffredo Baroncelli
@ 2012-10-03 11:43 ` Goffredo Baroncelli
2012-10-03 15:02 ` Ilya Dryomov
2012-10-03 11:43 ` [PATCH 2/2] Update help page Goffredo Baroncelli
` (2 subsequent siblings)
3 siblings, 1 reply; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 11:43 UTC (permalink / raw)
To: Chris Mason; +Cc: linux-btrfs, Goffredo Baroncelli
The command btrfs filesystem df is used to query the
status of the chunks, how many space on the disk(s) are used by
the chunks, how many space are available in the chunks, and an
estimation of the free space of the filesystem.
---
cmds-filesystem.c | 264 +++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 215 insertions(+), 49 deletions(-)
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index b1457de..6c3ebdc 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -22,6 +22,7 @@
#include <errno.h>
#include <uuid/uuid.h>
#include <ctype.h>
+#include <sys/vfs.h>
#include "kerncompat.h"
#include "ctree.h"
@@ -39,25 +40,55 @@ static const char * const filesystem_cmd_group_usage[] = {
NULL
};
-static const char * const cmd_df_usage[] = {
- "btrfs filesystem df <path>",
- "Show space usage information for a mount point",
- NULL
-};
+static u64 disk_size( char *path){
+ struct statfs sfs;
+
+ if( statfs(path, &sfs) < 0 )
+ return 0;
+ else
+ return sfs.f_bsize * sfs.f_blocks;
+
+}
+
+static void print_string(char *s, int w)
+{
+ int i;
+
+ printf("%s", s);
+ for( i = strlen(s) ; i < w ; i++ )
+ putchar(' ');
+}
+
+#define DF_SHOW_SUMMARY (1<<1)
+#define DF_SHOW_DETAIL (1<<2)
+#define DF_HUMAN_UNIT (1<<3)
+
+static void pretty_sizes_r(u64 size, int w, int mode)
+{
+ if( mode & DF_HUMAN_UNIT ){
+ char *s = pretty_sizes(size);
+ printf("%*s", w, s);
+ free(s);
+ } else {
+ printf("%*llu", w, size/1024);
+
+ }
+}
-static int cmd_df(int argc, char **argv)
+static int _cmd_disk_free(char *path, int mode)
{
struct btrfs_ioctl_space_args *sargs;
u64 count = 0, i;
int ret;
int fd;
- int e;
- char *path;
-
- if (check_argc_exact(argc, 2))
- usage(cmd_df_usage);
-
- path = argv[1];
+ int e, width;
+ u64 total_disk; /* fielsystem size == sum of
+ disk sizes */
+ u64 total_chunks; /* sum of chunks sizes on disk(s) */
+ u64 total_used; /* logical space used */
+ u64 total_free; /* logical space un-used */
+ double K;
+
fd = open_file_or_dir(path);
if (fd < 0) {
@@ -103,56 +134,191 @@ static int cmd_df(int argc, char **argv)
return ret;
}
- for (i = 0; i < sargs->total_spaces; i++) {
- char description[80];
- char *total_bytes;
- char *used_bytes;
- int written = 0;
- u64 flags = sargs->spaces[i].flags;
+ total_disk = disk_size(path);
+ e = errno;
+ if( total_disk == 0 ){
+ fprintf(stderr, "ERROR: couldn't get space info on '%s' - %s\n",
+ path, strerror(e));
+ close(fd);
+ free(sargs);
+ return ret;
+ }
+
+ total_chunks = total_used = total_free = 0;
- memset(description, 0, 80);
+ for (i = 0; i < sargs->total_spaces; i++) {
+ int ratio=1;
+ u64 allocated;
- if (flags & BTRFS_BLOCK_GROUP_DATA) {
- if (flags & BTRFS_BLOCK_GROUP_METADATA) {
- snprintf(description, 14, "%s",
- "Data+Metadata");
- written += 13;
- } else {
- snprintf(description, 5, "%s", "Data");
- written += 4;
- }
- } else if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
- snprintf(description, 7, "%s", "System");
- written += 6;
- } else if (flags & BTRFS_BLOCK_GROUP_METADATA) {
- snprintf(description, 9, "%s", "Metadata");
- written += 8;
- }
+ u64 flags = sargs->spaces[i].flags;
if (flags & BTRFS_BLOCK_GROUP_RAID0) {
- snprintf(description+written, 8, "%s", ", RAID0");
- written += 7;
+ ratio=1;
} else if (flags & BTRFS_BLOCK_GROUP_RAID1) {
- snprintf(description+written, 8, "%s", ", RAID1");
- written += 7;
+ ratio=2;
} else if (flags & BTRFS_BLOCK_GROUP_DUP) {
- snprintf(description+written, 6, "%s", ", DUP");
- written += 5;
+ ratio=2;
} else if (flags & BTRFS_BLOCK_GROUP_RAID10) {
- snprintf(description+written, 9, "%s", ", RAID10");
- written += 8;
+ ratio=2;
+ } else {
+ ratio=1;
}
- total_bytes = pretty_sizes(sargs->spaces[i].total_bytes);
- used_bytes = pretty_sizes(sargs->spaces[i].used_bytes);
- printf("%s: total=%s, used=%s\n", description, total_bytes,
- used_bytes);
+ allocated = sargs->spaces[i].total_bytes * ratio;
+
+ total_chunks += allocated;
+ total_used += sargs->spaces[i].used_bytes;
+ total_free += (sargs->spaces[i].total_bytes -
+ sargs->spaces[i].used_bytes);
+
+ }
+ K = ((double)total_used + (double)total_free) /
+ (double)total_chunks;
+
+ if( mode & DF_HUMAN_UNIT )
+ width = 12;
+ else
+ width = 18;
+
+ printf("Path: %s\n", path);
+ if( mode & DF_SHOW_SUMMARY ){
+ printf("Summary:\n");
+ printf(" Disk_size:\t\t");
+ pretty_sizes_r(total_disk, width, mode);
+ printf("\n Disk_allocated:\t");
+ pretty_sizes_r(total_chunks, width, mode);
+ printf("\n Disk_unallocated:\t");
+ pretty_sizes_r(total_disk-total_chunks, width, mode);
+ printf("\n Logical_size:\t\t");
+ pretty_sizes_r(total_used+total_free, width, mode);
+ printf("\n Used:\t\t\t");
+ pretty_sizes_r(total_used, width, mode);
+ printf("\n Free_(Estimated):\t");
+ pretty_sizes_r((u64)(K*total_disk-total_used), width, mode);
+ printf("\t(Max: ");
+ pretty_sizes_r(total_disk-total_chunks+total_free,
+ 0, mode );
+ printf(", Min: ");
+ pretty_sizes_r((total_disk-total_chunks)/2+total_free,
+ 0, mode );
+ printf(")\n Data_to_disk_ratio:\t%*.0f %%\n",
+ width-2, K*100);
+ }
+
+ if( ( mode & DF_SHOW_DETAIL ) && ( mode & DF_SHOW_SUMMARY ) )
+ printf("\n");
+
+ if( mode & DF_SHOW_DETAIL ){
+ /* Remember: the terminals have maximum 80 columns
+ do not believe to who says otherwise */
+ printf("Details:\n");
+ printf(" %-12s%-8s%*s%*s%*s\n",
+ "Chunk-type",
+ "Mode",
+ width, "Chunk-size",
+ 1+width, "Logical-size",
+ width, "Used"
+ );
+
+ for (i = 0; i < sargs->total_spaces; i++) {
+ char *description="";
+ int ratio=1;
+ char *r_mode;
+ u64 allocated;
+
+ u64 flags = sargs->spaces[i].flags;
+
+ if (flags & BTRFS_BLOCK_GROUP_DATA) {
+ if (flags & BTRFS_BLOCK_GROUP_METADATA){
+ description = "Data+M.data";
+ } else {
+ description = "Data";
+ }
+ } else if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
+ description = "System";
+ } else if (flags & BTRFS_BLOCK_GROUP_METADATA) {
+ description = "Metadata";
+ }
+
+ if (flags & BTRFS_BLOCK_GROUP_RAID0) {
+ r_mode = "RAID0";
+ ratio=1;
+ } else if (flags & BTRFS_BLOCK_GROUP_RAID1) {
+ r_mode = "RAID1";
+ ratio=2;
+ } else if (flags & BTRFS_BLOCK_GROUP_DUP) {
+ r_mode = "DUP";
+ ratio=2;
+ } else if (flags & BTRFS_BLOCK_GROUP_RAID10) {
+ r_mode = "RAID10";
+ ratio=2;
+ } else {
+ r_mode = "Single";
+ ratio=1;
+ }
+
+ allocated = sargs->spaces[i].total_bytes * ratio;
+
+ printf(" ");
+ print_string(description,12);
+ print_string(r_mode, 8);
+ pretty_sizes_r(allocated, width, mode);
+ pretty_sizes_r(sargs->spaces[i].total_bytes ,
+ width+1, mode);
+
+ pretty_sizes_r(sargs->spaces[i].used_bytes,
+ width, mode);
+ printf("\n");
+
+ }
}
free(sargs);
return 0;
}
+static const char * const cmd_disk_free_usage[] = {
+ "btrfs filesystem df [-d][-s][-k] <path> [<path>..]",
+ "Show space usage information for a mount point(s).",
+ "",
+ "-k\tSet KB (1024 bytes) as unit",
+ "-s\tDon't show the summary section",
+ "-d\tDon't show the detail section",
+ NULL
+};
+
+static int cmd_disk_free(int argc, char **argv)
+{
+
+ int flags=DF_SHOW_SUMMARY|DF_SHOW_DETAIL|DF_HUMAN_UNIT;
+ int i, more_than_one=0;
+
+ if (check_argc_min(argc, 2))
+ usage(cmd_disk_free_usage);
+
+ for(i=1; i< argc ; i++){
+ if(!strcmp(argv[i],"-d"))
+ flags &= ~DF_SHOW_DETAIL;
+ else if(!strcmp(argv[i],"-s"))
+ flags &= ~DF_SHOW_SUMMARY;
+ else if(!strcmp(argv[i],"-k"))
+ flags &= ~DF_HUMAN_UNIT;
+ else{
+ int r;
+ if(more_than_one)
+ printf("\n");
+ r = _cmd_disk_free(argv[i], flags);
+ if( r ) return r;
+ more_than_one=1;
+ }
+
+ }
+
+ return 0;
+}
+
+
+
static int uuid_search(struct btrfs_fs_devices *fs_devices, char *search)
{
char uuidbuf[37];
@@ -529,7 +695,7 @@ static int cmd_label(int argc, char **argv)
const struct cmd_group filesystem_cmd_group = {
filesystem_cmd_group_usage, NULL, {
- { "df", cmd_df, cmd_df_usage, NULL, 0 },
+ { "df", cmd_disk_free, cmd_disk_free_usage, NULL, 0 },
{ "show", cmd_show, cmd_show_usage, NULL, 0 },
{ "sync", cmd_sync, cmd_sync_usage, NULL, 0 },
{ "defragment", cmd_defrag, cmd_defrag_usage, NULL, 0 },
--
1.7.10.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/2] Update help page
2012-10-03 11:43 [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Goffredo Baroncelli
2012-10-03 11:43 ` [PATCH 1/2] Update btrfs filesystem df command Goffredo Baroncelli
@ 2012-10-03 11:43 ` Goffredo Baroncelli
2012-10-03 11:56 ` [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Hugo Mills
2012-10-03 15:01 ` Ilya Dryomov
3 siblings, 0 replies; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 11:43 UTC (permalink / raw)
To: Chris Mason; +Cc: linux-btrfs, Goffredo Baroncelli
---
man/btrfs.8.in | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
diff --git a/man/btrfs.8.in b/man/btrfs.8.in
index 4b0a9f9..ed20448 100644
--- a/man/btrfs.8.in
+++ b/man/btrfs.8.in
@@ -27,6 +27,8 @@ btrfs \- control a btrfs filesystem
.PP
\fBbtrfs\fP \fBfilesystem label\fP\fI <dev> [newlabel]\fP
.PP
+\fBbtrfs\fP \fBfilesystem df\fP\fI [-s][-d][-k] \fIpath [path..]\fR\fP
+.PP
\fBbtrfs\fP \fBsubvolume find-new\fP\fI <subvolume> <last_gen>\fP
.PP
\fBbtrfs\fP \fBfilesystem balance\fP\fI <path> \fP
@@ -214,6 +216,98 @@ NOTE: Currently there are the following limitations:
- the filesystem should not have more than one device.
.TP
+\fBfilesystem df\fP [-s][-d][-k] \fIpath [path..]\fR
+
+Show space usage information for a mount point.
+
+\fIOptions\fP
+
+\fB-k\fP Set KB (1024 bytes) as unit
+
+\fB-s\fP Don't show the summary section
+
+\fB-d\fP Don't show the detail section
+
+\fIUsage information\fP
+
+.\"
+.\" this section is extract from
+.\" http://en.wikipedia.org/wiki/Btrfs#Chunk_and_device_trees
+The disk(s) of a btrfs filesystem are divided into chunks of 256 MB or more.
+Chunks may be mirrored or striped across multiple devices, depending by
+the allocation policy.
+The mirroring/striping arrangement is transparent to the rest of the
+file system, which simply sees the single, logical address space that
+chunks are mapped into.
+Chunks are allocated on demand. In the default allocation policy
+the data chunks are not duplicated and the metadata chunks
+are duplicated. This default can be changed during the filesystem
+creation, and in general the chunks allocation policy may change
+during the filesystem life.
+
+A chunk DUPlicated or with a RAID1/RAID10 level
+requires a space two time greater than the logical one. Different RAID levels
+have a different ratio disk-usage / logical space offered.
+
+Because some files (the small ones) are stored in the
+metadata chunks the computation of the \fIfree space\fP and \fIused space\fP
+is complex: depending by the file size different allocation policies are used.
+
+The command \fBbtrfs filesystem df\fP is used to query the status
+of the chunks, how many space on the disk(s) are used by the chunks,
+how many space are available in the chunks, and an estimation of the free
+space of the filesystem.
+The output of the command \fBbtrfs filesystem df\fP shows:
+
+.RS
+.IP Disk\ size
+the total size of the disks which compose the filesystem.
+
+.IP Disk\ allocated
+the size of the area of the disks used by the chunks.
+
+.IP Disk\ unallocated
+the size of the area of the disks which is free (i.e.
+the differences of the values above).
+
+.IP Logical\ size
+the available logical space of chunk.
+
+.IP Used
+the portion of the logical space used by the file and metadata.
+
+.IP Free\ (estimated)
+the estimated free space available. The evaluation
+cannot be rigorous because it depends by the allocation policy (DUP,Single,
+RAID1...) of the metadata and data chunks. If every chunks is stored as
+"Single" the sum of the \fBfree (estimated)\fP space and the \fBused\fP
+space is equal to the \fBdisk size\fP.
+Otherwise if all the chunk are mirrored (raid1 or raid10) or duplicated
+the sum of the \fBfree (estimated)\fP space and the \fBused\fP space is
+half of the \fBdisk size\fP. Normally the \fBfree (estimated)\fP is between
+these two limits.
+
+.IP Data\ to\ disk\ ratio
+the ratio betwen the \fBlogical size\fP and the \fBdisk allocated\fP.
+
+.IP Mode
+the kind of allocation policy used by the chunk (e.g. DUPlicated,
+RAID1, RAID10, Single....)
+
+.RE
+.RS
+\fINOTE\fP
+
+When a chunk is allocated, its disk-area is used and its allocation
+policy is fixed.
+A rebalance operation could rearrange the chunks, moving data in the chunks
+and resizing the allocated chunks. This causes the change of all the values
+discussed above, with the exception of the \fBused\fP and
+\fBdisk size\fP values.
+
+.RE
+.TP
+
\fBfilesystem show\fR [--all-devices|<uuid>|<label>]\fR
Show the btrfs filesystem with some additional info. If no \fIUUID\fP or
\fIlabel\fP is passed, \fBbtrfs\fR show info of all the btrfs filesystem.
--
1.7.10.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 11:43 [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Goffredo Baroncelli
2012-10-03 11:43 ` [PATCH 1/2] Update btrfs filesystem df command Goffredo Baroncelli
2012-10-03 11:43 ` [PATCH 2/2] Update help page Goffredo Baroncelli
@ 2012-10-03 11:56 ` Hugo Mills
2012-10-03 16:17 ` Goffredo Baroncelli
2012-10-09 9:43 ` Bart Noordervliet
2012-10-03 15:01 ` Ilya Dryomov
3 siblings, 2 replies; 23+ messages in thread
From: Hugo Mills @ 2012-10-03 11:56 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]
Looks good. Only a few comments, inline.
On Wed, Oct 03, 2012 at 01:43:14PM +0200, Goffredo Baroncelli wrote:
> $ ./btrfs filesystem df --help
> usage: btrfs filesystem disk-usage [-d][-s][-k] <path> [<path>..]
>
> Show space usage information for a mount point(s).
>
> -k Set KB (1024 bytes) as unit
> -s Don't show the summary section
> -d Don't show the detail section
These are kind of logical, but I think would be hard to remember
the right way round. I would suggest swapping the actions of the
switches, and rewording the help:
-s Show only summary section
-d Show only detail section
> $ ./btrfs filesystem df /
> Path: /
> Summary:
> Disk_size: 72.57GB
^ space between the value and the
unit (as ISO says), throughout.
This also makes it easier to
parse, if anyone wants to.
Also, use kB, MB, GB, TB for powers-of-ten based units, and KiB,
MiB, GiB, TiB for powers-of-two based units, please. I don't care
which you report in, but please do make the distinction. (And note
that it's kB with a lower case k, but KiB with an upper case K). This
brings us in line with the relevant ISO and IEEE standards.
> Disk_allocated: 25.10GB
> Disk_unallocated: 47.48GB
> Logical_size: 23.06GB
> Used: 11.01GB
> Free_(Estimated): 55.66GB (Max: 59.52GB, Min: 35.78GB)
> Data_to_disk_ratio: 92 %
>
> Details:
> Chunk-type Mode Chunk-size Logical-size Used
> Data Single 21.01GB 21.01GB 10.34GB
> System DUP 80.00MB 40.00MB 4.00KB
> System Single 4.00MB 4.00MB 0.00
> Metadata DUP 4.00GB 2.00GB 686.93MB
> Metadata Single 8.00MB 8.00MB 0.00
Why are the field headings here using - where the field headings in
the first section used _? Should you be using _ in both places?
Hugo.
--
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
--- The most exciting phrase to hear in science, the one that ---
heralds new discoveries, is not "Eureka!",
but "That's funny..."
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 11:43 [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Goffredo Baroncelli
` (2 preceding siblings ...)
2012-10-03 11:56 ` [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Hugo Mills
@ 2012-10-03 15:01 ` Ilya Dryomov
2012-10-03 16:46 ` Goffredo Baroncelli
2012-10-12 9:55 ` Martin Steigerwald
3 siblings, 2 replies; 23+ messages in thread
From: Ilya Dryomov @ 2012-10-03 15:01 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
On Wed, Oct 03, 2012 at 01:43:14PM +0200, Goffredo Baroncelli wrote:
> This serie of patches updated the command "btrfs filesystem
> df". I update this command because it is not so easy to get
> the information about the disk usage from the command "fi df" and "fi show".
>
> >>From the man page (see 2nd patch):
>
> [...]
> The command btrfs filesystem df is used to query the
> status of the chunks, how many space on the disk(s) are used by
> the chunks, how many space are available in the chunks, and an
> estimation of the free space of the filesystem.
> [...]
>
> $ ./btrfs filesystem df --help
> usage: btrfs filesystem disk-usage [-d][-s][-k] <path> [<path>..]
>
> Show space usage information for a mount point(s).
>
> -k Set KB (1024 bytes) as unit
> -s Don't show the summary section
> -d Don't show the detail section
Hi Goffredo,
Why not show just the summary section by default and have a flag (-v)
for the details section?
> $ ./btrfs filesystem df /
> Path: /
> Summary:
> Disk_size: 72.57GB
> Disk_allocated: 25.10GB
> Disk_unallocated: 47.48GB
> Logical_size: 23.06GB
> Used: 11.01GB
> Free_(Estimated): 55.66GB (Max: 59.52GB, Min: 35.78GB)
> Data_to_disk_ratio: 92 %
>
> Details:
> Chunk-type Mode Chunk-size Logical-size Used
"Type" for the first column is probably enough.
Why is the third column called Chunk-size? If my understanding is
correct, it's just a break down of Disk_allocated from the summary
section. If so, why not call it Disk_allocated to avoid confusion?
Also, why do you use dashes instead of underbars for table headers?
Thanks,
Ilya
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] Update btrfs filesystem df command
2012-10-03 11:43 ` [PATCH 1/2] Update btrfs filesystem df command Goffredo Baroncelli
@ 2012-10-03 15:02 ` Ilya Dryomov
2012-10-03 16:34 ` Goffredo Baroncelli
2012-10-03 17:09 ` Goffredo Baroncelli
0 siblings, 2 replies; 23+ messages in thread
From: Ilya Dryomov @ 2012-10-03 15:02 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
On Wed, Oct 03, 2012 at 01:43:15PM +0200, Goffredo Baroncelli wrote:
> The command btrfs filesystem df is used to query the
> status of the chunks, how many space on the disk(s) are used by
> the chunks, how many space are available in the chunks, and an
> estimation of the free space of the filesystem.
> ---
> cmds-filesystem.c | 264 +++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 215 insertions(+), 49 deletions(-)
>
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index b1457de..6c3ebdc 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
(snipped)
> +static int cmd_disk_free(int argc, char **argv)
> +{
> +
> + int flags=DF_SHOW_SUMMARY|DF_SHOW_DETAIL|DF_HUMAN_UNIT;
> + int i, more_than_one=0;
> +
> + if (check_argc_min(argc, 2))
> + usage(cmd_disk_free_usage);
> +
> + for(i=1; i< argc ; i++){
> + if(!strcmp(argv[i],"-d"))
> + flags &= ~DF_SHOW_DETAIL;
> + else if(!strcmp(argv[i],"-s"))
> + flags &= ~DF_SHOW_SUMMARY;
> + else if(!strcmp(argv[i],"-k"))
> + flags &= ~DF_HUMAN_UNIT;
> + else{
> + int r;
> + if(more_than_one)
> + printf("\n");
> + r = _cmd_disk_free(argv[i], flags);
> + if( r ) return r;
> + more_than_one=1;
> + }
Is there any reason getopt(), or better yet, getopt_long() won't work?
> +
> + }
> +
> + return 0;
> +}
> +
> +
> +
> static int uuid_search(struct btrfs_fs_devices *fs_devices, char *search)
> {
> char uuidbuf[37];
> @@ -529,7 +695,7 @@ static int cmd_label(int argc, char **argv)
>
> const struct cmd_group filesystem_cmd_group = {
> filesystem_cmd_group_usage, NULL, {
> - { "df", cmd_df, cmd_df_usage, NULL, 0 },
> + { "df", cmd_disk_free, cmd_disk_free_usage, NULL, 0 },
If this command is going to replace df, you should change the function
name back to cmd_df.
Thanks,
Ilya
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 11:56 ` [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Hugo Mills
@ 2012-10-03 16:17 ` Goffredo Baroncelli
2012-10-03 16:34 ` Hugo Mills
2012-10-09 9:43 ` Bart Noordervliet
1 sibling, 1 reply; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 16:17 UTC (permalink / raw)
Cc: Hugo Mills, Chris Mason, linux-btrfs
On 10/03/2012 01:56 PM, Hugo Mills wrote:
> Looks good. Only a few comments, inline.
>
> On Wed, Oct 03, 2012 at 01:43:14PM +0200, Goffredo Baroncelli wrote:
>> $ ./btrfs filesystem df --help
>> usage: btrfs filesystem disk-usage [-d][-s][-k]<path> [<path>..]
>>
>> Show space usage information for a mount point(s).
>>
>> -k Set KB (1024 bytes) as unit
>> -s Don't show the summary section
>> -d Don't show the detail section
>
> These are kind of logical, but I think would be hard to remember
> the right way round. I would suggest swapping the actions of the
> switches, and rewording the help:
>
> -s Show only summary section
> -d Show only detail section
Yes this make sense...
>
>> $ ./btrfs filesystem df /
>> Path: /
>> Summary:
>> Disk_size: 72.57GB
>
> ^ space between the value and the
> unit (as ISO says), throughout.
> This also makes it easier to
> parse, if anyone wants to.
>
> Also, use kB, MB, GB, TB for powers-of-ten based units, and KiB,
> MiB, GiB, TiB for powers-of-two based units, please. I don't care
> which you report in, but please do make the distinction. (And note
> that it's kB with a lower case k, but KiB with an upper case K). This
> brings us in line with the relevant ISO and IEEE standards.
I forgot to reply you when you raised this question the first time. Even
though I am inclined to accept your suggestions, this change is not
related to my patches. My code uses the functions print_sizes(), which
is quite old (about 2008). This function is used in a lot of places.
This suggested to address this issue with another patch.
>
>> Disk_allocated: 25.10GB
>> Disk_unallocated: 47.48GB
>> Logical_size: 23.06GB
>> Used: 11.01GB
>> Free_(Estimated): 55.66GB (Max: 59.52GB, Min: 35.78GB)
>> Data_to_disk_ratio: 92 %
>>
>> Details:
>> Chunk-type Mode Chunk-size Logical-size Used
>> Data Single 21.01GB 21.01GB 10.34GB
>> System DUP 80.00MB 40.00MB 4.00KB
>> System Single 4.00MB 4.00MB 0.00
>> Metadata DUP 4.00GB 2.00GB 686.93MB
>> Metadata Single 8.00MB 8.00MB 0.00
>
> Why are the field headings here using - where the field headings in
> the first section used _? Should you be using _ in both places?
2 persons highlighted that :-( ... I will update the code
>
> Hugo.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 16:17 ` Goffredo Baroncelli
@ 2012-10-03 16:34 ` Hugo Mills
0 siblings, 0 replies; 23+ messages in thread
From: Hugo Mills @ 2012-10-03 16:34 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: Chris Mason, linux-btrfs
[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]
On Wed, Oct 03, 2012 at 06:17:53PM +0200, Goffredo Baroncelli wrote:
> On 10/03/2012 01:56 PM, Hugo Mills wrote:
> > Looks good. Only a few comments, inline.
> >
> >On Wed, Oct 03, 2012 at 01:43:14PM +0200, Goffredo Baroncelli wrote:
[snip]
> > Also, use kB, MB, GB, TB for powers-of-ten based units, and KiB,
> >MiB, GiB, TiB for powers-of-two based units, please. I don't care
> >which you report in, but please do make the distinction. (And note
> >that it's kB with a lower case k, but KiB with an upper case K). This
> >brings us in line with the relevant ISO and IEEE standards.
>
> I forgot to reply you when you raised this question the first time.
> Even though I am inclined to accept your suggestions, this change is
> not related to my patches. My code uses the functions print_sizes(),
> which is quite old (about 2008). This function is used in a lot of
> places. This suggested to address this issue with another patch.
OK.
[snip]
> > Why are the field headings here using - where the field headings in
> >the first section used _? Should you be using _ in both places?
>
> 2 persons highlighted that :-( ... I will update the code
It's just a niggle, really, but it's an obvious one.
Hugo.
--
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
--- There is no dark side to the Moon, really. As a matter of ---
fact, it's all dark.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] Update btrfs filesystem df command
2012-10-03 15:02 ` Ilya Dryomov
@ 2012-10-03 16:34 ` Goffredo Baroncelli
2012-10-03 17:20 ` Ilya Dryomov
2012-10-03 17:09 ` Goffredo Baroncelli
1 sibling, 1 reply; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 16:34 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
On 10/03/2012 05:02 PM, Ilya Dryomov wrote:
> On Wed, Oct 03, 2012 at 01:43:15PM +0200, Goffredo Baroncelli wrote:
>> The command btrfs filesystem df is used to query the
>> status of the chunks, how many space on the disk(s) are used by
>> the chunks, how many space are available in the chunks, and an
>> estimation of the free space of the filesystem.
>> ---
>> cmds-filesystem.c | 264 +++++++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 215 insertions(+), 49 deletions(-)
>>
>> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
>> index b1457de..6c3ebdc 100644
>> --- a/cmds-filesystem.c
>> +++ b/cmds-filesystem.c
>
> (snipped)
>
>> +static int cmd_disk_free(int argc, char **argv)
>> +{
>> +
>> + int flags=DF_SHOW_SUMMARY|DF_SHOW_DETAIL|DF_HUMAN_UNIT;
>> + int i, more_than_one=0;
>> +
>> + if (check_argc_min(argc, 2))
>> + usage(cmd_disk_free_usage);
>> +
>> + for(i=1; i< argc ; i++){
>> + if(!strcmp(argv[i],"-d"))
>> + flags&= ~DF_SHOW_DETAIL;
>> + else if(!strcmp(argv[i],"-s"))
>> + flags&= ~DF_SHOW_SUMMARY;
>> + else if(!strcmp(argv[i],"-k"))
>> + flags&= ~DF_HUMAN_UNIT;
>> + else{
>> + int r;
>> + if(more_than_one)
>> + printf("\n");
>> + r = _cmd_disk_free(argv[i], flags);
>> + if( r ) return r;
>> + more_than_one=1;
>> + }
>
> Is there any reason getopt(), or better yet, getopt_long() won't work?
In the beginning there were also the switches +d, +s, +k, then I dropped
them. So I leaved this style. The code is simple enough to not justify a
change.
>
>> +
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +
>> static int uuid_search(struct btrfs_fs_devices *fs_devices, char *search)
>> {
>> char uuidbuf[37];
>> @@ -529,7 +695,7 @@ static int cmd_label(int argc, char **argv)
>>
>> const struct cmd_group filesystem_cmd_group = {
>> filesystem_cmd_group_usage, NULL, {
>> - { "df", cmd_df, cmd_df_usage, NULL, 0 },
>> + { "df", cmd_disk_free, cmd_disk_free_usage, NULL, 0 },
>
> If this command is going to replace df, you should change the function
> name back to cmd_df.
I was never convinced to use 'df'. At the beginning when I wrote the
first parser of btrfs, was suggested (not by me) to use "long" command
and allow the parser to match a contracted command until there was any
ambiguity. I suggested to use disk-free, but everybody were confortable
with df.. so I leaved it as "official name". But I prefer for the
internal code a more verbose name.
>
> Thanks,
Thanks you for your review.
>
> Ilya
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 15:01 ` Ilya Dryomov
@ 2012-10-03 16:46 ` Goffredo Baroncelli
2012-10-03 17:46 ` Ilya Dryomov
2012-10-12 9:55 ` Martin Steigerwald
1 sibling, 1 reply; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 16:46 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
On 10/03/2012 05:01 PM, Ilya Dryomov wrote:
> "Type" for the first column is probably enough.
>
> Why is the third column called Chunk-size? If my understanding is
> correct, it's just a break down of Disk_allocated from the summary
> section. If so, why not call it Disk_allocated to avoid confusion?
Using everywhere Disk_<something> was my first attempt. But after some
thoughts I decided that these are two different kind of information. It
is true that Disk_allocated is the sum of Chunk-Sizes... But my feels is
that this is a kind of "implementation details". If some other type of
allocation unit will be added to BTRFS, then these will be added to
Disk_allocated, but not to Chunk list...
I prefer to not change the wording until an enough critical mass of
people converge to a unique solution .
>
> Also, why do you use dashes instead of underbars for table headers?
Yes, also Hugo noticed that.
>
> Thanks,
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] Update btrfs filesystem df command
2012-10-03 15:02 ` Ilya Dryomov
2012-10-03 16:34 ` Goffredo Baroncelli
@ 2012-10-03 17:09 ` Goffredo Baroncelli
1 sibling, 0 replies; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 17:09 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: linux-btrfs
On 10/03/2012 05:02 PM, Ilya Dryomov wrote:
>> +static int cmd_disk_free(int argc, char **argv)
>> > +{
>> > +
>> > + int flags=DF_SHOW_SUMMARY|DF_SHOW_DETAIL|DF_HUMAN_UNIT;
>> > + int i, more_than_one=0;
>> > +
>> > + if (check_argc_min(argc, 2))
>> > + usage(cmd_disk_free_usage);
>> > +
>> > + for(i=1; i< argc ; i++){
>> > + if(!strcmp(argv[i],"-d"))
>> > + flags&= ~DF_SHOW_DETAIL;
>> > + else if(!strcmp(argv[i],"-s"))
>> > + flags&= ~DF_SHOW_SUMMARY;
>> > + else if(!strcmp(argv[i],"-k"))
>> > + flags&= ~DF_HUMAN_UNIT;
>> > + else{
>> > + int r;
>> > + if(more_than_one)
>> > + printf("\n");
>> > + r = _cmd_disk_free(argv[i], flags);
>> > + if( r ) return r;
>> > + more_than_one=1;
>> > + }
> Is there any reason getopt(), or better yet, getopt_long() won't work?
I re-changed idea: I will use getopt()
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] Update btrfs filesystem df command
2012-10-03 16:34 ` Goffredo Baroncelli
@ 2012-10-03 17:20 ` Ilya Dryomov
2012-10-03 17:38 ` Goffredo Baroncelli
0 siblings, 1 reply; 23+ messages in thread
From: Ilya Dryomov @ 2012-10-03 17:20 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
On Wed, Oct 03, 2012 at 06:34:38PM +0200, Goffredo Baroncelli wrote:
> On 10/03/2012 05:02 PM, Ilya Dryomov wrote:
> >On Wed, Oct 03, 2012 at 01:43:15PM +0200, Goffredo Baroncelli wrote:
> >>The command btrfs filesystem df is used to query the
> >>status of the chunks, how many space on the disk(s) are used by
> >>the chunks, how many space are available in the chunks, and an
> >>estimation of the free space of the filesystem.
> >>---
> >> cmds-filesystem.c | 264 +++++++++++++++++++++++++++++++++++++++++++----------
> >> 1 file changed, 215 insertions(+), 49 deletions(-)
> >>
> >>diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> >>index b1457de..6c3ebdc 100644
> >>--- a/cmds-filesystem.c
> >>+++ b/cmds-filesystem.c
> >
> >(snipped)
> >
> >>+static int cmd_disk_free(int argc, char **argv)
> >>+{
> >>+
> >>+ int flags=DF_SHOW_SUMMARY|DF_SHOW_DETAIL|DF_HUMAN_UNIT;
> >>+ int i, more_than_one=0;
> >>+
> >>+ if (check_argc_min(argc, 2))
> >>+ usage(cmd_disk_free_usage);
> >>+
> >>+ for(i=1; i< argc ; i++){
> >>+ if(!strcmp(argv[i],"-d"))
> >>+ flags&= ~DF_SHOW_DETAIL;
> >>+ else if(!strcmp(argv[i],"-s"))
> >>+ flags&= ~DF_SHOW_SUMMARY;
> >>+ else if(!strcmp(argv[i],"-k"))
> >>+ flags&= ~DF_HUMAN_UNIT;
> >>+ else{
> >>+ int r;
> >>+ if(more_than_one)
> >>+ printf("\n");
> >>+ r = _cmd_disk_free(argv[i], flags);
> >>+ if( r ) return r;
> >>+ more_than_one=1;
> >>+ }
> >
> >Is there any reason getopt(), or better yet, getopt_long() won't work?
>
> In the beginning there were also the switches +d, +s, +k, then I
> dropped them. So I leaved this style. The code is simple enough to
> not justify a change.
It's not a matter of style. It is generally expected that several short
options can be specified after one dash. Further, if somebody else were
to add an option with an optional parameter, they'd have to spend time
rewriting your stuff.
In addition to that, other parts of progs already use getopt() in
similar situations. One example would be the scrub subgroup: scrub
commands are equally simple (a few short switches, no parameters), and
yet getopt() is used.
>
> >
> >>+
> >>+ }
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+
> >>+
> >> static int uuid_search(struct btrfs_fs_devices *fs_devices, char *search)
> >> {
> >> char uuidbuf[37];
> >>@@ -529,7 +695,7 @@ static int cmd_label(int argc, char **argv)
> >>
> >> const struct cmd_group filesystem_cmd_group = {
> >> filesystem_cmd_group_usage, NULL, {
> >>- { "df", cmd_df, cmd_df_usage, NULL, 0 },
> >>+ { "df", cmd_disk_free, cmd_disk_free_usage, NULL, 0 },
> >
> >If this command is going to replace df, you should change the function
> >name back to cmd_df.
>
> I was never convinced to use 'df'. At the beginning when I wrote the
> first parser of btrfs, was suggested (not by me) to use "long"
> command and allow the parser to match a contracted command until
> there was any ambiguity. I suggested to use disk-free, but everybody
> were confortable with df.. so I leaved it as "official name". But I
> prefer for the internal code a more verbose name.
Well, all your patch is doing is extending the functionality of an
existing command. The "official name" for that command is "df", and you
are not changing its name. Why change the name of an existing function?
Thanks,
Ilya
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] Update btrfs filesystem df command
2012-10-03 17:20 ` Ilya Dryomov
@ 2012-10-03 17:38 ` Goffredo Baroncelli
0 siblings, 0 replies; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 17:38 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
Hi,
On 10/03/2012 07:20 PM, Ilya Dryomov wrote:
>>>> filesystem_cmd_group_usage, NULL, {
>>>> > >>- { "df", cmd_df, cmd_df_usage, NULL, 0 },
>>>> > >>+ { "df", cmd_disk_free, cmd_disk_free_usage, NULL, 0 },
>>> > >
>>> > >If this command is going to replace df, you should change the function
>>> > >name back to cmd_df.
>> >
>> > I was never convinced to use 'df'. At the beginning when I wrote the
>> > first parser of btrfs, was suggested (not by me) to use "long"
>> > command and allow the parser to match a contracted command until
>> > there was any ambiguity. I suggested to use disk-free, but everybody
>> > were confortable with df.. so I leaved it as "official name". But I
>> > prefer for the internal code a more verbose name.
> Well, all your patch is doing is extending the functionality of an
> existing command. The "official name" for that command is "df", and you
> are not changing its name.
I would like change the name of the command from df to disk-free.
Unfortunately (for me) Chris ask to leaved the old name...
> Why change the name of an existing function?
Strictly speaking, I never changed the name of the function. At the
beginning I added another command (who was named disk-usage), then Chris
asked to replace df with the new one. So I removed the old one.
Seeing the patch it seems that I changed the name, but the truth is that
I added a new one then removed the old one. Due to how the patch is
generated it seems another thing.
But this doesn't change too much.
The point is that the function name doesn't match the command name. It
is not the single case: look at the function cmd_send_start()
(associated to the send command), cmd_defrag() )associated to the
command defragment....)
BR
G.Baroncelli
>
> Thanks,
>
> Ilya
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 16:46 ` Goffredo Baroncelli
@ 2012-10-03 17:46 ` Ilya Dryomov
2012-10-03 20:01 ` Goffredo Baroncelli
2012-10-12 10:01 ` Martin Steigerwald
0 siblings, 2 replies; 23+ messages in thread
From: Ilya Dryomov @ 2012-10-03 17:46 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
On Wed, Oct 03, 2012 at 06:46:00PM +0200, Goffredo Baroncelli wrote:
> On 10/03/2012 05:01 PM, Ilya Dryomov wrote:
> >"Type" for the first column is probably enough.
> >
> >Why is the third column called Chunk-size? If my understanding is
> >correct, it's just a break down of Disk_allocated from the summary
> >section. If so, why not call it Disk_allocated to avoid confusion?
>
> Using everywhere Disk_<something> was my first attempt. But after
> some thoughts I decided that these are two different kind of
> information. It is true that Disk_allocated is the sum of
> Chunk-Sizes... But my feels is that this is a kind of
> "implementation details". If some other type of allocation unit will
> be added to BTRFS, then these will be added to Disk_allocated, but
> not to Chunk list...
> I prefer to not change the wording until an enough critical mass of
> people converge to a unique solution .
It is the chunks that is the implementation detail that we want to hide.
Average Btrfs user wouldn't want to know anything about chunks, the only
thing he'd be interested in is Disk_allocated and similar fields.
Moreover, I am pretty sure "Chunk-Size" would actually confuse people.
I stared at your example output for a few seconds trying to comprehend a
21GB Chunk-Size on a 72GB partition. What you call "Chunk-Size" is
actually a sum of sizes of chunks of that particular type, and a few
lines above you call the same exact sum (only this time over all types
of chunks) "Disk_allocated". So I think it's only logical to rename the
column in question to "Disk_allocated" to match the summary section.
Thanks,
Ilya
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 17:46 ` Ilya Dryomov
@ 2012-10-03 20:01 ` Goffredo Baroncelli
2012-10-03 20:24 ` Ilya Dryomov
2012-10-12 10:01 ` Martin Steigerwald
1 sibling, 1 reply; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-03 20:01 UTC (permalink / raw)
To: Ilya Dryomov; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
On 10/03/2012 07:46 PM, Ilya Dryomov wrote:
> On Wed, Oct 03, 2012 at 06:46:00PM +0200, Goffredo Baroncelli wrote:
>> On 10/03/2012 05:01 PM, Ilya Dryomov wrote:
>>> "Type" for the first column is probably enough.
>>>
>>> Why is the third column called Chunk-size? If my understanding is
>>> correct, it's just a break down of Disk_allocated from the summary
>>> section. If so, why not call it Disk_allocated to avoid confusion?
>>
>> Using everywhere Disk_<something> was my first attempt. But after
>> some thoughts I decided that these are two different kind of
>> information. It is true that Disk_allocated is the sum of
>> Chunk-Sizes... But my feels is that this is a kind of
>> "implementation details". If some other type of allocation unit will
>> be added to BTRFS, then these will be added to Disk_allocated, but
>> not to Chunk list...
>> I prefer to not change the wording until an enough critical mass of
>> people converge to a unique solution .
>
> It is the chunks that is the implementation detail that we want to hide.
> Average Btrfs user wouldn't want to know anything about chunks, the only
> thing he'd be interested in is Disk_allocated and similar fields.
The "df" standard tool id sufficient for the "average user".
We need only to export these information via the standard syscall
stat[v]fs. Basically we should try to implement the algorithm of the
Free_(Estimated) space for the statfs(2) syscall.
Who uses btrfs tools, is an user with knowledge of btrfs higher than the
average.
> Moreover, I am pretty sure "Chunk-Size" would actually confuse people.
> I stared at your example output for a few seconds trying to comprehend a
> 21GB Chunk-Size on a 72GB partition. What you call "Chunk-Size" is
> actually a sum of sizes of chunks of that particular type, and a few
> lines above you call the same exact sum (only this time over all types
> of chunks) "Disk_allocated". So I think it's only logical to rename the
> column in question to "Disk_allocated" to match the summary section.
What about
[...]
Details:
Chunk_type Mode Size_(disk) Size_(logical) Used
Data Single 21.01GB 21.01GB 10.53GB
System DUP 80.00MB 40.00MB 4.00KB
[...]
?
Still I am still not entirely satisfied; I am open to other suggestions,
but until now every people has a different opinion. If we reach an
agreement between two/three persons (even different from my opinion) I
will update the patches. Otherwise I suggest to accept these patches "as
is" because there no is a "general" consensus to a wording...
> Thanks,
>
> Ilya
>
Ciao
Goffredo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 20:01 ` Goffredo Baroncelli
@ 2012-10-03 20:24 ` Ilya Dryomov
0 siblings, 0 replies; 23+ messages in thread
From: Ilya Dryomov @ 2012-10-03 20:24 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: Chris Mason, linux-btrfs, Goffredo Baroncelli
On Wed, Oct 03, 2012 at 10:01:00PM +0200, Goffredo Baroncelli wrote:
> On 10/03/2012 07:46 PM, Ilya Dryomov wrote:
> >On Wed, Oct 03, 2012 at 06:46:00PM +0200, Goffredo Baroncelli wrote:
> >>On 10/03/2012 05:01 PM, Ilya Dryomov wrote:
> >>>"Type" for the first column is probably enough.
> >>>
> >>>Why is the third column called Chunk-size? If my understanding is
> >>>correct, it's just a break down of Disk_allocated from the summary
> >>>section. If so, why not call it Disk_allocated to avoid confusion?
> >>
> >>Using everywhere Disk_<something> was my first attempt. But after
> >>some thoughts I decided that these are two different kind of
> >>information. It is true that Disk_allocated is the sum of
> >>Chunk-Sizes... But my feels is that this is a kind of
> >>"implementation details". If some other type of allocation unit will
> >>be added to BTRFS, then these will be added to Disk_allocated, but
> >>not to Chunk list...
> >>I prefer to not change the wording until an enough critical mass of
> >>people converge to a unique solution .
> >
> >It is the chunks that is the implementation detail that we want to hide.
> >Average Btrfs user wouldn't want to know anything about chunks, the only
> >thing he'd be interested in is Disk_allocated and similar fields.
>
> The "df" standard tool id sufficient for the "average user".
> We need only to export these information via the standard syscall
> stat[v]fs. Basically we should try to implement the algorithm of the
> Free_(Estimated) space for the statfs(2) syscall.
> Who uses btrfs tools, is an user with knowledge of btrfs higher than
> the average.
>
> >Moreover, I am pretty sure "Chunk-Size" would actually confuse people.
> >I stared at your example output for a few seconds trying to comprehend a
> >21GB Chunk-Size on a 72GB partition. What you call "Chunk-Size" is
> >actually a sum of sizes of chunks of that particular type, and a few
> >lines above you call the same exact sum (only this time over all types
> >of chunks) "Disk_allocated". So I think it's only logical to rename the
> >column in question to "Disk_allocated" to match the summary section.
>
> What about
> [...]
> Details:
> Chunk_type Mode Size_(disk) Size_(logical) Used
> Data Single 21.01GB 21.01GB 10.53GB
> System DUP 80.00MB 40.00MB 4.00KB
> [...]
>
> ?
This is definitely better. Can you also drop "Chunk_type" in favor of
"Type"?
Thanks,
Ilya
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 11:56 ` [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Hugo Mills
2012-10-03 16:17 ` Goffredo Baroncelli
@ 2012-10-09 9:43 ` Bart Noordervliet
2012-10-09 11:38 ` Goffredo Baroncelli
1 sibling, 1 reply; 23+ messages in thread
From: Bart Noordervliet @ 2012-10-09 9:43 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: linux-btrfs
Hi Goffredo,
thank you very much for your work on making the btrfs filesystem df
output that much more understandable. It is a real improvement
already. I would however like to bring this comment from Hugo to your
attention once again:
On Wed, Oct 3, 2012 at 1:56 PM, Hugo Mills <hugo@carfax.org.uk> wrote:
>> $ ./btrfs filesystem df /
>> Path: /
>> Summary:
>> Disk_size: 72.57GB
>
> Also, use kB, MB, GB, TB for powers-of-ten based units, and KiB,
> MiB, GiB, TiB for powers-of-two based units, please. I don't care
> which you report in, but please do make the distinction. (And note
> that it's kB with a lower case k, but KiB with an upper case K). This
> brings us in line with the relevant ISO and IEEE standards.
I strongly support this suggestion. With terabyte-disks being the norm
and the difference between TB and TiB being 10%, we really can't
afford ignoring the difference any longer. Contrary to Hugo, I also
have a strong preference to change the calculations to use
powers-of-ten. There is nothing binary about disk space; it's just a
function of how many bits they reliably manage to cram onto a platter.
And while the raw size of an SSD does tend to be binary, after
reservation of the garbage-collection/spare area the effective size is
also completely arbitrary. Any reference to powers-of-two is
completely artificial in these areas. But if you don't want to change
the calculation, please do change the units as Hugo requested.
Kind regards,
Bart Noordervliet
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-09 9:43 ` Bart Noordervliet
@ 2012-10-09 11:38 ` Goffredo Baroncelli
2012-10-09 12:51 ` Bart Noordervliet
0 siblings, 1 reply; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-09 11:38 UTC (permalink / raw)
To: Bart Noordervliet; +Cc: linux-btrfs, Hugo Mills
Hi Bart,
On Tue, Oct 9, 2012 at 11:43 AM, Bart Noordervliet
<bart@noordervliet.net> wrote:
> Hi Goffredo,
>
> thank you very much for your work on making the btrfs filesystem df
> output that much more understandable. It is a real improvement
> already. I would however like to bring this comment from Hugo to your
> attention once again:
>
> On Wed, Oct 3, 2012 at 1:56 PM, Hugo Mills <hugo@carfax.org.uk> wrote:
>>> $ ./btrfs filesystem df /
>>> Path: /
>>> Summary:
>>> Disk_size: 72.57GB
>>
>> Also, use kB, MB, GB, TB for powers-of-ten based units, and KiB,
>> MiB, GiB, TiB for powers-of-two based units, please. I don't care
>> which you report in, but please do make the distinction. (And note
>> that it's kB with a lower case k, but KiB with an upper case K). This
>> brings us in line with the relevant ISO and IEEE standards.
>
> I strongly support this suggestion. With terabyte-disks being the norm
> and the difference between TB and TiB being 10%, we really can't
> afford ignoring the difference any longer. Contrary to Hugo, I also
> have a strong preference to change the calculations to use
> powers-of-ten. There is nothing binary about disk space; it's just a
> function of how many bits they reliably manage to cram onto a platter.
> And while the raw size of an SSD does tend to be binary, after
> reservation of the garbage-collection/spare area the effective size is
> also completely arbitrary. Any reference to powers-of-two is
> completely artificial in these areas. But if you don't want to change
> the calculation, please do change the units as Hugo requested.
I replayed in another email to Hugo about that. Basically I am not contrary
to the change, only it is unrelated to my patches. In may patches I use the
function pretty_sizes() which adds the suffix KB, MB, and this function was
here from 2008....
I am reviewing this function to change the suffix. However I noticed
that it needs
more work, because often the results are returned as:
> $ sudo btrfs fi show
> Label: 'antares-btrfs' uuid: c0b83ac2-ded6-4b1a-a038-2d52ced11bba
> Total devices 1 FS bytes used 20.60GB
> devid 1 size 250.68GB used 39.79GB path /dev/vdb
Note " FS bytes used 20.60GB". To me the unit is or byte (the 2nd
word) or MB....
Regarding which unit we should use (MB or MiB), my feels is mixed. From one side
I am used to use the unit MiB, KiB; however from the other side the hard disks
are measured as GB and not GiB. For example an hard disk of 60GB
contains 60*10^9 bytes and not 60*2^30 bytes....
We could leave it configurable via a environmental variable. But this
could open
the door for a configuration file for the btrfs utility...
>
> Kind regards,
>
> Bart Noordervliet
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-09 11:38 ` Goffredo Baroncelli
@ 2012-10-09 12:51 ` Bart Noordervliet
2012-10-09 18:22 ` Goffredo Baroncelli
0 siblings, 1 reply; 23+ messages in thread
From: Bart Noordervliet @ 2012-10-09 12:51 UTC (permalink / raw)
To: Goffredo Baroncelli; +Cc: linux-btrfs, Hugo Mills
On Tue, Oct 9, 2012 at 1:38 PM, Goffredo Baroncelli <kreijack@gmail.com> wrote:
> Hi Bart,
>
> I replayed in another email to Hugo about that. Basically I am not contrary
> to the change, only it is unrelated to my patches. In may patches I use the
> function pretty_sizes() which adds the suffix KB, MB, and this function was
> here from 2008....
What we could do is change all callers to use pretty_sizes_r(), which
most already do. We could then add a mode flag for SI units, say
DF_HUMAN_SI_UNIT. I would then propose that we enable it by default,
like the human-readable mode itself, and create an option to disable
it.
I like options a lot, but I like sensible defaults as well. We're
hoping btrfs will be the next major filesystem for linux. If we
succeed, it will have a long lifespan and be used on petabyte and
maybe exabyte storage systems. If we keep showing binary bytes, the
discrepancy between what the drive's box says and what 'df' says will
become ever larger. This is probably the last chance we'll get to make
btrfs set a good example by getting rid of this outdated method of
display.
I can make the changes to btrfs-progs if you like.
Best regards,
Bart
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-09 12:51 ` Bart Noordervliet
@ 2012-10-09 18:22 ` Goffredo Baroncelli
2012-10-12 9:42 ` Martin Steigerwald
0 siblings, 1 reply; 23+ messages in thread
From: Goffredo Baroncelli @ 2012-10-09 18:22 UTC (permalink / raw)
To: Bart Noordervliet; +Cc: linux-btrfs, Hugo Mills
On 10/09/2012 02:51 PM, Bart Noordervliet wrote:
> On Tue, Oct 9, 2012 at 1:38 PM, Goffredo Baroncelli<kreijack@gmail.com> wrote:
>> Hi Bart,
>>
>> I replayed in another email to Hugo about that. Basically I am not contrary
>> to the change, only it is unrelated to my patches. In may patches I use the
>> function pretty_sizes() which adds the suffix KB, MB, and this function was
>> here from 2008....
>
> What we could do is change all callers to use pretty_sizes_r(), which
> most already do. We could then add a mode flag for SI units, say
> DF_HUMAN_SI_UNIT. I would then propose that we enable it by default,
> like the human-readable mode itself, and create an option to disable
> it.
>
> I like options a lot, but I like sensible defaults as well. We're
> hoping btrfs will be the next major filesystem for linux. If we
> succeed, it will have a long lifespan and be used on petabyte and
> maybe exabyte storage systems. If we keep showing binary bytes, the
> discrepancy between what the drive's box says and what 'df' says will
> become ever larger.
Pay attention to the fact that the hard-disk manufacturers use the SI
units, but all the OS works in IEC (KiB, MiB...) units. E.g. btrfs
allocates chunk in unit of 256MB; the chunks are divided in pages (4KiB).
I am not fully convinced that we should use SI everywhere; may be that
let the user to select in which unit system the result should be printed
may be the best compromise.
> This is probably the last chance we'll get to make
> btrfs set a good example by getting rid of this outdated method of
> display.
>
> I can make the changes to btrfs-progs if you like.
If you want, free feel to do anything. Please don't ask me the
permission because I am not the "owner" of the code :-)
My patches and the one which we are discussing are completely unrelated,
so these could be made/updated separately.
Pay attention only to the text alignment where needed.
>
> Best regards,
>
> Bart
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-09 18:22 ` Goffredo Baroncelli
@ 2012-10-12 9:42 ` Martin Steigerwald
0 siblings, 0 replies; 23+ messages in thread
From: Martin Steigerwald @ 2012-10-12 9:42 UTC (permalink / raw)
To: linux-btrfs; +Cc: Goffredo Baroncelli, Bart Noordervliet, Hugo Mills
Hi Goffredo, Bart, Hugo,
Am Dienstag, 9. Oktober 2012 schrieb Goffredo Baroncelli:
> On 10/09/2012 02:51 PM, Bart Noordervliet wrote:
> > On Tue, Oct 9, 2012 at 1:38 PM, Goffredo
Baroncelli<kreijack@gmail.com> wrote:
> >> Hi Bart,
> >>
> >> I replayed in another email to Hugo about that. Basically I am not
> >> contrary to the change, only it is unrelated to my patches. In may
> >> patches I use the function pretty_sizes() which adds the suffix KB,
> >> MB, and this function was here from 2008....
> >
> > What we could do is change all callers to use pretty_sizes_r(), which
> > most already do. We could then add a mode flag for SI units, say
> > DF_HUMAN_SI_UNIT. I would then propose that we enable it by default,
> > like the human-readable mode itself, and create an option to disable
> > it.
> >
> > I like options a lot, but I like sensible defaults as well. We're
> > hoping btrfs will be the next major filesystem for linux. If we
> > succeed, it will have a long lifespan and be used on petabyte and
> > maybe exabyte storage systems. If we keep showing binary bytes, the
> > discrepancy between what the drive's box says and what 'df' says will
> > become ever larger.
>
> Pay attention to the fact that the hard-disk manufacturers use the SI
> units, but all the OS works in IEC (KiB, MiB...) units. E.g. btrfs
> allocates chunk in unit of 256MB; the chunks are divided in pages
> (4KiB). I am not fully convinced that we should use SI everywhere; may
> be that let the user to select in which unit system the result should
> be printed may be the best compromise.
I second this.
df -h is binary by default, du -h too. There is an option to change to
power to ten (--si).
LVM commands are binary by default as well.
KDE applications like Dolphin show KiB, MiB, GiB since quite some time.
For file sizes and if enabled for amount of free space as well.
Only fdisk / cfdisk / parted are using power of ten, but they are rarely
used.
If now btrfs filesystem df shows power of ten by default, a user could be
surprised why a 100 MiB file does not fit on a drive with 103 MB free.
So I do not see much sense in using power of ten in just one place. If
changing to power of ten by default, I think its good to do it everywhere.
And that would require some kind of coordinated effort. And it would break
scripts relying on binary output of df -h and du -h. I think its not wise
of scripts to do that, heck I am quite skeptical of parsing command output
thats formatted for the user at all, but…
Ciao,
--
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA B82F 991B EAAC A599 84C7
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 15:01 ` Ilya Dryomov
2012-10-03 16:46 ` Goffredo Baroncelli
@ 2012-10-12 9:55 ` Martin Steigerwald
1 sibling, 0 replies; 23+ messages in thread
From: Martin Steigerwald @ 2012-10-12 9:55 UTC (permalink / raw)
To: linux-btrfs
Cc: Ilya Dryomov, Goffredo Baroncelli, Chris Mason, Goffredo Baroncelli
Am Mittwoch, 3. Oktober 2012 schrieb Ilya Dryomov:
> > $ ./btrfs filesystem df /
> > Path: /
> >
> > Summary:
> > Disk_size: 72.57GB
> > Disk_allocated: 25.10GB
> > Disk_unallocated: 47.48GB
> > Logical_size: 23.06GB
> > Used: 11.01GB
> > Free_(Estimated): 55.66GB (Max: 59.52GB, Min: 35.78GB)
> > Data_to_disk_ratio: 92 %
> >
> >
> > Details:
How about calling this "Chunks:"?
Its actually what is being displayed.
> > Chunk-type Mode Chunk-size Logical-size Used
>
> "Type" for the first column is probably enough.
Then Chunk-type can become "Type"...
> Why is the third column called Chunk-size? If my understanding is
> correct, it's just a break down of Disk_allocated from the summary
> section. If so, why not call it Disk_allocated to avoid confusion?
... and "Chunk-size" just "Size".
> Also, why do you use dashes instead of underbars for table headers?
I prefer dashes, or even spaces, but spaces are more difficult to parse.
So or so I think scripts better do not parse a user formatted output – it
sets the output format thats intended for the users viewing pleasure in
stone. Then its better to provide an option to get all of this in
parseable format. fio 2.0.10 has a JSON outputter for easy parsing, maybe
some of that can be used – in a different patch set.
Ciao,
--
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA B82F 991B EAAC A599 84C7
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH][BTRFS-PROGS][V1] btrfs filesystem df
2012-10-03 17:46 ` Ilya Dryomov
2012-10-03 20:01 ` Goffredo Baroncelli
@ 2012-10-12 10:01 ` Martin Steigerwald
1 sibling, 0 replies; 23+ messages in thread
From: Martin Steigerwald @ 2012-10-12 10:01 UTC (permalink / raw)
To: linux-btrfs
Cc: Ilya Dryomov, Goffredo Baroncelli, Chris Mason, Goffredo Baroncelli
Am Mittwoch, 3. Oktober 2012 schrieb Ilya Dryomov:
> On Wed, Oct 03, 2012 at 06:46:00PM +0200, Goffredo Baroncelli wrote:
> > On 10/03/2012 05:01 PM, Ilya Dryomov wrote:
> > >"Type" for the first column is probably enough.
> > >
> > >Why is the third column called Chunk-size? If my understanding is
> > >correct, it's just a break down of Disk_allocated from the summary
> > >section. If so, why not call it Disk_allocated to avoid confusion?
> >
> >
> >
> > Using everywhere Disk_<something> was my first attempt. But after
> > some thoughts I decided that these are two different kind of
> > information. It is true that Disk_allocated is the sum of
> > Chunk-Sizes... But my feels is that this is a kind of
> > "implementation details". If some other type of allocation unit will
> > be added to BTRFS, then these will be added to Disk_allocated, but
> > not to Chunk list...
> > I prefer to not change the wording until an enough critical mass of
> > people converge to a unique solution .
>
> It is the chunks that is the implementation detail that we want to
> hide. Average Btrfs user wouldn't want to know anything about chunks,
> the only thing he'd be interested in is Disk_allocated and similar
> fields.
Hmm, thats an argument as well.
But how to name it differently without completely hiding aways what it is.
One could speak of data types or so. So we have Data, Metadata and System
data areas on the disk. That are made of several chunks. But since the
command displays the summary of all chunks, one could speak about data
areas.
Anybody a better name for the summary of all chunks of one type?
Anyway, I like it as it is in this patch set way more than before. So from
my point of view: Put it in and probably change output a bit later when
there is some more feedback about it available.
Thanks,
--
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA B82F 991B EAAC A599 84C7
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2012-10-12 10:01 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-03 11:43 [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Goffredo Baroncelli
2012-10-03 11:43 ` [PATCH 1/2] Update btrfs filesystem df command Goffredo Baroncelli
2012-10-03 15:02 ` Ilya Dryomov
2012-10-03 16:34 ` Goffredo Baroncelli
2012-10-03 17:20 ` Ilya Dryomov
2012-10-03 17:38 ` Goffredo Baroncelli
2012-10-03 17:09 ` Goffredo Baroncelli
2012-10-03 11:43 ` [PATCH 2/2] Update help page Goffredo Baroncelli
2012-10-03 11:56 ` [PATCH][BTRFS-PROGS][V1] btrfs filesystem df Hugo Mills
2012-10-03 16:17 ` Goffredo Baroncelli
2012-10-03 16:34 ` Hugo Mills
2012-10-09 9:43 ` Bart Noordervliet
2012-10-09 11:38 ` Goffredo Baroncelli
2012-10-09 12:51 ` Bart Noordervliet
2012-10-09 18:22 ` Goffredo Baroncelli
2012-10-12 9:42 ` Martin Steigerwald
2012-10-03 15:01 ` Ilya Dryomov
2012-10-03 16:46 ` Goffredo Baroncelli
2012-10-03 17:46 ` Ilya Dryomov
2012-10-03 20:01 ` Goffredo Baroncelli
2012-10-03 20:24 ` Ilya Dryomov
2012-10-12 10:01 ` Martin Steigerwald
2012-10-12 9:55 ` Martin Steigerwald
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.