[PATCH 1/2] btrfs-progs: introduce btrfs_register_all_device()

[PATCH 1/2] btrfs-progs: introduce btrfs_register_all_device()

[Anand Jain]

This function is to register all devices found after scanning
the system. Before we had this functionality with in the
btrfs_scan_lblkid(), however scanning and registering are two
different distinct operation its better keep them separate.
Also we want to optimize btrfs_scan_lblkid and avoid multiple
system scans unless needed. As of now device scan uses this function.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 utils.h   |  1 +
 volumes.c | 25 +++++++++++++++++++++++++
 volumes.h |  1 +
 3 files changed, 27 insertions(+)

diff --git a/utils.h b/utils.h
index 3f7b388..237cf64 100644
--- a/utils.h
+++ b/utils.h
@@ -162,5 +162,6 @@ static inline u64 btrfs_min_dev_size(u32 leafsize)
 	return 2 * (BTRFS_MKFS_SYSTEM_GROUP_SIZE +
 		    btrfs_min_global_blk_rsv_size(leafsize));
 }
+int btrfs_register_one_device(char *fname);
 
 #endif
diff --git a/volumes.c b/volumes.c
index 266a474..c8057c8 100644
--- a/volumes.c
+++ b/volumes.c
@@ -30,6 +30,7 @@
 #include "print-tree.h"
 #include "volumes.h"
 #include "math.h"
+#include "utils.h"
 
 struct stripe {
 	struct btrfs_device *dev;
@@ -1533,6 +1534,30 @@ btrfs_find_device_by_devid(struct btrfs_fs_devices *fs_devices,
 	return NULL;
 }
 
+/*
+ * Register all devices in the fs_uuid list created in the user
+ * space. Ensure btrfs_scan_lblkid() is called before this func.
+ */
+int btrfs_register_all_devices(void)
+{
+	int err;
+	struct btrfs_fs_devices *fs_devices;
+	struct btrfs_device *device;
+
+	list_for_each_entry(fs_devices, &fs_uuids, list) {
+		list_for_each_entry(device, &fs_devices->devices, dev_list) {
+			if (strlen(device->name) != 0) {
+				err = btrfs_register_one_device(device->name);
+				if (err < 0)
+					return err;
+				if (err > 0)
+					return -err;
+			}
+		}
+	}
+	return 0;
+}
+
 int btrfs_chunk_readonly(struct btrfs_root *root, u64 chunk_offset)
 {
 	struct cache_extent *ce;
diff --git a/volumes.h b/volumes.h
index 447dd4b..ccba34b 100644
--- a/volumes.h
+++ b/volumes.h
@@ -195,4 +195,5 @@ int write_raid56_with_parity(struct btrfs_fs_info *info,
 			     struct extent_buffer *eb,
 			     struct btrfs_multi_bio *multi,
 			     u64 stripe_len, u64 *raid_map);
+int btrfs_register_all_devices(void);
 #endif
-- 
2.0.0.153.g79dcccc

[PATCH 2/2] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Anand Jain]

btrfs_scan_lblikd is called by most the device related command functions.
And btrfs_scan_lblkid is most expensive function and it becomes more expensive
as number of devices in the system increase.

This patch will:
  move out calling register_one_device with in btrfs_scan_lblkid()
  and so function setting the BTRFS_UPDATE_KERNEL to yes will
  call btrfs_register_all_devices() separately.

  introduce a global variable scan_done, which is set when scan is
  done succssfully per thread. So that following calls to this function
  will just return success.

  Further if any function needs to force scan after scan_done is set,
  then it can be done when there is such a requirement, but as of now there
  isn't any such requirement.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-device.c     |  6 +++++-
 cmds-filesystem.c |  2 +-
 disk-io.c         |  2 +-
 utils.c           | 14 ++++++++++----
 utils.h           |  2 +-
 5 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 57ad0b2..d4605fd 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -29,6 +29,7 @@
 #include "ioctl.h"
 #include "utils.h"
 #include "cmds-fi-disk_usage.h"
+#include "volumes.h"
 
 #include "commands.h"
 
@@ -236,9 +237,12 @@ static int cmd_scan_dev(int argc, char **argv)
 
 	if (all || argc == 1) {
 		printf("Scanning for Btrfs filesystems\n");
-		ret = btrfs_scan_lblkid(BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			fprintf(stderr, "ERROR: error %d while scanning\n", ret);
+		ret = btrfs_register_all_devices();
+		if (ret)
+			fprintf(stderr, "ERROR: error %d while registering\n", ret);
 		goto out;
 	}
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 6fcf111..4a3c09c 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -661,7 +661,7 @@ static int cmd_show(int argc, char **argv)
 		goto out;
 
 devs_only:
-	ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+	ret = btrfs_scan_lblkid();
 
 	if (ret) {
 		fprintf(stderr, "ERROR: %d while scanning\n", ret);
diff --git a/disk-io.c b/disk-io.c
index 7ef5ac4..6a053ea 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1013,7 +1013,7 @@ int btrfs_scan_fs_devices(int fd, const char *path,
 	}
 
 	if (total_devs != 1) {
-		ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			return ret;
 	}
diff --git a/utils.c b/utils.c
index 017c513..f18cc46 100644
--- a/utils.c
+++ b/utils.c
@@ -54,6 +54,8 @@
 #define BLKDISCARD	_IO(0x12,119)
 #endif
 
+int scan_done = 0;
+
 static char argv0_buf[ARGV0_BUF_SIZE] = "btrfs";
 
 void fixup_argv0(char **argv, const char *token)
@@ -1183,7 +1185,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 
 	/* scan other devices */
 	if (is_btrfs && total_devs > 1) {
-		ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			return ret;
 	}
@@ -2093,7 +2095,7 @@ int test_dev_for_mkfs(char *file, int force_overwrite, char *estr)
 	return 0;
 }
 
-int btrfs_scan_lblkid(int update_kernel)
+int btrfs_scan_lblkid()
 {
 	int fd = -1;
 	int ret;
@@ -2104,6 +2106,9 @@ int btrfs_scan_lblkid(int update_kernel)
 	blkid_cache cache = NULL;
 	char path[PATH_MAX];
 
+	if (scan_done)
+		return 0;
+
 	if (blkid_get_cache(&cache, 0) < 0) {
 		printf("ERROR: lblkid cache get failed\n");
 		return 1;
@@ -2132,11 +2137,12 @@ int btrfs_scan_lblkid(int update_kernel)
 		}
 
 		close(fd);
-		if (update_kernel)
-			btrfs_register_one_device(path);
 	}
 	blkid_dev_iterate_end(iter);
 	blkid_put_cache(cache);
+
+	scan_done = 1;
+
 	return 0;
 }
 
diff --git a/utils.h b/utils.h
index 237cf64..ab0c49e 100644
--- a/utils.h
+++ b/utils.h
@@ -126,7 +126,7 @@ int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 			   int verify);
 int ask_user(char *question);
 int lookup_ino_rootid(int fd, u64 *rootid);
