Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAOS-14181 control,bio,mgmt: Return NVMe details over dRPC #13382

Merged
merged 13 commits into from
Dec 13, 2023
Merged

DAOS-14181 control,bio,mgmt: Return NVMe details over dRPC #13382

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
729d730
DAOS-14181 control,bio,mgmt: Return NVMe details over dRPC
tanabarr Nov 23, 2023
6549812
Merge remote-tracking branch 'origin/master' into tanabarr/control-re…
tanabarr Nov 27, 2023
86310ad
address review comments from knard-intel pt1
tanabarr Nov 27, 2023
6acfb2b
address review comments from knard-intel pt2
tanabarr Nov 27, 2023
a101b2d
address review comments from NiuYawei
tanabarr Nov 28, 2023
d057d2c
address review comments from kjacque
tanabarr Nov 28, 2023
5cb4632
address review comments from mjmac
tanabarr Nov 29, 2023
6d8db1e
fix storage query functional tests
tanabarr Nov 29, 2023
735a3cf
appease python flake linter
tanabarr Nov 29, 2023
a49c4aa
clang-format fixes
tanabarr Nov 29, 2023
7ce581d
Merge remote-tracking branch 'origin/master' into tanabarr/control-re…
tanabarr Dec 4, 2023
6360b73
address review comments from wangshilong
tanabarr Dec 4, 2023
7afc2f7
Merge remote-tracking branch 'origin/master' into tanabarr/control-re…
tanabarr Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
200 changes: 175 additions & 25 deletions src/bio/bio_config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,20 @@
#define D_LOGFAC DD_FAC(bio)

#include <spdk/file.h>
#include <spdk/string.h>
#include <spdk/util.h>
#include <spdk/json.h>
#include <spdk/thread.h>
#include <spdk/nvme.h>
#include <spdk/nvmf_spec.h>
#include <daos_srv/control.h>

#include "bio_internal.h"

/* JSON tags should match encode/decode logic in src/control/server/storage/bdev/backend_json.go */

#define JSON_MAX_CHARS 4096

