From a7aa9d5737c96e3f9c0ad7ba93b16a85417186ed Mon Sep 17 00:00:00 2001 From: Dariusz Stojaczyk Date: Wed, 18 Apr 2018 14:33:52 +0200 Subject: [PATCH] io_channel: ensure io_device is always freed just once Imagine the following code flow: 1. `spdk_put_io_channel();` 2. `spdk_io_device_unregister();` Since putting an io_channel is always deferred, it is likely that spdk_io_device_unregister will lock the io_channel mutex first. It will set dev->unregistered flag and then quickly realize there are still open channels for this device (refcnt > 0) - so it'll return. All fine here. However, if the deferred put_io_channel happens to lock the mutex first from other thread, it will decrement the refcnt and attempt to free the device. Both the decrementation and freeing are done under a mutex, but there is a slight window inbetween where the mutex is re-locked. spdk_io_device_unregister is already sleeping on a mutex_lock(), so it might strike now. It'll see there are no more io_channels for this device and free the device. Once put_io_channels regains the lock again, it will attempt freeing the device for a second time. This patch removes the slight window mentioned above. Related to #278 Change-Id: I6c0f6014353529028d658211135196d97f1d8547 Signed-off-by: Dariusz Stojaczyk Reviewed-on: https://review.gerrithub.io/408193 Reviewed-by: Jim Harris Reviewed-by: Daniel Verkamp Reviewed-by: Ben Walker Tested-by: SPDK Automated Test System --- lib/util/io_channel.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/lib/util/io_channel.c b/lib/util/io_channel.c index 8643bfce3..b1d4e1597 100644 --- a/lib/util/io_channel.c +++ b/lib/util/io_channel.c @@ -341,22 +341,8 @@ _finish_unregister(void *arg) } static void -_spdk_io_device_attempt_free(struct io_device *dev) +_spdk_io_device_free(struct io_device *dev) { - pthread_mutex_lock(&g_devlist_mutex); - - if (!dev->unregistered) { - 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) { free(dev); } else { @@ -369,6 +355,7 @@ void spdk_io_device_unregister(void *io_device, spdk_io_device_unregister_cb unregister_cb) { struct io_device *dev; + uint32_t refcnt; pthread_mutex_lock(&g_devlist_mutex); TAILQ_FOREACH(dev, &g_io_devices, tailq) { @@ -392,9 +379,16 @@ spdk_io_device_unregister(void *io_device, spdk_io_device_unregister_cb unregist dev->unregister_cb = unregister_cb; dev->unregistered = true; TAILQ_REMOVE(&g_io_devices, dev, tailq); + refcnt = dev->refcnt; pthread_mutex_unlock(&g_devlist_mutex); dev->unregister_thread = spdk_get_thread(); - _spdk_io_device_attempt_free(dev); + + if (refcnt > 0) { + /* defer deletion */ + return; + } + + _spdk_io_device_free(dev); } struct spdk_io_channel * @@ -470,6 +464,7 @@ static void _spdk_put_io_channel(void *arg) { struct spdk_io_channel *ch = arg; + bool do_remove_dev = true; assert(ch->thread == spdk_get_thread()); assert(ch->ref == 0); @@ -478,9 +473,20 @@ _spdk_put_io_channel(void *arg) pthread_mutex_lock(&g_devlist_mutex); ch->dev->refcnt--; + + if (!ch->dev->unregistered) { + do_remove_dev = false; + } + + if (ch->dev->refcnt > 0) { + do_remove_dev = false; + } + pthread_mutex_unlock(&g_devlist_mutex); - _spdk_io_device_attempt_free(ch->dev); + if (do_remove_dev) { + _spdk_io_device_free(ch->dev); + } free(ch); }