-int btrfs_scan_lblkid(int update_kernel);
+int btrfs_scan_lblkid(void);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
 int find_mount_root(const char *path, char **mount_root);
 int get_device_info(int fd, u64 devid,
-- 
2.0.0.153.g79dcccc

[PATCH 2/2 v2] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Anand Jain]

btrfs_scan_lblikd() is called by most the device related command functions.
And btrfs_scan_lblkid() is most expensive function and it becomes more expensive
as number of devices in the system increase. Further some threads call this
function more than once for absolutely no extra benefit and the real waste of
resources.

  btrfs-find-root            1
  btrfs rescue super-recover 2
  btrfs-debug-tree           1
  btrfs-image -r             2
  btrfs check                2
  btrfs restore              2
  calc-size                  NC
  btrfs-corrupt-block        NC
  btrfs-image                NC
  btrfs-map-logical          1
  btrfs-select-super         NC
  btrfstune                  2
  btrfs-zero-log             NC
  tester                     NC
  quick-test.c               NC
  btrfs-convert              0
  mkfs                       #number of devices to be mkfs
  btrfs label set unmounted  2
  btrfs get label unmounted  2

This patch will:
  move out calling register_one_device with in btrfs_scan_lblkid()
  and so function setting the BTRFS_UPDATE_KERNEL to yes will
  call btrfs_register_all_devices() separately.

  introduce a global variable scan_done, which is set when scan is
  done succssfully per thread. So that following calls to this function
  will just return success.

  Further if any function needs to force scan after scan_done is set,
  then it can be done when there is such a requirement, but as of now there
  isn't any such requirement.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: rebase on 3.17. And add more info to commit

 cmds-device.c     |  7 ++++++-
 cmds-filesystem.c |  2 +-
 disk-io.c         |  2 +-
 utils.c           | 14 ++++++++++----
 utils.h           |  2 +-
 5 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 8a33634..519314b 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -28,6 +28,8 @@
 #include "ctree.h"
 #include "ioctl.h"
 #include "utils.h"
+#include "volumes.h"
+
 
 #include "commands.h"
 
