thread: don't immediately remove channel from list when put

When spdk_put_io_channel is called, if its the last reference,
we defer actual destruction of the channel, so that code
in the same context which may be referring to the channel
doesn't crash.

But it is possible that an io_channel for that same io_device
could be requested before the deferred message is processed.
This would result in a second io_channel being created for
that device on the same thread.

To avoid this case, don't immediately remove the channel from
the list when the last reference is put.  When the deferred
message is processed, if additional references were allocated
in the meantime, don't destroy the channel.

Signed-off-by: Jim Harris <james.r.harris@intel.com>
Signed-off-by: Ziye Yang <ziye.yang@intel.com>
Change-Id: Idb8d4705fda0eb9c338e4960430e04edbe537e05
Reviewed-on: https://review.gerrithub.io/418878
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Chandler-Test-Pool: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: GangCao <gang.cao@intel.com>
Reviewed-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>

lib/bdev/bdev.c

@@ -1100,10 +1100,11 @@ spdk_bdev_io_submit(struct spdk_bdev_io *bdev_io)
 	struct spdk_bdev *bdev = bdev_io->bdev;
 	struct spdk_thread *thread = spdk_io_channel_get_thread(bdev_io->internal.ch->channel);
 
+	assert(thread != NULL);
 	assert(bdev_io->internal.status == SPDK_BDEV_IO_STATUS_PENDING);
 
 	if (bdev_io->internal.ch->flags & BDEV_CH_QOS_ENABLED) {
-		if (thread == bdev->internal.qos->thread) {
+		if ((thread == bdev->internal.qos->thread) || !bdev->internal.qos->thread) {
 			_spdk_bdev_io_submit(bdev_io);
 		} else {
 			bdev_io->internal.io_submit_ch = bdev_io->internal.ch;
@@ -1487,8 +1488,12 @@ spdk_bdev_qos_destroy(struct spdk_bdev *bdev)
 
 	bdev->internal.qos = new_qos;
 
-	spdk_thread_send_msg(old_qos->thread, spdk_bdev_qos_channel_destroy,
-			     old_qos);
+	if (old_qos->thread == NULL) {
+		free(old_qos);
+	} else {
+		spdk_thread_send_msg(old_qos->thread, spdk_bdev_qos_channel_destroy,
+				     old_qos);
+	}
 
 	/* It is safe to continue with destroying the bdev even though the QoS channel hasn't
 	 * been destroyed yet. The destruction path will end up waiting for the final

lib/thread/thread.c

@@ -490,8 +490,21 @@ _spdk_put_io_channel(void *arg)
 	bool do_remove_dev = true;
 
 	assert(ch->thread == spdk_get_thread());
-	assert(ch->ref == 0);
 
+	if (ch->ref > 0) {
+		/*
+		 * Another reference to the associated io_device was requested
+		 *  after this message was sent but before it had a chance to
+		 *  execute.
+		 */
+		return;
+	}
+
+	pthread_mutex_lock(&g_devlist_mutex);
+	TAILQ_REMOVE(&ch->thread->io_channels, ch, tailq);
+	pthread_mutex_unlock(&g_devlist_mutex);
+
+	/* Don't hold the devlist mutex while the destroy_cb is called. */
 	ch->destroy_cb(ch->dev->io_device, spdk_io_channel_get_ctx(ch));
 
 	pthread_mutex_lock(&g_devlist_mutex);
@@ -519,11 +532,6 @@ spdk_put_io_channel(struct spdk_io_channel *ch)
 	ch->ref--;
 
 	if (ch->ref == 0) {
-		/* If this was the last reference, remove the channel from the list */
-		pthread_mutex_lock(&g_devlist_mutex);
-		TAILQ_REMOVE(&ch->thread->io_channels, ch, tailq);
-		pthread_mutex_unlock(&g_devlist_mutex);
-
 		spdk_thread_send_msg(ch->thread, _spdk_put_io_channel, ch);
 	}
 }