Commit 037c8b01 authored by Jim Harris's avatar Jim Harris Committed by Konrad Sztyber
Browse files

blob: remove short-circuiting path for blob_freeze 



If blob_freeze_io() is called twice in a row,
and the second time occurs before the for_each_channel
for the first completes, the second caller will
receive its callback too soon.

Instead just simplify the whole process, always do
the for_each_channel and don't try to optimize it
at all.  These are infrequent operations - correctness
and simplicity are in order.

A few additional changes:

1) Make same changes for unfreeze path.
2) Add blob_verify_md_op() calls, just to be sure
   these are only called from md_thread.  This was
   already checked in calling functions, but as these
   functions get called from new code paths (i.e.
   esnap clones) it can't hurt to add additional
   checks.
3) Add unit test that failed with original code, but
   passes with this patch.

Fixes issue #2935.

Signed-off-by: default avatarJim Harris <james.r.harris@intel.com>
Change-Id: Ibefba554547ddf3e26aaabfa4288c8073d3c04ff
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/17148


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Reviewed-by: default avatarMike Gerdts <mgerdts@nvidia.com>
Community-CI: Mellanox Build Bot
parent d4d57898

lib/blob/blobstore.c

+6 −12
Original line number Diff line number Diff line
@@ -388,6 +388,8 @@ blob_freeze_io(struct spdk_blob *blob, spdk_blob_op_complete cb_fn, void *cb_arg 
{

	struct freeze_io_ctx *ctx;



	blob_verify_md_op(blob);



	ctx = calloc(1, sizeof(*ctx));

	if (!ctx) {

		cb_fn(cb_arg, -ENOMEM);
@@ -402,12 +404,7 @@ blob_freeze_io(struct spdk_blob *blob, spdk_blob_op_complete cb_fn, void *cb_arg 
	/* Freeze I/O on blob */

	blob->frozen_refcnt++;



	if (blob->frozen_refcnt == 1) {

	spdk_for_each_channel(blob->bs, blob_io_sync, ctx, blob_io_cpl);

	} else {

		cb_fn(cb_arg, 0);

		free(ctx);

	}

}



static void
@@ -415,6 +412,8 @@ blob_unfreeze_io(struct spdk_blob *blob, spdk_blob_op_complete cb_fn, void *cb_a 
{

	struct freeze_io_ctx *ctx;



	blob_verify_md_op(blob);



	ctx = calloc(1, sizeof(*ctx));

	if (!ctx) {

		cb_fn(cb_arg, -ENOMEM);
@@ -430,12 +429,7 @@ blob_unfreeze_io(struct spdk_blob *blob, spdk_blob_op_complete cb_fn, void *cb_a 


	blob->frozen_refcnt--;



	if (blob->frozen_refcnt == 0) {

	spdk_for_each_channel(blob->bs, blob_execute_queued_io, ctx, blob_io_cpl);

	} else {

		cb_fn(cb_arg, 0);

		free(ctx);

	}

}



static int

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

+92 −0
Original line number Diff line number Diff line
@@ -7524,6 +7524,97 @@ blob_esnap_create(void) 
	g_blob = NULL;

}



static void

freeze_done(void *cb_arg, int bserrno)

{

	uint32_t *freeze_cnt = cb_arg;



	CU_ASSERT(bserrno == 0);

	(*freeze_cnt)++;

}



static void

unfreeze_done(void *cb_arg, int bserrno)

{

	uint32_t *unfreeze_cnt = cb_arg;



	CU_ASSERT(bserrno == 0);

	(*unfreeze_cnt)++;

}



static void

blob_nested_freezes(void)

{

	struct spdk_blob_store *bs = g_bs;

	struct spdk_blob *blob;

	struct spdk_io_channel *channel[2];

	struct spdk_blob_opts opts;

	uint32_t freeze_cnt, unfreeze_cnt;

	int i;



	for (i = 0; i < 2; i++) {

		set_thread(i);

		channel[i] = spdk_bs_alloc_io_channel(bs);

		SPDK_CU_ASSERT_FATAL(channel[i] != NULL);

	}



	set_thread(0);



	ut_spdk_blob_opts_init(&opts);

	blob = ut_blob_create_and_open(bs, &opts);



	/* First just test a single freeze/unfreeze. */

	freeze_cnt = 0;

	unfreeze_cnt = 0;

	CU_ASSERT(blob->frozen_refcnt == 0);

	blob_freeze_io(blob, freeze_done, &freeze_cnt);

	CU_ASSERT(blob->frozen_refcnt == 1);

	CU_ASSERT(freeze_cnt == 0);

	poll_threads();

	CU_ASSERT(freeze_cnt == 1);

	blob_unfreeze_io(blob, unfreeze_done, &unfreeze_cnt);

	CU_ASSERT(blob->frozen_refcnt == 0);

	CU_ASSERT(unfreeze_cnt == 0);

	poll_threads();

	CU_ASSERT(unfreeze_cnt == 1);



	/* Now nest multiple freeze/unfreeze operations.  We should

	 * expect a callback for each operation, but only after

	 * the threads have been polled to ensure a for_each_channel()

	 * was executed.

	 */

	freeze_cnt = 0;

	unfreeze_cnt = 0;

	CU_ASSERT(blob->frozen_refcnt == 0);

	blob_freeze_io(blob, freeze_done, &freeze_cnt);

	CU_ASSERT(blob->frozen_refcnt == 1);

	CU_ASSERT(freeze_cnt == 0);

	blob_freeze_io(blob, freeze_done, &freeze_cnt);

	CU_ASSERT(blob->frozen_refcnt == 2);

	CU_ASSERT(freeze_cnt == 0);

	poll_threads();

	CU_ASSERT(freeze_cnt == 2);

	blob_unfreeze_io(blob, unfreeze_done, &unfreeze_cnt);

	CU_ASSERT(blob->frozen_refcnt == 1);

	CU_ASSERT(unfreeze_cnt == 0);

	blob_unfreeze_io(blob, unfreeze_done, &unfreeze_cnt);

	CU_ASSERT(blob->frozen_refcnt == 0);

	CU_ASSERT(unfreeze_cnt == 0);

	poll_threads();

	CU_ASSERT(unfreeze_cnt == 2);



	for (i = 0; i < 2; i++) {

		set_thread(i);

		spdk_bs_free_io_channel(channel[i]);

	}

	set_thread(0);

	ut_blob_close_and_delete(bs, blob);



	poll_threads();

	g_blob = NULL;

	g_blobid = 0;

}



static void

suite_bs_setup(void)

{
@@ -7721,6 +7812,7 @@ main(int argc, char **argv) 
	CU_ADD_TEST(suite_bs, blob_decouple_snapshot);

	CU_ADD_TEST(suite_bs, blob_seek_io_unit);

	CU_ADD_TEST(suite_esnap_bs, blob_esnap_create);

	CU_ADD_TEST(suite_bs, blob_nested_freezes);



	allocate_threads(2);

	set_thread(0);