@@ -235,9 +237,12 @@ static int cmd_scan_dev(int argc, char **argv)
 
 	if (all || argc == 1) {
 		printf("Scanning for Btrfs filesystems\n");
-		ret = btrfs_scan_lblkid(BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			fprintf(stderr, "ERROR: error %d while scanning\n", ret);
+		ret = btrfs_register_all_devices();
+		if (ret)
+			fprintf(stderr, "ERROR: error %d while registering\n", ret);
 		goto out;
 	}
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 695a3d5..011907d 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -698,7 +698,7 @@ static int cmd_show(int argc, char **argv)
 		goto out;
 
 devs_only:
-	ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+	ret = btrfs_scan_lblkid();
 
 	if (ret) {
 		fprintf(stderr, "ERROR: %d while scanning\n", ret);
diff --git a/disk-io.c b/disk-io.c
index 77fc610..2fb50f9 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1012,7 +1012,7 @@ int btrfs_scan_fs_devices(int fd, const char *path,
 	}
 
 	if (total_devs != 1) {
-		ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			return ret;
 	}
diff --git a/utils.c b/utils.c
index 90e228b..a8691fe 100644
--- a/utils.c
+++ b/utils.c
@@ -52,6 +52,8 @@
 #define BLKDISCARD	_IO(0x12,119)
 #endif
 
+int scan_done = 0;
+
 static char argv0_buf[ARGV0_BUF_SIZE] = "btrfs";
 
 void fixup_argv0(char **argv, const char *token)
@@ -1186,7 +1188,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 
 	/* scan other devices */
 	if (is_btrfs && total_devs > 1) {
-		ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			return ret;
 	}
@@ -2169,7 +2171,7 @@ int test_dev_for_mkfs(char *file, int force_overwrite, char *estr)
 	return 0;
 }
 
-int btrfs_scan_lblkid(int update_kernel)
+int btrfs_scan_lblkid()
 {
 	int fd = -1;
 	int ret;
@@ -2180,6 +2182,9 @@ int btrfs_scan_lblkid(int update_kernel)
 	blkid_cache cache = NULL;
 	char path[PATH_MAX];
 
+	if (scan_done)
+		return 0;
+
 	if (blkid_get_cache(&cache, 0) < 0) {
 		printf("ERROR: lblkid cache get failed\n");
 		return 1;
@@ -2208,11 +2213,12 @@ int btrfs_scan_lblkid(int update_kernel)
 		}
 
 		close(fd);
-		if (update_kernel)
-			btrfs_register_one_device(path);
 	}
 	blkid_dev_iterate_end(iter);
 	blkid_put_cache(cache);
+
+	scan_done = 1;
+
 	return 0;
 }
 
diff --git a/utils.h b/utils.h
index fea26ef..d1af33d 100644
--- a/utils.h
+++ b/utils.h
@@ -127,7 +127,7 @@ int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 			   int verify);
 int ask_user(char *question);
 int lookup_ino_rootid(int fd, u64 *rootid);
-int btrfs_scan_lblkid(int update_kernel);
+int btrfs_scan_lblkid(void);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
 int find_mount_root(const char *path, char **mount_root);
 int get_device_info(int fd, u64 devid,
-- 
2.0.0.153.g79dcccc

Re: [PATCH 2/2 v2] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[David Sterba]

On Thu, Oct 23, 2014 at 02:30:08PM +0800, Anand Jain wrote:
>   introduce a global variable scan_done, which is set when scan is
>   done succssfully per thread. So that following calls to this function
>   will just return success.

The rest is good, I'm a bit concerned about the global variable. It
sholud be at least declared static and not visible outside of utils.c.

The function btrfs_scan_lblkid is called indirectly from various
callchains. For a moment I was thinking about declaring a variable to
hold the scanning status, but btrfs_scan_lblkid is not always called
from a scope of one cmd_something function. Eg. from btrfs_scan_fs_devices
that in turn is called from several other functions. As we're not
concerned about multi-threading, we can use the global variable.

>   Further if any function needs to force scan after scan_done is set,
>   then it can be done when there is such a requirement, but as of now there
>   isn't any such requirement.

Right, then we could introduce something like btrfs_scan_reset_status
and then btrfs_scan_lblkid would work as usual.

> --- a/utils.c
> +++ b/utils.c
> @@ -52,6 +52,8 @@
>  #define BLKDISCARD	_IO(0x12,119)
>  #endif
>  
> +int scan_done = 0;

I'll change it to the following and add to integration.

int btrfs_scan_done = 0;

Re: [PATCH 2/2 v2] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Anand Jain]

Hi David,

  Thanks for the review..


On 30/10/2014 22:18, David Sterba wrote:
> On Thu, Oct 23, 2014 at 02:30:08PM +0800, Anand Jain wrote:
>>    introduce a global variable scan_done, which is set when scan is
>>    done succssfully per thread. So that following calls to this function
>>    will just return success.
>
> The rest is good, I'm a bit concerned about the global variable. It
> sholud be at least declared static and not visible outside of utils.c.

