Btrfs devel - Oct 2013 - [PATCH 1/3 v4] btrfs-progs: use kernel for mounted disk for show

As of now btrfs filesystem show reads directly from
disks. So sometimes output can be stale, mainly when
user wants to cross verify their operation like,
label or device delete or add... etc. so this
patch will read from the kernel ioctl if it finds
that disk is mounted.

v4:
  rebase on integration-20131001-3
v3:
  rebase on 20130920 and splits new parameter to new patch
v2:
  accepts David suggested

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-filesystem.c |  200 +++++++++++++++++++++++++++++++++++++++++++++++++----
 man/btrfs.8.in    |   16 +++--
 utils.h           |   11 +++-
 3 files changed, 205 insertions(+), 22 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 780baad..9e0c8b9 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -25,6 +25,9 @@
 #include <ctype.h>
 #include <fcntl.h>
 #include <ftw.h>
+#include <mntent.h>
+#include <linux/limits.h>
+#include <getopt.h>
 
 #include "kerncompat.h"
 #include "ctree.h"
@@ -251,9 +254,131 @@ static void print_one_uuid(struct btrfs_fs_devices
*fs_devices)
 	printf("\n");
 }
 
+/* adds up all the used spaces as reported by the space info ioctl
+ */
+static u64 calc_used_bytes(struct btrfs_ioctl_space_args *si)
+{
+	u64 ret = 0;
+	int i;
+	for (i = 0; i < si->total_spaces; i++)
+		ret += si->spaces[i].used_bytes;
+	return ret;
+}
+
+static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
+		struct btrfs_ioctl_dev_info_args *dev_info,
+		struct btrfs_ioctl_space_args *space_info,
+		char *label, char *path)
+{
+	int i;
+	char uuidbuf[37];
+	struct btrfs_ioctl_dev_info_args *tmp_dev_info;
+
+	uuid_unparse(fs_info->fsid, uuidbuf);
+	printf("Label: %s  uuid: %s\n",
+		strlen(label) ? label : "none", uuidbuf);
+
+	printf("\tTotal devices %llu FS bytes used %s\n",
+				fs_info->num_devices,
+			pretty_size(calc_used_bytes(space_info)));
+
+	for (i = 0; i < fs_info->num_devices; i++) {
+		tmp_dev_info = (struct btrfs_ioctl_dev_info_args *)&dev_info[i];
+		printf("\tdevid    %llu size %s used %s path %s\n",
+			tmp_dev_info->devid,
+			pretty_size(tmp_dev_info->total_bytes),
+			pretty_size(tmp_dev_info->bytes_used),
+			tmp_dev_info->path);
+	}
+
+	printf("\n");
+	return 0;
+}
+
+/* This function checks if the given input parameter is
+ * an uuid or a path
+ * return -1: some error in the given input
+ * return 0: unknow input
+ * return 1: given input is uuid
+ * return 2: given input is path
+ */
+static int check_arg_type(char *input)
+{
+	uuid_t	out;
+	char path[PATH_MAX];
+
+	if (!input)
+		return BTRFS_ARG_UNKNOWN;
+
+	if (realpath(input, path))
+		return BTRFS_ARG_PATH;
+
+	if (!uuid_parse(input, out))
+		return BTRFS_ARG_UUID;
+
+	return BTRFS_ARG_UNKNOWN;
+}
+
+static int btrfs_scan_kernel(void *search)
+{
+	int ret = 0, fd, type;
+	FILE *f;
+	struct mntent *mnt;
+	struct btrfs_ioctl_fs_info_args fs_info_arg;
+	struct btrfs_ioctl_dev_info_args *dev_info_arg = NULL;
+	struct btrfs_ioctl_space_args *space_info_arg;
+	char label[BTRFS_LABEL_SIZE];
+	uuid_t uuid;
+
+	f = setmntent("/proc/self/mounts", "r");
+	if (f == NULL)
+		return 1;
+
+	type = check_arg_type(search);
+
+	while ((mnt = getmntent(f)) != NULL) {
+		if (strcmp(mnt->mnt_type, "btrfs"))
+			continue;
+		ret = get_fs_info(mnt->mnt_dir, &fs_info_arg,
+				&dev_info_arg);
+		if (ret)
+			return ret;
+
+		switch (type) {
+		case BTRFS_ARG_UUID:
+			ret = uuid_parse(search, uuid);
+			if (ret)
+				return 1;
+			if (uuid_compare(fs_info_arg.fsid, uuid))
+				continue;
+			break;
+		case BTRFS_ARG_PATH:
+			if (strcmp(search, mnt->mnt_dir))
+				continue;
+			break;
+		default:
+			break;
+		}
+
+		fd = open(mnt->mnt_dir, O_RDONLY);
+		if (fd > 0 && !get_df(fd, &space_info_arg)) {
+			get_label_mounted(mnt->mnt_dir, label);
+			print_one_fs(&fs_info_arg, dev_info_arg,
+					space_info_arg, label, mnt->mnt_dir);
+			free(space_info_arg);
+		}
+		if (fd > 0)
+			close(fd);
+		free(dev_info_arg);
+	}
+	return ret;
+}
+
 static const char * const cmd_show_usage[] = {
-	"btrfs filesystem show [--all-devices|<uuid>]",
+	"btrfs filesystem show [options] [<path>|<uuid>]",
 	"Show the structure of a filesystem",
+	"-d|--all-devices   show only disks under /dev containing btrfs
filesystem",
+	"-m|--mounted       show only mounted btrfs",
 	"If no argument is given, structure of all present filesystems is
shown.",
 	NULL
 };
