From a24d54903833ce46e5745cf0d7b82cb9546c6482 Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Mon, 30 Aug 2021 14:01:40 -0700 Subject: [PATCH] bdev/nvme: bdev_nvme_attach_controller now has a multipath parameter This parameter may have the values "disable" or "failover". The default is failover to match existing behavior. In the future we expect to change the default behavior to disable. Further, we expect to add an "enable" option soon to do full multipath. Change-Id: Iebbdc9b95f23101f18d64e085933463498e627be Signed-off-by: Ben Walker Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/9343 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Changpeng Liu Reviewed-by: Aleksey Marchuk --- doc/jsonrpc.md | 1 + module/bdev/nvme/bdev_nvme_rpc.c | 32 +++++++++++++++++++++++++++++-- scripts/rpc.py | 4 +++- scripts/rpc/bdev.py | 6 +++++- test/nvmf/host/multicontroller.sh | 5 +++++ 5 files changed, 44 insertions(+), 4 deletions(-) diff --git a/doc/jsonrpc.md b/doc/jsonrpc.md index e0d7101b2..f05c8262e 100644 --- a/doc/jsonrpc.md +++ b/doc/jsonrpc.md @@ -2892,6 +2892,7 @@ prchk_guard | Optional | bool | Enable checking of PI guar hdgst | Optional | bool | Enable TCP header digest ddgst | Optional | bool | Enable TCP data digest fabrics_connect_timeout_us | Optional | bool | Timeout for fabrics connect (in microseconds) +multipath | Optional | string | Multipathing behavior: disable, failover. Default is failover. #### Example diff --git a/module/bdev/nvme/bdev_nvme_rpc.c b/module/bdev/nvme/bdev_nvme_rpc.c index 50e449fcc..98854a757 100644 --- a/module/bdev/nvme/bdev_nvme_rpc.c +++ b/module/bdev/nvme/bdev_nvme_rpc.c @@ -180,6 +180,7 @@ struct rpc_bdev_nvme_attach_controller { bool prchk_reftag; bool prchk_guard; uint64_t fabrics_connect_timeout_us; + char *multipath; struct spdk_nvme_ctrlr_opts opts; }; @@ -196,6 +197,7 @@ free_rpc_bdev_nvme_attach_controller(struct rpc_bdev_nvme_attach_controller *req free(req->hostnqn); free(req->hostaddr); free(req->hostsvcid); + free(req->multipath); } static const struct spdk_json_object_decoder rpc_bdev_nvme_attach_controller_decoders[] = { @@ -216,6 +218,7 @@ static const struct spdk_json_object_decoder rpc_bdev_nvme_attach_controller_dec {"hdgst", offsetof(struct rpc_bdev_nvme_attach_controller, opts.header_digest), spdk_json_decode_bool, true}, {"ddgst", offsetof(struct rpc_bdev_nvme_attach_controller, opts.data_digest), spdk_json_decode_bool, true}, {"fabrics_connect_timeout_us", offsetof(struct rpc_bdev_nvme_attach_controller, opts.fabrics_connect_timeout_us), spdk_json_decode_uint64, true}, + {"multipath", offsetof(struct rpc_bdev_nvme_attach_controller, multipath), spdk_json_decode_string, true}, }; #define NVME_MAX_BDEVS_PER_RPC 128 @@ -387,8 +390,31 @@ rpc_bdev_nvme_attach_controller(struct spdk_jsonrpc_request *request, ctrlr = nvme_ctrlr_get_by_name(ctx->req.name); if (ctrlr) { - /* This controller already exists. Verify the parameters match sufficiently. */ - opts = spdk_nvme_ctrlr_get_opts(ctrlr->ctrlr); + if (ctx->req.multipath == NULL) { + /* For now, this means add a failover path. This maintains backward compatibility + * with past behavior. In the future, this behavior will change to "disable". */ + SPDK_ERRLOG("The multipath parameter was not specified to bdev_nvme_attach_controller but " + "it was used to add a failover path. This behavior will default to rejecting " + "the request in the future. Specify the 'multipath' parameter to control the behavior"); + ctx->req.multipath = strdup("failover"); + } + + /* This controller already exists. Check what the user wants to do. */ + if (strcasecmp(ctx->req.multipath, "disable") == 0) { + /* The user does not want to do any form of multipathing. */ + spdk_jsonrpc_send_error_response_fmt(request, -EALREADY, + "A controller named %s already exists and multipath is disabled\n", + ctx->req.name); + goto cleanup; + } else if (strcasecmp(ctx->req.multipath, "failover") == 0) { + /* The user wants to add this as a failover path. */ + } else { + /* Invalid multipath option */ + spdk_jsonrpc_send_error_response_fmt(request, -EINVAL, + "Invalid multipath parameter: %s\n", + ctx->req.multipath); + goto cleanup; + } if (strncmp(trid.subnqn, spdk_nvme_ctrlr_get_transport_id(ctrlr->ctrlr)->subnqn, @@ -400,6 +426,8 @@ rpc_bdev_nvme_attach_controller(struct spdk_jsonrpc_request *request, goto cleanup; } + opts = spdk_nvme_ctrlr_get_opts(ctrlr->ctrlr); + if (strncmp(ctx->req.opts.hostnqn, opts->hostnqn, SPDK_NVMF_NQN_MAX_LEN) != 0) { /* Different HOSTNQN is not allowed when specifying the same controller name. */ spdk_jsonrpc_send_error_response_fmt(request, -EINVAL, diff --git a/scripts/rpc.py b/scripts/rpc.py index f6575f2d0..235923b67 100755 --- a/scripts/rpc.py +++ b/scripts/rpc.py @@ -526,7 +526,8 @@ if __name__ == "__main__": prchk_guard=args.prchk_guard, hdgst=args.hdgst, ddgst=args.ddgst, - fabrics_timeout=args.fabrics_timeout)) + fabrics_timeout=args.fabrics_timeout, + multipath=args.multipath)) p = subparsers.add_parser('bdev_nvme_attach_controller', aliases=['construct_nvme_bdev'], help='Add bdevs with nvme backend') @@ -556,6 +557,7 @@ if __name__ == "__main__": p.add_argument('-d', '--ddgst', help='Enable TCP data digest.', action='store_true') p.add_argument('--fabrics-timeout', type=int, help='Fabrics connect timeout in microseconds') + p.add_argument('-x', '--multipath', help='Set multipath behavior (disable, failover)') p.set_defaults(func=bdev_nvme_attach_controller) def bdev_nvme_get_controllers(args): diff --git a/scripts/rpc/bdev.py b/scripts/rpc/bdev.py index b892473b0..a8684ffaa 100644 --- a/scripts/rpc/bdev.py +++ b/scripts/rpc/bdev.py @@ -511,7 +511,7 @@ def bdev_nvme_set_hotplug(client, enable, period_us=None): def bdev_nvme_attach_controller(client, name, trtype, traddr, adrfam=None, trsvcid=None, priority=None, subnqn=None, hostnqn=None, hostaddr=None, hostsvcid=None, prchk_reftag=None, prchk_guard=None, - hdgst=None, ddgst=None, fabrics_timeout=None): + hdgst=None, ddgst=None, fabrics_timeout=None, multipath=None): """Construct block device for each NVMe namespace in the attached controller. Args: @@ -530,6 +530,7 @@ def bdev_nvme_attach_controller(client, name, trtype, traddr, adrfam=None, trsvc hdgst: Enable TCP header digest (optional) ddgst: Enable TCP data digest (optional) fabrics_timeout: Fabrics connect timeout in us (optional) + multipath: The behavior when multiple paths are created ("disable", "failover"; failover if not specified) Returns: Names of created block devices. @@ -574,6 +575,9 @@ def bdev_nvme_attach_controller(client, name, trtype, traddr, adrfam=None, trsvc if fabrics_timeout: params['fabrics_connect_timeout_us'] = fabrics_timeout + if multipath: + params['multipath'] = multipath + return client.call('bdev_nvme_attach_controller', params) diff --git a/test/nvmf/host/multicontroller.sh b/test/nvmf/host/multicontroller.sh index 3a8b077b5..8798428d7 100755 --- a/test/nvmf/host/multicontroller.sh +++ b/test/nvmf/host/multicontroller.sh @@ -64,6 +64,11 @@ NOT $rpc_py -s $bdevperf_rpc_sock bdev_nvme_attach_controller -b NVMe0 -t $TEST_ NOT $rpc_py -s $bdevperf_rpc_sock bdev_nvme_attach_controller -b NVMe0 -t $TEST_TRANSPORT -a $NVMF_FIRST_TARGET_IP \ -s $NVMF_PORT -f ipv4 -n nqn.2016-06.io.spdk:cnode2 -i $NVMF_FIRST_TARGET_IP -c $NVMF_HOST_FIRST_PORT +# try to attach with the same controller name and multipathing disabled. Should fail +NOT $rpc_py -s $bdevperf_rpc_sock bdev_nvme_attach_controller -b NVMe0 -t $TEST_TRANSPORT -a $NVMF_FIRST_TARGET_IP \ + -s $NVMF_PORT -f ipv4 -n nqn.2016-06.io.spdk:cnode1 -i $NVMF_FIRST_TARGET_IP -c $NVMF_HOST_FIRST_PORT \ + -x disable + # Add a second path without specifying the host information. Should pass. $rpc_py -s $bdevperf_rpc_sock bdev_nvme_attach_controller -b NVMe0 -t $TEST_TRANSPORT -a $NVMF_FIRST_TARGET_IP \ -s $NVMF_SECOND_PORT -f ipv4 -n nqn.2016-06.io.spdk:cnode1