oh yes.

> The function btrfs_scan_lblkid is called indirectly from various
> callchains. For a moment I was thinking about declaring a variable to
> hold the scanning status, but btrfs_scan_lblkid is not always called
> from a scope of one cmd_something function. Eg. from btrfs_scan_fs_devices
> that in turn is called from several other functions. As we're not
> concerned about multi-threading, we can use the global variable.

yes. initially I was thinking the threads which needs the btrfs
device list would do something like init_device_list in the beginning
of the thread. And  the init device_list has to be created in
collaboration with the kernel, progs should take the mounted device
list from the kernel and read the unmounted devices only.
So I had the proposal of either

btrfs: introduce procfs interface for the device list
OR
btrfs: introduce BTRFS_IOC_GET_DEVS

but most suggest sysfs. sysfs is a state-full it does not suite the
purpose. previously we had sysfs bugs when device change its state.
we don't have that issue with profs/ioctl.

anyway for now I think btrfs_scan_done provides a simple workaround
fix as we don't have multi thread with in a process.

the other proposal I was thinking to make any device related operation
only thru the kernel, for example 'btrfs dev scan' will be only command
which will scan the system for the btrfs devices and register them with
the kernel (exclude the maintenance commands like btrfs check for now,
or move them into the kernel in the long run). And all further device
reporting will be from the kernel. With this progs can be very sleek..


>>    Further if any function needs to force scan after scan_done is set,
>>    then it can be done when there is such a requirement, but as of now there
>>    isn't any such requirement.
>
> Right, then we could introduce something like btrfs_scan_reset_status
> and then btrfs_scan_lblkid would work as usual.

Sent out V3 with this changes, Thanks.

>> --- a/utils.c
>> +++ b/utils.c
>> @@ -52,6 +52,8 @@
>>   #define BLKDISCARD	_IO(0x12,119)
>>   #endif
>>
>> +int scan_done = 0;
>
> I'll change it to the following and add to integration.
>
> int btrfs_scan_done = 0;
> --
> 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
>

[PATCH 2/2 v3] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Anand Jain]

btrfs_scan_lblikd() is called by most the device related command functions.
And btrfs_scan_lblkid() is most expensive function and it becomes more expensive
as number of devices in the system increase. Further some threads call this
function more than once for absolutely no extra benefit and the real waste of
resources.

  btrfs-find-root            1
  btrfs rescue super-recover 2
  btrfs-debug-tree           1
  btrfs-image -r             2
  btrfs check                2
  btrfs restore              2
  calc-size                  NC
  btrfs-corrupt-block        NC
  btrfs-image                NC
  btrfs-map-logical          1
  btrfs-select-super         NC
  btrfstune                  2
  btrfs-zero-log             NC
  tester                     NC
  quick-test.c               NC
  btrfs-convert              0
  mkfs                       #number of devices to be mkfs
  btrfs label set unmounted  2
  btrfs get label unmounted  2

This patch will:
  move out calling register_one_device with in btrfs_scan_lblkid()
  and so function setting the BTRFS_UPDATE_KERNEL to yes will
  call btrfs_register_all_devices() separately.

  introduce a global variable scan_done, which is set when scan is
  done succssfully per thread. So that following calls to this function
  will just return success.

  Further if any function needs to force scan after scan_done is set,
  then it can be done when there is such a requirement, but as of now there
  isn't any such requirement.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-device.c     |  7 ++++++-
 cmds-filesystem.c |  2 +-
 disk-io.c         |  2 +-
 utils.c           | 14 ++++++++++----
 utils.h           |  2 +-
 5 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 8a33634..519314b 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -28,6 +28,8 @@
 #include "ctree.h"
 #include "ioctl.h"
 #include "utils.h"
+#include "volumes.h"
+
 
 #include "commands.h"
 
