Merge pull request #124 from versity/zab/fix_quo_hb_mount_option

Zab/fix quo hb mount option

kmod/src/options.c

@@ -131,10 +131,8 @@ static void init_default_options(struct scoutfs_mount_options *opts)
 	opts->quorum_slot_nr = -1;
 }
 
-static int set_quorum_heartbeat_timeout_ms(struct super_block *sb, int ret, u64 val)
+static int verify_quorum_heartbeat_timeout_ms(struct super_block *sb, int ret, u64 val)
 {
-	DECLARE_OPTIONS_INFO(sb, optinf);
-
 	if (ret < 0) {
 		scoutfs_err(sb, "failed to parse quorum_heartbeat_timeout_ms value");
 		return -EINVAL;
@@ -145,10 +143,6 @@ static int set_quorum_heartbeat_timeout_ms(struct super_block *sb, int ret, u64
 		return -EINVAL;
 	}
 
-	write_seqlock(&optinf->seqlock);
-	optinf->opts.quorum_heartbeat_timeout_ms = val;
-	write_sequnlock(&optinf->seqlock);
-
 	return 0;
 }
 
@@ -232,9 +226,10 @@ static int parse_options(struct super_block *sb, char *options, struct scoutfs_m
 
 		case Opt_quorum_heartbeat_timeout_ms:
 			ret = match_u64(args, &nr64);
-			ret = set_quorum_heartbeat_timeout_ms(sb, ret, nr64);
+			ret = verify_quorum_heartbeat_timeout_ms(sb, ret, nr64);
 			if (ret < 0)
 				return ret;
+			opts->quorum_heartbeat_timeout_ms = nr64;
 			break;
 
 		case Opt_quorum_slot_nr:
@@ -493,6 +488,7 @@ static ssize_t quorum_heartbeat_timeout_ms_store(struct kobject *kobj, struct ko
 						 const char *buf, size_t count)
 {
 	struct super_block *sb = SCOUTFS_SYSFS_ATTRS_SB(kobj);
+	DECLARE_OPTIONS_INFO(sb, optinf);
 	char nullterm[30]; /* more than enough for octal -U64_MAX */
 	u64 val;
 	int len;
@@ -503,9 +499,13 @@ static ssize_t quorum_heartbeat_timeout_ms_store(struct kobject *kobj, struct ko
 	nullterm[len] = '\0';
 
 	ret = kstrtoll(nullterm, 0, &val);
-	ret = set_quorum_heartbeat_timeout_ms(sb, ret, val);
-	if (ret == 0)
+	ret = verify_quorum_heartbeat_timeout_ms(sb, ret, val);
+	if (ret == 0) {
+		write_seqlock(&optinf->seqlock);
+		optinf->opts.quorum_heartbeat_timeout_ms = val;
+		write_sequnlock(&optinf->seqlock);
 		ret = count;
+	}
 
 	return ret;
 }

tests/funcs/fs.sh

@@ -153,7 +153,27 @@ t_mount()
 	test "$nr" -lt "$T_NR_MOUNTS" || \
 		t_fail "fs nr $nr invalid"
 
-	eval t_quiet mount -t scoutfs \$T_O$nr \$T_DB$nr \$T_M$nr
+	eval t_quiet mount -t scoutfs \$T_O$nr\$opt \$T_DB$nr \$T_M$nr
+}
+
+#
+# Mount with an optional mount option string.  If the string is empty
+# then the saved mount options are used.  If the string has contents
+# then it is appended to the end of the saved options with a separating
+# comma.
+#
+# Unlike t_mount this won't inherently fail in t_quiet, errors are
+# returned so bad options can be tested.
+#
+t_mount_opt()
+{
+	local nr="$1"
+	local opt="${2:+,$2}"
+
+	test "$nr" -lt "$T_NR_MOUNTS" || \
+		t_fail "fs nr $nr invalid"
+
+	eval mount -t scoutfs \$T_O$nr\$opt \$T_DB$nr \$T_M$nr
 }
 
 t_umount()

tests/golden/quorum-heartbeat-timeout

@@ -1,2 +1,5 @@
 == bad timeout values fail
-== test different timeouts
+== bad mount option fails
+== mount option
+== sysfs
+== reset all options

tests/tests/quorum-heartbeat-timeout.sh

@@ -17,43 +17,52 @@ set_bad_timeout() {
 		t_fail "set bad q hb to $to"
 }
 
-set_quorum_timeouts()
+set_timeout()
 {
-	local to="$1"
-	local was
+	local nr="$1"
+	local how="$2"
+	local to="$3"
 	local is
 
-	for nr in $(t_quorum_nrs); do
-		local mnt="$(eval echo \$T_M$nr)"
-
-		was=$(t_get_sysfs_mount_option $nr quorum_heartbeat_timeout_ms)
+	if [ $how == "sysfs" ]; then
 		t_set_sysfs_mount_option $nr quorum_heartbeat_timeout_ms $to
-		is=$(t_get_sysfs_mount_option $nr quorum_heartbeat_timeout_ms)
+	fi
+	if [ $how == "mount" ]; then
+		t_umount $nr
+		t_mount_opt $nr "quorum_heartbeat_timeout_ms=$to"
+	fi
 
-		if [ "$is" != "$to" ]; then
-			t_fail "tried to set qhbto on $nr to $to but got $is"
-		fi
-	done
+	is=$(t_get_sysfs_mount_option $nr quorum_heartbeat_timeout_ms)
+
+	if [ "$is" != "$to" ]; then
+		t_fail "tried to set qhbto on $nr via $how to $to but got $is"
+	fi
 }
 
 test_timeout()
 {
-	local to="$1"
-	local orig_to
+	local how="$1"
+	local to="$2"
 	local start
 	local nr
+	local sv
 	local delay
+	local low
+	local high
 
-	# set new timeouts, saving original
-	orig_to=$(t_get_sysfs_mount_option 0 quorum_heartbeat_timeout_ms)
-	set_quorum_timeouts $to
+	# set timeout on non-server quorum mounts
+	sv=$(t_server_nr)
+	for nr in $(t_quorum_nrs); do
+		if [ $nr -ne $sv ]; then
+			set_timeout $nr $how $to
+		fi
+	done
 
 	# give followers time to recv heartbeats and reset timeouts
 	sleep 1
 
 	# tear down the current server/leader
-	nr=$(t_server_nr)
-	t_force_umount $nr
+	t_force_umount $sv
 
 	# see how long it takes for the next leader to start
 	start=$(time_ms)
@@ -64,15 +73,15 @@ test_timeout()
 	echo "to $to delay $delay" >> $T_TMP.delay
 
 	# restore the mount that we tore down
-	t_mount $nr
+	t_mount $sv
 
-	# reset the original timeouts
-	set_quorum_timeouts $orig_to
+	# make sure the new leader delay was reasonable, allowing for some slack
+	low=$((to - 1000))
+	high=$((to + 3000))
 
 	# make sure the new leader delay was reasonable
-	test "$delay" -gt "$to" || t_fail "delay $delay < to $to"
-	# allow 5 seconds of slop
-	test "$delay" -lt $(($to + 5000)) || t_fail "delay $delay > to $to + 5sec"
+	test "$delay" -lt "$low" && t_fail "delay $delay < low $low (to $to)"
+	test "$delay" -gt "$high" && t_fail "delay $delay > high $high (to $to)"
 }
 
 echo "== bad timeout values fail"
@@ -80,10 +89,29 @@ set_bad_timeout 0
 set_bad_timeout -1
 set_bad_timeout 1000000
 
-echo "== test different timeouts"
+echo "== bad mount option fails"
+if [ "$(t_server_nr)" == 0 ]; then
+	nr=1
+else
+	nr=0
+fi
+t_umount $nr
+t_mount_opt $nr "quorum_heartbeat_timeout_ms=1000000" 2>/dev/null && \
+	t_fail "bad mount option succeeded"
+t_mount $nr
+
+echo "== mount option"
 def=$(t_get_sysfs_mount_option 0 quorum_heartbeat_timeout_ms)
-test_timeout $def
-test_timeout 3000
-test_timeout $((def + 19000))
+test_timeout mount $def
+test_timeout mount 3000
+test_timeout mount $((def + 19000))
+
+echo "== sysfs"
+test_timeout sysfs $def
+test_timeout sysfs 3000
+test_timeout sysfs $((def + 19000))
+
+echo "== reset all options"
+t_remount_all
 
 t_pass