Commit a2f5e1c2 authored by Jinlong Chen's avatar Jinlong Chen Committed by Jim Harris
Browse files

blob: don't free bs when spdk_bs_destroy/spdk_bs_unload fails 



Error handling of spdk_bs_destroy and spdk_bs_unload is confusing. They
may or may not free the spdk_blob_store structure on error, depending on
when the error happens. And users can not know if the structure has been
freed after the processes finished, thus unable to handle it correctly.

To fix this problem, we only free the structure when there are no errors
happended. In this way, users can be sure that the structure pointer is
still valid after the failed opertation. They can then retry the
operation or debug the failure.

Fixes #3560.

Change-Id: I4f7194ab8fce4f1a408ce3e6500514fd214427d4
Signed-off-by: default avatarJinlong Chen <chenjinlong.cjl@alibaba-inc.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/25472


Reviewed-by: default avatarJim Harris <jim.harris@nvidia.com>
Reviewed-by: default avatarGangCao <gang.cao@intel.com>
Reviewed-by: default avatarYankun Li <845245370@qq.com>
Community-CI: Community CI Samsung <spdk.community.ci.samsung@gmail.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarBen Walker <ben@nvidia.com>
Community-CI: Mellanox Build Bot
parent 0f59982b

lib/blob/blobstore.c

+17 −7
Original line number Diff line number Diff line
@@ -5724,6 +5724,13 @@ bs_destroy_trim_cpl(spdk_bs_sequence_t *seq, void *cb_arg, int bserrno) 
	struct spdk_bs_load_ctx *ctx = cb_arg;

	struct spdk_blob_store *bs = ctx->bs;



	free(ctx);



	if (bserrno != 0) {

		bs_sequence_finish(seq, bserrno);

		return;

	}



	/*

	 * We need to defer calling bs_call_cpl() until after

	 * dev destruction, so tuck these away for later use.
@@ -5731,11 +5738,9 @@ bs_destroy_trim_cpl(spdk_bs_sequence_t *seq, void *cb_arg, int bserrno) 
	bs->unload_err = bserrno;

	memcpy(&bs->unload_cpl, &seq->cpl, sizeof(struct spdk_bs_cpl));

	seq->cpl.type = SPDK_BS_CPL_TYPE_NONE;



	bs_sequence_finish(seq, bserrno);



	bs_free(bs);

	free(ctx);

}



void
@@ -5788,21 +5793,26 @@ static void 
bs_unload_finish(struct spdk_bs_load_ctx *ctx, int bserrno)

{

	spdk_bs_sequence_t *seq = ctx->seq;

	struct spdk_blob_store *bs = ctx->bs;



	spdk_free(ctx->super);

	free(ctx);



	if (bserrno != 0) {

		bs_sequence_finish(seq, bserrno);

		return;

	}



	/*

	 * We need to defer calling bs_call_cpl() until after

	 * dev destruction, so tuck these away for later use.

	 */

	ctx->bs->unload_err = bserrno;

	memcpy(&ctx->bs->unload_cpl, &seq->cpl, sizeof(struct spdk_bs_cpl));

	bs->unload_err = bserrno;

	memcpy(&bs->unload_cpl, &seq->cpl, sizeof(struct spdk_bs_cpl));

	seq->cpl.type = SPDK_BS_CPL_TYPE_NONE;



	bs_sequence_finish(seq, bserrno);



	bs_free(ctx->bs);

	free(ctx);

	bs_free(bs);

}



static void

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

+28 −0
Original line number Diff line number Diff line
@@ -3351,6 +3351,7 @@ bs_unload(void) 
{

	struct spdk_blob_store *bs = g_bs;

	struct spdk_blob *blob;

	struct spdk_power_failure_thresholds thresholds = {};



	/* Create a blob and open it. */

	blob = ut_blob_create_and_open(bs, NULL);
@@ -3367,6 +3368,23 @@ bs_unload(void) 
	spdk_blob_close(blob, blob_op_complete, NULL);

	poll_threads();

	CU_ASSERT(g_bserrno == 0);



	/* Try to unload blobstore, should fail due to I/O error */

	thresholds.general_threshold = 2;

	dev_set_power_failure_thresholds(thresholds);

	g_bserrno = -1;

	spdk_bs_unload(bs, bs_op_complete, NULL);

	poll_threads();

	CU_ASSERT(g_bserrno == -EIO);

	dev_reset_power_failure_event();



	/* Try to unload blobstore, should fail with spdk_zmalloc returning NULL */

	g_bserrno = -1;

	spdk_bs_unload(bs, bs_op_complete, NULL);

	MOCK_SET(spdk_zmalloc, NULL);

	poll_threads();

	CU_ASSERT(g_bserrno == -ENOMEM);

	MOCK_CLEAR(spdk_zmalloc);

}



/*
@@ -3564,6 +3582,7 @@ bs_destroy(void) 
{

	struct spdk_blob_store *bs;

	struct spdk_bs_dev *dev;

	struct spdk_power_failure_thresholds thresholds = {};



	/* Initialize a new blob store */

	dev = init_dev();
@@ -3573,6 +3592,15 @@ bs_destroy(void) 
	SPDK_CU_ASSERT_FATAL(g_bs != NULL);

	bs = g_bs;



	/* Destroy the blobstore, should fail due to I/O error */

	thresholds.general_threshold = 1;

	dev_set_power_failure_thresholds(thresholds);

	g_bserrno = -1;

	spdk_bs_destroy(bs, bs_op_complete, NULL);

	poll_threads();

	CU_ASSERT(g_bserrno == -EIO);

	dev_reset_power_failure_event();



	/* Destroy the blob store */

	g_bserrno = -1;

	spdk_bs_destroy(bs, bs_op_complete, NULL);