Commit 674abd98 authored by Jim Harris's avatar Jim Harris
Browse files

nvmf: initialize trid param in get_***_trid paths 



When removing a listener, for example with
nvmf_subsystem_remove_listener RPC, we use the concept of a
"listen trid" to determine which existing connections
should be disconnected.

This listen trid has the trtype, adrfam, traddr and trsvcid
defined, but *not* the subnqn.  We use the subsystem pointer
itself to match the subsystem.

nvmf_stop_listen_disconnect_qpairs gets the listen trid
for each qpair, compares it to the trid passed by the
RPC, and if it matches, then it compares the subsystem
pointers and will disconnect the qpair if it matches.

The problem is that the spdk_nvmf_qpair_get_listen_trid
path does not initialize the subnqn to an empty string,
and in this case the caller does not initialize it either.
So sometimes the subnqn on the stack used to get the
qpair's listen trid ends up with some garbage as the subnqn
string, which causes the transport_id_compare to fail, and
then the qpair won't get disconnected even if the other
trid fields and subsystem pointers match.

For the failover.sh test, this means that the qpair doesn't
get disconnected, so we never go down the reset path
on the initiator side and don't see the "Resetting" strings
expected in the log.

This similarly impacts the host/timeout.sh test, which is
also fixed by this patch.  There were multiple failing
signatures, all related to remove_listener not working
correctly due to this bug.

While the get_listen_trid path is the one that caused
these bugs, the get_local_trid and get_peer_trid paths
have similar problems, so they are similarly fixed in
this patch.

Fixes issue #2862.
Fixes issue #2595.
Fixes issue #2865.
Fixes issue #2864.

Signed-off-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/17788

 (master)

(cherry picked from commit baf250e5)
Change-Id: I36eb519cd1f434d50eebf724ecd6dbc2528288c3
Signed-off-by: default avatarKrzysztof Karas <krzysztof.karas@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/17828


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
parent b1ad3a29

include/spdk/nvmf.h

+20 −1
Original line number Diff line number Diff line
@@ -304,6 +304,11 @@ int spdk_nvmf_qpair_disconnect(struct spdk_nvmf_qpair *qpair, nvmf_qpair_disconn 
/**

 * Get the peer's transport ID for this queue pair.

 *

 * This function will first zero the trid structure, and then fill

 * in the relevant trid fields to identify the listener. The relevant

 * fields will depend on the transport, but the subnqn will never

 * be a relevant field for purposes of this function.

 *

 * \param qpair The NVMe-oF qpair

 * \param trid Output parameter that will contain the transport id.

 *
@@ -316,6 +321,11 @@ int spdk_nvmf_qpair_get_peer_trid(struct spdk_nvmf_qpair *qpair, 
/**

 * Get the local transport ID for this queue pair.

 *

 * This function will first zero the trid structure, and then fill

 * in the relevant trid fields to identify the listener. The relevant

 * fields will depend on the transport, but the subnqn will never

 * be a relevant field for purposes of this function.

 *

 * \param qpair The NVMe-oF qpair

 * \param trid Output parameter that will contain the transport id.

 *
@@ -328,6 +338,11 @@ int spdk_nvmf_qpair_get_local_trid(struct spdk_nvmf_qpair *qpair, 
/**

 * Get the associated listener transport ID for this queue pair.

 *

 * This function will first zero the trid structure, and then fill

 * in the relevant trid fields to identify the listener. The relevant

 * fields will depend on the transport, but the subnqn will never

 * be a relevant field for purposes of this function.

 *

 * \param qpair The NVMe-oF qpair

 * \param trid Output parameter that will contain the transport id.

 *
@@ -1145,8 +1160,12 @@ spdk_nvmf_transport_stop_listen(struct spdk_nvmf_transport *transport, 
 * qpairs that are connected to the specified listener. Because

 * this function disconnects the qpairs, it has to be asynchronous.

 *

 * The subsystem is matched using the subsystem parameter, not the

 * subnqn field in the trid.

 *

 * \param transport The transport associated with the listen address.

 * \param trid The address to stop listening at.

 * \param trid The address to stop listening at. subnqn must be an empty

 *             string.

 * \param subsystem The subsystem to match for qpairs with the specified

 *                  trid. If NULL, it will disconnect all qpairs with the

 *                  specified trid.

lib/nvmf/nvmf.c

+3 −0
Original line number Diff line number Diff line
@@ -1301,6 +1301,7 @@ int 
spdk_nvmf_qpair_get_peer_trid(struct spdk_nvmf_qpair *qpair,

			      struct spdk_nvme_transport_id *trid)

{

	memset(trid, 0, sizeof(*trid));

	return nvmf_transport_qpair_get_peer_trid(qpair, trid);

}
@@ -1308,6 +1309,7 @@ int 
spdk_nvmf_qpair_get_local_trid(struct spdk_nvmf_qpair *qpair,

			       struct spdk_nvme_transport_id *trid)

{

	memset(trid, 0, sizeof(*trid));

	return nvmf_transport_qpair_get_local_trid(qpair, trid);

}
@@ -1315,6 +1317,7 @@ int 
spdk_nvmf_qpair_get_listen_trid(struct spdk_nvmf_qpair *qpair,

				struct spdk_nvme_transport_id *trid)

{

	memset(trid, 0, sizeof(*trid));

	return nvmf_transport_qpair_get_listen_trid(qpair, trid);

}

lib/nvmf/transport.c

+5 −0
Original line number Diff line number Diff line
@@ -423,6 +423,11 @@ spdk_nvmf_transport_stop_listen_async(struct spdk_nvmf_transport *transport, 
{

	struct nvmf_stop_listen_ctx *ctx;



	if (trid->subnqn[0] != '\0') {

		SPDK_ERRLOG("subnqn should be empty, use subsystem pointer instead\n");

		return -EINVAL;

	}



	ctx = calloc(1, sizeof(struct nvmf_stop_listen_ctx));

	if (ctx == NULL) {

		return -ENOMEM;