From b24fdae1a81f9cdb8e0084942a54bd1b93e78a81 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Mon, 12 Mar 2018 17:19:11 -0700 Subject: [PATCH] Revert "blob: queue sync requests if one already in progress" BlobFS shutdown path needs to be investigated more with these changes. This reverts commit a137b9afd0bc4e81df38a3a0847612269d038289. Signed-off-by: Jim Harris Change-Id: I8b04b24e178945d62db20668b9e500f278ae955b Reviewed-on: https://review.gerrithub.io/403600 Reviewed-by: Daniel Verkamp Tested-by: SPDK Automated Test System --- lib/blob/blobstore.c | 89 +++++----------------------- lib/blob/blobstore.h | 4 +- test/unit/lib/blob/blob.c/blob_ut.c | 91 +---------------------------- 3 files changed, 17 insertions(+), 167 deletions(-) diff --git a/lib/blob/blobstore.c b/lib/blob/blobstore.c index 5d9d12d8b..72907f73b 100644 --- a/lib/blob/blobstore.c +++ b/lib/blob/blobstore.c @@ -58,7 +58,6 @@ static int _spdk_blob_set_xattr(struct spdk_blob *blob, const char *name, const static int _spdk_blob_get_xattr_value(struct spdk_blob *blob, const char *name, const void **value, size_t *value_len, bool internal); static int _spdk_blob_remove_xattr(struct spdk_blob *blob, const char *name, bool internal); -static struct spdk_blob *_spdk_blob_lookup(struct spdk_blob_store *bs, spdk_blob_id blobid); static void _spdk_blob_verify_md_op(struct spdk_blob *blob) @@ -200,39 +199,10 @@ _spdk_xattrs_free(struct spdk_xattr_tailq *xattrs) } } -struct spdk_blob_persist_ctx { - struct spdk_blob *blob; - struct spdk_blob_md_page *pages; - uint64_t idx; - spdk_bs_sequence_t *seq; - spdk_bs_sequence_cpl cb_fn; - void *cb_arg; - TAILQ_ENTRY(spdk_blob_persist_ctx) link; -}; - static void _spdk_blob_free(struct spdk_blob *blob) { - struct spdk_blob_store *bs; - struct spdk_bs_channel *bs_channel; - struct spdk_blob_persist_ctx *ctx, *tmp; - assert(blob != NULL); - bs = blob->bs; - bs_channel = spdk_io_channel_get_ctx(bs->md_channel); - - /* - * If the blob was freed, there should be no remaining queued persist - * requests. But just to make sure, add an assert if any are found. - */ - TAILQ_FOREACH_SAFE(ctx, &bs_channel->queued_blob_persists, link, tmp) { - if (ctx->blob == blob) { - TAILQ_REMOVE(&bs_channel->queued_blob_persists, ctx, link); - spdk_dma_free(ctx->pages); - free(ctx); - assert(false); - } - } free(blob->active.clusters); free(blob->clean.clusters); @@ -976,18 +946,24 @@ _spdk_blob_load(spdk_bs_sequence_t *seq, struct spdk_blob *blob, _spdk_blob_load_cpl, ctx); } -static void _spdk_blob_persist_start(struct spdk_blob_persist_ctx *ctx); +struct spdk_blob_persist_ctx { + struct spdk_blob *blob; + + struct spdk_blob_md_page *pages; + + uint64_t idx; + + spdk_bs_sequence_t *seq; + spdk_bs_sequence_cpl cb_fn; + void *cb_arg; +}; static void _spdk_blob_persist_complete(spdk_bs_sequence_t *seq, void *cb_arg, int bserrno) { - struct spdk_blob_persist_ctx *ctx = cb_arg, *tmp; + struct spdk_blob_persist_ctx *ctx = cb_arg; struct spdk_blob *blob = ctx->blob; - struct spdk_blob_store *bs = blob->bs; - struct spdk_bs_channel *bs_channel = spdk_io_channel_get_ctx(bs->md_channel); - spdk_blob_id blobid = blob->id; - assert(blob->persist_in_progress == true); if (bserrno == 0) { _spdk_blob_mark_clean(blob); } @@ -998,37 +974,6 @@ _spdk_blob_persist_complete(spdk_bs_sequence_t *seq, void *cb_arg, int bserrno) /* Free the memory */ spdk_dma_free(ctx->pages); free(ctx); - - /* - * Sometimes the callback function we just executed will have freed - * the blob (create and delete calls for example). - * So we may not access the blob after the callback function in that - * case. Determine if the blob was deleted by looking up the blob - * by its blobid. If it cannot be found, we know it was freed and we - * just return immediately. - */ - blob = _spdk_blob_lookup(bs, blobid); - - if (blob == NULL) { - return; - } - - blob->persist_in_progress = false; - - TAILQ_FOREACH_SAFE(ctx, &bs_channel->queued_blob_persists, link, tmp) { - if (ctx->blob != blob) { - continue; - } - TAILQ_REMOVE(&bs_channel->queued_blob_persists, ctx, link); - if (blob->state == SPDK_BLOB_STATE_CLEAN) { - ctx->cb_fn(seq, ctx->cb_arg, 0); - spdk_dma_free(ctx->pages); - free(ctx); - } else { - _spdk_blob_persist_start(ctx); - break; - } - } } static void @@ -1330,9 +1275,6 @@ _spdk_blob_persist_start(struct spdk_blob_persist_ctx *ctx) uint32_t page_num; int rc; - assert(blob->persist_in_progress == false); - blob->persist_in_progress = true; - if (blob->active.num_pages == 0) { /* This is the signal that the blob should be deleted. * Immediately jump to the clean up routine. */ @@ -1418,11 +1360,7 @@ _spdk_blob_persist(spdk_bs_sequence_t *seq, struct spdk_blob *blob, ctx->cb_fn = cb_fn; ctx->cb_arg = cb_arg; - if (spdk_unlikely(blob->persist_in_progress)) { - TAILQ_INSERT_TAIL(&seq->channel->queued_blob_persists, ctx, link); - } else { - _spdk_blob_persist_start(ctx); - } + _spdk_blob_persist_start(ctx); } struct spdk_blob_copy_cluster_ctx { @@ -2036,7 +1974,6 @@ _spdk_bs_channel_create(void *io_device, void *ctx_buf) } TAILQ_INIT(&channel->need_cluster_alloc); - TAILQ_INIT(&channel->queued_blob_persists); return 0; } diff --git a/lib/blob/blobstore.h b/lib/blob/blobstore.h index b1c43de10..1d0b956f7 100644 --- a/lib/blob/blobstore.h +++ b/lib/blob/blobstore.h @@ -122,7 +122,6 @@ struct spdk_blob { struct spdk_blob_mut_data clean; struct spdk_blob_mut_data active; - bool persist_in_progress; bool invalid; bool data_ro; bool md_ro; @@ -183,8 +182,7 @@ struct spdk_bs_channel { struct spdk_bs_dev *dev; struct spdk_io_channel *dev_channel; - TAILQ_HEAD(, spdk_bs_request_set) need_cluster_alloc; - TAILQ_HEAD(, spdk_blob_persist_ctx) queued_blob_persists; + TAILQ_HEAD(, spdk_bs_request_set) need_cluster_alloc; }; /** operation type */ diff --git a/test/unit/lib/blob/blob.c/blob_ut.c b/test/unit/lib/blob/blob.c/blob_ut.c index c717aa849..cebeb32a1 100644 --- a/test/unit/lib/blob/blob.c/blob_ut.c +++ b/test/unit/lib/blob/blob.c/blob_ut.c @@ -46,7 +46,6 @@ struct spdk_blob_store *g_bs; spdk_blob_id g_blobid; struct spdk_blob *g_blob; -int g_blob_op_complete_count; int g_bserrno; struct spdk_xattr_names *g_names; int g_done; @@ -136,6 +135,7 @@ _bs_send_msg(spdk_thread_fn fn, void *ctx, void *thread_ctx) } } +#if 0 static void _bs_flush_scheduler(void) { @@ -148,6 +148,7 @@ _bs_flush_scheduler(void) free(ops); } } +#endif static void bs_op_complete(void *cb_arg, int bserrno) @@ -167,7 +168,6 @@ static void blob_op_complete(void *cb_arg, int bserrno) { g_bserrno = bserrno; - g_blob_op_complete_count++; } static void @@ -3285,91 +3285,7 @@ bs_load_iter(void) spdk_bs_unload(g_bs, bs_op_complete, NULL); CU_ASSERT(g_bserrno == 0); g_bs = NULL; -} -static void -blob_persist(void) -{ - struct spdk_bs_dev *dev; - struct spdk_blob *blob; - struct spdk_blob_opts opts; - struct spdk_bs_channel *bs_channel; - spdk_blob_id blobid; - int rc; - - dev = init_dev(); - - spdk_bs_init(dev, NULL, bs_op_with_handle_complete, NULL); - CU_ASSERT(g_bserrno == 0); - SPDK_CU_ASSERT_FATAL(g_bs != NULL); - bs_channel = spdk_io_channel_get_ctx(g_bs->md_channel); - - spdk_blob_opts_init(&opts); - - spdk_bs_create_blob_ext(g_bs, &opts, blob_op_with_id_complete, NULL); - CU_ASSERT(g_bserrno == 0); - CU_ASSERT(g_blobid != SPDK_BLOBID_INVALID); - blobid = g_blobid; - - spdk_bs_open_blob(g_bs, blobid, blob_op_with_handle_complete, NULL); - CU_ASSERT(g_bserrno == 0); - SPDK_CU_ASSERT_FATAL(g_blob != NULL); - blob = g_blob; - - CU_ASSERT(blob->state == SPDK_BLOB_STATE_CLEAN); - rc = spdk_blob_resize(blob, 5); - CU_ASSERT(rc == 0); - CU_ASSERT(blob->state == SPDK_BLOB_STATE_DIRTY); - CU_ASSERT(blob->persist_in_progress == false); - - g_scheduler_delay = true; - - g_blob_op_complete_count = 0; - spdk_blob_sync_md(blob, blob_op_complete, NULL); - /* scheduler is delayed, so blob_op_complete should not have been called yet. */ - CU_ASSERT(g_blob_op_complete_count == 0); - CU_ASSERT(blob->state == SPDK_BLOB_STATE_CLEAN); - CU_ASSERT(blob->persist_in_progress == true); - - /* Dirty the blob's metadata again. */ - rc = spdk_blob_set_xattr(blob, "test", "foo", strlen("foo") + 1); - CU_ASSERT(rc == 0); - CU_ASSERT(blob->state == SPDK_BLOB_STATE_DIRTY); - - spdk_blob_sync_md(blob, blob_op_complete, NULL); - /* scheduler is delayed, so blob_op_complete should not have been called yet. */ - CU_ASSERT(g_blob_op_complete_count == 0); - /* - * The blob's metadata state should still be dirty. We have not started persisting - * the xattr change yet, because the sync operation for the resize operation is - * still in progress. Instead we should see a persist operation queued to execute - * after the first one is done. - */ - CU_ASSERT(blob->state == SPDK_BLOB_STATE_DIRTY); - CU_ASSERT(blob->persist_in_progress == true); - CU_ASSERT(!TAILQ_EMPTY(&bs_channel->queued_blob_persists)); - - /* - * Start another sync. This just stresses the spdk_blob_persist_complete logic to - * make sure it processes all of the persist operations when multiple ones are - * queued. - */ - spdk_blob_sync_md(blob, blob_op_complete, NULL); - CU_ASSERT(g_blob_op_complete_count == 0); - CU_ASSERT(blob->state == SPDK_BLOB_STATE_DIRTY); - CU_ASSERT(blob->persist_in_progress == true); - CU_ASSERT(!TAILQ_EMPTY(&bs_channel->queued_blob_persists)); - - _bs_flush_scheduler(); - CU_ASSERT(g_blob_op_complete_count == 3); - CU_ASSERT(blob->state == SPDK_BLOB_STATE_CLEAN); - CU_ASSERT(blob->persist_in_progress == false); - CU_ASSERT(TAILQ_EMPTY(&bs_channel->queued_blob_persists)); - CU_ASSERT(g_bserrno == 0); - - spdk_blob_close(blob, blob_op_complete, NULL); - CU_ASSERT(g_bserrno == 0); - g_scheduler_delay = false; } int main(int argc, char **argv) @@ -3426,8 +3342,7 @@ int main(int argc, char **argv) CU_add_test(suite, "blob_insert_cluster_msg", blob_insert_cluster_msg) == NULL || CU_add_test(suite, "blob_thin_prov_rw", blob_thin_prov_rw) == NULL || CU_add_test(suite, "blob_thin_prov_rw_iov", blob_thin_prov_rw_iov) == NULL || - CU_add_test(suite, "bs_load_iter", bs_load_iter) == NULL || - CU_add_test(suite, "blob_persist", blob_persist) == NULL + CU_add_test(suite, "bs_load_iter", bs_load_iter) == NULL ) { CU_cleanup_registry(); return CU_get_error();