From 655e8e16e35bcf52bf74680ef73fc16433569398 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Wed, 15 Jan 2020 00:07:48 -0500 Subject: [PATCH] lib/thread: Assert if spdk_put_io_channel() is called on the wrong thread spdk_put_io_channel() was designed to be called on the same thread that called spdk_get_io_channel(). spdk_put_io_channel() sends a message to its own thread, to allow the context to unwind before releasing the resources. This had the side effect to allow an incorrect thread to call spdk_put_io_channel(). This patch will fix that. Bdevperf tool had a design flaw that needed the side effect, but it was fixed recently. We do not know if we have any other case. Hence add assert to spdk_put_io_channel() to find other case. We found that unit test for blobstore had called spdk_put_io_channel() and fix it together in this patch. Besides, correct the comment for spdk_put_io_channel() in include/spdk/thread.h not to create any other case in future. Signed-off-by: Shuhei Matsumoto Change-Id: I6ec7bf074818abef43b23ca40bc9385adac70a75 Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/479390 Community-CI: SPDK CI Jenkins Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Alexey Marchuk Reviewed-by: Ben Walker Reviewed-by: Changpeng Liu --- include/spdk/thread.h | 2 +- lib/thread/thread.c | 23 +++++++++++++++++++---- test/unit/lib/blob/blob.c/blob_ut.c | 2 ++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/include/spdk/thread.h b/include/spdk/thread.h index b68a73ea6..ca895f673 100644 --- a/include/spdk/thread.h +++ b/include/spdk/thread.h @@ -506,7 +506,7 @@ struct spdk_io_channel *spdk_get_io_channel(void *io_device); /** * Release a reference to an I/O channel. This happens asynchronously. * - * Actual release will happen on the same thread that called spdk_get_io_channel() + * This must be called on the same thread that called spdk_get_io_channel() * for the specified I/O channel. If this releases the last reference to the * I/O channel, The destroy_cb function specified in spdk_io_device_register() * will be invoked to release any associated resources. diff --git a/lib/thread/thread.c b/lib/thread/thread.c index 136e92bcb..b6e855037 100644 --- a/lib/thread/thread.c +++ b/lib/thread/thread.c @@ -1200,8 +1200,8 @@ _spdk_put_io_channel(void *arg) } SPDK_DEBUGLOG(SPDK_LOG_THREAD, - "Releasing io_channel %p for io_device %s (%p). Channel thread %p. Current thread %s\n", - ch, ch->dev->name, ch->dev->io_device, ch->thread, thread->name); + "Releasing io_channel %p for io_device %s (%p) on thread %s\n", + ch, ch->dev->name, ch->dev->io_device, thread->name); assert(ch->thread == thread); @@ -1245,15 +1245,30 @@ _spdk_put_io_channel(void *arg) void spdk_put_io_channel(struct spdk_io_channel *ch) { + struct spdk_thread *thread; + + thread = spdk_get_thread(); + if (!thread) { + SPDK_ERRLOG("called from non-SPDK thread\n"); + assert(false); + return; + } + + if (ch->thread != thread) { + SPDK_ERRLOG("different from the thread that called get_io_channel()\n"); + assert(false); + return; + } + SPDK_DEBUGLOG(SPDK_LOG_THREAD, "Putting io_channel %p for io_device %s (%p) on thread %s refcnt %u\n", - ch, ch->dev->name, ch->dev->io_device, ch->thread->name, ch->ref); + ch, ch->dev->name, ch->dev->io_device, thread->name, ch->ref); ch->ref--; if (ch->ref == 0) { ch->destroy_ref++; - spdk_thread_send_msg(ch->thread, _spdk_put_io_channel, ch); + spdk_thread_send_msg(thread, _spdk_put_io_channel, ch); } } diff --git a/test/unit/lib/blob/blob.c/blob_ut.c b/test/unit/lib/blob/blob.c/blob_ut.c index 355c891b7..4104fb0e8 100644 --- a/test/unit/lib/blob/blob.c/blob_ut.c +++ b/test/unit/lib/blob/blob.c/blob_ut.c @@ -4772,7 +4772,9 @@ blob_thin_prov_rw(void) CU_ASSERT(g_bserrno == 0); CU_ASSERT(free_clusters == spdk_bs_free_cluster_count(bs)); + set_thread(1); spdk_bs_free_io_channel(channel_thread1); + set_thread(0); spdk_bs_free_io_channel(channel); poll_threads();