[PATCH V2 blktests 0/2] blktests: Add ublk testcases

[PATCH V2 blktests 0/2] blktests: Add ublk testcases

[Ziyang Zhang]

Hi,

ublk can passthrough I/O requests to userspce daemons. It is very important
to test ublk crash handling since the userspace part is not reliable.
Especially we should test removing device, killing ublk daemons and user
recovery feature.

The first patch add user recovery support in miniublk.

The second patch add five new tests for ublk to cover above cases.

V2:
- Check parameters in recovery
- Add one small delay before deleting device
- Write informative description

Ziyang Zhang (2):
  src/miniublk: add user recovery
  tests: Add ublk tests

 common/ublk        |  10 +-
 src/miniublk.c     | 269 ++++++++++++++++++++++++++++++++++++++++++---
 tests/ublk/001     |  48 ++++++++
 tests/ublk/001.out |   2 +
 tests/ublk/002     |  63 +++++++++++
 tests/ublk/002.out |   2 +
 tests/ublk/003     |  48 ++++++++
 tests/ublk/003.out |   2 +
 tests/ublk/004     |  50 +++++++++
 tests/ublk/004.out |   2 +
 tests/ublk/005     |  79 +++++++++++++
 tests/ublk/005.out |   2 +
 tests/ublk/006     |  83 ++++++++++++++
 tests/ublk/006.out |   2 +
 tests/ublk/rc      |  15 +++
 15 files changed, 661 insertions(+), 16 deletions(-)
 create mode 100755 tests/ublk/001
 create mode 100644 tests/ublk/001.out
 create mode 100755 tests/ublk/002
 create mode 100644 tests/ublk/002.out
 create mode 100755 tests/ublk/003
 create mode 100644 tests/ublk/003.out
 create mode 100755 tests/ublk/004
 create mode 100644 tests/ublk/004.out
 create mode 100755 tests/ublk/005
 create mode 100644 tests/ublk/005.out
 create mode 100755 tests/ublk/006
 create mode 100644 tests/ublk/006.out
 create mode 100644 tests/ublk/rc

-- 
2.31.1

[PATCH V2 blktests 1/2] src/miniublk: add user recovery

[Ziyang Zhang]

We are going to test ublk's user recovery feature so add support in
miniublk.

Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
---
 src/miniublk.c | 269 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 254 insertions(+), 15 deletions(-)

diff --git a/src/miniublk.c b/src/miniublk.c
index fe10291..a3d6fce 100644
--- a/src/miniublk.c
+++ b/src/miniublk.c
@@ -17,6 +17,8 @@
 #include <pthread.h>
 #include <getopt.h>
 #include <limits.h>
+#include <string.h>
+#include <errno.h>
 #include <sys/syscall.h>
 #include <sys/mman.h>
 #include <sys/ioctl.h>
@@ -74,6 +76,7 @@ struct ublk_tgt_ops {
 	int (*queue_io)(struct ublk_queue *, int tag);
 	void (*tgt_io_done)(struct ublk_queue *,
 			int tag, const struct io_uring_cqe *);
+	int (*recover_tgt)(struct ublk_dev *);
 };
 
 struct ublk_tgt {
@@ -372,6 +375,29 @@ static int ublk_ctrl_get_params(struct ublk_dev *dev,
 	return __ublk_ctrl_cmd(dev, &data);
 }
 
+static int ublk_ctrl_start_user_recover(struct ublk_dev *dev)
+{
+	struct ublk_ctrl_cmd_data data = {
+		.cmd_op	= UBLK_CMD_START_USER_RECOVERY,
+		.flags	= 0,
+	};
+
+	return __ublk_ctrl_cmd(dev, &data);
+}
+
+static int ublk_ctrl_end_user_recover(struct ublk_dev *dev,
+		int daemon_pid)
+{
+	struct ublk_ctrl_cmd_data data = {
+		.cmd_op	= UBLK_CMD_END_USER_RECOVERY,
+		.flags	= CTRL_CMD_HAS_DATA,
+	};
+
+	dev->dev_info.ublksrv_pid = data.data[0] = daemon_pid;
+
+	return __ublk_ctrl_cmd(dev, &data);
+}
+
 static const char *ublk_dev_state_desc(struct ublk_dev *dev)
 {
 	switch (dev->dev_info.state) {
@@ -379,6 +405,8 @@ static const char *ublk_dev_state_desc(struct ublk_dev *dev)
 		return "DEAD";
 	case UBLK_S_DEV_LIVE:
 		return "LIVE";
+	case UBLK_S_DEV_QUIESCED:
+		return "QUIESCED";
 	default:
 		return "UNKNOWN";
 	};
@@ -550,9 +578,12 @@ static int ublk_dev_prep(struct ublk_dev *dev)
 		goto fail;
 	}
 
-	if (dev->tgt.ops->init_tgt)
+	if (dev->dev_info.state != UBLK_S_DEV_QUIESCED && dev->tgt.ops->init_tgt)
 		ret = dev->tgt.ops->init_tgt(dev);
 
+	else if (dev->dev_info.state == UBLK_S_DEV_QUIESCED && dev->tgt.ops->recover_tgt)
+		ret = dev->tgt.ops->recover_tgt(dev);
+
 	return ret;
 fail:
 	close(dev->fds[0]);
@@ -831,7 +862,7 @@ static void ublk_set_parameters(struct ublk_dev *dev)
 				dev->dev_info.dev_id, ret);
 }
 
-static int ublk_start_daemon(struct ublk_dev *dev)
+static int ublk_start_daemon(struct ublk_dev *dev, bool recovery)
 {
 	int ret, i;
 	void *thread_ret;
@@ -853,12 +884,22 @@ static int ublk_start_daemon(struct ublk_dev *dev)
 				&dev->q[i]);
 	}
 
-	ublk_set_parameters(dev);
 
 	/* everything is fine now, start us */
-	ret = ublk_ctrl_start_dev(dev, getpid());
-	if (ret < 0)
-		goto fail;
+	if (recovery) {
+		ret = ublk_ctrl_end_user_recover(dev, getpid());
+		if (ret < 0) {
+			ublk_err("%s: ublk_ctrl_end_user_recover failed: %d\n", __func__, ret);
+			goto fail;
+		}
+	} else {
+		ublk_set_parameters(dev);
+		ret = ublk_ctrl_start_dev(dev, getpid());
+		if (ret < 0) {
+			ublk_err("%s: ublk_ctrl_start_dev failed: %d\n", __func__, ret);
+			goto fail;
+		}
+	}
 
 	ublk_ctrl_get_info(dev);
 	ublk_ctrl_dump(dev, true);
@@ -880,6 +921,7 @@ static int cmd_dev_add(int argc, char *argv[])
 		{ "number",		1,	NULL, 'n' },
 		{ "queues",		1,	NULL, 'q' },
 		{ "depth",		1,	NULL, 'd' },
+		{ "recovery",		0,	NULL, 'r' },
 		{ "debug_mask",	1,	NULL, 0},
 		{ "quiet",	0,	NULL, 0},
 		{ NULL }
@@ -891,8 +933,9 @@ static int cmd_dev_add(int argc, char *argv[])
 	const char *tgt_type = NULL;
 	int dev_id = -1;
 	unsigned nr_queues = 2, depth = UBLK_QUEUE_DEPTH;
+	int user_recovery = 0;
 
