From 979d9997c9a800c85afe2e8d0d384ede62158101 Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Fri, 13 Apr 2018 14:46:05 -0700 Subject: [PATCH] io_channel: Fix race between put channel and get channel Previously, there was a period of time where the user had put the last reference to a channel, but when calling to get a new channel would end up with the previously created channel and channel destruction would never execute. This causes a number of unexpected issues to pop up, as often the creation and destruction of a channel is a key event. Change-Id: Ie68135f6efdf45093a5a227f8c51cd11b971fcff Signed-off-by: Ben Walker Reviewed-on: https://review.gerrithub.io/407602 Reviewed-by: Jim Harris Reviewed-by: Daniel Verkamp Tested-by: SPDK Automated Test System --- lib/util/io_channel.c | 65 +++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 37 deletions(-) diff --git a/lib/util/io_channel.c b/lib/util/io_channel.c index efb9c6692..8643bfce3 100644 --- a/lib/util/io_channel.c +++ b/lib/util/io_channel.c @@ -56,6 +56,8 @@ struct io_device { uint32_t for_each_count; TAILQ_ENTRY(io_device) tailq; + uint32_t refcnt; + bool unregistered; }; @@ -314,6 +316,7 @@ spdk_io_device_register(void *io_device, spdk_io_channel_create_cb create_cb, dev->ctx_size = ctx_size; dev->for_each_count = 0; dev->unregistered = false; + dev->refcnt = 0; pthread_mutex_lock(&g_devlist_mutex); TAILQ_FOREACH(tmp, &g_io_devices, tailq) { @@ -338,34 +341,20 @@ _finish_unregister(void *arg) } static void -_spdk_io_device_attempt_free(struct io_device *dev, struct spdk_io_channel *ch) +_spdk_io_device_attempt_free(struct io_device *dev) { - struct spdk_thread *thread; - pthread_mutex_lock(&g_devlist_mutex); - if (ch != NULL) { - TAILQ_REMOVE(&ch->thread->io_channels, ch, tailq); - } - if (!dev->unregistered) { pthread_mutex_unlock(&g_devlist_mutex); return; } - TAILQ_FOREACH(thread, &g_threads, tailq) { - /* ch parameter is no longer needed, so use that variable here for iterating. */ - TAILQ_FOREACH(ch, &thread->io_channels, tailq) { - if (ch->dev == dev) { - /* A channel that references this I/O - * device still exists. Defer deletion - * until it is removed. - */ - pthread_mutex_unlock(&g_devlist_mutex); - return; - } - } + if (dev->refcnt > 0) { + pthread_mutex_unlock(&g_devlist_mutex); + return; } + pthread_mutex_unlock(&g_devlist_mutex); if (dev->unregister_cb == NULL) { @@ -405,7 +394,7 @@ spdk_io_device_unregister(void *io_device, spdk_io_device_unregister_cb unregist TAILQ_REMOVE(&g_io_devices, dev, tailq); pthread_mutex_unlock(&g_devlist_mutex); dev->unregister_thread = spdk_get_thread(); - _spdk_io_device_attempt_free(dev, NULL); + _spdk_io_device_attempt_free(dev); } struct spdk_io_channel * @@ -460,12 +449,15 @@ spdk_get_io_channel(void *io_device) ch->ref = 1; TAILQ_INSERT_TAIL(&thread->io_channels, ch, tailq); + dev->refcnt++; + pthread_mutex_unlock(&g_devlist_mutex); rc = dev->create_cb(io_device, (uint8_t *)ch + sizeof(*ch)); if (rc == -1) { pthread_mutex_lock(&g_devlist_mutex); TAILQ_REMOVE(&ch->thread->io_channels, ch, tailq); + dev->refcnt--; free(ch); pthread_mutex_unlock(&g_devlist_mutex); return NULL; @@ -480,32 +472,31 @@ _spdk_put_io_channel(void *arg) struct spdk_io_channel *ch = arg; assert(ch->thread == spdk_get_thread()); - - if (ch->ref == 0) { - SPDK_ERRLOG("ref already zero\n"); - return; - } - - ch->ref--; - - if (ch->ref > 0) { - return; - } + assert(ch->ref == 0); ch->destroy_cb(ch->dev->io_device, spdk_io_channel_get_ctx(ch)); - /* - * _spdk_io_device_attempt_free() will remove the ch from the thread after it - * acquires the g_devlist_mutex lock. - */ - _spdk_io_device_attempt_free(ch->dev, ch); + pthread_mutex_lock(&g_devlist_mutex); + ch->dev->refcnt--; + pthread_mutex_unlock(&g_devlist_mutex); + + _spdk_io_device_attempt_free(ch->dev); free(ch); } void spdk_put_io_channel(struct spdk_io_channel *ch) { - spdk_thread_send_msg(ch->thread, _spdk_put_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); + } } struct spdk_io_channel *