| Message ID | 1414045808-6930-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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; -- 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
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 > -- 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-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,
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(-)