From 1bcf030dc0d4811ebefb000a87d6d696359cb798 Mon Sep 17 00:00:00 2001 From: Xiubo Li Date: Tue, 18 Jun 2019 10:12:31 +0800 Subject: rec update: disable the idbm_lock in read/write when updating the rec The iscsiadm is getting a file lock while writing out records, and updates are a read-modify-write operation and currently only the write is locked. So the parallel requests are reading in the pre-update record and then overwriting each other with the writes. Fixing RHBZ#1624670 Signed-off-by: Xiubo Li --- usr/idbm.c | 132 +++++++++++++++++++++++++++++++++++++-------------------- usr/idbm.h | 17 ++++---- usr/iface.c | 2 +- usr/iscsiadm.c | 18 ++++---- usr/iscsid.c | 2 +- 5 files changed, 105 insertions(+), 66 deletions(-) diff --git a/usr/idbm.c b/usr/idbm.c index 89a6c27..be4d4e3 100644 --- a/usr/idbm.c +++ b/usr/idbm.c @@ -1398,7 +1398,12 @@ static FILE *idbm_open_rec_r(char *portal, char *config) return fopen(portal, "r"); } -static int __idbm_rec_read(node_rec_t *out_rec, char *conf) +/* + * When the disable_lock param is true, the idbm_lock/idbm_unlock needs + * to be holt by the caller, this will avoid overwriting each other in + * case of updating(read-modify-write) the recs in parallel. + */ +static int __idbm_rec_read(node_rec_t *out_rec, char *conf, bool disable_lock) { recinfo_t *info; FILE *f; @@ -1408,9 +1413,11 @@ static int __idbm_rec_read(node_rec_t *out_rec, char *conf) if (!info) return ISCSI_ERR_NOMEM; - rc = idbm_lock(); - if (rc) - goto free_info; + if (!disable_lock) { + rc = idbm_lock(); + if (rc) + goto free_info; + } f = fopen(conf, "r"); if (!f) { @@ -1427,15 +1434,21 @@ static int __idbm_rec_read(node_rec_t *out_rec, char *conf) fclose(f); unlock: - idbm_unlock(); + if (!disable_lock) + idbm_unlock(); free_info: free(info); return rc; } +/* + * When the disable_lock param is true, the idbm_lock/idbm_unlock needs + * to be holt by the caller, this will avoid overwriting each other in + * case of updating(read-modify-write) the recs in parallel. + */ int idbm_rec_read(node_rec_t *out_rec, char *targetname, int tpgt, - char *ip, int port, struct iface_rec *iface) + char *ip, int port, struct iface_rec *iface, bool disable_lock) { struct stat statb; char *portal; @@ -1467,7 +1480,7 @@ idbm_rec_read(node_rec_t *out_rec, char *targetname, int tpgt, } read: - rc = __idbm_rec_read(out_rec, portal); + rc = __idbm_rec_read(out_rec, portal, disable_lock); free_portal: free(portal); return rc; @@ -1523,7 +1536,7 @@ static int idbm_print_discovered(discovery_rec_t *drec, int info_level) int num_found = 0; if (info_level < 1) - idbm_for_each_rec(&num_found, drec, print_discovered_flat); + idbm_for_each_rec(&num_found, drec, print_discovered_flat, false); else { struct discovered_tree_info tree_info; struct node_rec last_rec; @@ -1533,7 +1546,7 @@ static int idbm_print_discovered(discovery_rec_t *drec, int info_level) tree_info.drec = drec; tree_info.last_rec = &last_rec; - idbm_for_each_rec(&num_found, &tree_info, print_discovered_tree); + idbm_for_each_rec(&num_found, &tree_info, print_discovered_tree, false); } return num_found; } @@ -1705,9 +1718,9 @@ int idbm_print_all_discovery(int info_level) * fn should return -1 if it skipped the rec, an ISCSI_ERR error code if * the operation failed or 0 if fn was run successfully. */ -static int idbm_for_each_iface(int *found, void *data, - idbm_iface_op_fn *fn, - char *targetname, int tpgt, char *ip, int port) +static int idbm_for_each_iface(int *found, void *data, idbm_iface_op_fn *fn, + char *targetname, int tpgt, char *ip, int port, + bool ruw_lock) { DIR *iface_dirfd; struct dirent *iface_dent; @@ -1732,7 +1745,7 @@ static int idbm_for_each_iface(int *found, void *data, goto free_portal; } - rc = __idbm_rec_read(&rec, portal); + rc = __idbm_rec_read(&rec, portal, ruw_lock); if (rc) goto free_portal; @@ -1765,7 +1778,7 @@ read_iface: memset(portal, 0, PATH_MAX); snprintf(portal, PATH_MAX, "%s/%s/%s,%d,%d/%s", NODE_CONFIG_DIR, targetname, ip, port, tpgt, iface_dent->d_name); - if (__idbm_rec_read(&rec, portal)) + if (__idbm_rec_read(&rec, portal, ruw_lock)) continue; curr_rc = fn(data, &rec); @@ -1787,7 +1800,7 @@ free_portal: * The portal could be a file or dir with interfaces */ int idbm_for_each_portal(int *found, void *data, idbm_portal_op_fn *fn, - char *targetname) + char *targetname, bool ruw_lock) { DIR *portal_dirfd; struct dirent *portal_dent; @@ -1824,7 +1837,8 @@ int idbm_for_each_portal(int *found, void *data, idbm_portal_op_fn *fn, curr_rc = fn(found, data, targetname, tmp_tpgt ? atoi(tmp_tpgt) : -1, - portal_dent->d_name, atoi(tmp_port)); + portal_dent->d_name, atoi(tmp_port), + ruw_lock); /* less than zero means it was not a match */ if (curr_rc > 0 && !rc) rc = curr_rc; @@ -1835,7 +1849,7 @@ done: return rc; } -int idbm_for_each_node(int *found, void *data, idbm_node_op_fn *fn) +int idbm_for_each_node(int *found, void *data, idbm_node_op_fn *fn, bool ruw_lock) { DIR *node_dirfd; struct dirent *node_dent; @@ -1856,7 +1870,7 @@ int idbm_for_each_node(int *found, void *data, idbm_node_op_fn *fn) continue; log_debug(5, "searching %s", node_dent->d_name); - curr_rc = fn(found, data, node_dent->d_name); + curr_rc = fn(found, data, node_dent->d_name, ruw_lock); /* less than zero means it was not a match */ if (curr_rc > 0 && !rc) rc = curr_rc; @@ -1874,18 +1888,30 @@ static int iface_fn(void *data, node_rec_t *rec) } static int portal_fn(int *found, void *data, char *targetname, - int tpgt, char *ip, int port) + int tpgt, char *ip, int port, bool ruw_lock) { - return idbm_for_each_iface(found, data, iface_fn, targetname, - tpgt, ip, port); + int rc; + + if (ruw_lock) { + rc = idbm_lock(); + if (rc) + return rc; + } + + rc = idbm_for_each_iface(found, data, iface_fn, targetname, + tpgt, ip, port, ruw_lock); + if (ruw_lock) + idbm_unlock(); + + return rc; } -static int node_fn(int *found, void *data, char *targetname) +static int node_fn(int *found, void *data, char *targetname, bool ruw_lock) { - return idbm_for_each_portal(found, data, portal_fn, targetname); + return idbm_for_each_portal(found, data, portal_fn, targetname, ruw_lock); } -int idbm_for_each_rec(int *found, void *data, idbm_iface_op_fn *fn) +int idbm_for_each_rec(int *found, void *data, idbm_iface_op_fn *fn, bool ruw_lock) { struct rec_op_data op_data; @@ -1893,7 +1919,7 @@ int idbm_for_each_rec(int *found, void *data, idbm_iface_op_fn *fn) op_data.data = data; op_data.fn = fn; - return idbm_for_each_node(found, &op_data, node_fn); + return idbm_for_each_node(found, &op_data, node_fn, ruw_lock); } static struct { @@ -2004,7 +2030,12 @@ mkdir_portal: return f; } -static int idbm_rec_write(node_rec_t *rec) +/* + * When the disable_lock param is true, the idbm_lock/idbm_unlock needs + * to be holt by the caller, this will avoid overwriting each other in + * case of updating(read-modify-write) the recs in parallel. + */ +static int idbm_rec_write(node_rec_t *rec, bool disable_lock) { struct stat statb; FILE *f; @@ -2041,9 +2072,11 @@ static int idbm_rec_write(node_rec_t *rec) rec->name, rec->conn[0].address, rec->conn[0].port); log_debug(5, "Looking for config file %s", portal); - rc = idbm_lock(); - if (rc) - goto free_portal; + if (!disable_lock) { + rc = idbm_lock(); + if (rc) + goto free_portal; + } rc = stat(portal, &statb); if (rc) { @@ -2109,7 +2142,8 @@ open_conf: idbm_print(IDBM_PRINT_TYPE_NODE, rec, 1, f); fclose(f); unlock: - idbm_unlock(); + if (!disable_lock) + idbm_unlock(); free_portal: free(portal); return rc; @@ -2284,18 +2318,24 @@ static int setup_disc_to_node_link(char *disc_portal, node_rec_t *rec) int idbm_add_node(node_rec_t *newrec, discovery_rec_t *drec, int overwrite) { node_rec_t rec; - char *node_portal, *disc_portal; + char *node_portal = NULL, *disc_portal; int rc; + rc = idbm_lock(); + if (rc) + return rc; + if (!idbm_rec_read(&rec, newrec->name, newrec->tpgt, newrec->conn[0].address, newrec->conn[0].port, - &newrec->iface)) { - if (!overwrite) - return 0; + &newrec->iface, true)) { + if (!overwrite) { + rc = 0; + goto unlock; + } rc = idbm_delete_node(&rec); if (rc) - return rc; + goto unlock; log_debug(7, "overwriting existing record"); } else log_debug(7, "adding new DB record"); @@ -2306,17 +2346,19 @@ int idbm_add_node(node_rec_t *newrec, discovery_rec_t *drec, int overwrite) strcpy(newrec->disc_address, drec->address); } - rc = idbm_rec_write(newrec); + rc = idbm_rec_write(newrec, true); /* * if a old app passed in a bogus tpgt then we do not create links * since it will set a different tpgt in another iscsiadm call */ if (rc || !drec || newrec->tpgt == PORTAL_GROUP_TAG_UNKNOWN) - return rc; + goto unlock; node_portal = calloc(2, PATH_MAX); - if (!node_portal) - return ISCSI_ERR_NOMEM; + if (!node_portal) { + rc = ISCSI_ERR_NOMEM; + goto unlock; + } disc_portal = node_portal + PATH_MAX; snprintf(node_portal, PATH_MAX, "%s/%s/%s,%d,%d", NODE_CONFIG_DIR, @@ -2324,15 +2366,11 @@ int idbm_add_node(node_rec_t *newrec, discovery_rec_t *drec, int overwrite) newrec->tpgt); rc = setup_disc_to_node_link(disc_portal, newrec); if (rc) - goto free_portal; + goto unlock; log_debug(7, "node addition making link from %s to %s", node_portal, disc_portal); - rc = idbm_lock(); - if (rc) - goto free_portal; - if (symlink(node_portal, disc_portal)) { if (errno == EEXIST) log_debug(7, "link from %s to %s exists", node_portal, @@ -2344,8 +2382,8 @@ int idbm_add_node(node_rec_t *newrec, discovery_rec_t *drec, int overwrite) strerror(errno)); } } +unlock: idbm_unlock(); -free_portal: free(node_portal); return rc; } @@ -2584,7 +2622,7 @@ static int idbm_remove_disc_to_node_link(node_rec_t *rec, rec->name, rec->conn[0].address, rec->conn[0].port, rec->tpgt, rec->iface.name); - rc = __idbm_rec_read(tmprec, portal); + rc = __idbm_rec_read(tmprec, portal, false); if (rc) { /* old style recs will not have tpgt or a link so skip */ rc = 0; @@ -2811,7 +2849,7 @@ int idbm_node_set_param(void *data, node_rec_t *rec) if (rc) return rc; - return idbm_rec_write(rec); + return idbm_rec_write(rec, true); } int idbm_discovery_set_param(void *data, discovery_rec_t *rec) diff --git a/usr/idbm.h b/usr/idbm.h index b83c0bb..18c5025 100644 --- a/usr/idbm.h +++ b/usr/idbm.h @@ -23,6 +23,7 @@ #define IDBM_H #include +#include #include #include "initiator.h" #include "config.h" @@ -91,22 +92,22 @@ struct user_param { }; typedef int (idbm_iface_op_fn)(void *data, node_rec_t *rec); -typedef int (idbm_portal_op_fn)(int *found, void *data, - char *targetname, int tpgt, char *ip, int port); +typedef int (idbm_portal_op_fn)(int *found, void *data, char *targetname, + int tpgt, char *ip, int port, bool ruw_lock); typedef int (idbm_node_op_fn)(int *found, void *data, - char *targetname); + char *targetname, bool ruw_lock); struct rec_op_data { void *data; node_rec_t *match_rec; idbm_iface_op_fn *fn; }; -extern int idbm_for_each_portal(int *found, void *data, - idbm_portal_op_fn *fn, char *targetname); +extern int idbm_for_each_portal(int *found, void *data, idbm_portal_op_fn *fn, + char *targetname, bool ruw_lock); extern int idbm_for_each_node(int *found, void *data, - idbm_node_op_fn *fn); + idbm_node_op_fn *fn, bool ruw_lock); extern int idbm_for_each_rec(int *found, void *data, - idbm_iface_op_fn *fn); + idbm_iface_op_fn *fn, bool ruw_lock); typedef int (idbm_drec_op_fn)(void *data, discovery_rec_t *drec); @@ -144,7 +145,7 @@ extern int idbm_discovery_read(discovery_rec_t *rec, int type, char *addr, int port); extern int idbm_rec_read(node_rec_t *out_rec, char *target_name, int tpgt, char *addr, int port, - struct iface_rec *iface); + struct iface_rec *iface, bool disable_lock); extern int idbm_node_set_rec_from_param(struct list_head *params, node_rec_t *rec, int verify); extern int idbm_node_set_param(void *data, node_rec_t *rec); diff --git a/usr/iface.c b/usr/iface.c index 137138f..645b0b8 100644 --- a/usr/iface.c +++ b/usr/iface.c @@ -854,7 +854,7 @@ int iface_print_tree(void *data, struct iface_rec *iface) print_data.match_iface = iface; print_data.last_rec = &last_rec; - idbm_for_each_rec(&num_found, &print_data, iface_print_nodes); + idbm_for_each_rec(&num_found, &print_data, iface_print_nodes, false); return 0; } diff --git a/usr/iscsiadm.c b/usr/iscsiadm.c index a8f6f4a..9215dc1 100644 --- a/usr/iscsiadm.c +++ b/usr/iscsiadm.c @@ -370,7 +370,7 @@ __logout_by_startup(void *data, struct list_head *list, memset(&rec, 0, sizeof(node_rec_t)); if (idbm_rec_read(&rec, info->targetname, info->tpgt, info->persistent_address, - info->persistent_port, &info->iface)) { + info->persistent_port, &info->iface, false)) { /* * this is due to a HW driver or some other driver * not hooked in @@ -487,7 +487,7 @@ login_by_startup(char *mode) startup.mode = mode; INIT_LIST_HEAD(&startup.all_logins); INIT_LIST_HEAD(&startup.leading_logins); - err = idbm_for_each_rec(&nr_found, &startup, link_startup_recs); + err = idbm_for_each_rec(&nr_found, &startup, link_startup_recs, false); if (err && (!list_empty(&startup.all_logins) || !list_empty(&startup.leading_logins))) /* log msg and try to log into what we found */ @@ -634,7 +634,7 @@ static int __for_each_matched_rec(int verbose, struct node_rec *rec, op_data.match_rec = rec; op_data.fn = fn; - rc = idbm_for_each_rec(&nr_found, &op_data, rec_match_fn); + rc = idbm_for_each_rec(&nr_found, &op_data, rec_match_fn, true); if (rc) { if (verbose) log_error("Could not execute operation on all " @@ -994,8 +994,8 @@ done: return rc; } -static int add_static_portal(int *found, void *data, - char *targetname, int tpgt, char *ip, int port) +static int add_static_portal(int *found, void *data, char *targetname, + int tpgt, char *ip, int port, bool ruw_lock) { node_rec_t *rec = data; @@ -1011,7 +1011,7 @@ static int add_static_portal(int *found, void *data, } static int add_static_node(int *found, void *data, - char *targetname) + char *targetname, bool ruw_lock) { node_rec_t *rec = data; @@ -1029,14 +1029,14 @@ static int add_static_node(int *found, void *data, rec->conn[0].port, &rec->iface); search: return idbm_for_each_portal(found, data, add_static_portal, - targetname); + targetname, false); } static int add_static_recs(struct node_rec *rec) { int rc, nr_found = 0; - rc = idbm_for_each_node(&nr_found, rec, add_static_node); + rc = idbm_for_each_node(&nr_found, rec, add_static_node, false); if (rc) goto done; /* success */ @@ -1127,7 +1127,7 @@ exec_disc_op_on_recs(discovery_rec_t *drec, struct list_head *rec_list, /* clean up node db */ if (op & OP_DELETE) - idbm_for_each_rec(&found, rec_list, delete_stale_rec); + idbm_for_each_rec(&found, rec_list, delete_stale_rec, false); if (op & OP_NEW || op & OP_UPDATE) { /* now add/update records */ diff --git a/usr/iscsid.c b/usr/iscsid.c index 37c13b3..8f1c597 100644 --- a/usr/iscsid.c +++ b/usr/iscsid.c @@ -237,7 +237,7 @@ static int sync_session(void *data, struct session_info *info) if (idbm_rec_read(&rec, info->targetname, info->tpgt, info->persistent_address, info->persistent_port, - &info->iface)) { + &info->iface, false)) { log_warning("Could not read data from db. Using default and " "currently negotiated values"); setup_rec_from_negotiated_values(&rec, info); -- cgit v1.2.1