-	while ((opt = getopt_long(argc, argv, "-:t:n:d:q:",
+	while ((opt = getopt_long(argc, argv, "-:t:n:d:q:r",
 				  longopts, &option_idx)) != -1) {
 		switch (opt) {
 		case 'n':
@@ -907,6 +950,9 @@ static int cmd_dev_add(int argc, char *argv[])
 		case 'd':
 			depth = strtol(optarg, NULL, 10);
 			break;
+		case 'r':
+			user_recovery = 1;
+			break;
 		case 0:
 			if (!strcmp(longopts[option_idx].name, "debug_mask"))
 				ublk_dbg_mask = strtol(optarg, NULL, 16);
@@ -942,6 +988,8 @@ static int cmd_dev_add(int argc, char *argv[])
 	info->dev_id = dev_id;
         info->nr_hw_queues = nr_queues;
         info->queue_depth = depth;
+	if (user_recovery)
+		info->flags |= UBLK_F_USER_RECOVERY;
 	dev->tgt.ops = ops;
 	dev->tgt.argc = argc;
 	dev->tgt.argv = argv;
@@ -953,7 +1001,95 @@ static int cmd_dev_add(int argc, char *argv[])
 		goto fail;
 	}
 
-	ret = ublk_start_daemon(dev);
+	ret = ublk_start_daemon(dev, false);
+	if (ret < 0) {
+		ublk_err("%s: can't start daemon id %d, type %s\n",
+				__func__, dev_id, tgt_type);
+		goto fail_del;
+	}
+
+fail_del:
+	ublk_ctrl_del_dev(dev);
+fail:
+	ublk_ctrl_deinit(dev);
+	return ret;
+}
+
+static int cmd_dev_recover(int argc, char *argv[])
+{
+	static const struct option longopts[] = {
+		{ "type",		1,	NULL, 't' },
+		{ "number",		1,	NULL, 'n' },
+		{ "debug_mask",	1,	NULL, 0},
+		{ "quiet",	0,	NULL, 0},
+		{ NULL }
+	};
+	const struct ublk_tgt_ops *ops;
+	struct ublksrv_ctrl_dev_info *info;
+	struct ublk_dev *dev;
+	int ret, option_idx, opt;
+	const char *tgt_type = NULL;
+	int dev_id = -1;
+
+	while ((opt = getopt_long(argc, argv, "-:t:n:d:q:",
+				  longopts, &option_idx)) != -1) {
+		switch (opt) {
+		case 'n':
+			dev_id = strtol(optarg, NULL, 10);
+			break;
+		case 't':
+			tgt_type = optarg;
+			break;
+		case 0:
+			if (!strcmp(longopts[option_idx].name, "debug_mask"))
+				ublk_dbg_mask = strtol(optarg, NULL, 16);
+			if (!strcmp(longopts[option_idx].name, "quiet"))
+				ublk_dbg_mask = 0;
+			break;
+		}
+	}
+
+	optind = 0;
+
+	ops = ublk_find_tgt(tgt_type);
+	if (!ops) {
+		ublk_err("%s: no such tgt type, type %s\n",
+				__func__, tgt_type);
+		return -ENODEV;
+	}
+
+	dev = ublk_ctrl_init();
+	if (!dev) {
+		ublk_err("%s: can't alloc dev id %d, type %s\n",
+				__func__, dev_id, tgt_type);
+		return -ENOMEM;
+	}
+
+	info = &dev->dev_info;
+	info->dev_id = dev_id;
+	ret = ublk_ctrl_get_info(dev);
+	if (ret < 0) {
+		ublk_err("%s: can't get dev info from %d\n", __func__, dev_id);
+		goto fail;
+	}
+
+	ret = ublk_ctrl_get_params(dev, &dev->tgt.params);
+	if (ret) {
+		ublk_err("dev %d set basic parameter failed %d\n",
+				dev->dev_info.dev_id, ret);
+		goto fail;
+	}
+
+	dev->tgt.ops = ops;
+	dev->tgt.argc = argc;
+	dev->tgt.argv = argv;
+	ret = ublk_ctrl_start_user_recover(dev);
+	if (ret < 0) {
+		ublk_err("%s: can't start recovery for %d\n", __func__, dev_id);
+		goto fail;
+	}
+
+	ret = ublk_start_daemon(dev, true);
 	if (ret < 0) {
 		ublk_err("%s: can't start daemon id %d, type %s\n",
 				__func__, dev_id, tgt_type);
@@ -1125,7 +1261,9 @@ static int cmd_dev_help(int argc, char *argv[])
 	printf("\t -a delete all devices -n delete specified device\n");
 	printf("%s list [-n dev_id] -a \n", argv[0]);
 	printf("\t -a list all devices, -n list specified device, default -a \n");
-
+	printf("%s recover -t {null|loop} [-n dev_id] \n", argv[0]);
+	printf("\t -t loop -f backing_file \n");
+	printf("\t -t null\n");
 	return 0;
 }
 
@@ -1150,6 +1288,12 @@ static int ublk_null_tgt_init(struct ublk_dev *dev)
 	return 0;
 }
 
+static int ublk_null_tgt_recover(struct ublk_dev *dev)
+{
+	dev->tgt.dev_size = dev->tgt.params.basic.dev_sectors << 9;
+	return 0;
+}
+
 static int ublk_null_queue_io(struct ublk_queue *q, int tag)
 {
 	const struct ublksrv_io_desc *iod = ublk_get_iod(q, tag);
@@ -1264,15 +1408,17 @@ static int ublk_loop_tgt_init(struct ublk_dev *dev)
 		}
 	}
 
-	ublk_dbg(UBLK_DBG_DEV, "%s: file %s\n", __func__, file);
-
-	if (!file)
+	if (!file) {
+		ublk_err( "%s: backing file is unset!\n", __func__);
 		return -EINVAL;
+	}
+
+	ublk_dbg(UBLK_DBG_DEV, "%s: file %s\n", __func__, file);
 
 	fd = open(file, O_RDWR);
 	if (fd < 0) {
-		ublk_err( "%s: backing file %s can't be opened\n",
-				__func__, file);
+		ublk_err("%s: backing file %s can't be opened: %s\n",
+				__func__, file, strerror(errno));
 		return -EBADF;
 	}
 
@@ -1301,7 +1447,8 @@ static int ublk_loop_tgt_init(struct ublk_dev *dev)
 	if (fcntl(fd, F_SETFL, O_DIRECT)) {
 		p.basic.logical_bs_shift = 9;
 		p.basic.physical_bs_shift = 12;
-		ublk_log("%s: ublk-loop fallback to buffered IO\n", __func__);
+		ublk_log("%s: %s, ublk-loop fallback to buffered IO\n",
+				__func__, strerror(errno));
 	}
 
 	dev->tgt.dev_size = bytes;
@@ -1313,11 +1460,100 @@ static int ublk_loop_tgt_init(struct ublk_dev *dev)
 	return 0;
 }
 
+static int ublk_loop_tgt_recover(struct ublk_dev *dev)
+{
+	static const struct option lo_longopts[] = {
+		{ "file",		1,	NULL, 'f' },
+		{ NULL }
+	};
+	unsigned long long bytes;
+	char **argv = dev->tgt.argv;
+	int argc = dev->tgt.argc;
+	struct ublk_params p = dev->tgt.params;
+	char *file = NULL;
+	int fd, opt;
+	unsigned int bs = 1 << 9, pbs = 1 << 12;
+	struct stat st;
+
+	while ((opt = getopt_long(argc, argv, "-:f:",
+				  lo_longopts, NULL)) != -1) {
+		switch (opt) {
+		case 'f':
+			file = strdup(optarg);
+			break;
+		}
+	}
+
+	if (!file) {
+		ublk_err( "%s: backing file is unset!\n", __func__);
+		return -EINVAL;
+	}
+
+	ublk_dbg(UBLK_DBG_DEV, "%s: file %s\n", __func__, file);
+
+	fd = open(file, O_RDWR);
+	if (fd < 0) {
+		ublk_err( "%s: backing file %s can't be opened: %s\n",
+				__func__, file, strerror(errno));
+		return -EBADF;
+	}
+
+	if (fstat(fd, &st) < 0) {
+		close(fd);
+		return -EBADF;
+	}
+
+	if (S_ISBLK(st.st_mode)) {
+		if (ioctl(fd, BLKGETSIZE64, &bytes) != 0)
+			return -EBADF;
+		if (ioctl(fd, BLKSSZGET, &bs) != 0)
+			return -1;
+		if (ioctl(fd, BLKPBSZGET, &pbs) != 0)
+			return -1;			
+	} else if (S_ISREG(st.st_mode)) {
+		bytes = st.st_size;
+	} else {
+		bytes = 0;
+	}
+
+	if (fcntl(fd, F_SETFL, O_DIRECT)) {
+		/* buffered I/O */
+		ublk_log("%s: %s, ublk-loop fallback to buffered IO\n",
+				__func__, strerror(errno));
+	}
+	else {
+		/* direct I/O */
+		if (p.basic.logical_bs_shift != ilog2(bs)) {
+			ublk_err("%s: logical block size should be %d, I got %d\n",
+					__func__, 1 << p.basic.logical_bs_shift, bs);
+			return -1;
+		}
+		if (p.basic.physical_bs_shift != ilog2(pbs)) {
+			ublk_err("%s: physical block size should be %d, I got %d\n",
+					__func__, 1 << p.basic.physical_bs_shift, pbs);
+			return -1;
+		}
+	}
+	
+	if (p.basic.dev_sectors << 9 != bytes) {
+		ublk_err("%s: device size should be %lld, I got %lld\n",
+				__func__, p.basic.dev_sectors << 9, bytes);
+		return -1;
+	}
+
+	dev->tgt.dev_size = bytes;
+	dev->fds[1] = fd;
+	dev->nr_fds += 1;
+
+	return 0;
+}
+
 const struct ublk_tgt_ops tgt_ops_list[] = {
 	{
 		.name = "null",
 		.init_tgt = ublk_null_tgt_init,
 		.queue_io = ublk_null_queue_io,
+		.recover_tgt = ublk_null_tgt_recover,
 	},
 
 	{
@@ -1326,6 +1562,7 @@ const struct ublk_tgt_ops tgt_ops_list[] = {
 		.deinit_tgt = ublk_loop_tgt_deinit,
 		.queue_io = ublk_loop_queue_io,
 		.tgt_io_done = ublk_loop_io_done,
+		.recover_tgt = ublk_loop_tgt_recover,
 	},
 };
 
@@ -1359,6 +1596,8 @@ int main(int argc, char *argv[])
 		ret = cmd_dev_list(argc, argv);
 	else if (!strcmp(cmd, "help"))
 		ret = cmd_dev_help(argc, argv);
+	else if (!strcmp(cmd, "recover"))
+		ret = cmd_dev_recover(argc, argv);
 out:
 	if (ret)
 		cmd_dev_help(argc, argv);
-- 
2.31.1

[PATCH V2 blktests 2/2] tests: Add ublk tests

[Ziyang Zhang]

It is very important to test ublk crash handling since the userspace
part is not reliable. Especially we should test removing device, killing
ublk daemons and user recovery feature.

Add five new tests for ublk to cover these cases.

Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
---
 common/ublk        | 10 +++++-
 tests/ublk/001     | 48 +++++++++++++++++++++++++++
 tests/ublk/001.out |  2 ++
 tests/ublk/002     | 63 +++++++++++++++++++++++++++++++++++
 tests/ublk/002.out |  2 ++
 tests/ublk/003     | 48 +++++++++++++++++++++++++++
 tests/ublk/003.out |  2 ++
 tests/ublk/004     | 50 ++++++++++++++++++++++++++++
 tests/ublk/004.out |  2 ++
 tests/ublk/005     | 79 +++++++++++++++++++++++++++++++++++++++++++
 tests/ublk/005.out |  2 ++
 tests/ublk/006     | 83 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/ublk/006.out |  2 ++
 tests/ublk/rc      | 15 +++++++++
 14 files changed, 407 insertions(+), 1 deletion(-)
 create mode 100755 tests/ublk/001
 create mode 100644 tests/ublk/001.out
 create mode 100755 tests/ublk/002
 create mode 100644 tests/ublk/002.out
 create mode 100755 tests/ublk/003
 create mode 100644 tests/ublk/003.out
 create mode 100755 tests/ublk/004
 create mode 100644 tests/ublk/004.out
 create mode 100755 tests/ublk/005
 create mode 100644 tests/ublk/005.out
 create mode 100755 tests/ublk/006
 create mode 100644 tests/ublk/006.out
 create mode 100644 tests/ublk/rc

diff --git a/common/ublk b/common/ublk
index 932c534..7a951eb 100644
--- a/common/ublk
+++ b/common/ublk
@@ -15,8 +15,16 @@ _remove_ublk_devices() {
 	src/miniublk del -a
 }
 
+__get_ublk_dev_state() {
+	src/miniublk list -n "$1" | grep "state" | awk '{print $11}'
+}
+
+__get_ublk_daemon_pid() {
+	src/miniublk list -n "$1" | grep "pid" | awk '{print $7}'
+}
+
 _init_ublk() {
-	_remove_ublk_devices
+	_remove_ublk_devices > /dev/null 2>&1
 
 	modprobe -rq ublk_drv
 	if ! modprobe ublk_drv; then
diff --git a/tests/ublk/001 b/tests/ublk/001
new file mode 100755
index 0000000..36a43d7
--- /dev/null
+++ b/tests/ublk/001
@@ -0,0 +1,48 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Ziyang Zhang
+#
+# Test ublk delete
+
+. tests/ublk/rc
+
+DESCRIPTION="test ublk delete"
+
+__run() {
+	local type=$1
+
+	if [ "$type" == "null" ]; then
+		${ublk_prog} add -t null -n 0 > "$FULL" 2>&1
+	else
+		truncate -s 1G "$TMPDIR/img"
+		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 > "$FULL" 2>&1
+	fi
+
+	udevadm settle
+	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
+		echo "fail to list dev"
+	fi
+
+	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
+	sleep 2
+
+	${ublk_prog} del -n 0 >> "$FULL" 2>&1
+}
+
+test() {
+	local ublk_prog="src/miniublk"
+
+	echo "Running ${TEST_NAME}"
+
+	if ! _init_ublk; then
+		return 1
+	fi
+
+	for type in "null" "loop"; do
+		__run "$type"
+	done
+
+	_exit_ublk
+
+	echo "Test complete"
+}
diff --git a/tests/ublk/001.out b/tests/ublk/001.out
new file mode 100644
index 0000000..0d070b3
--- /dev/null
+++ b/tests/ublk/001.out
@@ -0,0 +1,2 @@
+Running ublk/001
+Test complete
diff --git a/tests/ublk/002 b/tests/ublk/002
new file mode 100755
index 0000000..e36589e
--- /dev/null
+++ b/tests/ublk/002
@@ -0,0 +1,63 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Ziyang Zhang
+#
+# Test ublk crash with delete after dead confirmation
+
+. tests/ublk/rc
+
+DESCRIPTION="test ublk crash with delete after dead confirmation"
+
+__run() {
+	local type=$1
+
+	if [ "$type" == "null" ]; then
+		${ublk_prog} add -t null -n 0 > "$FULL" 2>&1
+	else
+		truncate -s 1G "$TMPDIR/img"
+		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 > "$FULL" 2>&1
+	fi
+
+	udevadm settle
+	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
+		echo "fail to list dev"
+	fi
+
+	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
+	sleep 2
+
+	kill -9 "$(__get_ublk_daemon_pid 0)"
+	sleep 2
+
+	local secs=0
+	local state=""
+	while [ $secs -lt 20 ]; do
+		state="$(__get_ublk_dev_state 0)"
+		[  "$state" == "DEAD" ] && break
+		sleep 1
+		(( secs++ ))
+	done
+
+	state="$(__get_ublk_dev_state 0)"
+	[  "$state" != "DEAD" ] && echo "device is $state after killing queue daemon"
+
+	${ublk_prog} del -n 0 >> "$FULL" 2>&1
+}
+
+test() {
+	local ublk_prog="src/miniublk"
+
+	echo "Running ${TEST_NAME}"
+
+	if ! _init_ublk; then
+		return 1
+	fi
+
+	for type in "null" "loop"; do
+		__run "$type"
+	done
+
+	_exit_ublk
+
+	echo "Test complete"
+}
diff --git a/tests/ublk/002.out b/tests/ublk/002.out
new file mode 100644
index 0000000..93039b7
--- /dev/null
+++ b/tests/ublk/002.out
@@ -0,0 +1,2 @@
+Running ublk/002
+Test complete
diff --git a/tests/ublk/003 b/tests/ublk/003
new file mode 100755
index 0000000..b256b09
--- /dev/null
+++ b/tests/ublk/003
@@ -0,0 +1,48 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Ziyang Zhang
+#
+# Test mounting block device exported by ublk
+
+. tests/ublk/rc
+
+DESCRIPTION="test mounting block device exported by ublk"
+
+test() {
+	local ublk_prog="src/miniublk"
+	local ROOT_FSTYPE="$(findmnt -l -o FSTYPE -n /)"
+	local mnt="$TMPDIR/mnt"
+
+	echo "Running ${TEST_NAME}"
+
+	if ! _init_ublk; then
+		return 1
+	fi
+
+	truncate -s 1G "$TMPDIR/img"
+	${ublk_prog} add -t loop -f  "$TMPDIR/img" -n 0 > "$FULL" 2>&1
+
+	udevadm settle
+	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
+		echo "fail to list dev"
+	fi
+
+	wipefs -a /dev/ublkb0 >> "$FULL" 2>&1
+	mkfs.${ROOT_FSTYPE} /dev/ublkb0 >> "$FULL" 2>&1
+	mkdir -p "$mnt"
+	mount /dev/ublkb0 "$mnt" >> "$FULL" 2>&1
+
+	local UBLK_FSTYPE="$(findmnt -l -o FSTYPE -n $mnt)"
+	if [ "$UBLK_FSTYPE" != "$ROOT_FSTYPE" ]; then
+		echo "got $UBLK_FSTYPE, should be $ROOT_FSTYPE"
+	fi
+	umount "$mnt" > /dev/null 2>&1
+
+	${ublk_prog} del -n 0 >> "$FULL" 2>&1
+
+	_exit_ublk
+
+	echo "Test complete"
+}
+
+
diff --git a/tests/ublk/003.out b/tests/ublk/003.out
new file mode 100644
index 0000000..90a3bfa
--- /dev/null
+++ b/tests/ublk/003.out
@@ -0,0 +1,2 @@
+Running ublk/003
+Test complete
diff --git a/tests/ublk/004 b/tests/ublk/004
new file mode 100755
index 0000000..84e01d1
--- /dev/null
+++ b/tests/ublk/004
@@ -0,0 +1,50 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Ziyang Zhang
+#
+# Test ublk crash with delete just after daemon kill
+
+. tests/ublk/rc
+
+DESCRIPTION="test ublk crash with delete just after daemon kill"
+
+__run() {
+	local type=$1
+
+	if [ "$type" == "null" ]; then
+		${ublk_prog} add -t null -n 0 > "$FULL" 2>&1
+	else
+		truncate -s 1G "$TMPDIR/img"
+		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 > "$FULL" 2>&1
+	fi
+
+	udevadm settle
+	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
+		echo "fail to list dev"
+	fi
+
+	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
+	sleep 2
+
+	kill -9 "$(__get_ublk_daemon_pid 0)"
+
+	${ublk_prog} del -n 0 >> "$FULL" 2>&1
+}
+
+test() {
+	local ublk_prog="src/miniublk"
+
+	echo "Running ${TEST_NAME}"
+
+	if ! _init_ublk; then
+		return 1
+	fi
+
+	for type in "null" "loop"; do
+		__run "$type"
+	done
+
+	_exit_ublk
+
+	echo "Test complete"
+}
diff --git a/tests/ublk/004.out b/tests/ublk/004.out
new file mode 100644
index 0000000..a92cd50
--- /dev/null
+++ b/tests/ublk/004.out
@@ -0,0 +1,2 @@
+Running ublk/004
+Test complete
diff --git a/tests/ublk/005 b/tests/ublk/005
new file mode 100755
index 0000000..f365fd6
--- /dev/null
+++ b/tests/ublk/005
@@ -0,0 +1,79 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Ziyang Zhang
+#
+# Test ublk recovery with one time daemon kill:
+# (1)kill all ubq_deamon, (2)recover with new ubq_daemon,
+# (3)delete dev
+
+. tests/ublk/rc
+
+DESCRIPTION="test ublk recovery with one time daemon kill"
+
+__run() {
+	local type=$1
+
+	if [ "$type" == "null" ]; then
+		${ublk_prog} add -t null -n 0 -r > "$FULL" 2>&1
+	else
+		truncate -s 1G "$TMPDIR/img"
+		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 -r > "$FULL" 2>&1
+	fi
+
+	udevadm settle
+	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
+		echo "fail to list dev"
+	fi
+
+	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
+	sleep 2
+
+	kill -9 "$(__get_ublk_daemon_pid 0)"
+	sleep 2
+
+	local secs=0
+	local state=""
+	while [ $secs -lt 20 ]; do
+		state="$(__get_ublk_dev_state 0)"
+		[ "$state" == "QUIESCED" ] && break
+		sleep 1
+		(( secs++ ))
+	done
+
+	state="$(__get_ublk_dev_state 0)"
+	[ "$state" != "QUIESCED" ] && echo "device is $state after killing queue daemon"
+
+	if [ "$type" == "null" ]; then
+		${ublk_prog} recover -t null -n 0 >> "$FULL" 2>&1
+	else
+		${ublk_prog} recover -t loop -f "$TMPDIR/img" -n 0 >> "$FULL" 2>&1
+	fi
+
+	while [ $secs -lt 20 ]; do
+		state="$(__get_ublk_dev_state 0)"
+		[ "$state" == "LIVE" ] && break
+		sleep 1
+		(( secs++ ))
+	done
+	[ "$state" != "LIVE" ] && echo "device is $state after recovery"
+
+	${ublk_prog} del -n 0 >> "$FULL" 2>&1
+}
+
+test() {
+	local ublk_prog="src/miniublk"
+
+	echo "Running ${TEST_NAME}"
+
+	if ! _init_ublk; then
+		return 1
+	fi
+
+	for type in "null" "loop"; do
+		__run "$type"
+	done
+
+	_exit_ublk
+
+	echo "Test complete"
+}
diff --git a/tests/ublk/005.out b/tests/ublk/005.out
new file mode 100644
index 0000000..20d7b38
--- /dev/null
+++ b/tests/ublk/005.out
@@ -0,0 +1,2 @@
+Running ublk/005
+Test complete
diff --git a/tests/ublk/006 b/tests/ublk/006
new file mode 100755
index 0000000..0848939
--- /dev/null
+++ b/tests/ublk/006
@@ -0,0 +1,83 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Ziyang Zhang
+#
+# Test ublk recovery with two times daemon kill:
+# (1)kill all ubq_deamon, (2)recover with new ubq_daemon,
+# (3)kill all ubq_deamon, (4)delete dev
+
+. tests/ublk/rc
+
+DESCRIPTION="test ublk recovery with two times daemon kill"
+
+__run() {
+	local type=$1
+
+	if [ "$type" == "null" ]; then
+		${ublk_prog} add -t null -n 0 -r > "$FULL" 2>&1
+	else
+		truncate -s 1G "$TMPDIR/img"
+		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 -r > "$FULL" 2>&1
+	fi
+
+	udevadm settle
+	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
+		echo "fail to list dev"
+	fi
+
+	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
+	sleep 2
+
+	kill -9 "$(__get_ublk_daemon_pid 0)"
+	sleep 2
+
+	local secs=0
+	local state=""
+	while [ $secs -lt 20 ]; do
+		state="$(__get_ublk_dev_state 0)"
+		[ "$state" == "QUIESCED" ] && break
+		sleep 1
+		(( secs++ ))
+	done
+
+	state="$(__get_ublk_dev_state 0)"
+	[ "$state" != "QUIESCED" ] && echo "device is $state after killing queue daemon"
+
+	if [ "$type" == "null" ]; then
+		${ublk_prog} recover -t null -n 0 >> "$FULL" 2>&1
+	else
+		${ublk_prog} recover -t loop -f "$TMPDIR/img" -n 0 >> "$FULL" 2>&1
+	fi
+
+	secs=0
+	while [ $secs -lt 20 ]; do
+		state="$(__get_ublk_dev_state 0)"
+		[ "$state" == "LIVE" ] && break
+		sleep 1
+		(( secs++ ))
+	done
+	[ "$state" != "LIVE" ] && echo "device is $state after recovery"
+
+	kill -9 "$(__get_ublk_daemon_pid 0)"
+
+	${ublk_prog} del -n 0 >> "$FULL" 2>&1
+}
+
+test() {
+	local ublk_prog="src/miniublk"
+
+	echo "Running ${TEST_NAME}"
+
+	if ! _init_ublk; then
+		return 1
+	fi
+
+	for type in "null" "loop"; do
+		__run "$type"
+	done
+
+	_exit_ublk
+
+	echo "Test complete"
+}
+
diff --git a/tests/ublk/006.out b/tests/ublk/006.out
new file mode 100644
index 0000000..6d2a530
--- /dev/null
+++ b/tests/ublk/006.out
@@ -0,0 +1,2 @@
+Running ublk/006
+Test complete
diff --git a/tests/ublk/rc b/tests/ublk/rc
new file mode 100644
index 0000000..8cbc757
--- /dev/null
+++ b/tests/ublk/rc
@@ -0,0 +1,15 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Ziyang Zhang
+#
+# ublk tests.
+
+. common/rc
+. common/ublk
+. common/fio
+
+group_requires() {
+	_have_root
+	_have_ublk
+	_have_fio
+}
-- 
2.31.1

Re: [PATCH V2 blktests 1/2] src/miniublk: add user recovery

[Ming Lei]

On Fri, May 05, 2023 at 11:28:07AM +0800, Ziyang Zhang wrote:
> We are going to test ublk's user recovery feature so add support in
> miniublk.
> 
> Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>

Looks fine,

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming

Re: [PATCH V2 blktests 2/2] tests: Add ublk tests

[Ming Lei]

On Fri, May 05, 2023 at 11:28:08AM +0800, Ziyang Zhang wrote:
> It is very important to test ublk crash handling since the userspace
> part is not reliable. Especially we should test removing device, killing
> ublk daemons and user recovery feature.
> 
> Add five new tests for ublk to cover these cases.
> 
> Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>

Looks fine,

Reviewed-by: Ming Lei <ming.lei@redhat.com>

BTW, the following ublk driver patch is required on v6.3+ for passing
'./check ublk/':

https://lore.kernel.org/linux-block/20230505153142.1258336-1-ming.lei@redhat.com/T/#u


Thanks,
Ming

Re: [PATCH V2 blktests 1/2] src/miniublk: add user recovery

[Shinichiro Kawasaki]

On May 05, 2023 / 11:28, Ziyang Zhang wrote:
[...]
> +
> +	if (S_ISBLK(st.st_mode)) {
> +		if (ioctl(fd, BLKGETSIZE64, &bytes) != 0)
> +			return -EBADF;
> +		if (ioctl(fd, BLKSSZGET, &bs) != 0)
> +			return -1;
> +		if (ioctl(fd, BLKPBSZGET, &pbs) != 0)
> +			return -1;			

Nit: the line above has some trailing tabs.

> +	} else if (S_ISREG(st.st_mode)) {
> +		bytes = st.st_size;
> +	} else {
> +		bytes = 0;
> +	}
> +
> +	if (fcntl(fd, F_SETFL, O_DIRECT)) {
> +		/* buffered I/O */
> +		ublk_log("%s: %s, ublk-loop fallback to buffered IO\n",
> +				__func__, strerror(errno));
> +	}
> +	else {
> +		/* direct I/O */
> +		if (p.basic.logical_bs_shift != ilog2(bs)) {
> +			ublk_err("%s: logical block size should be %d, I got %d\n",
> +					__func__, 1 << p.basic.logical_bs_shift, bs);
> +			return -1;
> +		}
> +		if (p.basic.physical_bs_shift != ilog2(pbs)) {
> +			ublk_err("%s: physical block size should be %d, I got %d\n",
> +					__func__, 1 << p.basic.physical_bs_shift, pbs);
> +			return -1;
> +		}
> +	}
> +	

The line above also.

> +	if (p.basic.dev_sectors << 9 != bytes) {
> +		ublk_err("%s: device size should be %lld, I got %lld\n",
> +				__func__, p.basic.dev_sectors << 9, bytes);
> +		return -1;
> +	}
> +
> +	dev->tgt.dev_size = bytes;
> +	dev->fds[1] = fd;
> +	dev->nr_fds += 1;
> +
> +	return 0;
> +}
> +
>  const struct ublk_tgt_ops tgt_ops_list[] = {
>  	{
>  		.name = "null",
>  		.init_tgt = ublk_null_tgt_init,
>  		.queue_io = ublk_null_queue_io,
> +		.recover_tgt = ublk_null_tgt_recover,
>  	},
>  
>  	{
> @@ -1326,6 +1562,7 @@ const struct ublk_tgt_ops tgt_ops_list[] = {
>  		.deinit_tgt = ublk_loop_tgt_deinit,
>  		.queue_io = ublk_loop_queue_io,
>  		.tgt_io_done = ublk_loop_io_done,
> +		.recover_tgt = ublk_loop_tgt_recover,
>  	},
>  };
>  
> @@ -1359,6 +1596,8 @@ int main(int argc, char *argv[])
>  		ret = cmd_dev_list(argc, argv);
>  	else if (!strcmp(cmd, "help"))
>  		ret = cmd_dev_help(argc, argv);
> +	else if (!strcmp(cmd, "recover"))
> +		ret = cmd_dev_recover(argc, argv);
>  out:
>  	if (ret)
>  		cmd_dev_help(argc, argv);
> -- 
> 2.31.1
> 

Re: [PATCH V2 blktests 2/2] tests: Add ublk tests

[Shinichiro Kawasaki]

On May 05, 2023 / 11:28, Ziyang Zhang wrote:
> It is very important to test ublk crash handling since the userspace
> part is not reliable. Especially we should test removing device, killing
> ublk daemons and user recovery feature.
> 
> Add five new tests for ublk to cover these cases.
> 
> Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
> ---
>  common/ublk        | 10 +++++-
>  tests/ublk/001     | 48 +++++++++++++++++++++++++++
>  tests/ublk/001.out |  2 ++
>  tests/ublk/002     | 63 +++++++++++++++++++++++++++++++++++
>  tests/ublk/002.out |  2 ++
>  tests/ublk/003     | 48 +++++++++++++++++++++++++++
>  tests/ublk/003.out |  2 ++
>  tests/ublk/004     | 50 ++++++++++++++++++++++++++++
>  tests/ublk/004.out |  2 ++
>  tests/ublk/005     | 79 +++++++++++++++++++++++++++++++++++++++++++
>  tests/ublk/005.out |  2 ++
>  tests/ublk/006     | 83 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ublk/006.out |  2 ++
>  tests/ublk/rc      | 15 +++++++++
>  14 files changed, 407 insertions(+), 1 deletion(-)
>  create mode 100755 tests/ublk/001
>  create mode 100644 tests/ublk/001.out
>  create mode 100755 tests/ublk/002
>  create mode 100644 tests/ublk/002.out
>  create mode 100755 tests/ublk/003
>  create mode 100644 tests/ublk/003.out
>  create mode 100755 tests/ublk/004
>  create mode 100644 tests/ublk/004.out
>  create mode 100755 tests/ublk/005
>  create mode 100644 tests/ublk/005.out
>  create mode 100755 tests/ublk/006
>  create mode 100644 tests/ublk/006.out
>  create mode 100644 tests/ublk/rc
> 
> diff --git a/common/ublk b/common/ublk
> index 932c534..7a951eb 100644
> --- a/common/ublk
> +++ b/common/ublk
> @@ -15,8 +15,16 @@ _remove_ublk_devices() {
>  	src/miniublk del -a
>  }
>  
> +__get_ublk_dev_state() {

This will be the first function in blktests to have double underscores. Is there
any meaning of "__" ?  If not, single underscore will be enough. In blktests,
helper functions in common/* or tests/*/rc have single underscore to indicate
that the functions are not in each test case.

> +	src/miniublk list -n "$1" | grep "state" | awk '{print $11}'
> +}
> +
> +__get_ublk_daemon_pid() {
> +	src/miniublk list -n "$1" | grep "pid" | awk '{print $7}'
> +}
> +
>  _init_ublk() {
> -	_remove_ublk_devices
> +	_remove_ublk_devices > /dev/null 2>&1
>  
>  	modprobe -rq ublk_drv
>  	if ! modprobe ublk_drv; then
> diff --git a/tests/ublk/001 b/tests/ublk/001
> new file mode 100755
> index 0000000..36a43d7
> --- /dev/null
> +++ b/tests/ublk/001
> @@ -0,0 +1,48 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ziyang Zhang
> +#
> +# Test ublk delete
> +
> +. tests/ublk/rc
> +
> +DESCRIPTION="test ublk delete"
> +
> +__run() {

Same comment about the double underscores.

> +	local type=$1
> +
> +	if [ "$type" == "null" ]; then
> +		${ublk_prog} add -t null -n 0 > "$FULL" 2>&1
> +	else
> +		truncate -s 1G "$TMPDIR/img"
> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 > "$FULL" 2>&1
> +	fi
> +
> +	udevadm settle
> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
> +		echo "fail to list dev"
> +	fi
> +
> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &

Nit: this line is a bit long, so it would be the better to fold it to two lines
with a backslash.

> +	sleep 2
> +
> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
> +}
> +
> +test() {
> +	local ublk_prog="src/miniublk"

The line above iss repeated all test cases. How about to set it in
tests/ublk/rc, like 'export UBLK_PROG="src/miniublk"'?

> +
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _init_ublk; then
> +		return 1
> +	fi
> +
> +	for type in "null" "loop"; do
> +		__run "$type"
> +	done
> +
> +	_exit_ublk
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/ublk/001.out b/tests/ublk/001.out
> new file mode 100644
> index 0000000..0d070b3
> --- /dev/null
> +++ b/tests/ublk/001.out
> @@ -0,0 +1,2 @@
> +Running ublk/001
> +Test complete
> diff --git a/tests/ublk/002 b/tests/ublk/002
> new file mode 100755
> index 0000000..e36589e
> --- /dev/null
> +++ b/tests/ublk/002
> @@ -0,0 +1,63 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ziyang Zhang
> +#
> +# Test ublk crash with delete after dead confirmation
> +
> +. tests/ublk/rc
> +
> +DESCRIPTION="test ublk crash with delete after dead confirmation"
> +
> +__run() {
> +	local type=$1
> +
> +	if [ "$type" == "null" ]; then
> +		${ublk_prog} add -t null -n 0 > "$FULL" 2>&1
> +	else
> +		truncate -s 1G "$TMPDIR/img"
> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 > "$FULL" 2>&1
> +	fi
> +
> +	udevadm settle
> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
> +		echo "fail to list dev"
> +	fi
> +
> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &

Nit: long line

> +	sleep 2
> +
> +	kill -9 "$(__get_ublk_daemon_pid 0)"
> +	sleep 2
> +
> +	local secs=0
> +	local state=""
> +	while [ $secs -lt 20 ]; do
> +		state="$(__get_ublk_dev_state 0)"
> +		[  "$state" == "DEAD" ] && break

Nit: double spaces

> +		sleep 1
> +		(( secs++ ))
> +	done
> +
> +	state="$(__get_ublk_dev_state 0)"
> +	[  "$state" != "DEAD" ] && echo "device is $state after killing queue daemon"

Nit: double spaces and long line

> +
> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
> +}
> +
> +test() {
> +	local ublk_prog="src/miniublk"
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _init_ublk; then
> +		return 1
> +	fi
> +
> +	for type in "null" "loop"; do
> +		__run "$type"
> +	done
> +
> +	_exit_ublk
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/ublk/002.out b/tests/ublk/002.out
> new file mode 100644
> index 0000000..93039b7
> --- /dev/null
> +++ b/tests/ublk/002.out
> @@ -0,0 +1,2 @@
> +Running ublk/002
> +Test complete
> diff --git a/tests/ublk/003 b/tests/ublk/003
> new file mode 100755
> index 0000000..b256b09
> --- /dev/null
> +++ b/tests/ublk/003
> @@ -0,0 +1,48 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ziyang Zhang
> +#
> +# Test mounting block device exported by ublk
> +
> +. tests/ublk/rc
> +
> +DESCRIPTION="test mounting block device exported by ublk"
> +
> +test() {
> +	local ublk_prog="src/miniublk"
> +	local ROOT_FSTYPE="$(findmnt -l -o FSTYPE -n /)"

Oh, this test uses same filesystem as the root filesystem. This idea is
interesting, but it may have some drawbacks. Blktests users may run different
filesystems, and may see different results. Inconsistent test condition and
inconsistent test results. And if the blktests user have minor filesystem for
the root, it may results in the test case failure caused by the filesystem. It
would be the better just use ext4 as loop/007 or nbd/003 do to make this test
case more reliable

> +	local mnt="$TMPDIR/mnt"
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _init_ublk; then
> +		return 1
> +	fi
> +
> +	truncate -s 1G "$TMPDIR/img"
> +	${ublk_prog} add -t loop -f  "$TMPDIR/img" -n 0 > "$FULL" 2>&1
> +
> +	udevadm settle
> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
> +		echo "fail to list dev"
> +	fi
> +
> +	wipefs -a /dev/ublkb0 >> "$FULL" 2>&1
> +	mkfs.${ROOT_FSTYPE} /dev/ublkb0 >> "$FULL" 2>&1
> +	mkdir -p "$mnt"
> +	mount /dev/ublkb0 "$mnt" >> "$FULL" 2>&1
> +
> +	local UBLK_FSTYPE="$(findmnt -l -o FSTYPE -n $mnt)"
> +	if [ "$UBLK_FSTYPE" != "$ROOT_FSTYPE" ]; then
> +		echo "got $UBLK_FSTYPE, should be $ROOT_FSTYPE"
> +	fi
> +	umount "$mnt" > /dev/null 2>&1
> +
> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
> +
> +	_exit_ublk
> +
> +	echo "Test complete"
> +}
> +
> +
> diff --git a/tests/ublk/003.out b/tests/ublk/003.out
> new file mode 100644
> index 0000000..90a3bfa
> --- /dev/null
> +++ b/tests/ublk/003.out
> @@ -0,0 +1,2 @@
> +Running ublk/003
> +Test complete
> diff --git a/tests/ublk/004 b/tests/ublk/004
> new file mode 100755
> index 0000000..84e01d1
> --- /dev/null
> +++ b/tests/ublk/004
> @@ -0,0 +1,50 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ziyang Zhang
> +#
> +# Test ublk crash with delete just after daemon kill
> +
> +. tests/ublk/rc
> +
> +DESCRIPTION="test ublk crash with delete just after daemon kill"
> +
> +__run() {
> +	local type=$1
> +
> +	if [ "$type" == "null" ]; then
> +		${ublk_prog} add -t null -n 0 > "$FULL" 2>&1
> +	else
> +		truncate -s 1G "$TMPDIR/img"
> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 > "$FULL" 2>&1
> +	fi
> +
> +	udevadm settle
> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
> +		echo "fail to list dev"
> +	fi
> +
> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &

Nit: long line

> +	sleep 2
> +
> +	kill -9 "$(__get_ublk_daemon_pid 0)"

I think it would be the better to wait for the pid, to ensure that the ublk
daemon process completed.

> +
> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
> +}
> +
> +test() {
> +	local ublk_prog="src/miniublk"
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _init_ublk; then
> +		return 1
> +	fi
> +
> +	for type in "null" "loop"; do
> +		__run "$type"
> +	done
> +
> +	_exit_ublk
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/ublk/004.out b/tests/ublk/004.out
> new file mode 100644
> index 0000000..a92cd50
> --- /dev/null
> +++ b/tests/ublk/004.out
> @@ -0,0 +1,2 @@
> +Running ublk/004
> +Test complete
> diff --git a/tests/ublk/005 b/tests/ublk/005
> new file mode 100755
> index 0000000..f365fd6
> --- /dev/null
> +++ b/tests/ublk/005
> @@ -0,0 +1,79 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ziyang Zhang
> +#
> +# Test ublk recovery with one time daemon kill:
> +# (1)kill all ubq_deamon, (2)recover with new ubq_daemon,
> +# (3)delete dev
> +
> +. tests/ublk/rc
> +
> +DESCRIPTION="test ublk recovery with one time daemon kill"
> +
> +__run() {
> +	local type=$1
> +
> +	if [ "$type" == "null" ]; then
> +		${ublk_prog} add -t null -n 0 -r > "$FULL" 2>&1
> +	else
> +		truncate -s 1G "$TMPDIR/img"
> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 -r > "$FULL" 2>&1
> +	fi
> +
> +	udevadm settle
> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
> +		echo "fail to list dev"
> +	fi
> +
> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &

Nit: long line

> +	sleep 2
> +
> +	kill -9 "$(__get_ublk_daemon_pid 0)"
> +	sleep 2
> +
> +	local secs=0
> +	local state=""
> +	while [ $secs -lt 20 ]; do
> +		state="$(__get_ublk_dev_state 0)"
> +		[ "$state" == "QUIESCED" ] && break
> +		sleep 1
> +		(( secs++ ))
> +	done
> +
> +	state="$(__get_ublk_dev_state 0)"
> +	[ "$state" != "QUIESCED" ] && echo "device is $state after killing queue daemon"

Nit: long line

> +
> +	if [ "$type" == "null" ]; then
> +		${ublk_prog} recover -t null -n 0 >> "$FULL" 2>&1
> +	else
> +		${ublk_prog} recover -t loop -f "$TMPDIR/img" -n 0 >> "$FULL" 2>&1

Nit: long line

> +	fi
> +
> +	while [ $secs -lt 20 ]; do
> +		state="$(__get_ublk_dev_state 0)"
> +		[ "$state" == "LIVE" ] && break
> +		sleep 1
> +		(( secs++ ))
> +	done
> +	[ "$state" != "LIVE" ] && echo "device is $state after recovery"
> +
> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
> +}
> +
> +test() {
> +	local ublk_prog="src/miniublk"
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _init_ublk; then
> +		return 1
> +	fi
> +
> +	for type in "null" "loop"; do
> +		__run "$type"
> +	done
> +
> +	_exit_ublk
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/ublk/005.out b/tests/ublk/005.out
> new file mode 100644
> index 0000000..20d7b38
> --- /dev/null
> +++ b/tests/ublk/005.out
> @@ -0,0 +1,2 @@
> +Running ublk/005
> +Test complete
> diff --git a/tests/ublk/006 b/tests/ublk/006
> new file mode 100755
> index 0000000..0848939
> --- /dev/null
> +++ b/tests/ublk/006
> @@ -0,0 +1,83 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ziyang Zhang
> +#
> +# Test ublk recovery with two times daemon kill:
> +# (1)kill all ubq_deamon, (2)recover with new ubq_daemon,
> +# (3)kill all ubq_deamon, (4)delete dev
> +
> +. tests/ublk/rc
> +
> +DESCRIPTION="test ublk recovery with two times daemon kill"
> +
> +__run() {
> +	local type=$1
> +
> +	if [ "$type" == "null" ]; then
> +		${ublk_prog} add -t null -n 0 -r > "$FULL" 2>&1
> +	else
> +		truncate -s 1G "$TMPDIR/img"
> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 -r > "$FULL" 2>&1
> +	fi
> +
> +	udevadm settle
> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
> +		echo "fail to list dev"
> +	fi
> +
> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &

Nit: long line

> +	sleep 2
> +
> +	kill -9 "$(__get_ublk_daemon_pid 0)"

Same comment as ublk/005. How about to have the helper function
"_kill_ublk_daemon" in tests/ublk/rc which does both kill and wait?

> +	sleep 2
> +
> +	local secs=0
> +	local state=""
> +	while [ $secs -lt 20 ]; do
> +		state="$(__get_ublk_dev_state 0)"
> +		[ "$state" == "QUIESCED" ] && break
> +		sleep 1
> +		(( secs++ ))
> +	done
> +
> +	state="$(__get_ublk_dev_state 0)"
> +	[ "$state" != "QUIESCED" ] && echo "device is $state after killing queue daemon"

Nit: long line

> +
> +	if [ "$type" == "null" ]; then
> +		${ublk_prog} recover -t null -n 0 >> "$FULL" 2>&1
> +	else
> +		${ublk_prog} recover -t loop -f "$TMPDIR/img" -n 0 >> "$FULL" 2>&1

Nit: long line

> +	fi
> +
> +	secs=0
> +	while [ $secs -lt 20 ]; do
> +		state="$(__get_ublk_dev_state 0)"
> +		[ "$state" == "LIVE" ] && break
> +		sleep 1
> +		(( secs++ ))
> +	done
> +	[ "$state" != "LIVE" ] && echo "device is $state after recovery"
> +
> +	kill -9 "$(__get_ublk_daemon_pid 0)"
> +
> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
> +}
> +
> +test() {
> +	local ublk_prog="src/miniublk"
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	if ! _init_ublk; then
> +		return 1
> +	fi
> +
> +	for type in "null" "loop"; do
> +		__run "$type"
> +	done
> +
> +	_exit_ublk
> +
> +	echo "Test complete"
> +}
> +
> diff --git a/tests/ublk/006.out b/tests/ublk/006.out
> new file mode 100644
> index 0000000..6d2a530
> --- /dev/null
> +++ b/tests/ublk/006.out
> @@ -0,0 +1,2 @@
> +Running ublk/006
> +Test complete
> diff --git a/tests/ublk/rc b/tests/ublk/rc
> new file mode 100644
> index 0000000..8cbc757
> --- /dev/null
> +++ b/tests/ublk/rc
> @@ -0,0 +1,15 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Ziyang Zhang
> +#
> +# ublk tests.
> +
> +. common/rc
> +. common/ublk
> +. common/fio
> +
> +group_requires() {
> +	_have_root
> +	_have_ublk
> +	_have_fio
> +}
> -- 
> 2.31.1
> 

Re: [PATCH V2 blktests 0/2] blktests: Add ublk testcases

[Shinichiro Kawasaki]

On May 05, 2023 / 11:28, Ziyang Zhang wrote:
> Hi,
> 
> ublk can passthrough I/O requests to userspce daemons. It is very important
> to test ublk crash handling since the userspace part is not reliable.
> Especially we should test removing device, killing ublk daemons and user
> recovery feature.
> 
> The first patch add user recovery support in miniublk.
> 
> The second patch add five new tests for ublk to cover above cases.
> 
> V2:
> - Check parameters in recovery
> - Add one small delay before deleting device
> - Write informative description

Ziyang, thanks for the v2 patches and sorry for this slow response. Please find
my comments in line.

FYI, I also ran the new test cases on kernel v6.4-rc2, and observed failure of
ublk/001. The failure cause is the lockdep WARN [1]. The test case already found
an issue, so it proves that the test is valuable :)

[1]

[  204.288195] run blktests ublk/001 at 2023-05-16 17:52:14

[  206.755085] ======================================================
[  206.756063] WARNING: possible circular locking dependency detected
[  206.756595] 6.4.0-rc2 #6 Not tainted
[  206.756924] ------------------------------------------------------
[  206.757436] iou-wrk-1070/1071 is trying to acquire lock:
[  206.757891] ffff88811f1420a8 (&ctx->uring_lock){+.+.}-{3:3}, at: __io_req_complete_post+0x792/0xd50
[  206.758625] 
               but task is already holding lock:
[  206.759166] ffff88812c3f66c0 (&ub->mutex){+.+.}-{3:3}, at: ublk_stop_dev+0x2b/0x400 [ublk_drv]
[  206.759865] 
               which lock already depends on the new lock.

[  206.760623] 
               the existing dependency chain (in reverse order) is:
[  206.761282] 
               -> #1 (&ub->mutex){+.+.}-{3:3}:
[  206.761811]        __mutex_lock+0x185/0x18b0
[  206.762192]        ublk_ch_uring_cmd+0x511/0x1630 [ublk_drv]
[  206.762678]        io_uring_cmd+0x1ec/0x3d0
[  206.763081]        io_issue_sqe+0x461/0xb70
[  206.763477]        io_submit_sqes+0x794/0x1c50
[  206.763857]        __do_sys_io_uring_enter+0x736/0x1ce0
[  206.764368]        do_syscall_64+0x5c/0x90
[  206.764724]        entry_SYSCALL_64_after_hwframe+0x72/0xdc
[  206.765244] 
               -> #0 (&ctx->uring_lock){+.+.}-{3:3}:
[  206.765813]        __lock_acquire+0x2f25/0x5f00
[  206.766272]        lock_acquire+0x1a9/0x4e0
[  206.766633]        __mutex_lock+0x185/0x18b0
[  206.767042]        __io_req_complete_post+0x792/0xd50
[  206.767500]        io_uring_cmd_done+0x27d/0x300
[  206.767918]        ublk_cancel_dev+0x1c6/0x410 [ublk_drv]
[  206.768416]        ublk_stop_dev+0x2ad/0x400 [ublk_drv]
[  206.768853]        ublk_ctrl_uring_cmd+0x14fd/0x3bf0 [ublk_drv]
[  206.769411]        io_uring_cmd+0x1ec/0x3d0
[  206.769772]        io_issue_sqe+0x461/0xb70
[  206.770175]        io_wq_submit_work+0x2b5/0x710
[  206.770600]        io_worker_handle_work+0x6b8/0x1620
[  206.771066]        io_wq_worker+0x4ef/0xb50
[  206.771461]        ret_from_fork+0x2c/0x50
[  206.771817] 
               other info that might help us debug this:

[  206.773807]  Possible unsafe locking scenario:

[  206.775596]        CPU0                    CPU1
[  206.776607]        ----                    ----
[  206.777604]   lock(&ub->mutex);
[  206.778496]                                lock(&ctx->uring_lock);
[  206.779601]                                lock(&ub->mutex);
[  206.780656]   lock(&ctx->uring_lock);
[  206.781561] 
                *** DEADLOCK ***

[  206.783778] 1 lock held by iou-wrk-1070/1071:
[  206.784697]  #0: ffff88812c3f66c0 (&ub->mutex){+.+.}-{3:3}, at: ublk_stop_dev+0x2b/0x400 [ublk_drv]
[  206.786005] 
               stack backtrace:
[  206.787493] CPU: 1 PID: 1071 Comm: iou-wrk-1070 Not tainted 6.4.0-rc2 #6
[  206.788576] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc38 04/01/2014
[  206.789819] Call Trace:
[  206.790617]  <TASK>
[  206.791395]  dump_stack_lvl+0x57/0x90
[  206.792284]  check_noncircular+0x27b/0x310
[  206.793168]  ? __pfx_mark_lock+0x10/0x10
[  206.794068]  ? __pfx_check_noncircular+0x10/0x10
[  206.795017]  ? lock_acquire+0x1a9/0x4e0
[  206.795871]  ? lockdep_lock+0xca/0x1c0
[  206.796750]  ? __pfx_lockdep_lock+0x10/0x10
[  206.797665]  __lock_acquire+0x2f25/0x5f00
[  206.798569]  ? __pfx___lock_acquire+0x10/0x10
[  206.799492]  ? try_to_wake_up+0x806/0x1a30
[  206.800395]  ? __pfx_lock_release+0x10/0x10
[  206.801306]  lock_acquire+0x1a9/0x4e0
[  206.802143]  ? __io_req_complete_post+0x792/0xd50
[  206.803092]  ? __pfx_lock_acquire+0x10/0x10
[  206.803998]  ? lock_is_held_type+0xce/0x120
[  206.804866]  ? find_held_lock+0x2d/0x110
[  206.805760]  ? __pfx___might_resched+0x10/0x10
[  206.806684]  ? lock_release+0x378/0x650
[  206.807568]  __mutex_lock+0x185/0x18b0
[  206.808440]  ? __io_req_complete_post+0x792/0xd50
[  206.809379]  ? mark_held_locks+0x96/0xe0
[  206.810359]  ? __io_req_complete_post+0x792/0xd50
[  206.811294]  ? _raw_spin_unlock_irqrestore+0x4c/0x60
[  206.812208]  ? lockdep_hardirqs_on+0x7d/0x100
[  206.813078]  ? __pfx___mutex_lock+0x10/0x10
[  206.813936]  ? __wake_up_common_lock+0xe8/0x150
[  206.814817]  ? __pfx___wake_up_common_lock+0x10/0x10
[  206.815736]  ? percpu_counter_add_batch+0x9f/0x160
[  206.816643]  ? __io_req_complete_post+0x792/0xd50
[  206.817541]  __io_req_complete_post+0x792/0xd50
[  206.818429]  ? mark_held_locks+0x96/0xe0
[  206.819276]  io_uring_cmd_done+0x27d/0x300
[  206.820129]  ? kasan_quarantine_put+0xd6/0x1e0
[  206.821015]  ? __pfx_io_uring_cmd_done+0x10/0x10
[  206.821915]  ? per_cpu_remove_cache+0x80/0x80
[  206.822794]  ? slab_free_freelist_hook+0x9e/0x1c0
[  206.823697]  ublk_cancel_dev+0x1c6/0x410 [ublk_drv]
[  206.824665]  ? kobject_put+0x190/0x4a0
[  206.825503]  ublk_stop_dev+0x2ad/0x400 [ublk_drv]
[  206.826410]  ublk_ctrl_uring_cmd+0x14fd/0x3bf0 [ublk_drv]
[  206.827377]  ? __pfx_ublk_ctrl_uring_cmd+0x10/0x10 [ublk_drv]
[  206.828376]  ? selinux_uring_cmd+0x1cc/0x260
[  206.829268]  ? __pfx_selinux_uring_cmd+0x10/0x10
[  206.830169]  ? lock_acquire+0x1a9/0x4e0
[  206.831007]  io_uring_cmd+0x1ec/0x3d0
[  206.831833]  io_issue_sqe+0x461/0xb70
[  206.832651]  io_wq_submit_work+0x2b5/0x710
[  206.833488]  io_worker_handle_work+0x6b8/0x1620
[  206.834345]  io_wq_worker+0x4ef/0xb50
[  206.835143]  ? __pfx_io_wq_worker+0x10/0x10
[  206.835979]  ? lock_release+0x378/0x650
[  206.836784]  ? ret_from_fork+0x12/0x50
[  206.837586]  ? __pfx_lock_release+0x10/0x10
[  206.838419]  ? do_raw_spin_lock+0x12e/0x270
[  206.839250]  ? __pfx_do_raw_spin_lock+0x10/0x10
[  206.840111]  ? __pfx_io_wq_worker+0x10/0x10
[  206.840947]  ret_from_fork+0x2c/0x50
[  206.841738]  </TASK>

Re: [PATCH V2 blktests 2/2] tests: Add ublk tests

[Ziyang Zhang]

On 2023/5/16 16:47, Shinichiro Kawasaki wrote:
> On May 05, 2023 / 11:28, Ziyang Zhang wrote:
>> It is very important to test ublk crash handling since the userspace
>> part is not reliable. Especially we should test removing device, killing
>> ublk daemons and user recovery feature.
>>
>> Add five new tests for ublk to cover these cases.
>>
>> Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
>> ---

[...]

>> diff --git a/tests/ublk/004 b/tests/ublk/004
>> new file mode 100755
>> index 0000000..84e01d1
>> --- /dev/null
>> +++ b/tests/ublk/004
>> @@ -0,0 +1,50 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2023 Ziyang Zhang
>> +#
>> +# Test ublk crash with delete just after daemon kill
>> +
>> +. tests/ublk/rc
>> +
>> +DESCRIPTION="test ublk crash with delete just after daemon kill"

[1]

>> +
>> +__run() {
>> +	local type=$1
>> +
>> +	if [ "$type" == "null" ]; then
>> +		${ublk_prog} add -t null -n 0 > "$FULL" 2>&1
>> +	else
>> +		truncate -s 1G "$TMPDIR/img"
>> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 > "$FULL" 2>&1
>> +	fi
>> +
>> +	udevadm settle
>> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
>> +		echo "fail to list dev"
>> +	fi
>> +
>> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
> 
> Nit: long line
> 
>> +	sleep 2
>> +
>> +	kill -9 "$(__get_ublk_daemon_pid 0)"
> 
> I think it would be the better to wait for the pid, to ensure that the ublk
> daemon process completed.

Hi Shinichiro,

As the description[1] says, this test wants to delete ublk device just after killing
the daemon. So we should not wait for the pid here.

> 
>> +
>> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
>> +}
>> +
>> +test() {
>> +	local ublk_prog="src/miniublk"
>> +
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	if ! _init_ublk; then
>> +		return 1
>> +	fi
>> +
>> +	for type in "null" "loop"; do
>> +		__run "$type"
>> +	done
>> +
>> +	_exit_ublk
>> +
>> +	echo "Test complete"
>> +}
>> diff --git a/tests/ublk/004.out b/tests/ublk/004.out
>> new file mode 100644
>> index 0000000..a92cd50
>> --- /dev/null
>> +++ b/tests/ublk/004.out
>> @@ -0,0 +1,2 @@
>> +Running ublk/004
>> +Test complete
>> diff --git a/tests/ublk/005 b/tests/ublk/005
>> new file mode 100755
>> index 0000000..f365fd6
>> --- /dev/null
>> +++ b/tests/ublk/005
>> @@ -0,0 +1,79 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2023 Ziyang Zhang
>> +#
>> +# Test ublk recovery with one time daemon kill:
>> +# (1)kill all ubq_deamon, (2)recover with new ubq_daemon,
>> +# (3)delete dev
>> +
>> +. tests/ublk/rc
>> +
>> +DESCRIPTION="test ublk recovery with one time daemon kill"
>> +
>> +__run() {
>> +	local type=$1
>> +
>> +	if [ "$type" == "null" ]; then
>> +		${ublk_prog} add -t null -n 0 -r > "$FULL" 2>&1
>> +	else
>> +		truncate -s 1G "$TMPDIR/img"
>> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 -r > "$FULL" 2>&1
>> +	fi
>> +
>> +	udevadm settle
>> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
>> +		echo "fail to list dev"
>> +	fi
>> +
>> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
> 
> Nit: long line
> 
>> +	sleep 2
>> +
>> +	kill -9 "$(__get_ublk_daemon_pid 0)"
>> +	sleep 2
>> +
>> +	local secs=0
>> +	local state=""
>> +	while [ $secs -lt 20 ]; do
>> +		state="$(__get_ublk_dev_state 0)"
>> +		[ "$state" == "QUIESCED" ] && break
>> +		sleep 1
>> +		(( secs++ ))
>> +	done
>> +
>> +	state="$(__get_ublk_dev_state 0)"
>> +	[ "$state" != "QUIESCED" ] && echo "device is $state after killing queue daemon"
> 
> Nit: long line
> 
>> +
>> +	if [ "$type" == "null" ]; then
>> +		${ublk_prog} recover -t null -n 0 >> "$FULL" 2>&1
>> +	else
>> +		${ublk_prog} recover -t loop -f "$TMPDIR/img" -n 0 >> "$FULL" 2>&1
> 
> Nit: long line
> 
>> +	fi
>> +
>> +	while [ $secs -lt 20 ]; do
>> +		state="$(__get_ublk_dev_state 0)"
>> +		[ "$state" == "LIVE" ] && break
>> +		sleep 1
>> +		(( secs++ ))
>> +	done
>> +	[ "$state" != "LIVE" ] && echo "device is $state after recovery"
>> +
>> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
>> +}
>> +
>> +test() {
>> +	local ublk_prog="src/miniublk"
>> +
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	if ! _init_ublk; then
>> +		return 1
>> +	fi
>> +
>> +	for type in "null" "loop"; do
>> +		__run "$type"
>> +	done
>> +
>> +	_exit_ublk
>> +
>> +	echo "Test complete"
>> +}
>> diff --git a/tests/ublk/005.out b/tests/ublk/005.out
>> new file mode 100644
>> index 0000000..20d7b38
>> --- /dev/null
>> +++ b/tests/ublk/005.out
>> @@ -0,0 +1,2 @@
>> +Running ublk/005
>> +Test complete
>> diff --git a/tests/ublk/006 b/tests/ublk/006
>> new file mode 100755
>> index 0000000..0848939
>> --- /dev/null
>> +++ b/tests/ublk/006
>> @@ -0,0 +1,83 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2023 Ziyang Zhang
>> +#
>> +# Test ublk recovery with two times daemon kill:
>> +# (1)kill all ubq_deamon, (2)recover with new ubq_daemon,
>> +# (3)kill all ubq_deamon, (4)delete dev
>> +
>> +. tests/ublk/rc
>> +
>> +DESCRIPTION="test ublk recovery with two times daemon kill"
>> +
>> +__run() {
>> +	local type=$1
>> +
>> +	if [ "$type" == "null" ]; then
>> +		${ublk_prog} add -t null -n 0 -r > "$FULL" 2>&1
>> +	else
>> +		truncate -s 1G "$TMPDIR/img"
>> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 -r > "$FULL" 2>&1
>> +	fi
>> +
>> +	udevadm settle
>> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
>> +		echo "fail to list dev"
>> +	fi
>> +
>> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
> 
> Nit: long line
> 
>> +	sleep 2
>> +
>> +	kill -9 "$(__get_ublk_daemon_pid 0)"
> 
> Same comment as ublk/005. How about to have the helper function
> "_kill_ublk_daemon" in tests/ublk/rc which does both kill and wait?


This test wants to make sure ublk is quiesced after killing daemon.
And the daemon must exit before ublk is quiesced. So we do not wait
for the daemon pid here.

> 
>> +	sleep 2
>> +
>> +	local secs=0
>> +	local state=""
>> +	while [ $secs -lt 20 ]; do
>> +		state="$(__get_ublk_dev_state 0)"
>> +		[ "$state" == "QUIESCED" ] && break
>> +		sleep 1
>> +		(( secs++ ))
>> +	done
>> +
>> +	state="$(__get_ublk_dev_state 0)"
>> +	[ "$state" != "QUIESCED" ] && echo "device is $state after killing queue daemon"
> 
> Nit: long line
> 
>> +
>> +	if [ "$type" == "null" ]; then
>> +		${ublk_prog} recover -t null -n 0 >> "$FULL" 2>&1
>> +	else
>> +		${ublk_prog} recover -t loop -f "$TMPDIR/img" -n 0 >> "$FULL" 2>&1
> 
> Nit: long line
> 
>> +	fi
>> +
>> +	secs=0
>> +	while [ $secs -lt 20 ]; do
>> +		state="$(__get_ublk_dev_state 0)"
>> +		[ "$state" == "LIVE" ] && break
>> +		sleep 1
>> +		(( secs++ ))
>> +	done
>> +	[ "$state" != "LIVE" ] && echo "device is $state after recovery"
>> +
>> +	kill -9 "$(__get_ublk_daemon_pid 0)"
>> +
>> +	${ublk_prog} del -n 0 >> "$FULL" 2>&1
>> +}
>> +
>> +test() {
>> +	local ublk_prog="src/miniublk"
>> +
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	if ! _init_ublk; then
>> +		return 1
>> +	fi
>> +
>> +	for type in "null" "loop"; do
>> +		__run "$type"
>> +	done
>> +
>> +	_exit_ublk
>> +
>> +	echo "Test complete"
>> +}
>> +

Re: [PATCH V2 blktests 2/2] tests: Add ublk tests

[Shinichiro Kawasaki]

On May 24, 2023 / 16:40, Ziyang Zhang wrote:
> On 2023/5/16 16:47, Shinichiro Kawasaki wrote:
> > On May 05, 2023 / 11:28, Ziyang Zhang wrote:
> >> It is very important to test ublk crash handling since the userspace
> >> part is not reliable. Especially we should test removing device, killing
> >> ublk daemons and user recovery feature.
> >>
> >> Add five new tests for ublk to cover these cases.
> >>
> >> Signed-off-by: Ziyang Zhang <ZiyangZhang@linux.alibaba.com>
> >> ---
> 
> [...]
> 
> >> diff --git a/tests/ublk/004 b/tests/ublk/004
> >> new file mode 100755
> >> index 0000000..84e01d1
> >> --- /dev/null
> >> +++ b/tests/ublk/004
> >> @@ -0,0 +1,50 @@
> >> +#!/bin/bash
> >> +# SPDX-License-Identifier: GPL-3.0+
> >> +# Copyright (C) 2023 Ziyang Zhang
> >> +#
> >> +# Test ublk crash with delete just after daemon kill
> >> +
> >> +. tests/ublk/rc
> >> +
> >> +DESCRIPTION="test ublk crash with delete just after daemon kill"
> 
> [1]
> 
> >> +
> >> +__run() {
> >> +	local type=$1
> >> +
> >> +	if [ "$type" == "null" ]; then
> >> +		${ublk_prog} add -t null -n 0 > "$FULL" 2>&1
> >> +	else
> >> +		truncate -s 1G "$TMPDIR/img"
> >> +		${ublk_prog} add -t loop -f "$TMPDIR/img" -n 0 > "$FULL" 2>&1
> >> +	fi
> >> +
> >> +	udevadm settle
> >> +	if ! ${ublk_prog} list -n 0 >> "$FULL" 2>&1; then
> >> +		echo "fail to list dev"
> >> +	fi
> >> +
> >> +	_run_fio_rand_io --filename=/dev/ublkb0 --time_based --runtime=30 >> "$FULL" 2>&1 &
> > 
> > Nit: long line
> > 
> >> +	sleep 2
> >> +
> >> +	kill -9 "$(__get_ublk_daemon_pid 0)"
> > 
> > I think it would be the better to wait for the pid, to ensure that the ublk
> > daemon process completed.
> 
> Hi Shinichiro,
> 
> As the description[1] says, this test wants to delete ublk device just after killing
> the daemon. So we should not wait for the pid here.

Thanks for the clarification. I missed that point :)