Commit a7aa9d57 authored by Dariusz Stojaczyk's avatar Dariusz Stojaczyk Committed by Daniel Verkamp
Browse files

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: default avatarDariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/408193


Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
parent 6c007830

lib/util/io_channel.c

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

}