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