From 68063cd8b6ba9d45f41f7d5da828925f4fd87177 Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Tue, 13 Jul 2021 11:56:05 -0400 Subject: [PATCH] lib/blob: force md update during decouple parent Fixes #1933 When decoupling parent the updated parent_id was not persisted to the blob if it was a snapshot. Due to having md_ro set to true, blob_set_xattr() failed. Later on the incorrect parent_id could cause troubles like in the github issue, when deleting that snapshot. This patch adds return code check for blob_set_xattr and forces md_ro to false during blob md sync. Since some of code paths are shared between decouple, inflate and clone operations, the final callback for them is doing revert of the original md_ro. Signed-off-by: Tomasz Zawadzki Change-Id: If017455f72e4d809fe533d9f986e5ae6bb8e2035 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8420 Tested-by: SPDK CI Jenkins Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Reviewed-by: Jim Harris Reviewed-by: Ben Walker --- lib/blob/blobstore.c | 17 ++++- test/unit/lib/blob/blob.c/blob_ut.c | 108 ++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/lib/blob/blobstore.c b/lib/blob/blobstore.c index 57a336acc..ecdc0a455 100644 --- a/lib/blob/blobstore.c +++ b/lib/blob/blobstore.c @@ -5561,6 +5561,7 @@ struct spdk_clone_snapshot_ctx { struct { spdk_blob_id id; struct spdk_blob *blob; + bool md_ro; } original; struct { spdk_blob_id id; @@ -5618,6 +5619,9 @@ bs_snapshot_unfreeze_cpl(void *cb_arg, int bserrno) ctx->original.id = origblob->id; origblob->locked_operation_in_progress = false; + /* Revert md_ro to original state */ + origblob->md_ro = ctx->original.md_ro; + spdk_blob_close(origblob, bs_clone_snapshot_cleanup_finish, ctx); } @@ -5982,6 +5986,7 @@ bs_clone_origblob_open_cpl(void *cb_arg, struct spdk_blob *_blob, int bserrno) } ctx->original.blob = _blob; + ctx->original.md_ro = _blob->md_ro; if (!_blob->data_ro || !_blob->md_ro) { SPDK_DEBUGLOG(blob, "Clone not from read-only blob\n"); @@ -6056,12 +6061,19 @@ bs_inflate_blob_set_parent_cpl(void *cb_arg, struct spdk_blob *_parent, int bser return; } + /* Temporarily override md_ro flag for MD modification */ + _blob->md_ro = false; + + bserrno = blob_set_xattr(_blob, BLOB_SNAPSHOT, &_parent->id, sizeof(spdk_blob_id), true); + if (bserrno != 0) { + bs_clone_snapshot_origblob_cleanup(ctx, bserrno); + return; + } + assert(_parent != NULL); bs_blob_list_remove(_blob); _blob->parent_id = _parent->id; - blob_set_xattr(_blob, BLOB_SNAPSHOT, &_blob->parent_id, - sizeof(spdk_blob_id), true); _blob->back_bs_dev->destroy(_blob->back_bs_dev); _blob->back_bs_dev = bs_create_blob_bs_dev(_parent); @@ -6171,6 +6183,7 @@ bs_inflate_blob_open_cpl(void *cb_arg, struct spdk_blob *_blob, int bserrno) } ctx->original.blob = _blob; + ctx->original.md_ro = _blob->md_ro; if (_blob->locked_operation_in_progress) { SPDK_DEBUGLOG(blob, "Cannot inflate blob - another operation in progress\n"); diff --git a/test/unit/lib/blob/blob.c/blob_ut.c b/test/unit/lib/blob/blob.c/blob_ut.c index b11e002d4..a0eb8ea79 100644 --- a/test/unit/lib/blob/blob.c/blob_ut.c +++ b/test/unit/lib/blob/blob.c/blob_ut.c @@ -5481,6 +5481,113 @@ blob_relations2(void) g_bs = NULL; } +/** + * Snapshot-clones relation test 3 + * + * snapshot0 + * | + * snapshot1 + * | + * snapshot2 + * | + * blob + */ +static void +blob_relations3(void) +{ + struct spdk_blob_store *bs; + struct spdk_bs_dev *dev; + struct spdk_io_channel *channel; + struct spdk_bs_opts bs_opts; + struct spdk_blob_opts opts; + struct spdk_blob *blob; + spdk_blob_id blobid, snapshotid0, snapshotid1, snapshotid2; + + dev = init_dev(); + spdk_bs_opts_init(&bs_opts, sizeof(opts)); + snprintf(bs_opts.bstype.bstype, sizeof(bs_opts.bstype.bstype), "TESTTYPE"); + + spdk_bs_init(dev, &bs_opts, bs_op_with_handle_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + SPDK_CU_ASSERT_FATAL(g_bs != NULL); + bs = g_bs; + + channel = spdk_bs_alloc_io_channel(bs); + SPDK_CU_ASSERT_FATAL(channel != NULL); + + /* 1. Create blob with 10 clusters */ + ut_spdk_blob_opts_init(&opts); + opts.num_clusters = 10; + + blob = ut_blob_create_and_open(bs, &opts); + blobid = spdk_blob_get_id(blob); + + /* 2. Create snapshot0 */ + spdk_bs_create_snapshot(bs, blobid, NULL, blob_op_with_id_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + CU_ASSERT(g_blobid != SPDK_BLOBID_INVALID); + snapshotid0 = g_blobid; + + /* 3. Create snapshot1 */ + spdk_bs_create_snapshot(bs, blobid, NULL, blob_op_with_id_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + CU_ASSERT(g_blobid != SPDK_BLOBID_INVALID); + snapshotid1 = g_blobid; + + /* 4. Create snapshot2 */ + spdk_bs_create_snapshot(bs, blobid, NULL, blob_op_with_id_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + CU_ASSERT(g_blobid != SPDK_BLOBID_INVALID); + snapshotid2 = g_blobid; + + /* 5. Decouple blob */ + spdk_bs_blob_decouple_parent(bs, channel, blobid, blob_op_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + + /* 6. Decouple snapshot2. Make sure updating md of snapshot2 is possible */ + spdk_bs_blob_decouple_parent(bs, channel, snapshotid2, blob_op_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + + /* 7. Delete blob */ + spdk_blob_close(blob, blob_op_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + + spdk_bs_delete_blob(bs, blobid, blob_op_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + + /* 8. Delete snapshot2. + * If md of snapshot 2 was updated, it should be possible to delete it */ + spdk_bs_delete_blob(bs, snapshotid2, blob_op_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + + /* Remove remaining blobs and unload bs */ + spdk_bs_delete_blob(bs, snapshotid1, blob_op_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + + spdk_bs_delete_blob(bs, snapshotid0, blob_op_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + + spdk_bs_free_io_channel(channel); + poll_threads(); + + spdk_bs_unload(bs, bs_op_complete, NULL); + poll_threads(); + CU_ASSERT(g_bserrno == 0); + + g_bs = NULL; +} + static void blobstore_clean_power_failure(void) { @@ -6924,6 +7031,7 @@ int main(int argc, char **argv) CU_ADD_TEST(suite_bs, blob_snapshot_rw_iov); CU_ADD_TEST(suite, blob_relations); CU_ADD_TEST(suite, blob_relations2); + CU_ADD_TEST(suite, blob_relations3); CU_ADD_TEST(suite, blobstore_clean_power_failure); CU_ADD_TEST(suite, blob_delete_snapshot_power_failure); CU_ADD_TEST(suite, blob_create_snapshot_power_failure);