| Message ID | 1404463129-28350-2-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Jul 04, 2014 at 04:38:49PM +0800, Qu Wenruo wrote: > 'btrfs fi df' command is currently able to be executed on any file/dir > inside btrfs since it uses btrfs ioctl to get disk usage info. > > However it is somewhat confusing for some end users since normally such > command should only be executed on a mount point. I disagree here, it's much more convenient to run 'fi df' anywhere and get the output. The system 'df' command works the same way. The 'fi df' command itself is not that user friendly and the numbers need further interpretation. I'm using it heavily during debugging and restricting it to the mountpoint seems too artifical, the tool can cope with that. The 'fi usage' is supposed to give the user-friendly overview, but the patchset is stuck because I found the numbers wrong or misleading under some circumstances. I'll reread the thread that motivated this patch to see if there's something to address. -- 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 Fri, 04 Jul 2014 15:52:26 +0200 as excerpted: > I disagree here, it's much more convenient to run 'fi df' anywhere and > get the output. The system 'df' command works the same way. Agreed. I believe the proposed patch (at least based on its commit comment), however, simply warns if the fi df is not done on a mount-point, giving the mount-point that it's printing information for. IMO that /is/ helpful, and comparable to standard df, which has a mountpoint column, so with the patch the same mount-point information is available from fi df as from normal df, just a bit differently. It wasn't quite clear from the discussion whether that would be the case, and I was prepared to protest the patch if it wasn't, but the way things turned out (based on the commit comment, I don't claim to read code) seems fine to me.
-------- Original Message -------- Subject: Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command From: David Sterba <dsterba@suse.cz> To: Qu Wenruo <quwenruo@cn.fujitsu.com> Date: 2014?07?04? 21:52 > On Fri, Jul 04, 2014 at 04:38:49PM +0800, Qu Wenruo wrote: >> 'btrfs fi df' command is currently able to be executed on any file/dir >> inside btrfs since it uses btrfs ioctl to get disk usage info. >> >> However it is somewhat confusing for some end users since normally such >> command should only be executed on a mount point. > I disagree here, it's much more convenient to run 'fi df' anywhere and > get the output. The system 'df' command works the same way. > > The 'fi df' command itself is not that user friendly and the numbers > need further interpretation. I'm using it heavily during debugging and > restricting it to the mountpoint seems too artifical, the tool can cope > with that. Oh, if 'fi df' is mainly for debug, then I am OK for not merging this patchset. > > The 'fi usage' is supposed to give the user-friendly overview, but the > patchset is stuck because I found the numbers wrong or misleading under > some circumstances. Although these patchset may not be merged, I'm curious about the wrong numbers. Would you please provide some leads or hints? BTW, the indent in cmd_filesystem_df is 7 space, which is not normal 1 tab(8 space). Thanks, Qu > > I'll reread the thread that motivated this patch to see if there's > something to address. -- 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 Fri, Jul 04, 2014 at 03:52:26PM +0200, David Sterba wrote: >On Fri, Jul 04, 2014 at 04:38:49PM +0800, Qu Wenruo wrote: >> 'btrfs fi df' command is currently able to be executed on any file/dir >> inside btrfs since it uses btrfs ioctl to get disk usage info. >> >> However it is somewhat confusing for some end users since normally such >> command should only be executed on a mount point. > >I disagree here, it's much more convenient to run 'fi df' anywhere and >get the output. The system 'df' command works the same way. Just to clarify, in case my earlier mail did not convey the idea properly. The basic difference between traditional df & btrfs fi df is that traditional df does not errors out when no arg is given & <without an arg> outputs all the mounted FSes with their mount points. So to be consistent, btrfs fi df should output all BTRFSes with mount points if no arg is given. Btrfs fi df insists for an arg <path> but does not clarifies in its output if the given arg is a path inside of a mount point or is the mount point itself, which can become transparent, if the mount point is also shown in the output. This is a just a request & a pointer to an oversight/anomaly but if the developers do not feel in resonance with it right now then I just wish that they keep it in mind, think about it & remove this confusion caused by btrfs fi df as,when & how they feel fit. > >The 'fi df' command itself is not that user friendly and the numbers >need further interpretation. I'm using it heavily during debugging and >restricting it to the mountpoint seems too artifical, the tool can cope >with that. > >The 'fi usage' is supposed to give the user-friendly overview, but the >patchset is stuck because I found the numbers wrong or misleading under >some circumstances. > >I'll reread the thread that motivated this patch to see if there's >something to address. Thanks
On 7/4/14, 8:52 AM, David Sterba wrote: > On Fri, Jul 04, 2014 at 04:38:49PM +0800, Qu Wenruo wrote: >> 'btrfs fi df' command is currently able to be executed on any file/dir >> inside btrfs since it uses btrfs ioctl to get disk usage info. >> >> However it is somewhat confusing for some end users since normally such >> command should only be executed on a mount point. > > I disagree here, it's much more convenient to run 'fi df' anywhere and > get the output. The system 'df' command works the same way. I agree with that, and said as much in the original bug filed @Fedora. -Eric -- 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
-------- Original Message -------- Subject: Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command From: Vikram Goyal <vikigoyal@gmail.com> To: linux-btrfs@vger.kernel.org Date: 2014?07?07? 17:51 > On Fri, Jul 04, 2014 at 03:52:26PM +0200, David Sterba wrote: >> On Fri, Jul 04, 2014 at 04:38:49PM +0800, Qu Wenruo wrote: >>> 'btrfs fi df' command is currently able to be executed on any file/dir >>> inside btrfs since it uses btrfs ioctl to get disk usage info. >>> >>> However it is somewhat confusing for some end users since normally such >>> command should only be executed on a mount point. >> >> I disagree here, it's much more convenient to run 'fi df' anywhere and >> get the output. The system 'df' command works the same way. > > Just to clarify, in case my earlier mail did not convey the idea > properly. > > The basic difference between traditional df & btrfs fi df is that > traditional df does not errors out when no arg is given & <without an > arg> outputs all the mounted FSes with their mount points. So to be > consistent, btrfs fi df should output all BTRFSes with mount points if > no arg is given. > > Btrfs fi df insists for an arg <path> but does not clarifies in its > output if the given arg is a path inside of a mount point or is the > mount point itself, which can become transparent, if the mount point is > also shown in the output. IMO this is much better. Cc David. What about this idea? No extra warning but output the mount point? Since if calling find_mount_root(), it will check whether the mount point is btrfs, which can provide more meaningful error message than the original "ERROR: couldn't get space info - Inappropriate ioctl for device" error message. Thanks, Qu > > This is a just a request & a pointer to an oversight/anomaly but if the > developers do not feel in resonance with it right now then I just wish > that they keep it in mind, think about it & remove this confusion caused > by btrfs fi df as,when & how they feel fit. > >> >> The 'fi df' command itself is not that user friendly and the numbers >> need further interpretation. I'm using it heavily during debugging and >> restricting it to the mountpoint seems too artifical, the tool can cope >> with that. >> >> The 'fi usage' is supposed to give the user-friendly overview, but the >> patchset is stuck because I found the numbers wrong or misleading under >> some circumstances. >> >> I'll reread the thread that motivated this patch to see if there's >> something to address. > > Thanks > -- 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 Mon, Jul 07, 2014 at 08:44:56AM +0800, Qu Wenruo wrote: > >The 'fi usage' is supposed to give the user-friendly overview, but the > >patchset is stuck because I found the numbers wrong or misleading under > >some circumstances. > Although these patchset may not be merged, I'm curious about the wrong > numbers. > Would you please provide some leads or hints? The calculations led to bogus numbers in some cases for the minimum free due to rounding errors using the calculated factor 'K' and a small negative number appeared as 16E. I have patches that calculate the numbers a bit differently. -- 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/cmds-filesystem.c b/cmds-filesystem.c index 4b2d27e..bce882f 100644 --- a/cmds-filesystem.c +++ b/cmds-filesystem.c @@ -187,6 +187,8 @@ static int cmd_filesystem_df(int argc, char **argv) int ret; int fd; char *path; + char *real_path = NULL; + char *mount_point = NULL; DIR *dirstream = NULL; if (check_argc_exact(argc, 2)) @@ -194,9 +196,30 @@ static int cmd_filesystem_df(int argc, char **argv) path = argv[1]; + ret = find_mount_root(path, &mount_point); + if (ret < 0) { + fprintf(stderr, "ERROR: fail to find mount root of %s: %s\n", + path, strerror(-ret)); + return 1; + } + real_path = realpath(path, NULL); + if (!real_path) { + fprintf(stderr, "ERROR: failed to find realpath of %s: %s\n", + path, strerror(errno)); + free(mount_point); + return 1; + } + if (strcmp(real_path, mount_point)) { + fprintf(stderr, "WARNING: %s is not a mount point\n", path); + fprintf(stderr, "WARNING: report disk usage of %s instead\n", + mount_point); + } + free(real_path); + path = mount_point; fd = open_file_or_dir(path, &dirstream); if (fd < 0) { fprintf(stderr, "ERROR: can't access '%s'\n", path); + free(mount_point); return 1; } ret = get_df(fd, &sargs); @@ -209,6 +232,7 @@ static int cmd_filesystem_df(int argc, char **argv) } close_file_or_dir(fd, dirstream); + free(mount_point); return !!ret; }
'btrfs fi df' command is currently able to be executed on any file/dir inside btrfs since it uses btrfs ioctl to get disk usage info. However it is somewhat confusing for some end users since normally such command should only be executed on a mount point. This patch add mount point check in 'btrfs fi df' and if a file/dir inside a btrfs is given, a warning message will be printed and still output the disk usage info to keep the old behavior. Reported-by: Vikram Goyal <vikigoyal@gmail.com> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> --- cmds-filesystem.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)