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

lib/nvmf: Do not use cb_fn in spdk_nvmf_qpair_disconnect

Browse Source 
Current implementation of spdk_nvmf_qpair_disconnect
saves and calls user's callback correctly only on
the first call. If this function is called when
qpair is already in the process of disconnect, the
cb_fn is called immediately, that may lead to stack
overflow.

In most of places this function is called with
cb_fn = NULL, that means that the real qpair disconnect
is not important for the app logic. Only in several
places (nvmf tgt shutdown flow) that is important to
wait for all qpairs to be disconnected.

Taking into account complexity related to possible stack
overflow, do not pass the cb_fn to spdk_nvmf_qpair_disconnect.
Instead, wait until a list of qpairs is empty in shutdown path.

Next patches will change spdk_nvmf_qpair_disconnect behaviour
when disconnect is in progress and deprecate cb_fn and ctx
parameters.

Fixes issue #2765

Signed-off-by: Alexey Marchuk <alexeymar@nvidia.com>
Change-Id: Ie8d49c88cc009b774b45adab3e37c4dde4395549
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/17163
Reviewed-by: Shuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Michael Haeuptle <michaelhaeuptle@gmail.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
This commit is contained in:
Alexey Marchuk 2023-03-13 09:10:32 +01:00 committed by David Ko
parent d471bca4cf
commit 49415f8ece
2 changed files with 23 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
lib/nvmf/fc_ls.c
View File

@ -2,6 +2,7 @@
* Copyright (C) 2020 Intel Corporation. * Copyright (C) 2020 Intel Corporation.
* Copyright (c) 2018-2019 Broadcom. All Rights Reserved. * Copyright (c) 2018-2019 Broadcom. All Rights Reserved.
* The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries. * The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries.
* Copyright (c) 2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
*/ */
#include "spdk/env.h" #include "spdk/env.h"
@ -1482,8 +1483,8 @@ nvmf_fc_poller_conn_abort_done(void *hwqp, int32_t status, void *cb_args)
if (!conn_args->backend_initiated && (fc_conn->qpair.state != SPDK_NVMF_QPAIR_DEACTIVATING)) { if (!conn_args->backend_initiated && (fc_conn->qpair.state != SPDK_NVMF_QPAIR_DEACTIVATING)) {
/* disconnect qpair from nvmf controller */ /* disconnect qpair from nvmf controller */
spdk_nvmf_qpair_disconnect(&fc_conn->qpair, spdk_nvmf_qpair_disconnect(&fc_conn->qpair, NULL, NULL);
nvmf_fc_disconnect_qpair_cb, &conn_args->cb_info); nvmf_fc_disconnect_qpair_cb(&conn_args->cb_info);
} else { } else {
nvmf_fc_poller_api_perform_cb(&conn_args->cb_info, SPDK_NVMF_FC_POLLER_API_SUCCESS); nvmf_fc_poller_api_perform_cb(&conn_args->cb_info, SPDK_NVMF_FC_POLLER_API_SUCCESS);
} }
@ -1542,8 +1543,8 @@ nvmf_fc_poller_api_del_connection(void *arg)
if (!conn_args->backend_initiated && (fc_conn->qpair.state != SPDK_NVMF_QPAIR_DEACTIVATING)) { if (!conn_args->backend_initiated && (fc_conn->qpair.state != SPDK_NVMF_QPAIR_DEACTIVATING)) {
/* disconnect qpair from nvmf controller */ /* disconnect qpair from nvmf controller */
spdk_nvmf_qpair_disconnect(&fc_conn->qpair, nvmf_fc_disconnect_qpair_cb, spdk_nvmf_qpair_disconnect(&fc_conn->qpair, NULL, NULL);
&conn_args->cb_info); nvmf_fc_disconnect_qpair_cb(&conn_args->cb_info);
} else { } else {
nvmf_fc_poller_api_perform_cb(&conn_args->cb_info, SPDK_NVMF_FC_POLLER_API_SUCCESS); nvmf_fc_poller_api_perform_cb(&conn_args->cb_info, SPDK_NVMF_FC_POLLER_API_SUCCESS);
} }

62
lib/nvmf/nvmf.c
View File

