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>

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