From c64a92e32187d24dda44d983809f36e20d9cb92f Mon Sep 17 00:00:00 2001
From: Xiubo Li <xiubli@redhat.com>
Date: Tue, 18 Jun 2019 10:12:31 +0800
Subject: [PATCH 1/1] 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 <xiubli@redhat.com>
---
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 c238cc7..ca2557f 100644
--- a/usr/idbm.c
+++ b/usr/idbm.c
@@ -1401,7 +1401,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;
@@ -1411,9 +1416,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) {
@@ -1430,15 +1437,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;
@@ -1470,7 +1483,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;
@@ -1526,7 +1539,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;
@@ -1536,7 +1549,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;
}
@@ -1708,9 +1721,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;
@@ -1735,7 +1748,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;
@@ -1768,7 +1781,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);
@@ -1790,7 +1803,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;
@@ -1827,7 +1840,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;
@@ -1838,7 +1852,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;
@@ -1859,7 +1873,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;
@@ -1877,18 +1891,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;
@@ -1896,7 +1922,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 {
@@ -2141,7 +2167,12 @@ free_portal:
return rc;
}
-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)
{
char *portal;
int rc = 0;
@@ -2172,9 +2203,11 @@ static int idbm_rec_write(node_rec_t *rec)
}
}
- rc = idbm_lock();
- if (rc)
- goto free_portal;
+ if (!disable_lock) {
+ rc = idbm_lock();
+ if (rc)
+ goto free_portal;
+ }
if (rec->tpgt == PORTAL_GROUP_TAG_UNKNOWN)
/* old style portal as config */
@@ -2182,7 +2215,8 @@ static int idbm_rec_write(node_rec_t *rec)
else
rc = idbm_rec_write_new(rec);
- idbm_unlock();
+ if (!disable_lock)
+ idbm_unlock();
free_portal:
free(portal);
return rc;
@@ -2357,18 +2391,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");
@@ -2379,17 +2419,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,
@@ -2397,15 +2439,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,
@@ -2417,8 +2455,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;
}
@@ -2657,7 +2695,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;
@@ -2884,7 +2922,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 411dd82..8571a8c 100644
--- a/usr/idbm.h
+++ b/usr/idbm.h
@@ -23,6 +23,7 @@
#define IDBM_H
#include <stdio.h>
+#include <stdbool.h>
#include <sys/types.h>
#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);
@@ -145,7 +146,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 74e63f6..b2e6db6 100644
--- a/usr/iface.c
+++ b/usr/iface.c
@@ -855,7 +855,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 6245e89..c2b047e 100644
--- a/usr/iscsiadm.c
+++ b/usr/iscsiadm.c
@@ -398,7 +398,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
@@ -515,7 +515,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 */
@@ -662,7 +662,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 "
@@ -915,8 +915,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;
@@ -932,7 +932,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;
@@ -950,14 +950,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 */
@@ -1048,7 +1048,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 f0017e5..01b90c9 100644
--- a/usr/iscsid.c
+++ b/usr/iscsid.c
@@ -233,7 +233,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);
--
2.21.0