From 597c91abdf5696f7dd8257b937f0308a7b603e86 Mon Sep 17 00:00:00 2001 From: Tomasz Zawadzki Date: Fri, 20 Mar 2020 07:54:11 -0400 Subject: [PATCH] lib/blob: always add back the snapshot if delete fails After opening the blob for deletion, in _spdk_bs_delete_open_cpl(), the blob is removed from list of blobs in blobstore. This is to prevent future _spdk_blob_lookup()s from referencing blob while it is deleted. In usual blob deletion path, next step is proceeding with deletion of the blob by reducing its size to 0 and syncing the blob. Changes from this point forward are persisted. Meanwhile in special case of deleting snapshot which has single clone on it, before above occurs additional steps are performed. Each of the blobs are opened and their attributes changed. Failures on those steps are fully recoverable on any errors, and in such case blob should be added back to the bs list of blobs. Original code had condition on how many references there were to blob being deleted, which is incorrect. Any error on that path should clean up after itself (revert attributes and close blobs) and re-add the blob. This change is tested with blob_delete_snapshot_power_failure() UT, by adding error path in persist - which triggers error in aforementioned blob delete code path. Signed-off-by: Tomasz Zawadzki Change-Id: I926e7cbf3cb86170c69f31231399535859f290dd Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/985 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Jim Harris --- lib/blob/blobstore.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/blob/blobstore.c b/lib/blob/blobstore.c index aaa640c0f..4850bdddb 100644 --- a/lib/blob/blobstore.c +++ b/lib/blob/blobstore.c @@ -1978,6 +1978,11 @@ _spdk_blob_persist_write_extent_pages(spdk_bs_sequence_t *seq, void *cb_arg, int ctx->extent_page = NULL; } + if (bserrno != 0) { + _spdk_blob_persist_complete(seq, ctx, bserrno); + return; + } + /* Only write out changed extent pages */ for (i = ctx->next_extent_page; i < blob->active.num_extent_pages; i++) { extent_page_id = blob->active.extent_pages[i]; @@ -6097,10 +6102,8 @@ _spdk_delete_snapshot_cleanup_snapshot(void *cb_arg, int bserrno) SPDK_ERRLOG("Clone cleanup error %d\n", bserrno); } - /* open_ref == 1 menas that only deletion context has opened this snapshot - * open_ref == 2 menas that clone has opened this snapshot as well, - * so we have to add it back to the blobs list */ - if (ctx->snapshot->open_ref == 2) { + if (ctx->bserrno != 0) { + assert(_spdk_blob_lookup(ctx->snapshot->bs, ctx->snapshot->id) == NULL); TAILQ_INSERT_HEAD(&ctx->snapshot->bs->blobs, ctx->snapshot, link); }