lib/blob: add error path on persisting dirty bs

super->clean value signifies if blobstore was unloaded
cleanly.
If it was not, then during bs_load the _spdk_bs_recover()
procedure if called.

Meanwhile bs->clean is always set to 1 after load, causing very first
blob_persist to also re-write super block with the super->clean
set to 0. To signify that md has changed and possibly trigger
the recovery if clean bs unload does not occur.
When the re-write of super block succeeds the bs->clean is set to 0,
because further re-writes of super block are not needed on next
blob persist.

This patch resolves issue when:
1) reading super block fails - execution should backoff, to prevent
writing an empty buffer as super block !
2) writing super->clean = 0 to the super block fails - execution
again should fail, and bs->clean should not be set to 0. It will
cause next persist to attempt re-write again.

Signed-off-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Change-Id: Ia07cc5c6c107310059b50886edb7283c176b9169
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/1164
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>

lib/blob/blobstore.c

@@ -2050,10 +2050,15 @@ _spdk_blob_persist_dirty_cpl(spdk_bs_sequence_t *seq, void *cb_arg, int bserrno)
 {
 	struct spdk_blob_persist_ctx *ctx = cb_arg;
 
-	ctx->blob->bs->clean = 0;
-
 	spdk_free(ctx->super);
 
+	if (bserrno != 0) {
+		_spdk_blob_persist_complete(seq, ctx, bserrno);
+		return;
+	}
+
+	ctx->blob->bs->clean = 0;
+
 	_spdk_blob_persist_start(ctx);
 }
 
@@ -2067,6 +2072,12 @@ _spdk_blob_persist_dirty(spdk_bs_sequence_t *seq, void *cb_arg, int bserrno)
 {
 	struct spdk_blob_persist_ctx *ctx = cb_arg;
 
+	if (bserrno != 0) {
+		spdk_free(ctx->super);
+		_spdk_blob_persist_complete(seq, ctx, bserrno);
+		return;
+	}
+
 	ctx->super->clean = 0;
 	if (ctx->super->size == 0) {
 		ctx->super->size = ctx->blob->bs->dev->blockcnt * ctx->blob->bs->dev->blocklen;
@@ -2082,8 +2093,7 @@ _spdk_blob_persist_check_dirty(struct spdk_blob_persist_ctx *ctx)
 		ctx->super = spdk_zmalloc(sizeof(*ctx->super), 0x1000, NULL,
 					  SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_DMA);
 		if (!ctx->super) {
-			ctx->cb_fn(ctx->seq, ctx->cb_arg, -ENOMEM);
+			_spdk_blob_persist_complete(ctx->seq, ctx, -ENOMEM);
-			free(ctx);
 			return;
 		}
 

test/unit/lib/blob/blob.c/blob_ut.c

@@ -82,6 +82,8 @@ SPDK_STATIC_ASSERT(sizeof(struct spdk_bs_super_block_ver1) == 0x1000, "Invalid s
 static struct spdk_blob *ut_blob_create_and_open(struct spdk_blob_store *bs,
 		struct spdk_blob_opts *blob_opts);
 static void ut_blob_close_and_delete(struct spdk_blob_store *bs, struct spdk_blob *blob);
+static void suite_blob_setup(void);
+static void suite_blob_cleanup(void);
 
 static void
 _get_xattr_value(void *arg, const char *name,
@@ -5208,6 +5210,65 @@ blob_relations2(void)
 	g_bs = NULL;
 }
 
+static void
+blobstore_clean_power_failure(void)
+{
+	struct spdk_blob_store *bs;
+	struct spdk_blob *blob;
+	struct spdk_power_failure_thresholds thresholds = {};
+	bool clean = false;
+	struct spdk_bs_super_block *super = (struct spdk_bs_super_block *)&g_dev_buffer[0];
+	struct spdk_bs_super_block super_copy = {};
+
+	thresholds.general_threshold = 1;
+	while (!clean) {
+		/* Create bs and blob */
+		suite_blob_setup();
+		SPDK_CU_ASSERT_FATAL(g_bs != NULL);
+		SPDK_CU_ASSERT_FATAL(g_blob != NULL);
+		bs = g_bs;
+		blob = g_blob;
+
+		/* Super block should not change for rest of the UT,
+		 * save it and compare later. */
+		memcpy(&super_copy, super, sizeof(struct spdk_bs_super_block));
+		SPDK_CU_ASSERT_FATAL(super->clean == 0);
+		SPDK_CU_ASSERT_FATAL(bs->clean == 0);
+
+		/* Force bs/super block in a clean state.
+		 * Along with marking blob dirty, to cause blob persist. */
+		blob->state = SPDK_BLOB_STATE_DIRTY;
+		bs->clean = 1;
+		super->clean = 1;
+		super->crc = _spdk_blob_md_page_calc_crc(super);
+
+		g_bserrno = -1;
+		dev_set_power_failure_thresholds(thresholds);
+		spdk_blob_sync_md(blob, blob_op_complete, NULL);
+		poll_threads();
+		dev_reset_power_failure_event();
+
+		if (g_bserrno == 0) {
+			/* After successful md sync, both bs and super block
+			 * should be marked as not clean. */
+			SPDK_CU_ASSERT_FATAL(bs->clean == 0);
+			SPDK_CU_ASSERT_FATAL(super->clean == 0);
+			clean = true;
+		}
+
+		/* Depending on the point of failure, super block was either updated or not. */
+		super_copy.clean = super->clean;
+		super_copy.crc = _spdk_blob_md_page_calc_crc(&super_copy);
+		/* Compare that the values in super block remained unchanged. */
+		SPDK_CU_ASSERT_FATAL(!memcmp(&super_copy, super, sizeof(struct spdk_bs_super_block)));
+
+		/* Delete blob and unload bs */
+		suite_blob_cleanup();
+
+		thresholds.general_threshold++;
+	}
+}
+
 static void
 blob_delete_snapshot_power_failure(void)
 {
@@ -6558,6 +6619,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, blobstore_clean_power_failure);
 	CU_ADD_TEST(suite, blob_delete_snapshot_power_failure);
 	CU_ADD_TEST(suite, blob_create_snapshot_power_failure);
 	CU_ADD_TEST(suite_bs, blob_inflate_rw);