From f530abcab33850cbca9d24da7dfcb5aed4673554 Mon Sep 17 00:00:00 2001 From: Alexey Marchuk Date: Tue, 15 Mar 2022 20:57:35 +0400 Subject: [PATCH] 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: Alexey Marchuk Change-Id: I620e43bf5b1abd25cab412fe08346a6d767c9be9 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/11973 Community-CI: Mellanox Build Bot Community-CI: Broadcom CI Tested-by: SPDK CI Jenkins Reviewed-by: Paul Luse Reviewed-by: Jim Harris Reviewed-by: Shuhei Matsumoto --- module/bdev/compress/vbdev_compress.c | 35 ++++++++++++---- test/unit/lib/bdev/compress.c/compress_ut.c | 46 +++++++++++++++++++-- 2 files changed, 70 insertions(+), 11 deletions(-) diff --git a/module/bdev/compress/vbdev_compress.c b/module/bdev/compress/vbdev_compress.c index 498ea8778..2230204d1 100644 --- a/module/bdev/compress/vbdev_compress.c +++ b/module/bdev/compress/vbdev_compress.c @@ -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; diff --git a/test/unit/lib/bdev/compress.c/compress_ut.c b/test/unit/lib/bdev/compress.c/compress_ut.c index 2780f32e4..6c68d389f 100644 --- a/test/unit/lib/bdev/compress.c/compress_ut.c +++ b/test/unit/lib/bdev/compress.c/compress_ut.c @@ -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