@@ -235,9 +237,12 @@ static int cmd_scan_dev(int argc, char **argv)
 
 	if (all || argc == 1) {
 		printf("Scanning for Btrfs filesystems\n");
-		ret = btrfs_scan_lblkid(BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			fprintf(stderr, "ERROR: error %d while scanning\n", ret);
+		ret = btrfs_register_all_devices();
+		if (ret)
+			fprintf(stderr, "ERROR: error %d while registering\n", ret);
 		goto out;
 	}
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 695a3d5..011907d 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -698,7 +698,7 @@ static int cmd_show(int argc, char **argv)
 		goto out;
 
 devs_only:
-	ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+	ret = btrfs_scan_lblkid();
 
 	if (ret) {
 		fprintf(stderr, "ERROR: %d while scanning\n", ret);
diff --git a/disk-io.c b/disk-io.c
index 77fc610..2fb50f9 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1012,7 +1012,7 @@ int btrfs_scan_fs_devices(int fd, const char *path,
 	}
 
 	if (total_devs != 1) {
-		ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			return ret;
 	}
diff --git a/utils.c b/utils.c
index 90e228b..0689217 100644
--- a/utils.c
+++ b/utils.c
@@ -52,6 +52,8 @@
 #define BLKDISCARD	_IO(0x12,119)
 #endif
 
+static int btrfs_scan_done = 0;
+
 static char argv0_buf[ARGV0_BUF_SIZE] = "btrfs";
 
 void fixup_argv0(char **argv, const char *token)
@@ -1186,7 +1188,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 
 	/* scan other devices */
 	if (is_btrfs && total_devs > 1) {
-		ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			return ret;
 	}
@@ -2169,7 +2171,7 @@ int test_dev_for_mkfs(char *file, int force_overwrite, char *estr)
 	return 0;
 }
 
-int btrfs_scan_lblkid(int update_kernel)
+int btrfs_scan_lblkid()
 {
 	int fd = -1;
 	int ret;
@@ -2180,6 +2182,9 @@ int btrfs_scan_lblkid(int update_kernel)
 	blkid_cache cache = NULL;
 	char path[PATH_MAX];
 
+	if (btrfs_scan_done)
+		return 0;
+
 	if (blkid_get_cache(&cache, 0) < 0) {
 		printf("ERROR: lblkid cache get failed\n");
 		return 1;
@@ -2208,11 +2213,12 @@ int btrfs_scan_lblkid(int update_kernel)
 		}
 
 		close(fd);
-		if (update_kernel)
-			btrfs_register_one_device(path);
 	}
 	blkid_dev_iterate_end(iter);
 	blkid_put_cache(cache);
+
+	btrfs_scan_done = 1;
+
 	return 0;
 }
 
diff --git a/utils.h b/utils.h
index fea26ef..d1af33d 100644
--- a/utils.h
+++ b/utils.h
@@ -127,7 +127,7 @@ int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 			   int verify);
 int ask_user(char *question);
 int lookup_ino_rootid(int fd, u64 *rootid);