@@ -262,38 +387,87 @@ static int cmd_show(int argc, char **argv)
 {
 	struct list_head *all_uuids;
 	struct btrfs_fs_devices *fs_devices;
+	struct btrfs_device *device;
 	struct list_head *cur_uuid;
 	char *search = NULL;
 	int ret;
 	int where = BTRFS_SCAN_PROC;
-	int searchstart = 1;
-
-	if( argc > 1 && !strcmp(argv[1],"--all-devices")){
-		where = BTRFS_SCAN_DEV;
-		searchstart += 1;
+	int type = 0;
+
+	while (1) {
+		int long_index;
+		static struct option long_options[] = {
+			{ "all-devices", no_argument, NULL, ''d''},
+			{ "mounted", no_argument, NULL, ''m''},
+			{ NULL, no_argument, NULL, 0 },
+		};
+		int c = getopt_long(argc, argv, "dm", long_options,
+					&long_index);
+		if (c < 0)
+			break;
+		switch (c) {
+		case ''d'':
+			where = BTRFS_SCAN_DEV;
+			break;
+		case ''m'':
+			where = BTRFS_SCAN_MOUNTED;
+			break;
+		default:
+			usage(cmd_show_usage);
+		}
 	}
 
-	if (check_argc_max(argc, searchstart + 1))
+	if (check_argc_max(argc, optind + 1))
 		usage(cmd_show_usage);
+	if (argc > optind) {
+		search = argv[optind];
+		type = check_arg_type(search);
+		if (type == BTRFS_ARG_UNKNOWN) {
+			fprintf(stderr, "ERROR: arg type unknown\n");
+			usage(cmd_show_usage);
+		}
+	}
+
+	if (where == BTRFS_SCAN_DEV)
+		goto devs_only;
 
-	ret = scan_for_btrfs(where, 0);
+	/* show mounted btrfs */
+	btrfs_scan_kernel(search);
 
-	if (ret){
-		fprintf(stderr, "ERROR: error %d while scanning\n", ret);
+	/* shows mounted only */
+	if (where == BTRFS_SCAN_MOUNTED)
+		goto out;
+
+devs_only:
+	ret = scan_for_btrfs(where, !BTRFS_UPDATE_KERNEL);
+
+	if (ret) {
+		fprintf(stderr, "ERROR: %d while scanning\n", ret);
 		return 1;
 	}
 	
-	if(searchstart < argc)
-		search = argv[searchstart];
-
 	all_uuids = btrfs_scanned_uuids();
 	list_for_each(cur_uuid, all_uuids) {
 		fs_devices = list_entry(cur_uuid, struct btrfs_fs_devices,
 					list);
 		if (search && uuid_search(fs_devices, search) == 0)
 			continue;
+
+		/* skip mounted as they are already printed by
+		 * btrfs_scan_kernel
+		*/
+		/* do it only for the default, no option */
+		if (where == BTRFS_SCAN_PROC) {
+			device = list_entry(fs_devices->devices.next,
+					struct btrfs_device, dev_list);
+			ret = check_mounted(device->name);
+			if (ret)
+				continue;
+		}
 		print_one_uuid(fs_devices);
 	}
+
+out:
 	printf("%s\n", BTRFS_BUILD_VERSION);
 	return 0;
 }
diff --git a/man/btrfs.8.in b/man/btrfs.8.in
index 8a0a05d..ff2427e 100644
--- a/man/btrfs.8.in
+++ b/man/btrfs.8.in
@@ -25,7 +25,7 @@ btrfs \- control a btrfs filesystem
 .PP
 \fBbtrfs\fP \fBfilesystem df\fP\fI <path>\fP
 .PP
-\fBbtrfs\fP \fBfilesystem show\fP
[--all-devices|\fI<uuid>\fP|\fI<label>\fP]\fP
+\fBbtrfs\fP \fBfilesystem show\fP
[\fI--mounted\fP|\fI--all-devices\fP|\fI<uuid>\fP]\fP
 .PP
 \fBbtrfs\fP \fBfilesystem sync\fP\fI <path> \fP
 .PP
@@ -51,7 +51,7 @@ btrfs \- control a btrfs filesystem
 .PP
 \fBbtrfs\fP \fBdevice delete\fP \fI<device>\fP [\fI<device>...\fP]
\fI<path>\fP
 .PP
-\fBbtrfs\fP \fBdevice scan\fP [--all-devices|\fI<device>
\fP[\fI<device>...\fP]
+\fBbtrfs\fP \fBdevice scan\fP [\fI--all-devices\fP|\fI<device>
\P[\fI<device>...\fP]
 .PP
 \fBbtrfs\fP \fBdevice ready\fP\fI <device>\fP
 .PP
@@ -261,9 +261,11 @@ Show information of a given subvolume in the
\fI<path>\fR.
 Show space usage information for a mount point.
 .TP
 
-\fBfilesystem show\fR [--all-devices|\fI<uuid>\fR|\fI<label>\fR]\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.
+\fBfilesystem show\fR
[\fI--mounted\fP|\fI--all-devices\fP|\fI<uuid>\fR]\fR
+Show the btrfs filesystem with some additional info. If no option or \fIUUID\fP
+is passed, \fBbtrfs\fR shows information of all the btrfs filesystem both
mounted
+and unmounted.
+If \fB--mounted\fP is passed, it would probe btrfs kernel to list mounted btrfs
filesystem(s);
 If \fB--all-devices\fP is passed, all the devices under /dev are scanned;
 otherwise the devices list is extracted from the /proc/partitions file.
 .TP
@@ -410,8 +412,8 @@ Remove device(s) from a filesystem identified by
\fI<path>\fR.
 
 \fBdevice scan\fR [--all-devices|\fI<device>
\fP[\fI<device>...\fP]\fR
 If one or more devices are passed, these are scanned for a btrfs filesystem. 
-If no devices are passed, \fBbtrfs\fR scans all the block devices listed
-in the /proc/partitions file.
+If no devices are passed, \fBbtrfs\fR uses block devices containing btrfs
+filesystem as listed by blkid.
 Finally, if \fB--all-devices\fP is passed, all the devices under /dev are 
 scanned.
 .TP
diff --git a/utils.h b/utils.h
index a6d3e5e..0f31db7 100644
--- a/utils.h
+++ b/utils.h
@@ -25,10 +25,17 @@
 
 #define BTRFS_MKFS_SYSTEM_GROUP_SIZE (4 * 1024 * 1024)
 
-#define BTRFS_SCAN_PROC	1
-#define BTRFS_SCAN_DEV		2
+#define BTRFS_SCAN_PROC		(1ULL << 0)
+#define BTRFS_SCAN_DEV		(1ULL << 1)
+#define BTRFS_SCAN_MOUNTED	(1ULL << 2)
 #define BTRFS_SCAN_LBLKID	(1ULL << 3)
 
+#define BTRFS_UPDATE_KERNEL	1
+
+#define BTRFS_ARG_UNKNOWN	0
+#define BTRFS_ARG_PATH		1
+#define BTRFS_ARG_UUID		2
+
 int make_btrfs(int fd, const char *device, const char *label,
 	       u64 blocks[6], u64 num_bytes, u32 nodesize,
 	       u32 leafsize, u32 sectorsize, u32 stripesize, u64 features);
-- 
1.7.1

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

for mounted btrfs filesystem this patch proposes to add
mount point and group profile to the filesystem show output.
this helps user to quickly understand short details of the
btrfs filesystem quickly.

end user using this new btrfs fi show would surely notice this
will reduce other commands normally used following the current
btrfs fi show command. (like mount and btrfs fi df). of course
user should use fi df to know detailed info about the sizes.

preview as below..

Label: none  uuid: 26d539a5-8968-4cf0-b4b5-5fd50105f8a0 mounted: /btrfs
	Group profile: Metadata: single  Metadata: DUP  Data: single
	Total devices 1 FS bytes used 28.00KiB
	devid    1 size 1.98GiB used 238.25MiB path /dev/mapper/mpatha

v2: commit message edited.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-filesystem.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 9e0c8b9..74b7a06 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -273,10 +273,21 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args
*fs_info,
 	int i;
 	char uuidbuf[37];
 	struct btrfs_ioctl_dev_info_args *tmp_dev_info;
+	u64 flags;
 
 	uuid_unparse(fs_info->fsid, uuidbuf);
-	printf("Label: %s  uuid: %s\n",
-		strlen(label) ? label : "none", uuidbuf);
+	printf("Label: %s  uuid: %s mounted: %s\n",
+		strlen(label) ? label : "none", uuidbuf, path);
+	printf("\tGroup profile:");
+	for (i = space_info->total_spaces - 1; i >= 0; i--) {
+		flags = space_info->spaces[i].flags;
+		if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
+			continue;
+		printf(" %s: %s", group_type_str(flags),
+					group_profile_str(flags));
+		printf(" ");
+	}
+	printf("\n");
 
 	printf("\tTotal devices %llu FS bytes used %s\n",
 				fs_info->num_devices,
-- 
1.7.1

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

btrfs progs has to scan for the btrfs disks for two main reasons,
 one to register them with the btrfs kernel (under btrfs dev scan)
 2nd to report btrfs disks to the user (under btrfs fi show)
 (there few more minor reasons like check_mounted etc..).

 To facilitate the scan, in total we have the following methods
 to scan for the btrfs

 BTRFS_SCAN_PROC
  which uses the /proc/partitions to look for the disks, when
  scanning it does it twice first would look for non dm- paths
  and in the 2nd scan it would pick only dm- paths.

 BTRFS_SCAN_DEV
  which scans all the block dev under /dev as they appear during
  scanning.

 BTRFS_SCAN_LBLKID
  this uses the library functions provided  by the lblkid to get
  only disks which contains the btrfs SB.

 The better method to use would be BTRFS_SCAN_LBLKID for the obvious
 reasons we don''t have to reinvent that feature with in btrfs-progs.

 For the btrfs fi show - This patch will..
   - make BTRFS_SCAN_LBLKID as the default scan option

   (BTRFS_SCAN_DEV is accessible under the option --all-devices and
   BTRFS_SCAN_PROC won''t be used by btrfs fi show any more)

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-filesystem.c |   14 +-------------
 1 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 74b7a06..b737ec9 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -398,11 +398,10 @@ static int cmd_show(int argc, char **argv)
 {
 	struct list_head *all_uuids;
 	struct btrfs_fs_devices *fs_devices;
-	struct btrfs_device *device;
 	struct list_head *cur_uuid;
 	char *search = NULL;
 	int ret;
-	int where = BTRFS_SCAN_PROC;
+	int where = BTRFS_SCAN_LBLKID;
 	int type = 0;
 
 	while (1) {
@@ -464,17 +463,6 @@ devs_only:
 		if (search && uuid_search(fs_devices, search) == 0)
 			continue;
 
-		/* skip mounted as they are already printed by
-		 * btrfs_scan_kernel
-		*/
-		/* do it only for the default, no option */
-		if (where == BTRFS_SCAN_PROC) {
-			device = list_entry(fs_devices->devices.next,
-					struct btrfs_device, dev_list);
-			ret = check_mounted(device->name);
-			if (ret)
-				continue;
-		}
 		print_one_uuid(fs_devices);
 	}
 
-- 
1.7.1

--
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, Oct 08, 2013 at 11:41:38AM +0800, Anand Jain
wrote:
> As of now btrfs filesystem show reads directly from
> disks. So sometimes output can be stale, mainly when
> user wants to cross verify their operation like,
> label or device delete or add... etc. so this
> patch will read from the kernel ioctl if it finds
> that disk is mounted.
Sorry for holding this patch back for so long, I wanted to find out why
the output is different before and after. The reason is simple, for my
convenience I''ve added the user to the ''disk'' group
so I can access the
block devices without root. This patch stops to read the block devs
directly and uses the ioctl BTRFS_IOC_FS_INFO which gives me a silent
EPERM. If I run show with sudo, then all filesystems show up as
expected.

david
--
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, Oct 08, 2013 at 11:41:39AM +0800, Anand Jain
wrote:
> for mounted btrfs filesystem this patch proposes to add
> mount point and group profile to the filesystem show output.
> this helps user to quickly understand short details of the
> btrfs filesystem quickly.
> 
> end user using this new btrfs fi show would surely notice this
> will reduce other commands normally used following the current
> btrfs fi show command. (like mount and btrfs fi df). of course
> user should use fi df to know detailed info about the sizes.
> 
> preview as below..
> 
> Label: none  uuid: 26d539a5-8968-4cf0-b4b5-5fd50105f8a0 mounted: /btrfs
> 	Group profile: Metadata: single  Metadata: DUP  Data: single
It would look better if the profile types are grouped together,
otherwise it can drift to the right quite far. Why is
Sytem not listed here?

The ''fi show'' command is kind of high-level overview, so
I''m kind of ok
to print the group profiles here.

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

>> preview as below..
>>
>> Label: none  uuid: 26d539a5-8968-4cf0-b4b5-5fd50105f8a0 mounted: /btrfs
>> 	Group profile: Metadata: single  Metadata: DUP  Data: single
>
> It would look better if the profile types are grouped together,
  As of now mkfs.btrfs and balance don''t agree on the same set
  of default group profiles. When we fix mkfs.btrfs (and for
  now when user run balance) we won''t see multiple profile for
  Metadata/Data. So grouping them isn''t needed, rather we should
  fix mkfs and balance to be inline.
  For now (in the FS which contains some data) after the balance
  we won''t see multiple group-profile for same group-type.

  pls comment / let me know.
> otherwise it can drift to the right quite far.
  as of now do we practicality use/support multiple groups
  for the same type (metadata/data), if not the content will
  be always 6 words here (I am considering a system which is
  balanced).
> Why is Sytem not listed here?
  System is not an user choice (?), So users won''t be keen to
  know at least in this high-level display. IMO.

Thanks, Anand

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

> The reason is simple, for my
> convenience I''ve added the user to the ''disk''
group so I can access the
> block devices without root. This patch stops to read the block devs
> directly and uses the ioctl BTRFS_IOC_FS_INFO which gives me a silent
> EPERM.
 > If I run show with sudo, then all filesystems show up as
 > expected.

  Ah. good catch. but what do you suggest. ?
  as of now user on disk-group is able to do things like,
  check --repair. but won''t  be able to list the subvol /
  manage the snapshots, and is able to run btrfs fi show -d
  but won''t be able to run btrfs fi show / -m. what do we
  do in this case ?
  OR when /sys interface is ready that should help to manage
  based on the user-groups.?

Thanks Anand
--
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 Wed, Oct 16, 2013 at 05:25:37PM +0800, Anand Jain
wrote:
>  Ah. good catch. but what do you suggest. ?
>  as of now user on disk-group is able to do things like,
>  check --repair. but won''t  be able to list the subvol /
>  manage the snapshots, and is able to run btrfs fi show -d
>  but won''t be able to run btrfs fi show / -m. what do we
>  do in this case ?
We can drop the CAP_SYS_ADMIN check from btrfs_ioctl_fs_info, it does
not do or revealy sensitive information.
>  OR when /sys interface is ready that should help to manage
>  based on the user-groups.?
This could be decided case by case if a sysfs file should not be
publicly readable, otherwise let them be readable to everyone.

david
--
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, Oct 15, 2013 at 07:13:33PM +0200, David Sterba
wrote:
> On Tue, Oct 08, 2013 at 11:41:38AM +0800, Anand Jain wrote:
> > As of now btrfs filesystem show reads directly from
> > disks. So sometimes output can be stale, mainly when
> > user wants to cross verify their operation like,
> > label or device delete or add... etc. so this
> > patch will read from the kernel ioctl if it finds
> > that disk is mounted.
> 
> Sorry for holding this patch back for so long, I wanted to find out why
> the output is different before and after. The reason is simple, for my
> convenience I''ve added the user to the ''disk''
group so I can access the
> block devices without root. This patch stops to read the block devs
> directly and uses the ioctl BTRFS_IOC_FS_INFO which gives me a silent
> EPERM. If I run show with sudo, then all filesystems show up as
> expected.
> 
This patch needs to be dropped (I imagine the whole series too) as it breaks
xfstests btrfs/003.  If I do btrfs fi show on a mounted fs I get this

[root@destiny btrfs-progs]# btrfs fi show /dev/sdc
Btrfs v0.20-rc1-483-ge0173f6
[root@destiny btrfs-progs]# 

Chris this is in integration, so maybe a revert?  Thanks,

Josef
--
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 10/21/13 10:44 PM, Josef Bacik wrote:
> On Tue, Oct 15, 2013 at 07:13:33PM +0200, David Sterba wrote:
>> On Tue, Oct 08, 2013 at 11:41:38AM +0800, Anand Jain wrote:
>>> As of now btrfs filesystem show reads directly from
>>> disks. So sometimes output can be stale, mainly when
>>> user wants to cross verify their operation like,
>>> label or device delete or add... etc. so this
>>> patch will read from the kernel ioctl if it finds
>>> that disk is mounted.
>>
>> Sorry for holding this patch back for so long, I wanted to find out why
>> the output is different before and after. The reason is simple, for my
>> convenience I''ve added the user to the
''disk'' group so I can access the
>> block devices without root. This patch stops to read the block devs
>> directly and uses the ioctl BTRFS_IOC_FS_INFO which gives me a silent
>> EPERM. If I run show with sudo, then all filesystems show up as
>> expected.
>>
>
> This patch needs to be dropped (I imagine the whole series too) as it
breaks
> xfstests btrfs/003.  If I do btrfs fi show on a mounted fs I get this
>
> [root@destiny btrfs-progs]# btrfs fi show /dev/sdc
> Btrfs v0.20-rc1-483-ge0173f6
> [root@destiny btrfs-progs]#
>
> Chris this is in integration, so maybe a revert?  Thanks,
  That would be less productive approach as a whole.
  I have sent out the fix for this. Kindly find it.

---
[PATCH 1/2] btrfs-progs: make get_btrfs_mount callable
[PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk 
should work
---

Thanks, Anand
--
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, Oct 22, 2013 at 01:53:47PM +0800, Anand Jain
wrote:
> 
> 
> On 10/21/13 10:44 PM, Josef Bacik wrote:
> >On Tue, Oct 15, 2013 at 07:13:33PM +0200, David Sterba wrote:
> >>On Tue, Oct 08, 2013 at 11:41:38AM +0800, Anand Jain wrote:
> >>>As of now btrfs filesystem show reads directly from
> >>>disks. So sometimes output can be stale, mainly when
> >>>user wants to cross verify their operation like,
> >>>label or device delete or add... etc. so this
> >>>patch will read from the kernel ioctl if it finds
> >>>that disk is mounted.
> >>
> >>Sorry for holding this patch back for so long, I wanted to find out
why
> >>the output is different before and after. The reason is simple, for
my
> >>convenience I''ve added the user to the
''disk'' group so I can access the
> >>block devices without root. This patch stops to read the block devs
> >>directly and uses the ioctl BTRFS_IOC_FS_INFO which gives me a
silent
> >>EPERM. If I run show with sudo, then all filesystems show up as
> >>expected.
> >>
> >
> >This patch needs to be dropped (I imagine the whole series too) as it
breaks
> >xfstests btrfs/003.  If I do btrfs fi show on a mounted fs I get this
> >
> >[root@destiny btrfs-progs]# btrfs fi show /dev/sdc
> >Btrfs v0.20-rc1-483-ge0173f6
> >[root@destiny btrfs-progs]#
> >
> >Chris this is in integration, so maybe a revert?  Thanks,
> 
>  That would be less productive approach as a whole.
>  I have sent out the fix for this. Kindly find it.
> 
> ---
> [PATCH 1/2] btrfs-progs: make get_btrfs_mount callable
> [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk should
> work
> ---
> 
Did you test these?  Because they aren''t working for me, so I think a
revert is
the only solution.  Thanks,

Josef
--
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, Oct 22, 2013 at 09:21:47AM -0400, Josef Bacik
wrote:
> On Tue, Oct 22, 2013 at 01:53:47PM +0800, Anand Jain wrote:
> >  That would be less productive approach as a whole.
> >  I have sent out the fix for this. Kindly find it.
> > 
> > ---
> > [PATCH 1/2] btrfs-progs: make get_btrfs_mount callable
> > [PATCH 2/2] btrfs-progs: filesystem show of specified mounted disk
should
> > work
> > ---
> > 
> 
> Did you test these?  Because they aren''t working for me, so I
think a revert is
> the only solution.  Thanks,
   Just for the avoidance of doubt -- Josef means a full xfstests run
on the patches. It may take a long time, but it does mean that
everything(*) gets tested for regressions and helps prevent this
to-and-fro of patches trying to fix up earlier bugs.

   Hugo.

(*) OK, not quite everything, but Eric''s and Josef''s recent
work is
taking us closer to that goal.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ==  PGP
key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
  --- "What''s so bad about being drunk?" "You ask a
glass of water" ---

On Tue, Oct 22, 2013 at 09:21:47AM -0400, Josef Bacik
wrote:
> Did you test these?  Because they aren''t working for me, so I
think a revert is
> the only solution.  Thanks,
The impact of the failing test is imho not that big to justify a full
revert from integration branch (were it in master, then yes). I see the
bug and I hope Anand will fix it soon.

david
--
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 10/23/13 12:52 AM, David Sterba wrote:
> On Tue, Oct 22, 2013 at 09:21:47AM -0400, Josef Bacik wrote:
>> Did you test these?  Because they aren''t working for me, so I
think a revert is
>> the only solution.  Thanks,
>
> The impact of the failing test is imho not that big to justify a full
> revert from integration branch (were it in master, then yes). I see the
> bug and I hope Anand will fix it soon.
  Sure. but what is the bug in btrfs-progs with this patch-set ?

  if its about xfstests btrfs/006 apparently it isn''t working
  without this patch-set as well.

Thanks, Anand
--
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 10/23/13 12:52 AM, David Sterba wrote:
>> On Tue, Oct 22, 2013 at 09:21:47AM -0400, Josef Bacik wrote:
>>> Did you test these? Because they aren''t working for me, so
I think a
>>> revert is
>>> the only solution. Thanks,
>>
>> The impact of the failing test is imho not that big to justify a full
>> revert from integration branch (were it in master, then yes). I see the
>> bug and I hope Anand will fix it soon.
>
> Sure. but what is the bug in btrfs-progs with this patch-set ?
>
> if its about xfstests btrfs/006 apparently it isn''t working
> without this patch-set as well.
  and after btrfs/006 works with out this patch-set,
  we would need btrfs/006.out to be updated with the
  new btrfs fi show output/ IMO it should be part of
  this patch-set I will be sending a patch for xfstests.

Thanks, Anand
--
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