Commit 655e8e16 authored by Shuhei Matsumoto's avatar Shuhei Matsumoto Committed by Jim Harris
Browse files

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: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: I6ec7bf074818abef43b23ca40bc9385adac70a75
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/479390


Community-CI: SPDK CI Jenkins <sys_sgci@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarAlexey Marchuk <alexeymar@mellanox.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarChangpeng Liu <changpeng.liu@intel.com>
parent 05e43665

include/spdk/thread.h

+1 −1
Original line number Diff line number Diff line
@@ -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.

lib/thread/thread.c

+19 −4
Original line number Diff line number Diff line
@@ -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);

	}

}

test/unit/lib/blob/blob.c/blob_ut.c

+2 −0
Original line number Diff line number Diff line
@@ -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();