@ -1,7 +1,7 @@
/* SPDX-License-Identifier: BSD-3-Clause /* SPDX-License-Identifier: BSD-3-Clause
* Copyright (C) 2016 Intel Corporation. All rights reserved. * Copyright (C) 2016 Intel Corporation. All rights reserved.
* Copyright (c) 2018-2019, 2021 Mellanox Technologies LTD. All rights reserved. * Copyright (c) 2018-2019, 2021 Mellanox Technologies LTD. All rights reserved.
* Copyright (c) 2021 NVIDIA CORPORATION & AFFILIATES. All rights reserved. * Copyright (c) 2021, 2023 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
*/ */
#include "spdk/stdinc.h" #include "spdk/stdinc.h"
@ -46,7 +46,6 @@ struct nvmf_qpair_disconnect_many_ctx {
struct spdk_nvmf_poll_group *group; struct spdk_nvmf_poll_group *group;
spdk_nvmf_poll_group_mod_done cpl_fn; spdk_nvmf_poll_group_mod_done cpl_fn;
void *cpl_ctx; void *cpl_ctx;
uint32_t count;
}; };
static void static void
@ -222,26 +221,31 @@ nvmf_tgt_create_poll_group(void *io_device, void *ctx_buf)
} }
static void static void
_nvmf_tgt_disconnect_next_qpair(void *ctx) _nvmf_tgt_disconnect_qpairs(void *ctx)
{ {
struct spdk_nvmf_qpair *qpair; struct spdk_nvmf_qpair *qpair, *qpair_tmp;
struct nvmf_qpair_disconnect_many_ctx *qpair_ctx = ctx; struct nvmf_qpair_disconnect_many_ctx *qpair_ctx = ctx;
struct spdk_nvmf_poll_group *group = qpair_ctx->group; struct spdk_nvmf_poll_group *group = qpair_ctx->group;
struct spdk_io_channel *ch; struct spdk_io_channel *ch;
int rc = 0; int rc;
qpair = TAILQ_FIRST(&group->qpairs); TAILQ_FOREACH_SAFE(qpair, &group->qpairs, link, qpair_tmp) {
rc = spdk_nvmf_qpair_disconnect(qpair, NULL, NULL);
if (qpair) { if (rc) {
rc = spdk_nvmf_qpair_disconnect(qpair, _nvmf_tgt_disconnect_next_qpair, ctx); break;
}
} }
if (!qpair || rc != 0) { if (TAILQ_EMPTY(&group->qpairs)) {
/* When the refcount from the channels reaches 0, nvmf_tgt_destroy_poll_group will be called. */ /* When the refcount from the channels reaches 0, nvmf_tgt_destroy_poll_group will be called. */
ch = spdk_io_channel_from_ctx(group); ch = spdk_io_channel_from_ctx(group);
spdk_put_io_channel(ch); spdk_put_io_channel(ch);
free(qpair_ctx); free(qpair_ctx);
return;
} }
/* Some qpairs are in process of being disconnected. Send a message and try to remove them again */
spdk_thread_send_msg(spdk_get_thread(), _nvmf_tgt_disconnect_qpairs, ctx);
} }
static void static void
@ -258,7 +262,7 @@ nvmf_tgt_destroy_poll_group_qpairs(struct spdk_nvmf_poll_group *group)
} }
ctx->group = group; ctx->group = group;
_nvmf_tgt_disconnect_next_qpair(ctx); _nvmf_tgt_disconnect_qpairs(ctx);
} }
struct spdk_nvmf_tgt * struct spdk_nvmf_tgt *
@ -1574,23 +1578,6 @@ fini:
static void nvmf_poll_group_remove_subsystem_msg(void *ctx); static void nvmf_poll_group_remove_subsystem_msg(void *ctx);
static void
remove_subsystem_qpair_cb(void *ctx)
{
struct nvmf_qpair_disconnect_many_ctx *qpair_ctx = ctx;
assert(qpair_ctx->count > 0);
qpair_ctx->count--;
if (qpair_ctx->count == 0) {
/* All of the asynchronous callbacks for this context have been
* completed. Call nvmf_poll_group_remove_subsystem_msg() again
* to check if all associated qpairs for this subsystem have
* been removed from the poll group.
*/
nvmf_poll_group_remove_subsystem_msg(ctx);
}
}
static void static void
nvmf_poll_group_remove_subsystem_msg(void *ctx) nvmf_poll_group_remove_subsystem_msg(void *ctx)
{ {
@ -1604,38 +1591,23 @@ nvmf_poll_group_remove_subsystem_msg(void *ctx)
group = qpair_ctx->group; group = qpair_ctx->group;
subsystem = qpair_ctx->subsystem; subsystem = qpair_ctx->subsystem;
/* Initialize count to 1. This acts like a ref count, to ensure that if spdk_nvmf_qpair_disconnect
* immediately invokes the callback (i.e. the qpairs is already in process of being disconnected)
* that we don't recursively call nvmf_poll_group_remove_subsystem_msg before we've iterated the
* full list of qpairs.
*/
qpair_ctx->count = 1;
TAILQ_FOREACH_SAFE(qpair, &group->qpairs, link, qpair_tmp) { TAILQ_FOREACH_SAFE(qpair, &group->qpairs, link, qpair_tmp) {
if ((qpair->ctrlr != NULL) && (qpair->ctrlr->subsys == subsystem)) { if ((qpair->ctrlr != NULL) && (qpair->ctrlr->subsys == subsystem)) {
qpairs_found = true; qpairs_found = true;
qpair_ctx->count++; rc = spdk_nvmf_qpair_disconnect(qpair, NULL, NULL);
rc = spdk_nvmf_qpair_disconnect(qpair, remove_subsystem_qpair_cb, ctx);
if (rc) { if (rc) {
break; break;
} }
} }
} }
qpair_ctx->count--;
if (!qpairs_found) { if (!qpairs_found) {
_nvmf_poll_group_remove_subsystem_cb(ctx, 0); _nvmf_poll_group_remove_subsystem_cb(ctx, 0);
return; return;
} }
if (qpair_ctx->count == 0 || rc) { /* Some qpairs are in process of being disconnected. Send a message and try to remove them again */
/* If count == 0, it means there were some qpairs in the poll group but they
* were already in process of being disconnected. So we send a message to this
* same thread so that this function executes again later. We won't actually
* invoke the remove_subsystem_cb until all of the qpairs are actually removed
* from the poll group.
*/
spdk_thread_send_msg(spdk_get_thread(), nvmf_poll_group_remove_subsystem_msg, ctx); spdk_thread_send_msg(spdk_get_thread(), nvmf_poll_group_remove_subsystem_msg, ctx);
}
} }
void void