[ISS2062] fix(vol_destroy): race fixed between IO receiver and ack sender threads (#130)

Signed-off-by: Vishnu Itta <[email protected]>

Author: vishnuitta

include/zrepl_mgmt.h

@@ -72,6 +72,7 @@ struct zvol_io_cmd_s;
 typedef struct inject_delay_s {
 	int helping_replica_rebuild_step;
 	int pre_uzfs_write_data;
+	int io_receiver_exit;
 } inject_delay_t;
 
 typedef struct inject_error_s {
@@ -96,6 +97,11 @@ typedef struct zvol_info_s {
 	zvol_state_t	*zv;
 	uint64_t	refcnt;
 
+	/*
+	 * While checking for these big flags, do as below,
+	 * if (zinfo->is_io_ack_sender_created) (or)
+	 * if (!zinfo->is_io_ack_sender_created)
+	 */
 	union {
 		struct {
 			int	is_io_ack_sender_created	: 1;

lib/libzpool/zrepl_mgmt.c

@@ -213,8 +213,8 @@ uzfs_mark_offline_and_free_zinfo(zvol_info_t *zinfo)
 
 	/* Wait for refcounts to be drained */
 	while (zinfo->refcnt > 0) {
-		LOG_DEBUG("Waiting for refcount to go down to"
-		    " zero on zvol:%s", zinfo->name);
+		LOG_INFO("Waiting for refcount (%d) to go down to"
+		    " zero on zvol:%s", zinfo->refcnt, zinfo->name);
 		sleep(5);
 	}
 

lib/libzrepl/data_conn.c

@@ -276,7 +276,7 @@ uzfs_zvol_worker(void *arg)
 	rebuild_cmd_req = hdr->flags & ZVOL_OP_FLAG_REBUILD;
 	read_metadata = hdr->flags & ZVOL_OP_FLAG_READ_METADATA;
 
-	if (zinfo->is_io_ack_sender_created == B_FALSE) {
+	if (!zinfo->is_io_ack_sender_created) {
 		if (!(rebuild_cmd_req && (hdr->opcode == ZVOL_OPCODE_WRITE)))
 			zio_cmd_free(&zio_cmd);
 		if (hdr->opcode == ZVOL_OPCODE_WRITE)
@@ -992,7 +992,7 @@ uzfs_zvol_rebuild_scanner_callback(off_t offset, size_t len,
 
 	while (1) {
 		if ((zinfo->state == ZVOL_INFO_STATE_OFFLINE) ||
-		    (zinfo->is_io_ack_sender_created == B_FALSE))
+		    (!zinfo->is_io_ack_sender_created))
 			return (-1);
 		if (IS_REBUILD_HIT_MAX_CMD_LIMIT(zinfo))
 			usleep(100);
@@ -1043,14 +1043,14 @@ uzfs_zvol_rebuild_scanner(void *arg)
 read_socket:
 	if ((zinfo != NULL) &&
 	    ((zinfo->state == ZVOL_INFO_STATE_OFFLINE) ||
-	    (zinfo->is_io_ack_sender_created == B_FALSE)))
+	    (!zinfo->is_io_ack_sender_created)))
 		goto exit;
 
 	rc = uzfs_zvol_read_header(fd, &hdr);
 	if ((rc != 0) ||
 	    ((zinfo != NULL) &&
 	    ((zinfo->state == ZVOL_INFO_STATE_OFFLINE) ||
-	    (zinfo->is_io_ack_sender_created == B_FALSE))))
+	    (!zinfo->is_io_ack_sender_created))))
 		goto exit;
 
 	LOG_DEBUG("op_code=%d io_seq=%ld", hdr.opcode, hdr.io_seq);
@@ -1351,19 +1351,14 @@ uzfs_zvol_io_ack_sender(void *arg)
 exit:
 	zinfo->zio_cmd_in_ack = NULL;
 	shutdown(fd, SHUT_RDWR);
-	LOG_INFO("Data connection for zvol %s closed on fd: %d",
-	    zinfo->name, fd);
-
-	(void) pthread_mutex_lock(&zinfo->zinfo_mutex);
-	zinfo->is_io_ack_sender_created = B_FALSE;
-	(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);
-
-	remove_pending_cmds_to_ack(fd, zinfo);
 
 	(void) pthread_mutex_lock(&zinfo->zinfo_mutex);
+	zinfo->is_io_ack_sender_created = 0;
 	zinfo->conn_closed = B_FALSE;
 	(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);
 
+	LOG_INFO("Data connection for zvol %s closed on fd: %d",
+	    zinfo->name, fd);
 	uzfs_zinfo_drop_refcnt(zinfo);
 
 	zk_thread_exit();
@@ -1424,14 +1419,14 @@ open_zvol(int fd, zvol_info_t **zinfopp)
 		(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);
 		goto open_reply;
 	}
-	if (zinfo->is_io_ack_sender_created != B_FALSE) {
+	if (zinfo->is_io_ack_sender_created) {
 		LOG_ERR("zvol %s ack sender already present",
 		    open_data.volname);
 		hdr.status = ZVOL_OP_STATUS_FAILED;
 		(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);
 		goto open_reply;
 	}
-	if (zinfo->is_io_receiver_created != B_FALSE) {
+	if (zinfo->is_io_receiver_created) {
 		LOG_ERR("zvol %s io receiver already present",
 		    open_data.volname);
 		hdr.status = ZVOL_OP_STATUS_FAILED;
@@ -1498,8 +1493,8 @@ open_zvol(int fd, zvol_info_t **zinfopp)
 	*zinfopp = zinfo;
 
 	zinfo->conn_closed = B_FALSE;
-	zinfo->is_io_ack_sender_created = B_TRUE;
-	zinfo->is_io_receiver_created = B_TRUE;
+	zinfo->is_io_ack_sender_created = 1;
+	zinfo->is_io_receiver_created = 1;
 	(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);
 	thrd_arg = kmem_alloc(sizeof (thread_args_t), KM_SLEEP);
 	thrd_arg->fd = fd;
@@ -1602,8 +1597,15 @@ uzfs_zvol_io_receiver(void *arg)
 		    zio_cmd, TQ_SLEEP);
 	}
 exit:
+#if DEBUG
+	if (inject_error.delay.io_receiver_exit == 1)
+		sleep(5);
+#endif
 	(void) pthread_mutex_lock(&zinfo->zinfo_mutex);
-	zinfo->conn_closed = B_TRUE;
+
+	if (zinfo->is_io_ack_sender_created)
+		zinfo->conn_closed = B_TRUE;
+
 	/*
 	 * Send signal to ack sender so that it can free
 	 * zio_cmd, close fd and exit.
@@ -1617,18 +1619,23 @@ uzfs_zvol_io_receiver(void *arg)
 	 */
 	while (zinfo->conn_closed || zinfo->is_io_ack_sender_created) {
 		(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);
-		usleep(1000);
+		LOG_INFO("Waiting for conn_closed (%d) and ack_sender (%d)"
+		    "to be false for %s", zinfo->conn_closed,
+		    zinfo->is_io_ack_sender_created, zinfo->name);
+		sleep(1);
 		(void) pthread_mutex_lock(&zinfo->zinfo_mutex);
 	}
 	(void) pthread_mutex_unlock(&zinfo->zinfo_mutex);
 
+	remove_pending_cmds_to_ack(fd, zinfo);
+
 	shutdown_fds_related_to_zinfo(zinfo);
 
 	zinfo->io_ack_waiting = 0;
 
 	taskq_wait(zinfo->uzfs_zvol_taskq);
 	reinitialize_zv_state(zinfo->zv);
-	zinfo->is_io_receiver_created = B_FALSE;
+	zinfo->is_io_receiver_created = 0;
 	uzfs_zinfo_drop_refcnt(zinfo);
 thread_exit:
 	close(fd);

tests/cbtest/gtest/test_uzfs.cc

@@ -37,6 +37,7 @@
 #include <mgmt_conn.h>
 #include <data_conn.h>
 #include <uzfs_mgmt.h>
+#include <sys/eventfd.h>
 #include <sys/epoll.h>
 
 #include <uzfs_rebuilding.h>
@@ -45,14 +46,17 @@
 
 char *ds_name;
 char *ds_name2;
+char *ds_name_todelete;
 char *pool;
 spa_t *spa;
 zvol_state_t *zv;
 zvol_state_t *zv2;
+zvol_state_t *zv_todelete;
 zvol_info_t *zinfo;
 zvol_info_t *zinfo2;
 int rebuild_test_case = 0;
 int data_conn_fd = -1;
+int data_conn_fd_todelete = -1;
 
 extern void (*zinfo_create_hook)(zvol_info_t *, nvlist_t *);
 extern void (*zinfo_destroy_hook)(zvol_info_t *);
@@ -202,7 +206,7 @@ uzfs_mock_rebuild_scanner(void *arg)
 
 	if (rebuild_test_case == 6) {
 		close(data_conn_fd);
-		while (zinfo->is_io_receiver_created == B_TRUE)
+		while (zinfo->is_io_receiver_created)
 			sleep(2);
 		sleep(5);
 	}
@@ -257,25 +261,31 @@ TEST(uZFS, Setup) {
 	int ret;
 	char *pool_ds;
 	char *pool_ds2;
+	char *pool_ds_todelete;
 	ds_name = (char *)malloc(MAXNAMELEN);
 	ds_name2 = (char *)malloc(MAXNAMELEN);
+	ds_name_todelete = (char *)malloc(MAXNAMELEN);
 	pool_ds = (char *)malloc(MAXNAMELEN);
 	pool_ds2 = (char *)malloc(MAXNAMELEN);
+	pool_ds_todelete = (char *)malloc(MAXNAMELEN);
 	path = (char *)malloc(MAXNAMELEN);
 	pool = (char *)malloc(MAXNAMELEN);
 
 	GtestUtils::strlcpy(path, "/tmp/uztest.1a", MAXNAMELEN);
 	GtestUtils::strlcpy(pool, "pool1", MAXNAMELEN);
 	GtestUtils::strlcpy(ds_name, "vol1", MAXNAMELEN);
 	GtestUtils::strlcpy(ds_name2, "vol3", MAXNAMELEN);
+	GtestUtils::strlcpy(ds_name_todelete, "vol_todelete", MAXNAMELEN);
 	GtestUtils::strlcpy(pool_ds, "pool1/vol1", MAXNAMELEN);
 	GtestUtils::strlcpy(pool_ds2, "pool1/vol3", MAXNAMELEN);
+	GtestUtils::strlcpy(pool_ds_todelete, "pool1/vol_todelete", MAXNAMELEN);
 	signal(SIGPIPE, SIG_IGN);
 
 	mutex_init(&conn_list_mtx, NULL, MUTEX_DEFAULT, NULL);
 	SLIST_INIT(&uzfs_mgmt_conns);
 	mutex_init(&async_tasks_mtx, NULL, MUTEX_DEFAULT, NULL);
-	mgmt_eventfd = -1;
+
+	mgmt_eventfd = eventfd(0, EFD_NONBLOCK);
 
 	uzfs_init();
 	init_zrepl();
@@ -306,6 +316,12 @@ TEST(uZFS, Setup) {
 	zinfo2 = uzfs_zinfo_lookup(ds_name2);
 	EXPECT_EQ(0, !zinfo2);
 
+	uzfs_create_dataset(spa, ds_name_todelete, 1024*1024*1024, 512, &zv_todelete);
+	uzfs_hold_dataset(zv_todelete);
+	uzfs_update_metadata_granularity(zv_todelete, 512);
+	strncpy(zv_todelete->zv_target_host,"127.0.0.1:5959", MAXNAMELEN);
+	uzfs_zinfo_init(zv_todelete, pool_ds_todelete, NULL);
+
 	uzfs_zinfo_init(zv, pool_ds, NULL);
 	zinfo = uzfs_zinfo_lookup(ds_name);
 	EXPECT_EQ(0, !zinfo);
@@ -416,12 +432,12 @@ TEST(uZFS, asyncTaskProps) {
 TEST(uZFS, EmptyCreateProps) {
 	uzfs_mgmt_conn_t *conn;
 
-	EXPECT_EQ(2, uzfs_mgmt_conn_list_count(&uzfs_mgmt_conns));
+	EXPECT_EQ(3, uzfs_mgmt_conn_list_count(&uzfs_mgmt_conns));
 	conn = SLIST_FIRST(&uzfs_mgmt_conns);
 	EXPECT_EQ(1, conn->conn_refcount);
 
 	zinfo_create_cb(zinfo, NULL);
-	EXPECT_EQ(2, uzfs_mgmt_conn_list_count(&uzfs_mgmt_conns));
+	EXPECT_EQ(3, uzfs_mgmt_conn_list_count(&uzfs_mgmt_conns));
 	conn = SLIST_FIRST(&uzfs_mgmt_conns);
 	EXPECT_EQ(2, conn->conn_refcount);
 }
@@ -514,7 +530,7 @@ TEST(uZFS, TestStartRebuild) {
 	rebuild_status[3] = ZVOL_REBUILDING_ERRORED;
 	rebuild_status[4] = ZVOL_REBUILDING_FAILED;
 
-	EXPECT_EQ(2, uzfs_mgmt_conn_list_count(&uzfs_mgmt_conns));
+	EXPECT_EQ(3, uzfs_mgmt_conn_list_count(&uzfs_mgmt_conns));
 	EXPECT_EQ(2, zinfo->refcnt);
 	conn = SLIST_FIRST(&uzfs_mgmt_conns);
 
@@ -1174,6 +1190,23 @@ static void do_data_connection(int &data_fd, std::string host, uint16_t port,
 	data_fd = fd;
 }
 
+/*
+ * This testcase is to let ack sender to exit before io_receiver to detect any races
+ */
+TEST(uZFS, CreateAndDestroyDelayIOReceiverExit) {
+	io_receiver = &uzfs_zvol_io_receiver;
+	do_data_connection(data_conn_fd_todelete, "127.0.0.1", IO_SERVER_PORT, "vol_todelete");
+#if DEBUG
+	inject_error.delay.io_receiver_exit = 1;
+#endif
+	close(data_conn_fd_todelete);
+	uzfs_zinfo_destroy("pool1/vol_todelete", spa);
+#if DEBUG
+	sleep(5);
+	inject_error.delay.io_receiver_exit = 0;
+#endif
+}
+
 TEST(uZFS, TestRebuildCompleteWithDataConn) {
 	io_receiver = &uzfs_zvol_io_receiver;
 
@@ -1191,7 +1224,7 @@ TEST(uZFS, TestRebuildComplete) {
 	EXPECT_EQ(ZVOL_STATUS_HEALTHY, uzfs_zvol_get_status(zinfo->zv));
 
 	close(data_conn_fd);
-	while (zinfo->is_io_receiver_created == B_TRUE)
+	while (zinfo->is_io_receiver_created)
 		sleep(2);
 	sleep(5);
 	memset(&zinfo->zv->rebuild_info, 0, sizeof (zvol_rebuild_info_t));