Friendly ping ... Is the series of patches for vecs->lock ready to be merged into mainline? Thanks 在 2022/10/17 19:31, Wu Guanghao 写道: > > > 在 2022/9/20 17:20, Wu Guanghao 写道: >> >> >> 在 2022/9/16 2:17, Benjamin Marzinski 写道: >>> On Thu, Sep 15, 2022 at 02:56:36PM +0800, Wu Guanghao wrote: >>>> Sorry for the late feedback. >>>> >>>> The version we are currently testing is 0.8.4, so we only merge the >>>> first 3 patches in this series of patches. Then after the actual test, >>>> it was found that the effect improvement is not very obvious. >>>> >>>> Test environment: 1000 multipath devices, 16 paths per device >>>> Test command: Aggregate multipath devices using multipathd add path >>>> Time consuming (time for 16 paths to aggregate 1 multipath device): >>>> 1. Original: >>>> < 4s: 76.9%; >>>> 4s~10s: 18.4%; >>>> >10s: 4.7%; >>>> 2. After using the patches: >>>> < 4s: 79.1%; >>>> 4s~10s: 18.2%; >>>> >10s: 2.7%; >>>> >>>> >From the results, the time-consuming improvement of the patch is not obvious, >>>> so we made a few changes to the patch and it worked as expected. The modification >>>> of the patch is as follows: >>>> >>>> Paths_checked is changed to configurable, current setting n = 2. >>>> Removed judgment on diff_time. >>>> Sleep change from 10us to 5ms >>> >>> I worry that this is giving too much deference to the uevents. If there >>> is a uevent storm, checking will stop for 5ms every 2 paths checked. I'm >>> worried that this will make it take significantly longer for the the >>> path checker to complete a cycle. Making paths_checked configurable, so >>> that this doesn't trigger too often does help to avoid the issue that >>> checking the time from chk_start_time was dealing with, and makes the >>> mechanics of this simpler. But 5ms seems like a long time to have to >>> wait, just to make sure that another process had the time to grab the >>> vecs lock. Perhaps it would be better to sleep for a shorter length of >>> time, but in a loop, where we can check to see if progress has been >>> made, perhaps by checking some counter of events and user commands >>> serviced. This way, we aren't sleeping too long for no good reason. >>> I agree with this, we are also going to adjust the sleep time, and then >> continue the test. > > We have tested the delay times of 10us, 100us and 1ms, but the results > weren't very good. More than 30% of the luns aggregation time is greater > than 4s. Whether the delay time can also be used as a configurable item > and configured according to the situation. > > The following is the patch content after we have modified checker_loop(). > > Signed-off-by: wuguanghao <wuguanghao3@xxxxxxxxxx> > --- > libmultipath/config.c | 3 ++ > libmultipath/config.h | 3 ++ > libmultipath/defaults.h | 3 ++ > libmultipath/dict.c | 58 +++++++++++++++++++++++++++ > libmultipath/structs.h | 1 + > multipathd/main.c | 87 +++++++++++++++++++++++++++++++++-------- > 6 files changed, 138 insertions(+), 17 deletions(-) > > diff --git a/libmultipath/config.c b/libmultipath/config.c > index 7d0d711..7658626 100644 > --- a/libmultipath/config.c > +++ b/libmultipath/config.c > @@ -752,6 +752,9 @@ load_config (char * file) > conf->remove_retries = 0; > conf->ghost_delay = DEFAULT_GHOST_DELAY; > conf->all_tg_pt = DEFAULT_ALL_TG_PT; > + conf->check_delay_time = DEFAULT_CHECK_DELAY_TIME; > + conf->check_path_num_per_splice = DEFAULT_CHECK_PATH_NUM_PER_SPLICE; > + > /* > * preload default hwtable > */ > diff --git a/libmultipath/config.h b/libmultipath/config.h > index ceecff2..a26dd99 100644 > --- a/libmultipath/config.h > +++ b/libmultipath/config.h > @@ -229,6 +229,9 @@ struct config { > vector elist_property; > vector elist_protocol; > char *enable_foreign; > + > + int check_delay_time; > + int check_path_num_per_splice; > }; > > extern struct udev * udev; > diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h > index 52fe05b..b3fb1bc 100644 > --- a/libmultipath/defaults.h > +++ b/libmultipath/defaults.h > @@ -50,6 +50,9 @@ > #define DEFAULT_FIND_MULTIPATHS_TIMEOUT -10 > #define DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT 1 > #define DEFAULT_ALL_TG_PT ALL_TG_PT_OFF > +#define DEFAULT_CHECK_DELAY_TIME 10 > +#define DEFAULT_CHECK_PATH_NUM_PER_SPLICE 1 > + > /* Enable all foreign libraries by default */ > #define DEFAULT_ENABLE_FOREIGN "" > > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > index 2c8ea10..2262caa 100644 > --- a/libmultipath/dict.c > +++ b/libmultipath/dict.c > @@ -1407,6 +1407,46 @@ def_uxsock_timeout_handler(struct config *conf, vector strvec) > free(buff); > return 0; > } > +static int > +def_check_delay_time_handler(struct config *conf, vector strvec) > +{ > + int local_check_delay_time; > + char *buff; > + > + buff = set_value(strvec); > + if (!buff) > + return 1; > + > + if (sscanf(buff, "%u", &local_check_delay_time) == 1 && > + local_check_delay_time > DEFAULT_CHECK_DELAY_TIME) > + conf->check_delay_time = local_check_delay_time; > + else > + conf->check_delay_time = DEFAULT_CHECK_DELAY_TIME; > + > + free(buff); > + return 0; > +} > + > +static int > +def_check_path_num_per_splice_handler(struct config *conf, vector strvec) > +{ > + int local_check_path_num_per_splice; > + char *buff; > + > + buff = set_value(strvec); > + if (!buff) > + return 1; > + > + if (sscanf(buff, "%u", &local_check_path_num_per_splice) == 1 && > + local_check_path_num_per_splice > DEFAULT_CHECK_PATH_NUM_PER_SPLICE) > + conf->check_path_num_per_splice = local_check_path_num_per_splice; > + else > + conf->check_path_num_per_splice = DEFAULT_CHECK_PATH_NUM_PER_SPLICE; > + > + free(buff); > + return 0; > +} > + > > static int > hw_vpd_vendor_handler(struct config *conf, vector strvec) > @@ -1546,6 +1586,21 @@ snprint_def_uxsock_timeout(struct config *conf, char * buff, int len, > return snprintf(buff, len, "%u", conf->uxsock_timeout); > } > > +static int > +snprint_def_check_delay_time(struct config *conf, char * buff, int len, > + const void *data) > +{ > + return snprintf(buff, len, "%u", conf->check_delay_time); > +} > + > +static int > +snprint_def_check_path_num_per_splice(struct config *conf, char * buff, int len, > + const void *data) > +{ > + return snprintf(buff, len, "%u", conf->check_path_num_per_splice); > +} > + > + > static int > snprint_ble_simple (struct config *conf, char * buff, int len, > const void * data) > @@ -1804,6 +1859,9 @@ init_keywords(vector keywords) > install_keyword("enable_foreign", &def_enable_foreign_handler, > &snprint_def_enable_foreign); > install_keyword("marginal_pathgroups", &def_marginal_pathgroups_handler, &snprint_def_marginal_pathgroups); > + install_keyword("check_delay_time", &def_check_delay_time_handler, &snprint_def_check_delay_time); > + install_keyword("check_path_num_per_splice", &def_check_path_num_per_splice_handler, &snprint_def_check_path_num_per_splice); > + > __deprecated install_keyword("default_selector", &def_selector_handler, NULL); > __deprecated install_keyword("default_path_grouping_policy", &def_pgpolicy_handler, NULL); > __deprecated install_keyword("default_uid_attribute", &def_uid_attribute_handler, NULL); > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > index 11e516a..391de96 100644 > --- a/libmultipath/structs.h > +++ b/libmultipath/structs.h > @@ -288,6 +288,7 @@ struct path { > int find_multipaths_timeout; > int marginal; > int vpd_vendor_id; > + bool is_checked; > /* configlet pointers */ > vector hwe; > struct gen_path generic_path; > diff --git a/multipathd/main.c b/multipathd/main.c > index 5035d5b..865b7b5 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2388,6 +2388,11 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) > } > return 1; > } > +enum checker_state { > + CHECKER_STARTING, > + CHECKER_RUNNING, > + CHECKER_FINISHED, > +}; > > static void * > checkerloop (void *ap) > @@ -2395,11 +2400,11 @@ checkerloop (void *ap) > struct vectors *vecs; > struct path *pp; > int count = 0; > - unsigned int i; > struct timespec last_time; > struct config *conf; > int foreign_tick = 0; > bool use_watchdog; > + int check_delay_time, check_path_num_per_splice; > > pthread_cleanup_push(rcu_unregister, NULL); > rcu_register_thread(); > @@ -2414,12 +2419,15 @@ checkerloop (void *ap) > /* use_watchdog is set from process environment and never changes */ > conf = get_multipath_config(); > use_watchdog = conf->use_watchdog; > + check_delay_time = conf->check_delay_time; > + check_path_num_per_splice = conf->check_path_num_per_splice; > put_multipath_config(conf); > > while (1) { > struct timespec diff_time, start_time, end_time; > - int num_paths = 0, strict_timing, rc = 0; > + int num_paths = 0, strict_timing, rc = 0, i = 0; > unsigned int ticks = 0; > + enum checker_state checker_state = CHECKER_STARTING; > > get_monotonic_time(&start_time); > if (start_time.tv_sec && last_time.tv_sec) { > @@ -2443,21 +2451,66 @@ checkerloop (void *ap) > } else if (rc == EINVAL) > /* daemon shutdown */ > break; > - > - pthread_cleanup_push(cleanup_lock, &vecs->lock); > - lock(&vecs->lock); > - pthread_testcancel(); > - vector_foreach_slot (vecs->pathvec, pp, i) { > - rc = check_path(vecs, pp, ticks); > - if (rc < 0) { > - vector_del_slot(vecs->pathvec, i); > - free_path(pp); > - i--; > - } else > - num_paths += rc; > - } > - lock_cleanup_pop(vecs->lock); > - > + condlog(4, "check_delay_time is %u, check_path_num_per_splice is %u", check_delay_time, check_path_num_per_splice); > + > + while (checker_state != CHECKER_FINISHED) { > + unsigned int paths_checked = 0; > + struct timespec chk_start_time; > + pthread_cleanup_push(cleanup_lock, &vecs->lock); > + lock(&vecs->lock); > + pthread_testcancel(); > + get_monotonic_time(&chk_start_time); > + if (checker_state == CHECKER_STARTING) { > + vector_foreach_slot(vecs->pathvec, pp, i) > + pp->is_checked = false; > + i = 0; > + checker_state = CHECKER_RUNNING; > + } else { > + /* > + * Paths could have been removed since we > + * dropped the lock. Find the path to continue > + * checking at. Since paths can be removed from > + * anywhere in the vector, but can only be added > + * at the end, the last checked path must be > + * between its old location, and the start or > + * the vector. > + */ > + if (i >= VECTOR_SIZE(vecs->pathvec)) > + i = VECTOR_SIZE(vecs->pathvec) - 1; > + while ((pp = VECTOR_SLOT(vecs->pathvec, i))) { > + if (pp->is_checked == true) > + break; > + i--; > + } > + i++; > + } > + vector_foreach_slot_after (vecs->pathvec, pp, i) { > + pp->is_checked = true; > + rc = check_path(vecs, pp, ticks); > + if (rc < 0) { > + condlog(1, "%s: check_path() failed, removing", > + pp->dev); > + vector_del_slot(vecs->pathvec, i); > + free_path(pp); > + i--; > + } else > + num_paths += rc; > + if (++paths_checked % check_path_num_per_splice == 0 && > + lock_has_waiters(&vecs->lock)) { > + get_monotonic_time(&end_time); > + timespecsub(&end_time, &chk_start_time, > + &diff_time); > + goto unlock; > + } > + } > + checker_state = CHECKER_FINISHED; > +unlock: > + lock_cleanup_pop(vecs->lock); > + if (checker_state != CHECKER_FINISHED) { > + /* Yield to waiters */ > + usleep(check_delay_time); > + } > + } > pthread_cleanup_push(cleanup_lock, &vecs->lock); > lock(&vecs->lock); > pthread_testcancel(); > -- dm-devel mailing list dm-devel@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel