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: Remove bdev scan cache #13256

Closed
wants to merge 19 commits into from
Closed

DAOS-14181 control: Remove bdev scan cache #13256

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
142c11f
DAOS-14181 control: Remove bdev scan cache
tanabarr Oct 30, 2023
1bbace2
move bdevScanEngine and smdGetHealth into instance_storage_rpc.go to …
tanabarr Oct 31, 2023
6e8927e
link SmdDevice to NvmeController proto
tanabarr Nov 1, 2023
d7d8fb1
remove SmdDeviceWithHealth type now SmdDevice references NvmeControll…
tanabarr Nov 3, 2023
35eb943
remove addition of list nvme drpc as nvme info to be included in list…
tanabarr Nov 3, 2023
983d53d
reconstruct controller scan response from smd and bio-health drpc que…
tanabarr Nov 3, 2023
54293b6
fix drpc handlers to use updated smd-device protobuf definitions
tanabarr Nov 3, 2023
8cf41ff
share ctrlr_t and ns_t types between control-plane and engine
tanabarr Nov 7, 2023
c373d2a
create global flag for vmd enablement and skip led ops if uset
tanabarr Nov 7, 2023
40a393f
collect nvme controller details with spdk_nvme_connect
tanabarr Nov 8, 2023
83c1ddc
check vmd dev type before querying led state
tanabarr Nov 13, 2023
926d90d
Update src/control/server/ctl_smd_rpc_test.go
tanabarr Nov 9, 2023
cd320fc
populate and return vendor_id and pci_dev_type over drpc
tanabarr Nov 16, 2023
6f3eca8
improve json parsing of bdev params with spdk api
tanabarr Nov 17, 2023
23311fe
improve json parsing of bdev params with spdk api pt2
tanabarr Nov 20, 2023
4ee62f9
add namespace info to proto response
tanabarr Nov 21, 2023
9e57c48
address code review comments from kjacque
tanabarr Nov 21, 2023
4952da1
remove unrelated changes to control/server/config and drpc_modules.h
tanabarr Nov 22, 2023
a435cf5
revert unnecessary changes to fill_in_traddr
tanabarr Nov 22, 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
184 changes: 161 additions & 23 deletions src/bio/bio_config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@
#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 */
Expand Down Expand Up @@ -320,15 +323,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 @@ -375,12 +377,12 @@ load_vmd_subsystem_config(struct json_config_ctx *ctx, bool *vmd_enabled)
int rc;

D_ASSERT(ctx->config_it != NULL);
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.

We should leave the null check assert in.

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


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 +407,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 +493,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 +554,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 +617,11 @@ check_vmd_status(struct json_config_ctx *ctx, struct spdk_json_val *vmd_ss, bool
{
int rc;

D_ASSERT(*vmd_enabled == false);

if (vmd_ss == NULL)
return 0;

D_ASSERT(vmd_enabled != NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should leave it in, and move it before the pointer is dereferenced.

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


rc = decode_subsystem_configs(vmd_ss, ctx);
if (rc != 0)
return rc;
Expand All @@ -642,20 +644,22 @@ 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;
int rc = 0;

D_ASSERT(nvme_conf != NULL);
D_ASSERT(opts != 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.

should null check before dereferencing

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


D_ALLOC_PTR(ctx);
if (ctx == NULL)
Expand All @@ -668,7 +672,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 +688,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 +708,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 +717,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 +745,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 +768,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 +803,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 +878,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 +920,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 +936,136 @@ 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;

size_t values_cnt;
struct spdk_json_val *values;
};

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why void * instead of char *?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is called by bio_device.c:json_write_cb which has signature json_write_cb(void *cb_ctx, const void *json, size_t json_size). This is used to write the JSON from spdk_bdev_dump_info_json using spdk_json_write_{begin,end}. So basically its a requirement of the API and I don't feel that casting it within the callback is a good idea.

{
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);

/* Trim chars to get single valid "nvme" object from array. */
tmp = strstr(json, "{");
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably be checking for null and for json_size > 0 before doing anything here. I also find myself wondering about the nature of the input. strstr and strchr are unsafe if the string has been corrupted or isn't null-terminated for some other reason. Not sure if we have macros implementing safe versions of those functions. Otherwise we should probably check if the string is null-terminated using strnlen since we have a maximum length value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spdk_bdev_dump_info_json As part of the API should be pretty reliable and not provide corrupted or not-null-terminated strings. I am surprised that it doesn't output enclosing braces. I'm adding the checks as suggested. Done.

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)
return -DER_NOMEM;

/* Calculate number of values in tree. */
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 JSON tree keys and values. */
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