| Message ID | 1414728680-25468-2-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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
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 -- 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, 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
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). -- 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, 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
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,
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(-)