Re: [PATCH V2 0/6] allowing path checking to be interrupted.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




在 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();
-- 
2.27.0

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux