[PATCH 1/2] btrfs-progs: Check fstype in find_mount_root()

[PATCH 1/2] btrfs-progs: Check fstype in find_mount_root()

[Qu Wenruo]

When calling find_mount_root(), caller in fact wants to find the mount
point of *BTRFS*.

So also check ent->fstype in find_mount_root() and output proper error
messages if needed.
This will suppress a lot of "Inapproiate ioctl for device" error
message.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 utils.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/utils.c b/utils.c
index 993d085..592a73b 100644
--- a/utils.c
+++ b/utils.c
@@ -2412,6 +2412,7 @@ int find_mount_root(const char *path, char **mount_root)
 	struct mntent *ent;
 	int len;
 	int ret;
+	int not_btrfs;
 	int longest_matchlen = 0;
 	char *longest_match = NULL;
 
@@ -2432,6 +2433,10 @@ int find_mount_root(const char *path, char **mount_root)
 				free(longest_match);
 				longest_matchlen = len;
 				longest_match = strdup(ent->mnt_dir);
+				if (strcmp(ent->mnt_type, "btrfs"))
+					not_btrfs = 1;
+				else
+					not_btrfs = 0;
 			}
 		}
 	}
@@ -2443,6 +2448,12 @@ int find_mount_root(const char *path, char **mount_root)
 			path);
 		return -ENOENT;
 	}
+	if (not_btrfs) {
+		fprintf(stderr,
+			"ERROR: %s does not belong to a btrfs mount points.\n",
+			path);
+		return -ENOENT;
+	}
 
 	ret = 0;
 	*mount_root = realpath(longest_match, NULL);
-- 
2.0.1

[PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command

[Qu Wenruo]

'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(+)

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;
 }
 
-- 
2.0.1

Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command

[David Sterba]

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.

Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command

[Duncan]

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.

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

Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command

[Qu Wenruo]

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

Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command

[David Sterba]

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.

Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command

[Vikram Goyal]

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

-- 
vikram...
         ||||||||
         ||||||||
^^'''''^^||root||^^^'''''''^^
        // \\   ))
       //(( \\// \\
      // /\\ ||   \\
     || / )) ((    \\
-- 
Rule of Life #1 -- Never get separated from your luggage.
-- 
  _
 ~|~
  =

Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command

[Qu Wenruo]

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

Re: [PATCH 2/2] btrfs-progs: Add mount point check for 'btrfs fi df' command

[Eric Sandeen]

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