-int btrfs_scan_lblkid(int update_kernel);
+int btrfs_scan_lblkid(void);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
 int find_mount_root(const char *path, char **mount_root);
 int get_device_info(int fd, u64 devid,
-- 
2.0.0.153.g79dcccc

Re: [PATCH 1/2] btrfs-progs: introduce btrfs_register_all_device()

[David Sterba]

On Wed, Oct 15, 2014 at 08:51:09AM +0800, Anand Jain wrote:
> --- a/volumes.c
> +++ b/volumes.c
> @@ -30,6 +30,7 @@
>  #include "print-tree.h"
>  #include "volumes.h"
>  #include "math.h"
> +#include "utils.h"
>  
>  struct stripe {
>  	struct btrfs_device *dev;
> @@ -1533,6 +1534,30 @@ btrfs_find_device_by_devid(struct btrfs_fs_devices *fs_devices,
>  	return NULL;
>  }
>  
> +/*
> + * Register all devices in the fs_uuid list created in the user
> + * space. Ensure btrfs_scan_lblkid() is called before this func.
> + */
> +int btrfs_register_all_devices(void)

I think this belongs to utils.c, btrfs_register_one_device is also there
and it is an API funcion for the commands. You can use
btrfs_scanned_uuids() to get to the fs_uuids variable that's locally
defined in volumes.c .

Re: [PATCH 1/2] btrfs-progs: introduce btrfs_register_all_device()

[Anand Jain]

On 30/10/2014 22:33, David Sterba wrote:
> On Wed, Oct 15, 2014 at 08:51:09AM +0800, Anand Jain wrote:
>> --- a/volumes.c
>> +++ b/volumes.c
>> @@ -30,6 +30,7 @@
>>   #include "print-tree.h"
>>   #include "volumes.h"
>>   #include "math.h"
>> +#include "utils.h"
>>
>>   struct stripe {
>>   	struct btrfs_device *dev;
>> @@ -1533,6 +1534,30 @@ btrfs_find_device_by_devid(struct btrfs_fs_devices *fs_devices,
>>   	return NULL;
>>   }
>>
>> +/*
>> + * Register all devices in the fs_uuid list created in the user
>> + * space. Ensure btrfs_scan_lblkid() is called before this func.
>> + */
>> +int btrfs_register_all_devices(void)
>
> I think this belongs to utils.c, btrfs_register_one_device is also there
> and it is an API funcion for the commands. You can use
> btrfs_scanned_uuids() to get to the fs_uuids variable that's locally
> defined in volumes.c .

wonderful comment. Thanks. V4 is sent out.
> --
> 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
>

[PATCH 1/2 v4] btrfs-progs: introduce btrfs_register_all_device()

[Anand Jain]

This function is to register all devices found after scanning
the system. Before we had this functionality with in the
btrfs_scan_lblkid(), however scanning and registering are two
different distinct operation its better keep them separate.
Also we want to optimize btrfs_scan_lblkid and avoid multiple
system scans unless needed. As of now device scan uses this function.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1-v3: nothing changed, match version with its partner patch
   v4: changes per review comments. moved func to utils.c
       tested as below:
        cat /proc/fs/btrfs/devlist | egrep fsid
	btrfs dev scan
	cat /proc/fs/btrfs/devlist | egrep fsid
	[fsid: 2440b569-02d7-45a3-966c-8277f0c43f6a]
	[fsid: 6c5e1019-db9e-4a60-affd-4de88db14b04]
	[fsid: 7ddbe346-f50e-42fc-89c7-a60297772b1f]


 utils.c | 27 +++++++++++++++++++++++++++
 utils.h |  1 +
 2 files changed, 28 insertions(+)

diff --git a/utils.c b/utils.c
index 90e228b..aef0e5e 100644
--- a/utils.c
+++ b/utils.c
@@ -1266,6 +1266,33 @@ int btrfs_register_one_device(char *fname)
 	return ret;
 }
 
+/*
+ * Register all devices in the fs_uuid list created in the user
+ * space. Ensure btrfs_scan_lblkid() is called before this func.
+ */
+int btrfs_register_all_devices(void)
+{
+	int err;
+	struct btrfs_fs_devices *fs_devices;
+	struct btrfs_device *device;
+	struct list_head *all_uuids;
+
+	all_uuids = btrfs_scanned_uuids();
+
+	list_for_each_entry(fs_devices, all_uuids, list) {
+		list_for_each_entry(device, &fs_devices->devices, dev_list) {
+			if (strlen(device->name) != 0) {
+				err = btrfs_register_one_device(device->name);
+				if (err < 0)
+					return err;
+				if (err > 0)
+					return -err;
+			}
+		}
+	}
+	return 0;
+}
+
 int btrfs_device_already_in_root(struct btrfs_root *root, int fd,
 				 int super_offset)
 {
diff --git a/utils.h b/utils.h
index fea26ef..65df2e4 100644
--- a/utils.h
+++ b/utils.h
@@ -82,6 +82,7 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 		      u32 sectorsize);
 int btrfs_scan_for_fsid(int run_ioctls);
 int btrfs_register_one_device(char *fname);
+int btrfs_register_all_devices(void);
 char *canonicalize_dm_name(const char *ptname);
 char *canonicalize_path(const char *path);
 int check_mounted(const char *devicename);
-- 
2.0.0.153.g79dcccc

[PATCH 2/2 v4] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Anand Jain]

btrfs_scan_lblikd() is called by most the device related command functions.
And btrfs_scan_lblkid() is most expensive function and it becomes more expensive
as number of devices in the system increase. Further some threads call this
function more than once for absolutely no extra benefit and the real waste of
resources. Below list of threads and number of times btrfs_scan_lblkid()
is called in that thread.

  btrfs-find-root            1
  btrfs rescue super-recover 2
  btrfs-debug-tree           1
  btrfs-image -r             2
  btrfs check                2
  btrfs restore              2
  calc-size                  NC
  btrfs-corrupt-block        NC
  btrfs-image                NC
  btrfs-map-logical          1
  btrfs-select-super         NC
  btrfstune                  2
  btrfs-zero-log             NC
  tester                     NC
  quick-test.c               NC
  btrfs-convert              0
  mkfs                       #number of devices to be mkfs
  btrfs label set unmounted  2
  btrfs get label unmounted  2

This patch will:
  move out calling register_one_device with in btrfs_scan_lblkid()
  and so function setting the BTRFS_UPDATE_KERNEL to yes will
  call btrfs_register_all_devices() separately.

  introduce a global variable scan_done, which is set when scan is
  done succssfully per thread. So that following calls to this function
  will just return success.

  Further if any function needs to force scan after scan_done is set,
  then it can be done when there is such a requirement, but as of now there
  isn't any such requirement.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 v2: rebase on 3.17. And add more info to commit
 v3: accepts David review comments. thanks.
 v4: now no need to include volumes.h, update commit a bit.

 cmds-device.c     |  6 ++++--
 cmds-filesystem.c |  2 +-
 disk-io.c         |  2 +-
 utils.c           | 14 ++++++++++----
 utils.h           |  2 +-
 5 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 8a33634..23784c5 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -28,7 +28,6 @@
 #include "ctree.h"
 #include "ioctl.h"
 #include "utils.h"
-
 #include "commands.h"
 
 static const char * const device_cmd_group_usage[] = {
@@ -235,9 +234,12 @@ static int cmd_scan_dev(int argc, char **argv)
 
 	if (all || argc == 1) {
 		printf("Scanning for Btrfs filesystems\n");
-		ret = btrfs_scan_lblkid(BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			fprintf(stderr, "ERROR: error %d while scanning\n", ret);
+		ret = btrfs_register_all_devices();
+		if (ret)
+			fprintf(stderr, "ERROR: error %d while registering\n", ret);
 		goto out;
 	}
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 695a3d5..011907d 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -698,7 +698,7 @@ static int cmd_show(int argc, char **argv)
 		goto out;
 
 devs_only:
-	ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+	ret = btrfs_scan_lblkid();
 
 	if (ret) {
 		fprintf(stderr, "ERROR: %d while scanning\n", ret);
diff --git a/disk-io.c b/disk-io.c
index 77fc610..2fb50f9 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1012,7 +1012,7 @@ int btrfs_scan_fs_devices(int fd, const char *path,
 	}
 
 	if (total_devs != 1) {
-		ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			return ret;
 	}
diff --git a/utils.c b/utils.c
index aef0e5e..b83d980 100644
--- a/utils.c
+++ b/utils.c
@@ -52,6 +52,8 @@
 #define BLKDISCARD	_IO(0x12,119)
 #endif
 
+static int btrfs_scan_done = 0;
+
 static char argv0_buf[ARGV0_BUF_SIZE] = "btrfs";
 
 void fixup_argv0(char **argv, const char *token)
@@ -1186,7 +1188,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 
 	/* scan other devices */
 	if (is_btrfs && total_devs > 1) {
-		ret = btrfs_scan_lblkid(!BTRFS_UPDATE_KERNEL);
+		ret = btrfs_scan_lblkid();
 		if (ret)
 			return ret;
 	}
@@ -2196,7 +2198,7 @@ int test_dev_for_mkfs(char *file, int force_overwrite, char *estr)
 	return 0;
 }
 
-int btrfs_scan_lblkid(int update_kernel)
+int btrfs_scan_lblkid()
 {
 	int fd = -1;
 	int ret;
@@ -2207,6 +2209,9 @@ int btrfs_scan_lblkid(int update_kernel)
 	blkid_cache cache = NULL;
 	char path[PATH_MAX];
 
+	if (btrfs_scan_done)
+		return 0;
+
 	if (blkid_get_cache(&cache, 0) < 0) {
 		printf("ERROR: lblkid cache get failed\n");
 		return 1;
@@ -2235,11 +2240,12 @@ int btrfs_scan_lblkid(int update_kernel)
 		}
 
 		close(fd);
-		if (update_kernel)
-			btrfs_register_one_device(path);
 	}
 	blkid_dev_iterate_end(iter);
 	blkid_put_cache(cache);
+
+	btrfs_scan_done = 1;
+
 	return 0;
 }
 
diff --git a/utils.h b/utils.h
index 65df2e4..926ef7d 100644
--- a/utils.h
+++ b/utils.h
@@ -128,7 +128,7 @@ int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 			   int verify);
 int ask_user(char *question);
 int lookup_ino_rootid(int fd, u64 *rootid);
-int btrfs_scan_lblkid(int update_kernel);
+int btrfs_scan_lblkid(void);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
 int find_mount_root(const char *path, char **mount_root);
 int get_device_info(int fd, u64 devid,
-- 
2.0.0.153.g79dcccc

Re: [PATCH 2/2 v4] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Karel Zak]

On Fri, Oct 31, 2014 at 12:11:20PM +0800, Anand Jain wrote:
> btrfs_scan_lblikd() is called by most the device related command functions.
> And btrfs_scan_lblkid() is most expensive function and it becomes more expensive
> as number of devices in the system increase. Further some threads call this

wouldn't be possible to ask udev rather than scan all devices? I
understand than in some cases it's necessary to have robust and
independent solution, but for usual use-cases it would be less
expensive to read the info from udev where we already keep track about
all block devices and where we call libblkid. 

It would be possible to implement it as optional feature (#ifdev HAVE_LIBUDEV),
the library API is very easy to use.

(For example lsblk uses libblkid as fallback, the default is udev).

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2 v4] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Anand Jain]

