ivampiresp/Spdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

bdev/nvme: Fix aborting fuse commands

Browse Source 
When sending a fused compare and write command, we pass a callback
bdev_nvme_comparev_and_writev_done that we expect to be called twice
before marking the io as completed. In order to detect if a call to
bdev_nvme_comparev_and_writev_done is the first or the second one, we
currently rely on the opcode in cdw0. However, cdw0 may be set to 0,
especially when aborting the command. This may cause use-after-free
issues and this may call the user callbacks twice instead of once.
Use a bit in the nvme_bdev_io instead to keep track of the number of
calls to bdev_nvme_comparev_and_writev_done.

BACKPORTING NOTE:
I had to change the nvme_qpair1 name to ctrlr_ch1, because some
of the changes to multipath are not introduced to LTS version.
-Krzysztof Karas

Signed-off-by: Alex Michon <amichon@kalrayinc.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12180 (master)

(cherry picked from commit 2bc134eb4b)
Change-Id: I0474329e87648e44b08998d0552b2a9dd5d34ac2
Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12489
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: Konrad Sztyber <konrad.sztyber@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
This commit is contained in:
Alex Michon 2022-04-06 14:43:21 +02:00 committed by Keith Lucas
parent 5a3769e871
commit 980d507e9b
2 changed files with 34 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
module/bdev/nvme/bdev_nvme.c
View File

@ -102,6 +102,9 @@ struct nvme_bdev_io {
/** Keeps track if first of fused commands was submitted */
bool first_fused_submitted;
/** Keeps track if first of fused commands was completed */
bool first_fused_completed;
/** Temporary pointer to zone report buffer */
struct spdk_nvme_zns_zone_report *zone_report_buf;
@ -4739,9 +4742,10 @@ bdev_nvme_comparev_and_writev_done(void *ref, const struct spdk_nvme_cpl *cpl)
struct nvme_bdev_io *bio = ref;
/* Compare operation completion */
if ((cpl->cdw0 & 0xFF) == SPDK_NVME_OPC_COMPARE) {
if (!bio->first_fused_completed) {
/* Save compare result for write callback */
bio->cpl = *cpl;
bio->first_fused_completed = true;
return;
}
@ -5274,6 +5278,7 @@ bdev_nvme_comparev_and_writev(struct nvme_bdev_io *bio, struct iovec *cmp_iov, i
if (bdev_io->num_retries == 0) {
bio->first_fused_submitted = false;
bio->first_fused_completed = false;
}
if (!bio->first_fused_submitted) {

29
test/unit/lib/bdev/nvme/bdev_nvme.c/bdev_nvme_ut.c
View File

@ -2463,7 +2463,7 @@ test_abort(void)
const int STRING_SIZE = 32;
const char *attached_names[STRING_SIZE];
struct nvme_bdev *bdev;
struct spdk_bdev_io *write_io, *admin_io, *abort_io;
struct spdk_bdev_io *write_io, *fuse_io, *admin_io, *abort_io;
struct spdk_io_channel *ch1, *ch2;
struct nvme_bdev_channel *nbdev_ch1;
struct nvme_io_path *io_path1;
@ -2501,6 +2501,9 @@ test_abort(void)
write_io = ut_alloc_bdev_io(SPDK_BDEV_IO_TYPE_WRITE, bdev, NULL);
ut_bdev_io_set_buf(write_io);
fuse_io = ut_alloc_bdev_io(SPDK_BDEV_IO_TYPE_COMPARE_AND_WRITE, bdev, NULL);
ut_bdev_io_set_buf(fuse_io);
admin_io = ut_alloc_bdev_io(SPDK_BDEV_IO_TYPE_NVME_ADMIN, bdev, NULL);
admin_io->u.nvme_passthru.cmd.opc = SPDK_NVME_OPC_GET_FEATURES;
@ -2522,6 +2525,7 @@ test_abort(void)
SPDK_CU_ASSERT_FATAL(ch2 != NULL);
write_io->internal.ch = (struct spdk_bdev_channel *)ch1;
fuse_io->internal.ch = (struct spdk_bdev_channel *)ch1;
abort_io->internal.ch = (struct spdk_bdev_channel *)ch1;
/* Aborting the already completed request should fail. */
@ -2586,6 +2590,28 @@ test_abort(void)
CU_ASSERT(write_io->internal.status == SPDK_BDEV_IO_STATUS_ABORTED);
CU_ASSERT(ctrlr_ch1->qpair->num_outstanding_reqs == 0);
/* Aborting the fuse request should succeed. */
fuse_io->internal.in_submit_request = true;
bdev_nvme_submit_request(ch1, fuse_io);
CU_ASSERT(fuse_io->internal.in_submit_request == true);
CU_ASSERT(ctrlr_ch1->qpair->num_outstanding_reqs == 2);
abort_io->u.abort.bio_to_abort = fuse_io;
abort_io->internal.in_submit_request = true;
bdev_nvme_submit_request(ch1, abort_io);
spdk_delay_us(10000);
poll_threads();
CU_ASSERT(abort_io->internal.in_submit_request == false);
CU_ASSERT(abort_io->internal.status == SPDK_BDEV_IO_STATUS_SUCCESS);
CU_ASSERT(ctrlr->adminq.num_outstanding_reqs == 0);
CU_ASSERT(fuse_io->internal.in_submit_request == false);
CU_ASSERT(fuse_io->internal.status == SPDK_BDEV_IO_STATUS_ABORTED);
CU_ASSERT(ctrlr_ch1->qpair->num_outstanding_reqs == 0);
/* Aborting the admin request should succeed. */
admin_io->internal.in_submit_request = true;
bdev_nvme_submit_request(ch1, admin_io);
@ -2651,6 +2677,7 @@ test_abort(void)
poll_threads();
free(write_io);
free(fuse_io);
free(admin_io);
free(abort_io);