From 130d278adf62d046ab88392f2fcc9dabf318b484 Mon Sep 17 00:00:00 2001 From: Paul Luse Date: Sun, 10 Sep 2017 14:46:40 -0700 Subject: [PATCH] blob: defer calling unload callback until dev is destroyed Fixes condition where blobstore was prematurely calling the application callback on spdk_bs_unload(), if the application tries to do something too quickly bad things happen. To avoid application changes with how the g_devlist_mutex is held, it is no longer held while calling _spdk_io_device_attempt_free() because the app unload CB is called from that function and may want to call spdk_io_device_unregister() from its unload CB. So the lock is now held and releases strictly around the list its protecting which allows the CB from _spdk_io_device_attempt_free() to be called without issue. Change-Id: Ib451cfe6b33ea0c3f9e66c86785316f9d88837c7 Signed-off-by: Paul Luse Reviewed-on: https://review.gerrithub.io/377872 Reviewed-by: Daniel Verkamp Reviewed-by: Jim Harris Tested-by: SPDK Automated Test System --- lib/blob/blobstore.c | 15 ++++++++++++++- lib/blob/blobstore.h | 5 +++++ lib/blob/request.c | 3 +++ lib/blob/request.h | 1 + lib/util/io_channel.c | 10 +++++----- test/unit/lib/blob/blob.c/blob_ut.c | 15 +++------------ 6 files changed, 31 insertions(+), 18 deletions(-) diff --git a/lib/blob/blobstore.c b/lib/blob/blobstore.c index b92d77514e..4535beb62d 100644 --- a/lib/blob/blobstore.c +++ b/lib/blob/blobstore.c @@ -44,7 +44,6 @@ #include "spdk_internal/log.h" #include "blobstore.h" -#include "request.h" #define BLOB_CRC32C_INITIAL 0xffffffffUL @@ -1342,6 +1341,12 @@ _spdk_bs_dev_destroy(void *io_device) spdk_bit_array_free(&bs->used_md_pages); spdk_bit_array_free(&bs->used_clusters); + /* + * If this function is called for any reason except a successful unload, + * the unload_cpl type will be NONE and this will be a nop. + */ + spdk_bs_call_cpl(&bs->unload_cpl, bs->unload_err); + free(bs); } @@ -1833,6 +1838,14 @@ _spdk_bs_unload_write_super_cpl(spdk_bs_sequence_t *seq, void *cb_arg, int bserr spdk_dma_free(ctx->super); + /* + * We need to defer calling spdk_bs_call_cpl() until after + * dev destuction, so tuck these away for later use. + */ + ctx->bs->unload_err = bserrno; + memcpy(&ctx->bs->unload_cpl, &seq->cpl, sizeof(struct spdk_bs_cpl)); + seq->cpl.type = SPDK_BS_CPL_TYPE_NONE; + spdk_bs_sequence_finish(seq, bserrno); _spdk_bs_free(ctx->bs); diff --git a/lib/blob/blobstore.h b/lib/blob/blobstore.h index 8c955ca448..a8ef2c2511 100644 --- a/lib/blob/blobstore.h +++ b/lib/blob/blobstore.h @@ -39,6 +39,8 @@ #include "spdk/queue.h" #include "spdk/util.h" +#include "request.h" + /* In Memory Data Structures * * The following data structures exist only in memory. @@ -156,6 +158,9 @@ struct spdk_blob_store { spdk_blob_id super_blob; + struct spdk_bs_cpl unload_cpl; + int unload_err; + TAILQ_HEAD(, spdk_blob) blobs; }; diff --git a/lib/blob/request.c b/lib/blob/request.c index 153f324458..a422528bf1 100644 --- a/lib/blob/request.c +++ b/lib/blob/request.c @@ -73,6 +73,9 @@ spdk_bs_call_cpl(struct spdk_bs_cpl *cpl, int bserrno) cpl->u.nested_seq.parent, bserrno); break; + case SPDK_BS_CPL_TYPE_NONE: + /* this completion's callback is handled elsewhere */ + break; } } diff --git a/lib/blob/request.h b/lib/blob/request.h index 67fdd0605c..17e258906c 100644 --- a/lib/blob/request.h +++ b/lib/blob/request.h @@ -39,6 +39,7 @@ #include "spdk/blob.h" enum spdk_bs_cpl_type { + SPDK_BS_CPL_TYPE_NONE, SPDK_BS_CPL_TYPE_BS_BASIC, SPDK_BS_CPL_TYPE_BS_HANDLE, SPDK_BS_CPL_TYPE_BLOB_BASIC, diff --git a/lib/util/io_channel.c b/lib/util/io_channel.c index b91549451f..4a1b1bbc88 100644 --- a/lib/util/io_channel.c +++ b/lib/util/io_channel.c @@ -241,6 +241,7 @@ _spdk_io_device_attempt_free(struct io_device *dev) struct spdk_thread *thread; struct spdk_io_channel *ch; + pthread_mutex_lock(&g_devlist_mutex); TAILQ_FOREACH(thread, &g_threads, tailq) { TAILQ_FOREACH(ch, &thread->io_channels, tailq) { if (ch->dev == dev) { @@ -248,10 +249,12 @@ _spdk_io_device_attempt_free(struct io_device *dev) * device still exists. Defer deletion * until it is removed. */ + pthread_mutex_unlock(&g_devlist_mutex); return; } } } + pthread_mutex_unlock(&g_devlist_mutex); if (dev->unregister_cb) { dev->unregister_cb(dev->io_device); @@ -287,9 +290,8 @@ 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); - _spdk_io_device_attempt_free(dev); - pthread_mutex_unlock(&g_devlist_mutex); + _spdk_io_device_attempt_free(dev); } struct spdk_io_channel * @@ -377,15 +379,13 @@ _spdk_put_io_channel(void *arg) ch->destroy_cb(ch->dev->io_device, spdk_io_channel_get_ctx(ch)); pthread_mutex_lock(&g_devlist_mutex); - TAILQ_REMOVE(&ch->thread->io_channels, ch, tailq); + pthread_mutex_unlock(&g_devlist_mutex); if (ch->dev->unregistered) { _spdk_io_device_attempt_free(ch->dev); } free(ch); - - pthread_mutex_unlock(&g_devlist_mutex); } void diff --git a/test/unit/lib/blob/blob.c/blob_ut.c b/test/unit/lib/blob/blob.c/blob_ut.c index bf4732a990..6ff04a2b43 100644 --- a/test/unit/lib/blob/blob.c/blob_ut.c +++ b/test/unit/lib/blob/blob.c/blob_ut.c @@ -939,12 +939,10 @@ bs_load(void) } /* - * Create a blobstore and then unload it, while delaying all scheduled operations - * until after spdk_bs_unload call has finished. This ensures the memory associated - * with the internal blobstore channels is not touched after it is freed. + * Create a blobstore and then unload it. */ static void -bs_unload_delayed(void) +bs_unload(void) { struct spdk_bs_dev *dev; @@ -954,17 +952,10 @@ bs_unload_delayed(void) CU_ASSERT(g_bserrno == 0); SPDK_CU_ASSERT_FATAL(g_bs != NULL); - g_scheduler_delay = true; - g_bserrno = -1; spdk_bs_unload(g_bs, bs_op_complete, NULL); CU_ASSERT(g_bserrno == 0); g_bs = NULL; - - _bs_flush_scheduler(); - CU_ASSERT(TAILQ_EMPTY(&g_scheduled_ops)); - - g_scheduler_delay = false; } /* @@ -1295,7 +1286,7 @@ int main(int argc, char **argv) CU_add_test(suite, "blob_iter", blob_iter) == NULL || CU_add_test(suite, "blob_xattr", blob_xattr) == NULL || CU_add_test(suite, "bs_load", bs_load) == NULL || - CU_add_test(suite, "bs_unload_delayed", bs_unload_delayed) == NULL || + CU_add_test(suite, "bs_unload", bs_unload) == NULL || CU_add_test(suite, "bs_cluster_sz", bs_cluster_sz) == NULL || CU_add_test(suite, "bs_resize_md", bs_resize_md) == NULL || CU_add_test(suite, "blob_serialize", blob_serialize) == NULL ||