On 31/10/2014 17:08, Karel Zak wrote:
> On Fri, Oct 31, 2014 at 12:11:20PM +0800, Anand Jain wrote:
>> btrfs_scan_lblikd() is called by most the device related command functions.
>> And btrfs_scan_lblkid() is most expensive function and it becomes more expensive
>> as number of devices in the system increase. Further some threads call this
>
> wouldn't be possible to ask udev rather than scan all devices? I
> understand than in some cases it's necessary to have robust and
> independent solution, but for usual use-cases it would be less
> expensive to read the info from udev where we already keep track about
> all block devices and where we call libblkid.
>
> It would be possible to implement it as optional feature (#ifdev HAVE_LIBUDEV),
> the library API is very easy to use.
>
> (For example lsblk uses libblkid as fallback, the default is udev).
>
>      Karel
>
>

I might be missing something, correct me if wrong. Is there any udev API 
which gives me a list of devices which hold btrfs ? I just browsed
through there isn't any.

Anand

Re: [PATCH 2/2 v4] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Karel Zak]

On Fri, Oct 31, 2014 at 07:04:58PM +0800, Anand Jain wrote:
> 
> 
> 
> 
> On 31/10/2014 17:08, Karel Zak wrote:
> >On Fri, Oct 31, 2014 at 12:11:20PM +0800, Anand Jain wrote:
> >>btrfs_scan_lblikd() is called by most the device related command functions.
> >>And btrfs_scan_lblkid() is most expensive function and it becomes more expensive
> >>as number of devices in the system increase. Further some threads call this
> >
> >wouldn't be possible to ask udev rather than scan all devices? I
> >understand than in some cases it's necessary to have robust and
> >independent solution, but for usual use-cases it would be less
> >expensive to read the info from udev where we already keep track about
> >all block devices and where we call libblkid.
> >
> >It would be possible to implement it as optional feature (#ifdev HAVE_LIBUDEV),
> >the library API is very easy to use.
> >
> >(For example lsblk uses libblkid as fallback, the default is udev).
> >
> >     Karel
> >
> >
> 
> I might be missing something, correct me if wrong. Is there any udev API
> which gives me a list of devices which hold btrfs ? I just browsed
> through there isn't any.

 See  udev_enumerate_* API, nice example:
 http://www.signal11.us/oss/udev/

 Anyway, I have sent patches that implement this feature to btrfs-progs. 

 What I see critical is missing ./configure, because it's pretty ugly
 to add hardcoded dependencies (e.g. libudev), there is also no checks
 for another libs, Makefile does not care about place where libs are
 installed, header files,  etc. etc.

 Is there any fundamental problem with autoconf? If no, then I'm ready
 to send patches with some autotools stuff. Comments?

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2 v4] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[David Sterba]

