Commit cf467578 authored by paul luse's avatar paul luse Committed by Ben Walker
Browse files

bdev/reduce: fix issue with vol_load 



Discovered in bdev compression test.  Vol load was using the index
of the logical map to set the allocated chunk map bits as well as
retrieve the chunk. Exposed with overwrite situations. Also clarrified
one var name that I missed in previous patch.

Also added UT that fails without the fix, passes with the fix.

Change-Id: I776a5af9364cc3d4e8b7dafec22acf23bd463630
Signed-off-by: default avatarpaul luse <paul.e.luse@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/447969


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
parent 228396f8

lib/reduce/reduce.c

+10 −9
Original line number Diff line number Diff line
@@ -550,7 +550,7 @@ _load_read_super_and_path_cpl(void *cb_arg, int reduce_errno) 
	struct reduce_init_load_ctx *load_ctx = cb_arg;

	struct spdk_reduce_vol *vol = load_ctx->vol;

	uint64_t backing_dev_size;

	uint64_t i, num_chunks;

	uint64_t i, num_chunks, logical_map_index;

	uint64_t *chunk;

	size_t mapped_len;

	uint32_t j;
@@ -608,11 +608,12 @@ _load_read_super_and_path_cpl(void *cb_arg, int reduce_errno) 


	num_chunks = vol->params.vol_size / vol->params.chunk_size;

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

		if (vol->pm_logical_map[i] == REDUCE_EMPTY_MAP_ENTRY) {

		logical_map_index = vol->pm_logical_map[i];

		if (logical_map_index == REDUCE_EMPTY_MAP_ENTRY) {

			continue;

		}

		spdk_bit_array_set(vol->allocated_chunk_maps, i);

		chunk = _reduce_vol_get_chunk_map(vol, i);

		spdk_bit_array_set(vol->allocated_chunk_maps, logical_map_index);

		chunk = _reduce_vol_get_chunk_map(vol, logical_map_index);

		for (j = 0; j < vol->backing_io_units_per_chunk; j++) {

			if (chunk[j] != REDUCE_EMPTY_MAP_ENTRY) {

				spdk_bit_array_set(vol->allocated_backing_io_units, chunk[j]);
@@ -819,7 +820,7 @@ _write_complete_req(void *_req, int reduce_errno) 
{

	struct spdk_reduce_vol_request *req = _req;

	struct spdk_reduce_vol *vol = req->vol;

	uint64_t logical_map_index, old_chunk_map_index;

	uint64_t logical_map_index, old_logical_map_index;

	uint64_t *old_chunk;

	uint32_t i;
@@ -839,9 +840,9 @@ _write_complete_req(void *_req, int reduce_errno) 


	logical_map_index = req->offset / vol->logical_blocks_per_chunk;



	old_chunk_map_index = vol->pm_logical_map[logical_map_index];

	if (old_chunk_map_index != REDUCE_EMPTY_MAP_ENTRY) {

		old_chunk = _reduce_vol_get_chunk_map(vol, old_chunk_map_index);

	old_logical_map_index = vol->pm_logical_map[logical_map_index];

	if (old_logical_map_index != REDUCE_EMPTY_MAP_ENTRY) {

		old_chunk = _reduce_vol_get_chunk_map(vol, old_logical_map_index);

		for (i = 0; i < vol->backing_io_units_per_chunk; i++) {

			if (old_chunk[i] == REDUCE_EMPTY_MAP_ENTRY) {

				break;
@@ -850,7 +851,7 @@ _write_complete_req(void *_req, int reduce_errno) 
			spdk_bit_array_clear(vol->allocated_backing_io_units, old_chunk[i]);

			old_chunk[i] = REDUCE_EMPTY_MAP_ENTRY;

		}

		spdk_bit_array_clear(vol->allocated_chunk_maps, old_chunk_map_index);

		spdk_bit_array_clear(vol->allocated_chunk_maps, old_logical_map_index);

	}



	/*

test/unit/lib/reduce/reduce.c/reduce_ut.c

+42 −1
Original line number Diff line number Diff line
@@ -611,6 +611,47 @@ _read_write(uint32_t backing_blocklen) 
	spdk_reduce_vol_unload(g_vol, unload_cb, NULL);

	CU_ASSERT(g_reduce_errno == 0);



	/* Overwrite what we just wrote with 0xCC */

	g_vol = NULL;

	g_reduce_errno = -1;

	spdk_reduce_vol_load(&backing_dev, load_cb, NULL);

	CU_ASSERT(g_reduce_errno == 0);

	SPDK_CU_ASSERT_FATAL(g_vol != NULL);

	CU_ASSERT(g_vol->params.vol_size == params.vol_size);

	CU_ASSERT(g_vol->params.chunk_size == params.chunk_size);

	CU_ASSERT(g_vol->params.backing_io_unit_size == params.backing_io_unit_size);



	memset(buf, 0xCC, 2 * params.logical_block_size);

	iov.iov_base = buf;

	iov.iov_len = 2 * params.logical_block_size;

	g_reduce_errno = -1;

	spdk_reduce_vol_writev(g_vol, &iov, 1, 2, 2, write_cb, NULL);

	CU_ASSERT(g_reduce_errno == 0);



	memset(compare_buf, 0xCC, sizeof(compare_buf));

	for (i = 0; i < params.chunk_size / params.logical_block_size; i++) {

		memset(buf, 0xFF, params.logical_block_size);

		iov.iov_base = buf;

		iov.iov_len = params.logical_block_size;

		g_reduce_errno = -1;

		spdk_reduce_vol_readv(g_vol, &iov, 1, i, 1, read_cb, NULL);

		CU_ASSERT(g_reduce_errno == 0);



		switch (i) {

		case 2:

		case 3:

			CU_ASSERT(memcmp(buf, compare_buf, params.logical_block_size) == 0);

			break;

		default:

			CU_ASSERT(spdk_mem_all_zero(buf, params.logical_block_size));

			break;

		}

	}



	g_reduce_errno = -1;

	spdk_reduce_vol_unload(g_vol, unload_cb, NULL);

	CU_ASSERT(g_reduce_errno == 0);



	g_vol = NULL;

	g_reduce_errno = -1;

	spdk_reduce_vol_load(&backing_dev, load_cb, NULL);
@@ -647,7 +688,7 @@ _read_write(uint32_t backing_blocklen) 
		switch (i) {

		case 2:

		case 3:

			memset(compare_buf, 0xAA, sizeof(compare_buf));

			memset(compare_buf, 0xCC, sizeof(compare_buf));

			CU_ASSERT(memcmp(buf, compare_buf, params.logical_block_size) == 0);

			break;

		case 37: