Commit f530abca authored by Alexey Marchuk's avatar Alexey Marchuk Committed by Tomasz Zawadzki
Browse files

bdev/compress: Verify mbuf chain if the driver doesn't suppot SGL 



With recent changes libreduce should provide correct buffers
if the driver doesn't support SGL in/out. This patch verifies
that we don't use SGLs when they are not supported.
Since even a single buffer can be split on 2MB page
boundary, it is not enough just to check iovs count.

Added asserts that the first elements of mbufs are
not null to avoid scan build errors

Signed-off-by: default avatarAlexey Marchuk <alexeymar@mellanox.com>
Change-Id: I620e43bf5b1abd25cab412fe08346a6d767c9be9
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11973


Community-CI: Mellanox Build Bot
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarPaul Luse <paul.e.luse@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
parent 731ddc71

module/bdev/compress/vbdev_compress.c

+28 −7
Original line number Diff line number Diff line
@@ -565,6 +565,7 @@ _compress_operation(struct spdk_reduce_backing_dev *backing_dev, struct iovec *s 
		rc = -ENOMEM;

		goto error_get_src;

	}

	assert(src_mbufs[0]);



	rc = rte_pktmbuf_alloc_bulk(g_mbuf_mp, (struct rte_mbuf **)&dst_mbufs[0], dst_iovcnt);

	if (rc) {
@@ -572,28 +573,48 @@ _compress_operation(struct spdk_reduce_backing_dev *backing_dev, struct iovec *s 
		rc = -ENOMEM;

		goto error_get_dst;

	}

	assert(dst_mbufs[0]);



	/* There is a 1:1 mapping between a bdev_io and a compression operation, but

	 * all compression PMDs that SPDK uses support chaining so build our mbuf chain

	 * and associate with our single comp_op.

	/* There is a 1:1 mapping between a bdev_io and a compression operation

	 * Some PMDs that SPDK uses don't support chaining, but reduce library should

	 * provide correct buffers

	 * Build our mbuf chain and associate it with our single comp_op.

	 */



	rc = _setup_compress_mbuf(&src_mbufs[0], &src_mbuf_total, &total_length,

	rc = _setup_compress_mbuf(src_mbufs, &src_mbuf_total, &total_length,

				  src_iovs, src_iovcnt, reduce_cb_arg);

	if (rc < 0) {

		goto error_src_dst;

	}

	if (!comp_bdev->backing_dev.sgl_in && src_mbufs[0]->next != NULL) {

		if (src_iovcnt == 1) {

			SPDK_ERRLOG("Src buffer crosses 2MB boundary but driver %s doesn't support SGL input\n",

				    comp_bdev->drv_name);

		} else {

			SPDK_ERRLOG("Driver %s doesn't support SGL input\n", comp_bdev->drv_name);

		}

		rc = -EINVAL;

		goto error_src_dst;

	}



	comp_op->m_src = src_mbufs[0];

	comp_op->src.offset = 0;

	comp_op->src.length = total_length;



	/* setup dst mbufs, for the current test being used with this code there's only one vector */

	rc = _setup_compress_mbuf(&dst_mbufs[0], &dst_mbuf_total, NULL,

	rc = _setup_compress_mbuf(dst_mbufs, &dst_mbuf_total, NULL,

				  dst_iovs, dst_iovcnt, reduce_cb_arg);

	if (rc < 0) {

		goto error_src_dst;

	}

	if (!comp_bdev->backing_dev.sgl_out && dst_mbufs[0]->next != NULL) {

		if (dst_iovcnt == 1) {

			SPDK_ERRLOG("Dst buffer crosses 2MB boundary but driver %s doesn't support SGL output\n",

				    comp_bdev->drv_name);

		} else {

			SPDK_ERRLOG("Driver %s doesn't support SGL output\n", comp_bdev->drv_name);

		}

		rc = -EINVAL;

		goto error_src_dst;

	}



	comp_op->m_dst = dst_mbufs[0];

	comp_op->dst.offset = 0;

test/unit/lib/bdev/compress.c/compress_ut.c

+42 −4
Original line number Diff line number Diff line
@@ -468,15 +468,12 @@ rte_compressdev_enqueue_burst(uint8_t dev_id, uint16_t qp_id, struct rte_comp_op 
	case FAKE_ENQUEUE_BUSY:

		op->status = RTE_COMP_OP_STATUS_NOT_PROCESSED;

		return 0;

		break;

	case FAKE_ENQUEUE_SUCCESS:

		op->status = RTE_COMP_OP_STATUS_SUCCESS;

		return 1;

		break;

	case FAKE_ENQUEUE_ERROR:

		op->status = RTE_COMP_OP_STATUS_ERROR;

		return 0;

		break;

	default:

		break;

	}
@@ -501,7 +498,6 @@ rte_compressdev_enqueue_burst(uint8_t dev_id, uint16_t qp_id, struct rte_comp_op 
		ut_boundary_alloc = false;

	}





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

		CU_ASSERT(op_mbuf[i]->buf_addr == exp_mbuf[i]->buf_addr);

		CU_ASSERT(op_mbuf[i]->buf_iova == exp_mbuf[i]->buf_iova);
@@ -546,6 +542,7 @@ test_setup(void) 
	thread = spdk_thread_create(NULL, NULL);

	spdk_set_thread(thread);



	g_comp_bdev.drv_name = "test";

	g_comp_bdev.reduce_thread = thread;

	g_comp_bdev.backing_dev.unmap = _comp_reduce_unmap;

	g_comp_bdev.backing_dev.readv = _comp_reduce_readv;
@@ -554,6 +551,8 @@ test_setup(void) 
	g_comp_bdev.backing_dev.decompress = _comp_reduce_decompress;

	g_comp_bdev.backing_dev.blocklen = 512;

	g_comp_bdev.backing_dev.blockcnt = 1024 * 16;

	g_comp_bdev.backing_dev.sgl_in = true;

	g_comp_bdev.backing_dev.sgl_out = true;



	g_comp_bdev.device_qp = &g_device_qp;

	g_comp_bdev.device_qp->device = &g_device;
@@ -758,6 +757,25 @@ test_compress_operation(void) 
	CU_ASSERT(TAILQ_EMPTY(&g_comp_bdev.queued_comp_ops) == true);

	CU_ASSERT(rc == 0);



	/* test sgl out failure */

	g_comp_bdev.backing_dev.sgl_out = false;

	CU_ASSERT(TAILQ_EMPTY(&g_comp_bdev.queued_comp_ops) == true);

	rc = _compress_operation(&g_comp_bdev.backing_dev, &src_iovs[0], 1,

				 &dst_iovs[0], dst_iovcnt, true, &cb_arg);

	CU_ASSERT(rc == -EINVAL);

	CU_ASSERT(TAILQ_EMPTY(&g_comp_bdev.queued_comp_ops) == true);

	g_comp_bdev.backing_dev.sgl_out = true;



	/* test sgl in failure */

	g_comp_bdev.backing_dev.sgl_in = false;

	CU_ASSERT(TAILQ_EMPTY(&g_comp_bdev.queued_comp_ops) == true);

	rc = _compress_operation(&g_comp_bdev.backing_dev, &src_iovs[0], src_iovcnt,

				 &dst_iovs[0], 1, true, &cb_arg);

	CU_ASSERT(rc == -EINVAL);

	CU_ASSERT(TAILQ_EMPTY(&g_comp_bdev.queued_comp_ops) == true);

	g_comp_bdev.backing_dev.sgl_in = true;





}



static void
@@ -902,6 +920,26 @@ test_compress_operation_cross_boundary(void) 
				 &dst_iovs[0], dst_iovcnt, false, &cb_arg);

	CU_ASSERT(TAILQ_EMPTY(&g_comp_bdev.queued_comp_ops) == true);

	CU_ASSERT(rc == 0);



	/* Single input iov is split on page boundary, sgl_in is not supported */

	g_comp_bdev.backing_dev.sgl_in = false;

	g_small_size_counter = 0;

	g_small_size_modify = 1;

	g_small_size = 0x800;

	rc = _compress_operation(&g_comp_bdev.backing_dev, src_iovs, 1,

				 dst_iovs, 1, false, &cb_arg);

	CU_ASSERT(rc == -EINVAL);

	g_comp_bdev.backing_dev.sgl_in = true;



	/* Single output iov is split on page boundary, sgl_out is not supported */

	g_comp_bdev.backing_dev.sgl_out = false;

	g_small_size_counter = 0;

	g_small_size_modify = 2;

	g_small_size = 0x800;

	rc = _compress_operation(&g_comp_bdev.backing_dev, src_iovs, 1,

				 dst_iovs, 1, false, &cb_arg);

	CU_ASSERT(rc == -EINVAL);

	g_comp_bdev.backing_dev.sgl_out = true;

}



static void