On Tue, Nov 11, 2014 at 12:47:35PM +0100, Karel Zak wrote:
>  What I see critical is missing ./configure, because it's pretty ugly
>  to add hardcoded dependencies (e.g. libudev), there is also no checks
>  for another libs, Makefile does not care about place where libs are
>  installed, header files,  etc. etc.

It does, prefix and libdir are set conditionally, DESTDIR works.

>  Is there any fundamental problem with autoconf? If no, then I'm ready
>  to send patches with some autotools stuff. Comments?

Yeah the build dependencies checks would be nice, there's no problem
with autoconf.

The Makefile has been manualy crafted and supports some macro magic to
build several binaries from one rule, static targets, quiet/verbose
build. I want to preserve all of this so transition to automake may take
time (or may not happen in the end).

Re: [PATCH 2/2 v4] btrfs-progs: optimize btrfs_scan_lblkid() for multiple calls

[Karel Zak]

On Fri, Nov 14, 2014 at 05:51:27PM +0100, David Sterba wrote:
> On Tue, Nov 11, 2014 at 12:47:35PM +0100, Karel Zak wrote:
> >  What I see critical is missing ./configure, because it's pretty ugly
> >  to add hardcoded dependencies (e.g. libudev), there is also no checks
> >  for another libs, Makefile does not care about place where libs are
> >  installed, header files,  etc. etc.
> 
> It does, prefix and libdir are set conditionally, DESTDIR works.
> 
> >  Is there any fundamental problem with autoconf? If no, then I'm ready
> >  to send patches with some autotools stuff. Comments?
> 
> Yeah the build dependencies checks would be nice, there's no problem
> with autoconf.

 OK, I'll try to prepare something next week.

> The Makefile has been manualy crafted and supports some macro magic to
> build several binaries from one rule, static targets, quiet/verbose
> build. I want to preserve all of this so transition to automake may take
> time (or may not happen in the end).

 Just note, it's fine to expect (require) some build-system features,
 but IMHO it's bad idea to think about build system as about stable
 and always backwardly compatible interface (./configure options,
 Makefile vars, etc).

 For example for util-linux we have changed many many things in last
 (~7) years without negative feedback from downstream maintainers or
 users. IMHO more important is to follow usual conventions than assume
 that my "make FOO=bar" will work forever.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com