struct
json_config_ctx {
/* Current "subsystems" array */
Expand Down Expand Up @@ -181,21 +186,26 @@ is_addr_in_allowlist(char *pci_addr, const struct spdk_pci_addr *allowlist,
static int
traddr_to_vmd(char *dst, const char *src)
{
char *traddr_tmp = NULL, *vmd_addr = NULL;
char *traddr_tmp = NULL;
char *vmd_addr = NULL;
char *ptr;
const char ch = ':';
char addr_split[3];
int position;
int iteration;
int n, rc = 0;
int vmd_addr_left_len;
int len;

D_ALLOC(vmd_addr, SPDK_NVMF_TRADDR_MAX_LEN + 1);
if (vmd_addr == NULL)
return -DER_NOMEM;

strncat(vmd_addr, "0000:", SPDK_NVMF_TRADDR_MAX_LEN);
vmd_addr_left_len = SPDK_NVMF_TRADDR_MAX_LEN - strlen(vmd_addr);
len = strnlen(vmd_addr, SPDK_NVMF_TRADDR_MAX_LEN);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] if SPDK_NVMF_TRADDR_MAX_LEN should be valid, it should be like:
len = strnlen(vm_addr, SPDK_NVMF_TRADDR_MAX_LEN + 1)
if (len == 0 || len == SPDK_NVMF_TRADDR_MAX_LEN + 1)
return -DER_INVAL;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? the max refers to the maximum number of characters which is what strnlen returns (excluding the null terminator).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If max string size is SPDK_NVMF_TRADDR_MAX_LEN(including the null terminator) then code is correct for me, if max string is is SPDK_NVMF_TRADDR_MAX_LEN + 1 (including the null terminator), it should be what i suggested.
Since when allocating memory, we do D_ALLOC(vmd_addr, SPDK_NVMF_TRADDR_MAX_LEN + 1), i suppose it should be second case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I see, that correction is allowing for the case where strlen(addr) == MAX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if ((len == 0) || (len == SPDK_NVMF_TRADDR_MAX_LEN))
return -DER_INVAL;
vmd_addr_left_len = SPDK_NVMF_TRADDR_MAX_LEN - len;

D_STRNDUP(traddr_tmp, src, SPDK_NVMF_TRADDR_MAX_LEN);
if (traddr_tmp == NULL) {
Expand Down Expand Up @@ -320,15 +330,14 @@ read_config(const char *config_file, struct json_config_ctx *ctx)

json = read_file(config_file, &json_size);
if (!json) {
D_ERROR("Read config file %s failed: '%s'\n",
config_file, strerror(errno));
D_ERROR("Read config file %s failed: '%s'\n", config_file, spdk_strerror(errno));
return -DER_INVAL;
}

rc = spdk_json_parse(json, json_size, NULL, 0, &end,
SPDK_JSON_PARSE_FLAG_ALLOW_COMMENTS);
if (rc < 0) {
D_ERROR("Parsing config failed: %s\n", strerror(-rc));
D_ERROR("Parsing config failed: %s\n", spdk_strerror(-rc));
D_GOTO(free_json, rc = -DER_INVAL);
}

Expand Down Expand Up @@ -380,7 +389,7 @@ load_vmd_subsystem_config(struct json_config_ctx *ctx, bool *vmd_enabled)
rc = spdk_json_decode_object(ctx->config_it, config_entry_decoders,
SPDK_COUNTOF(config_entry_decoders), &cfg);
if (rc < 0) {
D_ERROR("Failed to decode config entry: %s\n", strerror(-rc));
D_ERROR("Failed to decode config entry: %s\n", spdk_strerror(-rc));
return -DER_INVAL;
}

Expand All @@ -405,7 +414,7 @@ add_traddrs_from_bdev_subsys(struct json_config_ctx *ctx, bool vmd_enabled,
rc = spdk_json_decode_object(ctx->config_it, config_entry_decoders,
SPDK_COUNTOF(config_entry_decoders), &cfg);
if (rc < 0) {
D_ERROR("Failed to decode config entry: %s\n", strerror(-rc));
D_ERROR("Failed to decode config entry: %s\n", spdk_strerror(-rc));
return -DER_INVAL;
}

Expand Down Expand Up @@ -491,7 +500,7 @@ check_name_from_bdev_subsys(struct json_config_ctx *ctx)
rc = spdk_json_decode_object(ctx->config_it, config_entry_decoders,
SPDK_COUNTOF(config_entry_decoders), &cfg);
if (rc < 0) {
D_ERROR("Failed to decode config entry: %s\n", strerror(-rc));
D_ERROR("Failed to decode config entry: %s\n", spdk_strerror(-rc));
return -DER_INVAL;
}

Expand Down Expand Up @@ -552,7 +561,7 @@ decode_subsystem_configs(struct spdk_json_val *json_val, struct json_config_ctx
rc = spdk_json_decode_object(json_val, subsystem_decoders, SPDK_COUNTOF(subsystem_decoders),
ctx);
if (rc < 0) {
D_ERROR("Failed to parse vmd subsystem: %s\n", strerror(-rc));
D_ERROR("Failed to parse vmd subsystem: %s\n", spdk_strerror(-rc));
return -DER_INVAL;
}

Expand Down Expand Up @@ -615,11 +624,12 @@ check_vmd_status(struct json_config_ctx *ctx, struct spdk_json_val *vmd_ss, bool
{
int rc;

D_ASSERT(vmd_enabled != NULL);
D_ASSERT(*vmd_enabled == false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be an assertion? If the calling code changes somehow so that this was set to true elsewhere, should this really fail, or should it just return early?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a valid assertion, it ensures that when check_vmd_status is called, vmd has not been enabled. check_vmd_status should only be called once and when it is vmd should not have been enabled yet, any situation to the contrary means the flow has been corrupted. if the flow is adjusted in the calling code then this will fail and prompt the person making the change to fix the flow.


if (vmd_ss == NULL)
return 0;

D_ASSERT(vmd_enabled != NULL);

rc = decode_subsystem_configs(vmd_ss, ctx);
if (rc != 0)
return rc;
Expand All @@ -642,20 +652,23 @@ check_vmd_status(struct json_config_ctx *ctx, struct spdk_json_val *vmd_ss, bool
* \param[in] nvme_conf JSON config file path
* \param[out] opts SPDK environment options
* \param[out] roles global nvme bdev roles
* \param[out] vmd_enabled global VMD-enablement flag
*
* \returns Zero on success, negative on failure (DER)
*/
int
bio_add_allowed_alloc(const char *nvme_conf, struct spdk_env_opts *opts, int *roles)
bio_add_allowed_alloc(const char *nvme_conf, struct spdk_env_opts *opts, int *roles,
bool *vmd_enabled)
{
struct json_config_ctx *ctx;
struct spdk_json_val *bdev_ss = NULL;
struct spdk_json_val *vmd_ss = NULL;
bool vmd_enabled = false;
struct spdk_json_val *vmd_ss = NULL;
int rc = 0;

D_ASSERT(nvme_conf != NULL);
D_ASSERT(opts != NULL);
D_ASSERT(vmd_enabled != NULL);
D_ASSERT(*vmd_enabled == false);

D_ALLOC_PTR(ctx);
if (ctx == NULL)
Expand All @@ -668,7 +681,7 @@ bio_add_allowed_alloc(const char *nvme_conf, struct spdk_env_opts *opts, int *ro
/* Capture subsystems array */
rc = spdk_json_find_array(ctx->values, "subsystems", NULL, &ctx->subsystems);
if (rc < 0) {
D_ERROR("Failed to find subsystems key: %s\n", strerror(-rc));
D_ERROR("Failed to find subsystems key: %s\n", spdk_strerror(-rc));
D_GOTO(out, rc = -DER_INVAL);
}

Expand All @@ -684,14 +697,15 @@ bio_add_allowed_alloc(const char *nvme_conf, struct spdk_env_opts *opts, int *ro
rc = spdk_json_decode_object(ctx->subsystems_it, subsystem_decoders,
SPDK_COUNTOF(subsystem_decoders), ctx);
if (rc < 0) {
D_ERROR("Failed to parse subsystem configuration: %s\n", strerror(-rc));
D_ERROR("Failed to parse subsystem configuration: %s\n",
spdk_strerror(-rc));
D_GOTO(out, rc = -DER_INVAL);
}

if (spdk_json_strequal(ctx->subsystem_name, "bdev"))
bdev_ss = ctx->subsystems_it;

if (spdk_json_strequal(ctx->subsystem_name, BIO_DEV_TYPE_VMD))
if (spdk_json_strequal(ctx->subsystem_name, NVME_PCI_DEV_TYPE_VMD))
vmd_ss = ctx->subsystems_it;

/* Move on to next subsystem */
Expand All @@ -703,7 +717,7 @@ bio_add_allowed_alloc(const char *nvme_conf, struct spdk_env_opts *opts, int *ro
D_GOTO(out, rc = -DER_INVAL);
}

rc = check_vmd_status(ctx, vmd_ss, &vmd_enabled);
rc = check_vmd_status(ctx, vmd_ss, vmd_enabled);
if (rc < 0)
goto out;

Expand All @@ -712,7 +726,7 @@ bio_add_allowed_alloc(const char *nvme_conf, struct spdk_env_opts *opts, int *ro
goto out;
*roles = rc;

rc = add_bdevs_to_opts(ctx, bdev_ss, vmd_enabled, opts);
rc = add_bdevs_to_opts(ctx, bdev_ss, *vmd_enabled, opts);
out:
free_json_config_ctx(ctx);
return rc;
Expand Down Expand Up @@ -740,15 +754,15 @@ decode_daos_data(const char *nvme_conf, const char *method_name, struct config_e
rc = spdk_json_find(ctx->values, "daos_data", NULL, &daos_data,
SPDK_JSON_VAL_OBJECT_BEGIN);
if (rc < 0) {
D_ERROR("Failed to find 'daos_data' key: %s\n", strerror(-rc));
D_ERROR("Failed to find 'daos_data' key: %s\n", spdk_strerror(-rc));
D_GOTO(out, rc = -DER_INVAL);
}

/* Capture config array in ctx */
rc = spdk_json_decode_object(daos_data, daos_data_decoders,
SPDK_COUNTOF(daos_data_decoders), ctx);
if (rc < 0) {
D_ERROR("Failed to parse 'daos_data' entry: %s\n", strerror(-rc));
D_ERROR("Failed to parse 'daos_data' entry: %s\n", spdk_strerror(-rc));
D_GOTO(out, rc = -DER_INVAL);
}

Expand All @@ -763,7 +777,7 @@ decode_daos_data(const char *nvme_conf, const char *method_name, struct config_e
rc = spdk_json_decode_object(ctx->config_it, config_entry_decoders,
SPDK_COUNTOF(config_entry_decoders), cfg);
if (rc < 0) {
D_ERROR("Failed to decode 'config' entry: %s\n", strerror(-rc));
D_ERROR("Failed to decode 'config' entry: %s\n", spdk_strerror(-rc));
D_GOTO(out, rc = -DER_INVAL);
}

Expand Down Expand Up @@ -798,7 +812,7 @@ get_hotplug_busid_range(const char *nvme_conf)
&hotplug_busid_range);
if (rc < 0) {
D_ERROR("Failed to decode '%s' entry: %s)\n", NVME_CONF_SET_HOTPLUG_RANGE,
strerror(-rc));
spdk_strerror(-rc));
D_GOTO(out, rc = -DER_INVAL);
}

Expand Down Expand Up @@ -873,7 +887,7 @@ bio_read_accel_props(const char *nvme_conf)
&accel_props);
if (rc < 0) {
D_ERROR("Failed to decode '%s' entry (%s)\n", NVME_CONF_SET_ACCEL_PROPS,
strerror(-rc));
spdk_strerror(-rc));
D_GOTO(out, rc = -DER_INVAL);
}

Expand Down Expand Up @@ -915,7 +929,7 @@ bio_read_rpc_srv_settings(const char *nvme_conf, bool *enable, const char **sock
&rpc_srv_settings);
if (rc < 0) {
D_ERROR("Failed to decode '%s' entry: %s)\n", NVME_CONF_SET_SPDK_RPC_SERVER,
strerror(-rc));
spdk_strerror(-rc));
D_GOTO(out, rc = -DER_INVAL);
}

Expand All @@ -931,3 +945,139 @@ bio_read_rpc_srv_settings(const char *nvme_conf, bool *enable, const char **sock
rc = 0;
return rc;
}

struct json_bdev_nvme_ctx {
struct spdk_json_val *pci_address;
struct spdk_json_val *ctrlr_data;
struct spdk_json_val *ns_data;
struct spdk_json_val *values;
size_t values_cnt;
};

static struct spdk_json_object_decoder nvme_decoders[] = {
{"pci_address", offsetof(struct json_bdev_nvme_ctx, pci_address), cap_string},
{"ctrlr_data", offsetof(struct json_bdev_nvme_ctx, ctrlr_data), cap_object, false},
{"ns_data", offsetof(struct json_bdev_nvme_ctx, ns_data), cap_object, false}};

static struct spdk_json_object_decoder nvme_ctrlr_decoders[] = {
{"model_number", offsetof(struct ctrlr_t, model), spdk_json_decode_string},
{"serial_number", offsetof(struct ctrlr_t, serial), spdk_json_decode_string},
{"firmware_revision", offsetof(struct ctrlr_t, fw_rev), spdk_json_decode_string},
{"vendor_id", offsetof(struct ctrlr_t, vendor_id), spdk_json_decode_string}};

static struct spdk_json_object_decoder nvme_ns_decoders[] = {
{"id", offsetof(struct ns_t, id), spdk_json_decode_uint32}};

/**
* Fetch bdev controller parameters from spdk_bdev_dump_info_json output.
*
* \param[out] *b_info Device info struct to populate
* \param[in] json Raw JSON to parse
* \param[in] json_size Number of JSON chars
*
* \returns Zero on success, negative on failure (DER)
*/
int
bio_decode_bdev_params(struct bio_dev_info *b_info, const void *json, int json_size)
{
char *tmp = NULL;
char *end1 = NULL;
ssize_t rc = 0;
char *json_data = NULL;
struct json_bdev_nvme_ctx *ctx;
void *end;

D_ASSERT(b_info != NULL);
D_ASSERT(b_info->bdi_ctrlr != NULL);
D_ASSERT(b_info->bdi_ctrlr->nss != NULL);
D_ASSERT(json != NULL);
D_ASSERT(json_size > 0);

/* Check input is null-terminated */
if (strnlen(json, JSON_MAX_CHARS) == JSON_MAX_CHARS)
return -DER_INVAL;

/* Trim chars to get single valid "nvme" object from array. */
tmp = strstr(json, "{");
if (tmp == NULL)
return -DER_INVAL;
end1 = strchr(tmp, ']');
if (end1 == NULL)
return -DER_INVAL;

/* Copy input JSON so we don't mutate the original. */
D_STRNDUP(json_data, tmp, end1 - tmp);
if (json_data == NULL)
return -DER_NOMEM;
json_data[end1 - tmp - 1] = '\0';

D_ALLOC_PTR(ctx);
if (ctx == NULL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[defect] json_data memory leaked here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks

return -DER_NOMEM;

/* Calculate number of values in tree before mem alloc. */
rc = spdk_json_parse(json_data, strnlen(json_data, json_size), NULL, 0, &end,
SPDK_JSON_PARSE_FLAG_ALLOW_COMMENTS);
if (rc < 0) {
D_ERROR("Parsing bdev-nvme dump failed: %s\n", spdk_strerror(-rc));
D_GOTO(free_ctx, rc = -DER_INVAL);
}

ctx->values_cnt = rc;
D_ALLOC_ARRAY(ctx->values, ctx->values_cnt);
if (ctx->values == NULL)
D_GOTO(free_ctx, rc = -DER_NOMEM);

/* Populate tree of keys and values from JSON. */
rc = spdk_json_parse(json_data, strnlen(json_data, json_size), ctx->values, ctx->values_cnt,
&end, SPDK_JSON_PARSE_FLAG_ALLOW_COMMENTS);
if (rc < 0) {
D_ERROR("Parsing bdev-nvme dump failed: %s\n", spdk_strerror(-rc));
D_GOTO(free_values, rc = -DER_INVAL);
}
if (rc != ctx->values_cnt) {
D_ERROR("Parsing bdev-nvme dump failed, want %zd values got %zd\n", ctx->values_cnt,
rc);
D_GOTO(free_values, rc = -DER_INVAL);
}

rc = spdk_json_decode_object_relaxed(ctx->values, nvme_decoders,
SPDK_COUNTOF(nvme_decoders), ctx);
if (rc < 0) {
D_ERROR("Failed to decode nvme entry (%s)\n", spdk_strerror(-rc));
D_GOTO(free_values, rc = -DER_INVAL);
}

D_ASSERT(ctx->pci_address != NULL);
D_ASSERT(ctx->ctrlr_data != NULL);
D_ASSERT(ctx->ns_data != NULL);

rc = spdk_json_decode_string(ctx->pci_address, &b_info->bdi_traddr);
if (rc < 0) {
D_ERROR("Failed to decode string value for pci_address: %s\n", spdk_strerror(-rc));
D_GOTO(free_values, rc = -DER_INVAL);
}

rc = spdk_json_decode_object_relaxed(ctx->ctrlr_data, nvme_ctrlr_decoders,
SPDK_COUNTOF(nvme_ctrlr_decoders), b_info->bdi_ctrlr);
if (rc < 0) {
D_ERROR("Failed to decode nvme ctrlr_data entry (%s)\n", spdk_strerror(-rc));
D_GOTO(free_values, rc = -DER_INVAL);
}

rc = spdk_json_decode_object_relaxed(
ctx->ns_data, nvme_ns_decoders, SPDK_COUNTOF(nvme_ns_decoders), b_info->bdi_ctrlr->nss);
if (rc < 0) {
D_ERROR("Failed to decode nvme ns_data entry (%s)\n", spdk_strerror(-rc));
D_GOTO(free_values, rc = -DER_INVAL);
}

rc = 0;
free_values:
D_FREE(ctx->values);
free_ctx:
D_FREE(ctx);
D_FREE(json_data);

return rc;
}
Loading
Loading