From 318de85f628c41570dfef79558868fe786c4c356 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Wed, 14 Jun 2023 09:32:16 -0400 Subject: [PATCH] mgmtd: simplify locking, removing read locks Signed-off-by: Christian Hopps --- mgmtd/mgmt_be_adapter.c | 11 +- mgmtd/mgmt_be_adapter.h | 6 +- mgmtd/mgmt_ds.c | 78 ++++++------ mgmtd/mgmt_ds.h | 21 ++-- mgmtd/mgmt_fe_adapter.c | 263 ++++++++++++---------------------------- mgmtd/mgmt_history.c | 4 +- mgmtd/mgmt_txn.c | 152 ++++++++--------------- mgmtd/mgmt_txn.h | 21 ++-- 8 files changed, 196 insertions(+), 360 deletions(-) diff --git a/mgmtd/mgmt_be_adapter.c b/mgmtd/mgmt_be_adapter.c index 49a307e9c2..512cc49feb 100644 --- a/mgmtd/mgmt_be_adapter.c +++ b/mgmtd/mgmt_be_adapter.c @@ -648,8 +648,7 @@ static void mgmt_be_adapter_process_msg(uint8_t version, uint8_t *data, mgmtd__be_message__free_unpacked(be_msg, NULL); } -static void mgmt_be_iter_and_get_cfg(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, struct lyd_node *node, +static void mgmt_be_iter_and_get_cfg(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx) { struct mgmt_be_get_adapter_config_params *parms = ctx; @@ -806,10 +805,10 @@ mgmt_be_get_adapter_by_name(const char *name) } int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, - struct mgmt_ds_ctx *ds_ctx, - struct nb_config_cbs **cfg_chgs) + struct nb_config_cbs **cfg_chgs) { struct mgmt_be_get_adapter_config_params parms; + struct nb_config *cfg_root = mgmt_ds_get_nb_config(mm->running_ds); assert(cfg_chgs); @@ -825,8 +824,8 @@ int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, parms.cfg_chgs = &adapter->cfg_chgs; parms.seq = 0; - mgmt_ds_iter_data(ds_ctx, "", mgmt_be_iter_and_get_cfg, - (void *)&parms); + mgmt_ds_iter_data(MGMTD_DS_RUNNING, cfg_root, "", + mgmt_be_iter_and_get_cfg, (void *)&parms); } *cfg_chgs = &adapter->cfg_chgs; diff --git a/mgmtd/mgmt_be_adapter.h b/mgmtd/mgmt_be_adapter.h index e1676e63af..ca8f55c457 100644 --- a/mgmtd/mgmt_be_adapter.h +++ b/mgmtd/mgmt_be_adapter.h @@ -110,10 +110,8 @@ extern struct mgmt_be_client_adapter * mgmt_be_get_adapter_by_id(enum mgmt_be_client_id id); /* Fetch backend adapter config. */ -extern int -mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, - struct mgmt_ds_ctx *ds_ctx, - struct nb_config_cbs **cfg_chgs); +extern int mgmt_be_get_adapter_config(struct mgmt_be_client_adapter *adapter, + struct nb_config_cbs **cfg_chgs); /* Create/destroy a transaction. */ extern int mgmt_be_send_txn_req(struct mgmt_be_client_adapter *adapter, diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index 5a4b00d309..027e306141 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -22,7 +22,9 @@ struct mgmt_ds_ctx { Mgmtd__DatastoreId ds_id; - int lock; /* 0 unlocked, >0 read locked < write locked */ + + bool locked; + uint64_t vty_session_id; /* Owner of the lock or 0 */ bool config_ds; @@ -244,40 +246,33 @@ bool mgmt_ds_is_config(struct mgmt_ds_ctx *ds_ctx) return ds_ctx->config_ds; } -int mgmt_ds_read_lock(struct mgmt_ds_ctx *ds_ctx) +bool mgmt_ds_is_locked(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id) { - if (!ds_ctx) - return EINVAL; - if (ds_ctx->lock < 0) - return EBUSY; - ++ds_ctx->lock; - return 0; + assert(ds_ctx); + return (ds_ctx->locked && ds_ctx->vty_session_id == session_id); } -int mgmt_ds_write_lock(struct mgmt_ds_ctx *ds_ctx) +int mgmt_ds_lock(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id) { - if (!ds_ctx) - return EINVAL; - if (ds_ctx->lock != 0) + assert(ds_ctx); + + if (ds_ctx->locked) return EBUSY; - ds_ctx->lock = -1; + + ds_ctx->locked = true; + ds_ctx->vty_session_id = session_id; return 0; } -int mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx) +void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx) { - if (!ds_ctx) - return EINVAL; - if (ds_ctx->lock > 0) - --ds_ctx->lock; - else if (ds_ctx->lock < 0) { - assert(ds_ctx->lock == -1); - ds_ctx->lock = 0; - } else { - assert(ds_ctx->lock != 0); - return EINVAL; - } - return 0; + assert(ds_ctx); + if (!ds_ctx->locked) + zlog_warn( + "%s: WARNING: unlock on unlocked in DS:%s last session-id %" PRIu64, + __func__, mgmt_ds_id2name(ds_ctx->ds_id), + ds_ctx->vty_session_id); + ds_ctx->locked = 0; } int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, @@ -314,10 +309,9 @@ struct nb_config *mgmt_ds_get_nb_config(struct mgmt_ds_ctx *ds_ctx) } static int mgmt_walk_ds_nodes( - struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, + struct nb_config *root, const char *base_xpath, struct lyd_node *base_dnode, - void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, struct lyd_node *node, + void (*mgmt_ds_node_iter_fn)(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), void *ctx) { @@ -336,10 +330,7 @@ static int mgmt_walk_ds_nodes( * This function only returns the first node of a possible set * of matches issuing a warning if more than 1 matches */ - base_dnode = yang_dnode_get( - ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode - : ds_ctx->root.dnode_root, - base_xpath); + base_dnode = yang_dnode_get(root->dnode, base_xpath); if (!base_dnode) return -1; @@ -348,7 +339,7 @@ static int mgmt_walk_ds_nodes( sizeof(xpath))); nbnode = (struct nb_node *)base_dnode->schema->priv; - (*mgmt_ds_node_iter_fn)(ds_ctx, base_xpath, base_dnode, nbnode, ctx); + (*mgmt_ds_node_iter_fn)(base_xpath, base_dnode, nbnode, ctx); /* * If the base_xpath points to a leaf node we can skip the tree walk. @@ -370,7 +361,7 @@ static int mgmt_walk_ds_nodes( MGMTD_DS_DBG(" -- Child xpath: %s", xpath); - ret = mgmt_walk_ds_nodes(ds_ctx, xpath, dnode, + ret = mgmt_walk_ds_nodes(root, xpath, dnode, mgmt_ds_node_iter_fn, ctx); if (ret != 0) break; @@ -459,9 +450,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst, return 0; } -int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, - void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, +int mgmt_ds_iter_data(Mgmtd__DatastoreId ds_id, struct nb_config *root, + const char *base_xpath, + void (*mgmt_ds_node_iter_fn)(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), @@ -472,7 +463,7 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, struct lyd_node *base_dnode = NULL; struct lyd_node *node; - if (!ds_ctx) + if (!root) return -1; strlcpy(xpath, base_xpath, sizeof(xpath)); @@ -484,12 +475,11 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, * Oper-state should be kept in mind though for the prefix walk */ - MGMTD_DS_DBG(" -- START DS walk for DSid: %d", ds_ctx->ds_id); + MGMTD_DS_DBG(" -- START DS walk for DSid: %d", ds_id); /* If the base_xpath is empty then crawl the sibblings */ if (xpath[0] == 0) { - base_dnode = ds_ctx->config_ds ? ds_ctx->root.cfg_root->dnode - : ds_ctx->root.dnode_root; + base_dnode = root->dnode; /* get first top-level sibling */ while (base_dnode->parent) @@ -499,11 +489,11 @@ int mgmt_ds_iter_data(struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, base_dnode = base_dnode->prev; LY_LIST_FOR (base_dnode, node) { - ret = mgmt_walk_ds_nodes(ds_ctx, xpath, node, + ret = mgmt_walk_ds_nodes(root, xpath, node, mgmt_ds_node_iter_fn, ctx); } } else - ret = mgmt_walk_ds_nodes(ds_ctx, xpath, base_dnode, + ret = mgmt_walk_ds_nodes(root, xpath, base_dnode, mgmt_ds_node_iter_fn, ctx); return ret; diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h index e5c88742dd..1cf4816027 100644 --- a/mgmtd/mgmt_ds.h +++ b/mgmtd/mgmt_ds.h @@ -179,19 +179,19 @@ extern struct mgmt_ds_ctx *mgmt_ds_get_ctx_by_id(struct mgmt_master *mm, extern bool mgmt_ds_is_config(struct mgmt_ds_ctx *ds_ctx); /* - * Acquire read lock to a ds given a ds_handle + * Check if a given datastore is locked by a given session */ -extern int mgmt_ds_read_lock(struct mgmt_ds_ctx *ds_ctx); +extern bool mgmt_ds_is_locked(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id); /* * Acquire write lock to a ds given a ds_handle */ -extern int mgmt_ds_write_lock(struct mgmt_ds_ctx *ds_ctx); +extern int mgmt_ds_lock(struct mgmt_ds_ctx *ds_ctx, uint64_t session_id); /* * Remove a lock from ds given a ds_handle */ -extern int mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx); +extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx); /* * Copy from source to destination datastore. @@ -233,8 +233,11 @@ extern int mgmt_ds_delete_data_nodes(struct mgmt_ds_ctx *ds_ctx, /* * Iterate over datastore data. * - * ds_ctx - * Datastore context. + * ds_id + * Datastore ID.. + * + * root + * The root of the tree to iterate over. * * base_xpath * Base YANG xpath from where needs to be iterated. @@ -252,9 +255,9 @@ extern int mgmt_ds_delete_data_nodes(struct mgmt_ds_ctx *ds_ctx, * 0 on success, -1 on failure. */ extern int mgmt_ds_iter_data( - struct mgmt_ds_ctx *ds_ctx, const char *base_xpath, - void (*mgmt_ds_node_iter_fn)(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, struct lyd_node *node, + Mgmtd__DatastoreId ds_id, struct nb_config *root, + const char *base_xpath, + void (*mgmt_ds_node_iter_fn)(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx), void *ctx); diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index 4b104abb98..1b6f45a05d 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -40,8 +40,7 @@ struct mgmt_fe_session_ctx { uint64_t client_id; uint64_t txn_id; uint64_t cfg_txn_id; - uint8_t ds_write_locked[MGMTD_DS_MAX_ID]; - uint8_t ds_read_locked[MGMTD_DS_MAX_ID]; + uint8_t ds_locked[MGMTD_DS_MAX_ID]; uint8_t ds_locked_implict[MGMTD_DS_MAX_ID]; struct event *proc_cfg_txn_clnp; struct event *proc_show_txn_clnp; @@ -72,8 +71,12 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id, struct mgmt_ds_ctx *ds_ctx, struct mgmt_fe_session_ctx *session) { - if (!session->ds_write_locked[ds_id]) { - if (mgmt_ds_write_lock(ds_ctx) != 0) { + if (session->ds_locked[ds_id]) + zlog_warn("multiple lock taken by session-id: %" PRIu64 + " on DS:%s", + session->session_id, mgmt_ds_id2name(ds_id)); + else { + if (mgmt_ds_lock(ds_ctx, session->session_id)) { MGMTD_FE_ADAPTER_DBG( "Failed to lock the DS:%s for session-id: %" PRIu64 " from %s!", @@ -82,7 +85,7 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id, return -1; } - session->ds_write_locked[ds_id] = true; + session->ds_locked[ds_id] = true; MGMTD_FE_ADAPTER_DBG( "Write-Locked the DS:%s for session-id: %" PRIu64 " from %s", @@ -93,74 +96,22 @@ mgmt_fe_session_write_lock_ds(Mgmtd__DatastoreId ds_id, return 0; } -static int -mgmt_fe_session_read_lock_ds(Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - struct mgmt_fe_session_ctx *session) +static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id, + struct mgmt_ds_ctx *ds_ctx, + struct mgmt_fe_session_ctx *session) { - if (!session->ds_read_locked[ds_id]) { - if (mgmt_ds_read_lock(ds_ctx) != 0) { - MGMTD_FE_ADAPTER_DBG( - "Failed to lock the DS:%s for session-is: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - return -1; - } - - session->ds_read_locked[ds_id] = true; - MGMTD_FE_ADAPTER_DBG( - "Read-Locked the DS:%s for session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - } - - return 0; -} - -static int mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - struct mgmt_fe_session_ctx *session, - bool unlock_write, bool unlock_read) -{ - if (unlock_write && session->ds_write_locked[ds_id]) { - session->ds_write_locked[ds_id] = false; - session->ds_locked_implict[ds_id] = false; - if (mgmt_ds_unlock(ds_ctx) != 0) { - MGMTD_FE_ADAPTER_DBG( - "Failed to unlock the DS:%s taken earlier by session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - return -1; - } - - MGMTD_FE_ADAPTER_DBG( - "Unlocked DS:%s write-locked earlier by session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - } else if (unlock_read && session->ds_read_locked[ds_id]) { - session->ds_read_locked[ds_id] = false; - session->ds_locked_implict[ds_id] = false; - if (mgmt_ds_unlock(ds_ctx) != 0) { - MGMTD_FE_ADAPTER_DBG( - "Failed to unlock the DS:%s taken earlier by session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - return -1; - } + if (!session->ds_locked[ds_id]) + zlog_warn("unlock unlocked by session-id: %" PRIu64 " on DS:%s", + session->session_id, mgmt_ds_id2name(ds_id)); - MGMTD_FE_ADAPTER_DBG( - "Unlocked DS:%s read-locked earlier by session-id: %" PRIu64 - " from %s", - mgmt_ds_id2name(ds_id), session->session_id, - session->adapter->name); - } - - return 0; + session->ds_locked[ds_id] = false; + session->ds_locked_implict[ds_id] = false; + mgmt_ds_unlock(ds_ctx); + MGMTD_FE_ADAPTER_DBG( + "Unlocked DS:%s write-locked earlier by session-id: %" PRIu64 + " from %s", + mgmt_ds_id2name(ds_id), session->session_id, + session->adapter->name); } static void @@ -177,11 +128,8 @@ mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session) for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) { ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id); - if (ds_ctx) { - if (session->ds_locked_implict[ds_id]) - mgmt_fe_session_unlock_ds( - ds_id, ds_ctx, session, true, false); - } + if (ds_ctx && session->ds_locked_implict[ds_id]) + mgmt_fe_session_unlock_ds(ds_id, ds_ctx, session); } /* @@ -194,17 +142,6 @@ mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session) static void mgmt_fe_session_show_txn_cleanup(struct mgmt_fe_session_ctx *session) { - Mgmtd__DatastoreId ds_id; - struct mgmt_ds_ctx *ds_ctx; - - for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) { - ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id); - if (ds_ctx) { - mgmt_fe_session_unlock_ds(ds_id, ds_ctx, session, - false, true); - } - } - /* * Destroy the transaction created recently. */ @@ -245,25 +182,29 @@ mgmt_fe_session_compute_commit_timers(struct mgmt_commit_stats *cmt_stats) } } -static void mgmt_fe_cleanup_session(struct mgmt_fe_session_ctx **session) +static void mgmt_fe_cleanup_session(struct mgmt_fe_session_ctx **sessionp) { - if ((*session)->adapter) { - mgmt_fe_session_cfg_txn_cleanup((*session)); - mgmt_fe_session_show_txn_cleanup((*session)); - mgmt_fe_session_unlock_ds(MGMTD_DS_CANDIDATE, mm->candidate_ds, - *session, true, true); - mgmt_fe_session_unlock_ds(MGMTD_DS_RUNNING, mm->running_ds, - *session, true, true); - - mgmt_fe_sessions_del(&(*session)->adapter->fe_sessions, - *session); - assert((*session)->adapter->refcount > 1); - mgmt_fe_adapter_unlock(&(*session)->adapter); + Mgmtd__DatastoreId ds_id; + struct mgmt_ds_ctx *ds_ctx; + struct mgmt_fe_session_ctx *session = *sessionp; + + if (session->adapter) { + mgmt_fe_session_cfg_txn_cleanup(session); + mgmt_fe_session_show_txn_cleanup(session); + for (ds_id = 0; ds_id < MGMTD_DS_MAX_ID; ds_id++) { + ds_ctx = mgmt_ds_get_ctx_by_id(mm, ds_id); + if (ds_ctx && session->ds_locked[ds_id]) + mgmt_fe_session_unlock_ds(ds_id, ds_ctx, + session); + } + mgmt_fe_sessions_del(&session->adapter->fe_sessions, session); + assert(session->adapter->refcount > 1); + mgmt_fe_adapter_unlock(&session->adapter); } - hash_release(mgmt_fe_sessions, *session); - XFREE(MTYPE_MGMTD_FE_SESSION, *session); - *session = NULL; + hash_release(mgmt_fe_sessions, session); + XFREE(MTYPE_MGMTD_FE_SESSION, session); + *sessionp = NULL; } static struct mgmt_fe_session_ctx * @@ -735,7 +676,7 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, session->ds_locked_implict[lockds_req->ds_id] = false; } else { - if (!session->ds_write_locked[lockds_req->ds_id]) { + if (!session->ds_locked[lockds_req->ds_id]) { mgmt_fe_send_lockds_reply( session, lockds_req->ds_id, lockds_req->req_id, lockds_req->lock, false, @@ -743,8 +684,7 @@ mgmt_fe_session_handle_lockds_req_msg(struct mgmt_fe_session_ctx *session, return 0; } - (void)mgmt_fe_session_unlock_ds(lockds_req->ds_id, ds_ctx, - session, true, false); + mgmt_fe_session_unlock_ds(lockds_req->ds_id, ds_ctx, session); } if (mgmt_fe_send_lockds_reply(session, lockds_req->ds_id, @@ -821,7 +761,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, /* * Try taking write-lock on the requested DS (if not already). */ - if (!session->ds_write_locked[setcfg_req->ds_id]) { + if (!session->ds_locked[setcfg_req->ds_id]) { MGMTD_FE_ADAPTER_ERR( "SETCFG_REQ on session-id: %" PRIu64 " without obtaining lock", @@ -832,7 +772,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, mgmt_fe_send_setcfg_reply( session, setcfg_req->ds_id, setcfg_req->req_id, false, - "Failed to lock the DS!", + "Failed to lock the target DS", setcfg_req->implicit_commit); goto mgmt_fe_sess_handle_setcfg_req_failed; } @@ -915,9 +855,8 @@ mgmt_fe_sess_handle_setcfg_req_failed: */ if (session->cfg_txn_id != MGMTD_TXN_ID_NONE) mgmt_destroy_txn(&session->cfg_txn_id); - if (ds_ctx && session->ds_write_locked[setcfg_req->ds_id]) - mgmt_fe_session_unlock_ds(setcfg_req->ds_id, ds_ctx, session, - true, false); + if (ds_ctx && session->ds_locked[setcfg_req->ds_id]) + mgmt_fe_session_unlock_ds(setcfg_req->ds_id, ds_ctx, session); return 0; } @@ -927,6 +866,7 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, Mgmtd__FeGetConfigReq *getcfg_req) { struct mgmt_ds_ctx *ds_ctx; + struct nb_config *cfg_root = NULL; /* * Get the DS handle. @@ -939,11 +879,7 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, return 0; } - /* - * Next check first if the GETCFG_REQ is for Candidate DS - * or not. Report failure if its not. MGMTD currently only - * supports editing the Candidate DS. - */ + /* GETCFG must be on candidate or running DS */ if (getcfg_req->ds_id != MGMTD_DS_CANDIDATE && getcfg_req->ds_id != MGMTD_DS_RUNNING) { mgmt_fe_send_getcfg_reply( @@ -954,27 +890,6 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, } if (session->txn_id == MGMTD_TXN_ID_NONE) { - /* - * Try taking read-lock on the requested DS (if not already - * locked). If the DS has already been write-locked by a ongoing - * CONFIG transaction we may allow reading the contents of the - * same DS. - */ - if (!session->ds_read_locked[getcfg_req->ds_id] - && !session->ds_write_locked[getcfg_req->ds_id]) { - if (mgmt_fe_session_read_lock_ds(getcfg_req->ds_id, - ds_ctx, session) - != 0) { - mgmt_fe_send_getcfg_reply( - session, getcfg_req->ds_id, - getcfg_req->req_id, false, NULL, - "Failed to lock the DS! Another session might have locked it!"); - goto mgmt_fe_sess_handle_getcfg_req_failed; - } - - session->ds_locked_implict[getcfg_req->ds_id] = true; - } - /* * Start a SHOW Transaction (if not started already) */ @@ -992,19 +907,24 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, " for session-id: %" PRIu64, session->txn_id, session->session_id); } else { + /* XXX chopps: Why would we already have a TXN here? */ MGMTD_FE_ADAPTER_DBG("Show txn-id: %" PRIu64 " for session-id: %" PRIu64 " already created", session->txn_id, session->session_id); } + /* + * Get a copy of the datastore config root, avoids locking. + */ + cfg_root = nb_config_dup(mgmt_ds_get_nb_config(ds_ctx)); + /* * Create a GETConfig request under the transaction. */ - if (mgmt_txn_send_get_config_req(session->txn_id, getcfg_req->req_id, - getcfg_req->ds_id, ds_ctx, - getcfg_req->data, getcfg_req->n_data) - != 0) { + if (mgmt_txn_send_get_config_req( + session->txn_id, getcfg_req->req_id, getcfg_req->ds_id, + cfg_root, getcfg_req->data, getcfg_req->n_data) != 0) { mgmt_fe_send_getcfg_reply( session, getcfg_req->ds_id, getcfg_req->req_id, false, NULL, "Request processing for GET-CONFIG failed!"); @@ -1015,14 +935,13 @@ mgmt_fe_session_handle_getcfg_req_msg(struct mgmt_fe_session_ctx *session, mgmt_fe_sess_handle_getcfg_req_failed: + if (cfg_root) + nb_config_free(cfg_root); /* * Destroy the transaction created recently. */ if (session->txn_id != MGMTD_TXN_ID_NONE) mgmt_destroy_txn(&session->txn_id); - if (ds_ctx && session->ds_read_locked[getcfg_req->ds_id]) - mgmt_fe_session_unlock_ds(getcfg_req->ds_id, ds_ctx, session, - false, true); return -1; } @@ -1044,28 +963,16 @@ mgmt_fe_session_handle_getdata_req_msg(struct mgmt_fe_session_ctx *session, return 0; } - if (session->txn_id == MGMTD_TXN_ID_NONE) { - /* - * Try taking read-lock on the requested DS (if not already - * locked). If the DS has already been write-locked by a ongoing - * CONFIG transaction we may allow reading the contents of the - * same DS. - */ - if (!session->ds_read_locked[getdata_req->ds_id] - && !session->ds_write_locked[getdata_req->ds_id]) { - if (mgmt_fe_session_read_lock_ds(getdata_req->ds_id, - ds_ctx, session) - != 0) { - mgmt_fe_send_getdata_reply( - session, getdata_req->ds_id, - getdata_req->req_id, false, NULL, - "Failed to lock the DS! Another session might have locked it!"); - goto mgmt_fe_sess_handle_getdata_req_failed; - } - - session->ds_locked_implict[getdata_req->ds_id] = true; - } + /* GETDATA must be on operational DS */ + if (getdata_req->ds_id != MGMTD_DS_OPERATIONAL) { + mgmt_fe_send_getdata_reply( + session, getdata_req->ds_id, getdata_req->req_id, false, + NULL, + "Get-Data on datastore other than Operational DS not permitted!"); + return 0; + } + if (session->txn_id == MGMTD_TXN_ID_NONE) { /* * Start a SHOW Transaction (if not started already) */ @@ -1092,9 +999,8 @@ mgmt_fe_session_handle_getdata_req_msg(struct mgmt_fe_session_ctx *session, * Create a GETData request under the transaction. */ if (mgmt_txn_send_get_data_req(session->txn_id, getdata_req->req_id, - getdata_req->ds_id, ds_ctx, - getdata_req->data, getdata_req->n_data) - != 0) { + getdata_req->ds_id, getdata_req->data, + getdata_req->n_data) != 0) { mgmt_fe_send_getdata_reply( session, getdata_req->ds_id, getdata_req->req_id, false, NULL, "Request processing for GET-CONFIG failed!"); @@ -1111,10 +1017,6 @@ mgmt_fe_sess_handle_getdata_req_failed: if (session->txn_id != MGMTD_TXN_ID_NONE) mgmt_destroy_txn(&session->txn_id); - if (ds_ctx && session->ds_read_locked[getdata_req->ds_id]) - mgmt_fe_session_unlock_ds(getdata_req->ds_id, ds_ctx, - session, false, true); - return -1; } @@ -1192,7 +1094,7 @@ static int mgmt_fe_session_handle_commit_config_req_msg( /* * Try taking write-lock on the destination DS (if not already). */ - if (!session->ds_write_locked[commcfg_req->dst_ds_id]) { + if (!session->ds_locked[commcfg_req->dst_ds_id]) { if (mgmt_fe_session_write_lock_ds(commcfg_req->dst_ds_id, dst_ds_ctx, session) != 0) { @@ -1215,8 +1117,7 @@ static int mgmt_fe_session_handle_commit_config_req_msg( session->cfg_txn_id, commcfg_req->req_id, commcfg_req->src_ds_id, src_ds_ctx, commcfg_req->dst_ds_id, dst_ds_ctx, commcfg_req->validate_only, commcfg_req->abort, - false) - != 0) { + false) != 0) { mgmt_fe_send_commitcfg_reply( session, commcfg_req->src_ds_id, commcfg_req->dst_ds_id, commcfg_req->req_id, MGMTD_INTERNAL_ERROR, @@ -1744,16 +1645,12 @@ void mgmt_fe_adapter_status_write(struct vty *vty, bool detail) session->session_id); vty_out(vty, " DS-Locks:\n"); FOREACH_MGMTD_DS_ID (ds_id) { - if (session->ds_write_locked[ds_id] - || session->ds_read_locked[ds_id]) { + if (session->ds_locked[ds_id]) { locked = true; - vty_out(vty, - " %s\t\t\t%s, %s\n", + vty_out(vty, " %s\t\t\t%s\n", mgmt_ds_id2name(ds_id), - session->ds_write_locked[ds_id] - ? "Write" - : "Read", - session->ds_locked_implict[ds_id] + session->ds_locked_implict + [ds_id] ? "Implicit" : "Explicit"); } diff --git a/mgmtd/mgmt_history.c b/mgmtd/mgmt_history.c index 54eb45fdf4..89a7a60166 100644 --- a/mgmtd/mgmt_history.c +++ b/mgmtd/mgmt_history.c @@ -211,7 +211,7 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, return -1; } - ret = mgmt_ds_write_lock(dst_ds_ctx); + ret = mgmt_ds_lock(dst_ds_ctx, vty->mgmt_session_id); if (ret != 0) { vty_out(vty, "Failed to lock the DS %u for rollback Reason: %s!\n", @@ -243,6 +243,8 @@ static int mgmt_history_rollback_to_cmt(struct vty *vty, mgmt_history_dump_cmt_record_index(); + /* XXX chopps when does this get unlocked? */ + /* * Block the rollback command from returning till the rollback * is completed. On rollback completion mgmt_history_rollback_complete() diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index e64cbe1425..c1f674556a 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -164,7 +164,7 @@ struct mgmt_get_data_reply { struct mgmt_get_data_req { Mgmtd__DatastoreId ds_id; - struct mgmt_ds_ctx *ds_ctx; + struct nb_config *cfg_root; char *xpaths[MGMTD_MAX_NUM_DATA_REQ_IN_BATCH]; int num_xpaths; @@ -576,6 +576,10 @@ static void mgmt_txn_req_free(struct mgmt_txn_req **txn_req) if ((*txn_req)->req.get_data->reply) XFREE(MTYPE_MGMTD_TXN_GETDATA_REPLY, (*txn_req)->req.get_data->reply); + + if ((*txn_req)->req.get_data->cfg_root) + nb_config_free((*txn_req)->req.get_data->cfg_root); + XFREE(MTYPE_MGMTD_TXN_GETDATA_REQ, (*txn_req)->req.get_data); break; case MGMTD_TXN_PROC_GETDATA: @@ -683,8 +687,8 @@ static void mgmt_txn_process_set_cfg(struct event *thread) assert(mgmt_txn_reqs_count(&txn->set_cfg_reqs) == 1); assert(txn_req->req.set_cfg->dst_ds_ctx); - ret = mgmt_ds_write_lock( - txn_req->req.set_cfg->dst_ds_ctx); + ret = mgmt_ds_lock(txn_req->req.set_cfg->dst_ds_ctx, + txn->session_id); if (ret != 0) { MGMTD_TXN_ERR( "Failed to lock DS %u txn-id: %" PRIu64 @@ -703,8 +707,8 @@ static void mgmt_txn_process_set_cfg(struct event *thread) txn_req->req.set_cfg->ds_id, txn_req->req.set_cfg->ds_ctx, txn_req->req.set_cfg->dst_ds_id, - txn_req->req.set_cfg->dst_ds_ctx, false, - false, true); + txn_req->req.set_cfg->dst_ds_ctx, false, false, + true); if (mm->perf_stats_en) gettimeofday(&cmt_stats->last_start, NULL); @@ -746,7 +750,6 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, enum mgmt_result result, const char *error_if_any) { - int ret = 0; bool success, create_cmt_info_rec; if (!txn->commit_cfg_req) @@ -830,12 +833,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, } if (txn->commit_cfg_req->req.commit_cfg.rollback) { - ret = mgmt_ds_unlock( - txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); - if (ret != 0) - MGMTD_TXN_ERR( - "Failed to unlock the dst DS during rollback : %s", - strerror(ret)); + mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); /* * Resume processing the rollback command. @@ -844,12 +842,7 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, } if (txn->commit_cfg_req->req.commit_cfg.implicit) - if (mgmt_ds_unlock( - txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx) - != 0) - MGMTD_TXN_ERR( - "Failed to unlock the dst DS during implicit : %s", - strerror(ret)); + mgmt_ds_unlock(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx); txn->commit_cfg_req->req.commit_cfg.cmt_stats = NULL; mgmt_txn_req_free(&txn->commit_cfg_req); @@ -1724,8 +1717,7 @@ static void mgmt_txn_send_getcfg_reply_data(struct mgmt_txn_req *txn_req, mgmt_reset_get_data_reply_buf(get_req); } -static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx, - const char *xpath, +static void mgmt_txn_iter_and_send_get_cfg_reply(const char *xpath, struct lyd_node *node, struct nb_node *nb_node, void *ctx) @@ -1770,7 +1762,7 @@ static void mgmt_txn_iter_and_send_get_cfg_reply(struct mgmt_ds_ctx *ds_ctx, static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn, struct mgmt_txn_req *txn_req, - struct mgmt_ds_ctx *ds_ctx) + struct nb_config *root) { int indx; struct mgmt_get_data_req *get_data; @@ -1805,7 +1797,8 @@ static int mgmt_txn_get_config(struct mgmt_txn_ctx *txn, * want to also use an xpath regexp we need to add this * functionality. */ - if (mgmt_ds_iter_data(get_data->ds_ctx, get_data->xpaths[indx], + if (mgmt_ds_iter_data(get_data->ds_id, root, + get_data->xpaths[indx], mgmt_txn_iter_and_send_get_cfg_reply, (void *)txn_req) == -1) { MGMTD_TXN_DBG("Invalid Xpath '%s", @@ -1837,7 +1830,7 @@ static void mgmt_txn_process_get_cfg(struct event *thread) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; - struct mgmt_ds_ctx *ds_ctx; + struct nb_config *cfg_root; int num_processed = 0; bool error; @@ -1852,18 +1845,10 @@ static void mgmt_txn_process_get_cfg(struct event *thread) FOREACH_TXN_REQ_IN_LIST (&txn->get_cfg_reqs, txn_req) { error = false; assert(txn_req->req_event == MGMTD_TXN_PROC_GETCFG); - ds_ctx = txn_req->req.get_data->ds_ctx; - if (!ds_ctx) { - mgmt_fe_send_get_cfg_reply( - txn->session_id, txn->txn_id, - txn_req->req.get_data->ds_id, txn_req->req_id, - MGMTD_INTERNAL_ERROR, NULL, - "No such datastore!"); - error = true; - goto mgmt_txn_process_get_cfg_done; - } + cfg_root = txn_req->req.get_data->cfg_root; + assert(cfg_root); - if (mgmt_txn_get_config(txn, txn_req, ds_ctx) != 0) { + if (mgmt_txn_get_config(txn, txn_req, cfg_root) != 0) { MGMTD_TXN_ERR( "Unable to retrieve config from DS %d txn-id: %" PRIu64 " session-id: %" PRIu64 " req-id: %" PRIu64, @@ -1872,8 +1857,6 @@ static void mgmt_txn_process_get_cfg(struct event *thread) error = true; } - mgmt_txn_process_get_cfg_done: - if (error) { /* * Delete the txn request. @@ -1904,9 +1887,7 @@ static void mgmt_txn_process_get_data(struct event *thread) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; - struct mgmt_ds_ctx *ds_ctx; int num_processed = 0; - bool error; txn = (struct mgmt_txn_ctx *)EVENT_ARG(thread); assert(txn); @@ -1917,54 +1898,23 @@ static void mgmt_txn_process_get_data(struct event *thread) txn->session_id); FOREACH_TXN_REQ_IN_LIST (&txn->get_data_reqs, txn_req) { - error = false; assert(txn_req->req_event == MGMTD_TXN_PROC_GETDATA); - ds_ctx = txn_req->req.get_data->ds_ctx; - if (!ds_ctx) { - mgmt_fe_send_get_data_reply( - txn->session_id, txn->txn_id, - txn_req->req.get_data->ds_id, txn_req->req_id, - MGMTD_INTERNAL_ERROR, NULL, - "No such datastore!"); - error = true; - goto mgmt_txn_process_get_data_done; - } - if (mgmt_ds_is_config(ds_ctx)) { - if (mgmt_txn_get_config(txn, txn_req, ds_ctx) - != 0) { - MGMTD_TXN_ERR( - "Unable to retrieve config from DS %d txn-id %" PRIu64 - " session-id: %" PRIu64 - " req-id: %" PRIu64, - txn_req->req.get_data->ds_id, - txn->txn_id, txn->session_id, - txn_req->req_id); - error = true; - } - } else { - /* - * TODO: Trigger GET procedures for Backend - * For now return back error. - */ - mgmt_fe_send_get_data_reply( - txn->session_id, txn->txn_id, - txn_req->req.get_data->ds_id, txn_req->req_id, - MGMTD_INTERNAL_ERROR, NULL, - "GET-DATA on Oper DS is not supported yet!"); - error = true; - } - - mgmt_txn_process_get_data_done: - - if (error) { - /* - * Delete the txn request. - * Note: The following will remove it from the list - * as well. - */ - mgmt_txn_req_free(&txn_req); - } + /* + * TODO: Trigger GET procedures for Backend + * For now return back error. + */ + mgmt_fe_send_get_data_reply( + txn->session_id, txn->txn_id, + txn_req->req.get_data->ds_id, txn_req->req_id, + MGMTD_INTERNAL_ERROR, NULL, + "GET-DATA on Oper DS is not supported yet!"); + /* + * Delete the txn request. + * Note: The following will remove it from the list + * as well. + */ + mgmt_txn_req_free(&txn_req); /* * Else the transaction would have been already deleted or @@ -2344,12 +2294,12 @@ int mgmt_txn_send_set_config_req(uint64_t txn_id, uint64_t req_id, } int mgmt_txn_send_commit_config_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId src_ds_id, - struct mgmt_ds_ctx *src_ds_ctx, - Mgmtd__DatastoreId dst_ds_id, - struct mgmt_ds_ctx *dst_ds_ctx, - bool validate_only, bool abort, - bool implicit) + Mgmtd__DatastoreId src_ds_id, + struct mgmt_ds_ctx *src_ds_ctx, + Mgmtd__DatastoreId dst_ds_id, + struct mgmt_ds_ctx *dst_ds_ctx, + bool validate_only, bool abort, + bool implicit) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; @@ -2395,9 +2345,8 @@ int mgmt_txn_notify_be_adapter_conn(struct mgmt_be_client_adapter *adapter, memset(&dummy_stats, 0, sizeof(dummy_stats)); if (connect) { /* Get config for this single backend client */ - mgmt_be_get_adapter_config(adapter, mm->running_ds, - &adapter_cfgs); + mgmt_be_get_adapter_config(adapter, &adapter_cfgs); if (!adapter_cfgs || RB_EMPTY(nb_config_cbs, adapter_cfgs)) { SET_FLAG(adapter->flags, MGMTD_BE_ADAPTER_FLAGS_CFG_SYNCED); @@ -2619,10 +2568,10 @@ int mgmt_txn_notify_be_cfg_apply_reply(uint64_t txn_id, bool success, } int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - Mgmtd__YangGetDataReq **data_req, - size_t num_reqs) + Mgmtd__DatastoreId ds_id, + struct nb_config *cfg_root, + Mgmtd__YangGetDataReq **data_req, + size_t num_reqs) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; @@ -2634,7 +2583,7 @@ int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, txn_req = mgmt_txn_req_alloc(txn, req_id, MGMTD_TXN_PROC_GETCFG); txn_req->req.get_data->ds_id = ds_id; - txn_req->req.get_data->ds_ctx = ds_ctx; + txn_req->req.get_data->cfg_root = cfg_root; for (indx = 0; indx < num_reqs && indx < MGMTD_MAX_NUM_DATA_REPLY_IN_BATCH; indx++) { @@ -2650,10 +2599,9 @@ int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, } int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - Mgmtd__YangGetDataReq **data_req, - size_t num_reqs) + Mgmtd__DatastoreId ds_id, + Mgmtd__YangGetDataReq **data_req, + size_t num_reqs) { struct mgmt_txn_ctx *txn; struct mgmt_txn_req *txn_req; @@ -2665,7 +2613,7 @@ int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id, txn_req = mgmt_txn_req_alloc(txn, req_id, MGMTD_TXN_PROC_GETDATA); txn_req->req.get_data->ds_id = ds_id; - txn_req->req.get_data->ds_ctx = ds_ctx; + txn_req->req.get_data->cfg_root = NULL; for (indx = 0; indx < num_reqs && indx < MGMTD_MAX_NUM_DATA_REPLY_IN_BATCH; indx++) { diff --git a/mgmtd/mgmt_txn.h b/mgmtd/mgmt_txn.h index 1a9f6d8502..69d75fed07 100644 --- a/mgmtd/mgmt_txn.h +++ b/mgmtd/mgmt_txn.h @@ -169,12 +169,12 @@ extern int mgmt_txn_send_set_config_req(uint64_t txn_id, uint64_t req_id, * 0 on success, -1 on failures. */ extern int mgmt_txn_send_commit_config_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId src_ds_id, - struct mgmt_ds_ctx *dst_ds_ctx, - Mgmtd__DatastoreId dst_ds_id, - struct mgmt_ds_ctx *src_ds_ctx, - bool validate_only, bool abort, - bool implicit); + Mgmtd__DatastoreId src_ds_id, + struct mgmt_ds_ctx *dst_ds_ctx, + Mgmtd__DatastoreId dst_ds_id, + struct mgmt_ds_ctx *src_ds_ctx, + bool validate_only, bool abort, + bool implicit); /* * Send get-config request to be processed later in transaction. @@ -182,10 +182,10 @@ extern int mgmt_txn_send_commit_config_req(uint64_t txn_id, uint64_t req_id, * Similar to set-config request. */ extern int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, - Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, - Mgmtd__YangGetDataReq **data_req, - size_t num_reqs); + Mgmtd__DatastoreId ds_id, + struct nb_config *cfg_root, + Mgmtd__YangGetDataReq **data_req, + size_t num_reqs); /* * Send get-data request to be processed later in transaction. @@ -194,7 +194,6 @@ extern int mgmt_txn_send_get_config_req(uint64_t txn_id, uint64_t req_id, */ extern int mgmt_txn_send_get_data_req(uint64_t txn_id, uint64_t req_id, Mgmtd__DatastoreId ds_id, - struct mgmt_ds_ctx *ds_ctx, Mgmtd__YangGetDataReq **data_req, size_t num_reqs); -- 2.39.5