[PATCH 1/2] Safeguard against writing to an active device of another node

[PATCH 1/2] Safeguard against writing to an active device of another node

[Guoqing Jiang]

Modifying an exiting device's superblock or creating a new superblock
on an existing device needs to be checked because the device could be
in use by another node in another array. So, we check this by taking
all superblock locks in userspace so that we don't  step onto an active
device used by another node and safeguard against accidental edits.
After the edit is complete, we release all locks and the lockspace so
that it can be used by the kernel space.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
 Makefile |   3 +-
 mdadm.c  |  11 +++++
 mdadm.h  |  46 ++++++++++++++++++
 super1.c |  51 ++++++++++++++++++++
 util.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 275 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index c189279..370ef75 100644
--- a/Makefile
+++ b/Makefile
@@ -81,11 +81,12 @@ FAILED_SLOTS_DIR = $(RUN_DIR)/failed-slots
 SYSTEMD_DIR=/lib/systemd/system
 
 COROSYNC:=$(shell [ -d /usr/include/corosync ] || echo -DNO_COROSYNC)
+DLM:=$(shell [ -f /usr/include/libdlm.h ] || echo -DNO_DLM)
 
 DIRFLAGS = -DMAP_DIR=\"$(MAP_DIR)\" -DMAP_FILE=\"$(MAP_FILE)\"
 DIRFLAGS += -DMDMON_DIR=\"$(MDMON_DIR)\"
 DIRFLAGS += -DFAILED_SLOTS_DIR=\"$(FAILED_SLOTS_DIR)\"
-CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(DIRFLAGS) $(COROSYNC)
+CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(DIRFLAGS) $(COROSYNC) $(DLM)
 
 VERSION = $(shell [ -d .git ] && git describe HEAD | sed 's/mdadm-//')
 VERS_DATE = $(shell [ -d .git ] && date --date="`git log -n1 --format=format:%cd --date=short`" '+%0dth %B %Y' | sed -e 's/1th/1st/' -e 's/2th/2nd/' -e 's/11st/11th/' -e 's/12nd/12th/')
diff --git a/mdadm.c b/mdadm.c
index c4daf25..d8032c1 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1322,6 +1322,8 @@ int main(int argc, char *argv[])
 		/* --scan implied --brief unless -vv */
 		c.brief = 1;
 
+	set_dlm_hookers(); /* get dlm funcs from libdlm_lt.so.3 */
+
 	rv = 0;
 	switch(mode) {
 	case MANAGE:
@@ -1362,10 +1364,12 @@ int main(int argc, char *argv[])
 		else if (devs_found > 0) {
 			if (c.update && devs_found > 1) {
 				pr_err("can only update a single array at a time\n");
+				free_dlm_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			if (c.backup_file && devs_found > 1) {
 				pr_err("can only assemble a single array when providing a backup file.\n");
+				free_dlm_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			for (dv = devlist ; dv ; dv=dv->next) {
@@ -1384,10 +1388,12 @@ int main(int argc, char *argv[])
 		} else {
 			if (c.update) {
 				pr_err("--update not meaningful with a --scan assembly.\n");
+				free_dlm_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			if (c.backup_file) {
 				pr_err("--backup_file not meaningful with a --scan assembly.\n");
+				free_dlm_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			rv = scan_assemble(ss, &c, &ident);
@@ -1455,12 +1461,14 @@ int main(int argc, char *argv[])
 		if (devmode == 'E') {
 			if (devlist == NULL && !c.scan) {
 				pr_err("No devices to examine\n");
+				free_dlm_hookers(); /* close dlm stuffs */
 				exit(2);
 			}
 			if (devlist == NULL)
 				devlist = conf_get_devs();
 			if (devlist == NULL) {
 				pr_err("No devices listed in %s\n", configfile?configfile:DefaultConfFile);
+				free_dlm_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			rv = Examine(devlist, &c, ss);
@@ -1477,6 +1485,7 @@ int main(int argc, char *argv[])
 				rv = Write_rules(udev_filename);
 			else {
 				pr_err("No devices given.\n");
+				free_dlm_hookers(); /* close dlm stuffs */
 				exit(2);
 			}
 		} else
@@ -1615,6 +1624,8 @@ int main(int argc, char *argv[])
 		autodetect();
 		break;
 	}
+
+	free_dlm_hookers(); /* close dlm stuffs */
 	exit(rv);
 }
 
diff --git a/mdadm.h b/mdadm.h
index 97892e6..c53adc5 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -35,6 +35,7 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
 
 #include	<sys/types.h>
 #include	<sys/stat.h>
+#include	<stdint.h>
 #include	<stdlib.h>
 #include	<time.h>
 #include	<sys/time.h>
@@ -51,6 +52,25 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
 #define srandom srand
 #endif
 
+#ifndef NO_DLM
+#include	<libdlm.h>
+#include	<errno.h>
+#else
+#define LKF_NOQUEUE	0x00000001
+#define LKF_CONVERT	0x00000004
+#define LKM_PWMODE	4
+#define EUNLOCK		0x10002
+
+typedef void *dlm_lshandle_t;
+
+struct dlm_lksb {
+	int sb_status;
+	uint32_t sb_lkid;
+	char sb_flags;
+	char *sb_lvbptr;
+};
+#endif
+
 #include	<linux/kdev_t.h>
 /*#include	<linux/fs.h> */
 #include	<sys/mount.h>
@@ -1428,7 +1448,33 @@ extern char *stat2devnm(struct stat *st);
 extern char *fd2devnm(int fd);
 
 extern int in_initrd(void);
+
+struct dlm_hookers {
+	void *dlm_handle;	/* dlm lib related */
+
+	dlm_lshandle_t (*create_lockspace)(const char *name,
+					   unsigned int mode);
+	int (*release_lockspace)(const char *name, dlm_lshandle_t ls,
+				 int force);
+	int (*ls_lock)(dlm_lshandle_t lockspace, uint32_t mode,
+		       struct dlm_lksb *lksb, uint32_t flags,
+		       const void *name, unsigned int namelen,
+		       uint32_t parent, void (*astaddr) (void *astarg),
+		       void *astarg, void (*bastaddr) (void *astarg),
+		       void *range);
+	int (*ls_unlock)(dlm_lshandle_t lockspace, uint32_t lkid,
+			 uint32_t flags, struct dlm_lksb *lksb,
+			 void *astarg);
+	int (*ls_get_fd)(dlm_lshandle_t ls);
+	int (*dispatch)(int fd);
+};
+
 extern int get_cluster_name(char **name);
+extern int is_clustered(struct supertype *st);
+extern int cluster_get_dlmlock(struct supertype *st, int *lockid);
+extern int cluster_release_dlmlock(struct supertype *st, int lockid);
+extern void set_dlm_hookers(void);
+extern void free_dlm_hookers(void);
 
 #define _ROUND_UP(val, base)	(((val) + (base) - 1) & ~(base - 1))
 #define ROUND_UP(val, base)	_ROUND_UP(val, (typeof(val))(base))
diff --git a/super1.c b/super1.c
index fda71e3..bd88c36 100644
--- a/super1.c
+++ b/super1.c
@@ -1072,8 +1072,18 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 	 * ignored.
 	 */
 	int rv = 0;
+	int lockid;
 	struct mdp_superblock_1 *sb = st->sb;
 
+	if (is_clustered(st)) {
+		rv = cluster_get_dlmlock(st, &lockid);
+		if (rv) {
+			pr_err("Cannot get dlmlock in %s return %d\n", __func__, rv);
+			cluster_release_dlmlock(st, lockid);
+			return rv;
+		}
+	}
+
 	if (strcmp(update, "homehost") == 0 &&
 	    homehost) {
 		/* Note that 'homehost' is special as it is really
@@ -1330,6 +1340,9 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 		rv = -1;
 
 	sb->sb_csum = calc_sb_1_csum(sb);
+	if (is_clustered(st))
+		cluster_release_dlmlock(st, lockid);
+
 	return rv;
 }
 
@@ -1433,6 +1446,16 @@ static int add_to_super1(struct supertype *st, mdu_disk_info_t *dk,
 	struct mdp_superblock_1 *sb = st->sb;
 	__u16 *rp = sb->dev_roles + dk->number;
 	struct devinfo *di, **dip;
+	int rv, lockid;
+
+	if (is_clustered(st)) {
+		rv = cluster_get_dlmlock(st, &lockid);
+		if (rv) {
+			pr_err("Cannot get dlmlock in %s return %d\n", __func__, rv);
+			cluster_release_dlmlock(st, lockid);
+			return rv;
+		}
+	}
 
 	if ((dk->state & 6) == 6) /* active, sync */
 		*rp = __cpu_to_le16(dk->raid_disk);
@@ -1460,6 +1483,9 @@ static int add_to_super1(struct supertype *st, mdu_disk_info_t *dk,
 	di->next = NULL;
 	*dip = di;
 
+	if (is_clustered(st))
+		cluster_release_dlmlock(st, lockid);
+
 	return 0;
 }
 #endif
@@ -1473,6 +1499,16 @@ static int store_super1(struct supertype *st, int fd)
 	struct align_fd afd;
 	int sbsize;
 	unsigned long long dsize;
+	int rv, lockid;
+
+	if (is_clustered(st)) {
+		rv = cluster_get_dlmlock(st, &lockid);
+		if (rv) {
+			pr_err("Cannot get dlmlock in %s return %d\n", __func__, rv);
+			cluster_release_dlmlock(st, lockid);
+			return rv;
+		}
+	}
 
 	if (!get_dev_size(fd, NULL, &dsize))
 		return 1;
@@ -1533,6 +1569,9 @@ static int store_super1(struct supertype *st, int fd)
 		}
 	}
 	fsync(fd);
+	if (is_clustered(st))
+		cluster_release_dlmlock(st, lockid);
+
 	return 0;
 }
 
@@ -2282,6 +2321,16 @@ static int write_bitmap1(struct supertype *st, int fd, enum bitmap_update update
 
 static void free_super1(struct supertype *st)
 {
+	int rv, lockid;
+	if (is_clustered(st)) {
+		rv = cluster_get_dlmlock(st, &lockid);
+		if (rv) {
+			pr_err("Cannot get dlmlock in %s return %d\n", __func__, rv);
+			cluster_release_dlmlock(st, lockid);
+			return;
+		}
+	}
+
 	if (st->sb)
 		free(st->sb);
 	while (st->info) {
@@ -2292,6 +2341,8 @@ static void free_super1(struct supertype *st)
 		free(di);
 	}
 	st->sb = NULL;
+	if (is_clustered(st))
+		cluster_release_dlmlock(st, lockid);
 }
 
 #ifndef MDASSEMBLE
diff --git a/util.c b/util.c
index ea6e688..19ecf9f 100644
--- a/util.c
+++ b/util.c
@@ -24,6 +24,7 @@
 
 #include	"mdadm.h"
 #include	"md_p.h"
+#include	<sys/poll.h>
 #include	<sys/socket.h>
 #include	<sys/utsname.h>
 #include	<sys/wait.h>
@@ -88,6 +89,135 @@ struct blkpg_partition {
    aren't permitted). */
 #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
 
+static struct dlm_hookers *dlm_hookers = NULL;
+static int is_dlm_hookers_ready = 0;
+static struct dlm_lock_resource *dlm_lock_res = NULL;
+static int ast_called = 0;
+
+struct dlm_lock_resource {
+	dlm_lshandle_t *ls;
+	struct dlm_lksb lksb;
+};
+
+int is_clustered(struct supertype *st)
+{
+	/* is it a cluster md or not */
+	if (is_dlm_hookers_ready && st->cluster_name)
+		return 1;
+	else
+		return 0;
+}
+
+/* Using poll(2) to wait for and dispatch ASTs */
+static int poll_for_ast(dlm_lshandle_t ls)
+{
+	struct pollfd pfd;
+
+	pfd.fd = dlm_hookers->ls_get_fd(ls);
+	pfd.events = POLLIN;
+
+	while (!ast_called)
+	{
+		if (poll(&pfd, 1, 0) < 0)
+		{
+			perror("poll");
+			return -1;
+		}
+		dlm_hookers->dispatch(dlm_hookers->ls_get_fd(ls));
+	}
+	ast_called = 0;
+
+	return 0;
+}
+
+static void dlm_ast(void *arg)
+{
+	ast_called = 1;
+}
+
+/* Create the lockspace, take bitmapXXX locks on all the bitmaps. */
+int cluster_get_dlmlock(struct supertype *st, int *lockid)
+{
+	int ret = -1;
+	char str[64];
+	int flags = LKF_NOQUEUE;
+
+	dlm_lock_res = xmalloc(sizeof(struct dlm_lock_resource));
+	if (!dlm_lock_res)
+                goto out;
+
+	dlm_lock_res->ls = dlm_hookers->create_lockspace(st->cluster_name, O_RDWR);
+	if (!dlm_lock_res->ls) {
+		pr_err("%s failed to create lockspace\n", st->cluster_name);
+                goto out;
+	}
+
+	/* Conversions need the lockid in the LKSB */
+	if (flags & LKF_CONVERT)
+		dlm_lock_res->lksb.sb_lkid = *lockid;
+
+	snprintf(str, 64, "bitmap%04d", st->nodes);
+	/* if flags with LKF_CONVERT causes below return ENOENT which means
+	 * "No such file or directory" */
+	ret = dlm_hookers->ls_lock(dlm_lock_res->ls, LKM_PWMODE, &dlm_lock_res->lksb,
+			  flags, str, strlen(str), 0, dlm_ast,
+			  dlm_lock_res, NULL, NULL);
+	if (ret) {
+		pr_err("error %d when get PW mode on lock %s\n", errno, str);
+                goto out;
+	}
+
+	/* Wait for it to complete */
+	poll_for_ast(dlm_lock_res->ls);
+	*lockid = dlm_lock_res->lksb.sb_lkid;
+
+	errno =	dlm_lock_res->lksb.sb_status;
+	if (errno) {
+		pr_err("error %d happened in ast with lock %s\n", errno, str);
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+int cluster_release_dlmlock(struct supertype *st, int lockid)
+{
+	int ret = -1;
+
+	/* if flags with LKF_CONVERT causes below return EINVAL which means
+	 * "Invalid argument" */
+	ret = dlm_hookers->ls_unlock(dlm_lock_res->ls, lockid, 0,
+				     &dlm_lock_res->lksb, dlm_lock_res);
+	if (ret) {
+		pr_err("error %d happened when unlock\n", errno);
+		/* XXX make sure the lock is unlocked eventually */
+                goto out;
+	}
+
+	/* Wait for it to complete */
+	poll_for_ast(dlm_lock_res->ls);
+
+	errno =	dlm_lock_res->lksb.sb_status;
+	if (errno != EUNLOCK) {
+		pr_err("error %d happened in ast when unlock lockspace\n", errno);
+		/* XXX make sure the lockspace is unlocked eventually */
+                goto out;
+	}
+
+	ret = dlm_hookers->release_lockspace(st->cluster_name, dlm_lock_res->ls, 1);
+	if (ret) {
+		pr_err("error %d happened when release lockspace\n", errno);
+		/* XXX make sure the lockspace is released eventually */
+                goto out;
+	}
+	free(dlm_lock_res);
+
+out:
+	return ret;
+}
+
+
 /*
  * Parse a 128 bit uuid in 4 integers
  * format is 32 hexx nibbles with options :.<space> separator
@@ -2043,3 +2173,38 @@ out:
         dlclose(lib_handle);
         return rv;
 }
+
+void set_dlm_hookers(void)
+{
+	dlm_hookers = xmalloc(sizeof(struct dlm_hookers));
+	if (!dlm_hookers)
+		return;
+
+	dlm_hookers->dlm_handle = dlopen("libdlm_lt.so.3", RTLD_NOW | RTLD_LOCAL);
+	if (!dlm_hookers->dlm_handle)
+		return;
+
+	dlm_hookers->create_lockspace = dlsym(dlm_hookers->dlm_handle, "dlm_create_lockspace");
+	dlm_hookers->release_lockspace = dlsym(dlm_hookers->dlm_handle, "dlm_release_lockspace");
+	dlm_hookers->ls_lock = dlsym(dlm_hookers->dlm_handle, "dlm_ls_lock");
+	dlm_hookers->ls_unlock = dlsym(dlm_hookers->dlm_handle, "dlm_ls_unlock");
+	dlm_hookers->ls_get_fd = dlsym(dlm_hookers->dlm_handle, "dlm_ls_get_fd");
+	dlm_hookers->dispatch = dlsym(dlm_hookers->dlm_handle, "dlm_dispatch");
+
+	if (!dlm_hookers->create_lockspace || !dlm_hookers->ls_lock ||
+	    !dlm_hookers->ls_unlock || !dlm_hookers->release_lockspace ||
+	    !dlm_hookers->ls_get_fd || !dlm_hookers->dispatch)
+		dlclose(dlm_hookers->dlm_handle);
+	else
+		is_dlm_hookers_ready = 1;
+}
+
+void free_dlm_hookers(void)
+{
+	if (is_dlm_hookers_ready) {
+		dlclose(dlm_hookers->dlm_handle);
+		is_dlm_hookers_ready = 0;
+	}
+	if (dlm_hookers)
+		free(dlm_hookers);
+}
-- 
1.7.12.4

[PATCH 2/2] Make cmap_* also has same policy as dlm_*

[Guoqing Jiang]

Let libcmap lib and related funs also only need one-time
setup during mdadm running period.

Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
 mdadm.c | 23 +++++++++++--------
 mdadm.h | 20 ++++++++++++++++
 util.c  | 81 +++++++++++++++++++++++++++++++++++++++--------------------------
 3 files changed, 82 insertions(+), 42 deletions(-)

diff --git a/mdadm.c b/mdadm.c
index d8032c1..4812fc5 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1293,18 +1293,22 @@ int main(int argc, char *argv[])
 		c.require_homehost = 0;
 	}
 
+	set_hookers(); /* set hookers from libs */
+
 	if (c.homecluster == NULL && (c.nodes > 0)) {
 		c.homecluster = conf_get_homecluster();
 		if (c.homecluster == NULL)
 			rv = get_cluster_name(&c.homecluster);
 		if (rv != 0) {
 			pr_err("The md can't get cluster name\n");
+			free_hookers();
 			exit(1);
 		}
 	}
 
 	if (c.backup_file && data_offset != INVALID_SECTORS) {
 		pr_err("--backup-file and --data-offset are incompatible\n");
+		free_hookers();
 		exit(2);
 	}
 
@@ -1313,6 +1317,7 @@ int main(int argc, char *argv[])
 		/* Anyone may try this */;
 	else if (geteuid() != 0) {
 		pr_err("must be super-user to perform this action\n");
+		free_hookers();
 		exit(1);
 	}
 
@@ -1322,8 +1327,6 @@ int main(int argc, char *argv[])
 		/* --scan implied --brief unless -vv */
 		c.brief = 1;
 
-	set_dlm_hookers(); /* get dlm funcs from libdlm_lt.so.3 */
-
 	rv = 0;
 	switch(mode) {
 	case MANAGE:
@@ -1364,12 +1367,12 @@ int main(int argc, char *argv[])
 		else if (devs_found > 0) {
 			if (c.update && devs_found > 1) {
 				pr_err("can only update a single array at a time\n");
-				free_dlm_hookers(); /* close dlm stuffs */
+				free_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			if (c.backup_file && devs_found > 1) {
 				pr_err("can only assemble a single array when providing a backup file.\n");
-				free_dlm_hookers(); /* close dlm stuffs */
+				free_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			for (dv = devlist ; dv ; dv=dv->next) {
@@ -1388,12 +1391,12 @@ int main(int argc, char *argv[])
 		} else {
 			if (c.update) {
 				pr_err("--update not meaningful with a --scan assembly.\n");
-				free_dlm_hookers(); /* close dlm stuffs */
+				free_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			if (c.backup_file) {
 				pr_err("--backup_file not meaningful with a --scan assembly.\n");
-				free_dlm_hookers(); /* close dlm stuffs */
+				free_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			rv = scan_assemble(ss, &c, &ident);
@@ -1461,14 +1464,14 @@ int main(int argc, char *argv[])
 		if (devmode == 'E') {
 			if (devlist == NULL && !c.scan) {
 				pr_err("No devices to examine\n");
-				free_dlm_hookers(); /* close dlm stuffs */
+				free_hookers(); /* close dlm stuffs */
 				exit(2);
 			}
 			if (devlist == NULL)
 				devlist = conf_get_devs();
 			if (devlist == NULL) {
 				pr_err("No devices listed in %s\n", configfile?configfile:DefaultConfFile);
-				free_dlm_hookers(); /* close dlm stuffs */
+				free_hookers(); /* close dlm stuffs */
 				exit(1);
 			}
 			rv = Examine(devlist, &c, ss);
@@ -1485,7 +1488,7 @@ int main(int argc, char *argv[])
 				rv = Write_rules(udev_filename);
 			else {
 				pr_err("No devices given.\n");
-				free_dlm_hookers(); /* close dlm stuffs */
+				free_hookers(); /* close dlm stuffs */
 				exit(2);
 			}
 		} else
@@ -1625,7 +1628,7 @@ int main(int argc, char *argv[])
 		break;
 	}
 
-	free_dlm_hookers(); /* close dlm stuffs */
+	free_hookers(); /* close dlm stuffs */
 	exit(rv);
 }
 
diff --git a/mdadm.h b/mdadm.h
index c53adc5..dd7fef4 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -52,6 +52,12 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
 #define srandom srand
 #endif
 
+#ifdef NO_COROSYNC
+#define CS_OK 1
+#else
+#include	<corosync/cmap.h>
+#endif
+
 #ifndef NO_DLM
 #include	<libdlm.h>
 #include	<errno.h>
@@ -1449,6 +1455,16 @@ extern char *fd2devnm(int fd);
 
 extern int in_initrd(void);
 
+struct cmap_hookers {
+	void *cmap_handle;	/* corosync lib related */
+
+	int (*initialize)(cmap_handle_t *handle);
+	int (*get_string)(cmap_handle_t handle,
+			  const char *string,
+			  char **name);
+	int (*finalize)(cmap_handle_t handle);
+};
+
 struct dlm_hookers {
 	void *dlm_handle;	/* dlm lib related */
 
@@ -1475,6 +1491,10 @@ extern int cluster_get_dlmlock(struct supertype *st, int *lockid);
 extern int cluster_release_dlmlock(struct supertype *st, int lockid);
 extern void set_dlm_hookers(void);
 extern void free_dlm_hookers(void);
+extern void set_cmap_hookers(void);
+extern void free_cmap_hookers(void);
+extern void set_hookers(void);
+extern void free_hookers(void);
 
 #define _ROUND_UP(val, base)	(((val) + (base) - 1) & ~(base - 1))
 #define ROUND_UP(val, base)	_ROUND_UP(val, (typeof(val))(base))
diff --git a/util.c b/util.c
index 19ecf9f..1c87eb5 100644
--- a/util.c
+++ b/util.c
@@ -36,13 +36,6 @@
 #include	<dirent.h>
 #include	<signal.h>
 #include	<dlfcn.h>
-#include	<stdint.h>
-#ifdef NO_COROSYNC
- typedef uint64_t cmap_handle_t;
- #define CS_OK 1
-#else
- #include	<corosync/cmap.h>
-#endif
 
 
 /*
@@ -2126,40 +2119,53 @@ void reopen_mddev(int mdfd)
 		dup2(fd, mdfd);
 }
 
-int get_cluster_name(char **cluster_name)
+static struct cmap_hookers *cmap_hookers = NULL;
+static int is_cmap_hookers_ready = 0;
+
+void set_cmap_hookers(void)
 {
-        void *lib_handle = NULL;
-        int rv = -1;
+	cmap_hookers = xmalloc(sizeof(struct cmap_hookers));
+	if (!cmap_hookers)
+		return;
 
-        cmap_handle_t handle;
-        static int (*initialize)(cmap_handle_t *handle);
-        static int (*get_string)(cmap_handle_t handle,
-				 const char *string,
-				 char **name);
-        static int (*finalize)(cmap_handle_t handle);
+	cmap_hookers->cmap_handle = dlopen("libcmap.so.4", RTLD_NOW | RTLD_LOCAL);
+	if (!cmap_hookers->cmap_handle)
+		return;
 
+	cmap_hookers->initialize = dlsym(cmap_hookers->cmap_handle, "cmap_initialize");
+	cmap_hookers->get_string = dlsym(cmap_hookers->cmap_handle, "cmap_get_string");
+	cmap_hookers->finalize = dlsym(cmap_hookers->cmap_handle, "cmap_finalize");
 
-        lib_handle = dlopen("libcmap.so.4", RTLD_NOW | RTLD_LOCAL);
-        if (!lib_handle)
-                return rv;
+	if (!cmap_hookers->initialize || !cmap_hookers->get_string ||
+	    !cmap_hookers->finalize)
+		dlclose(cmap_hookers->cmap_handle);
+	else
+		is_cmap_hookers_ready = 1;
+}
 
-        initialize = dlsym(lib_handle, "cmap_initialize");
-        if (!initialize)
-                goto out;
+void free_cmap_hookers(void)
+{
+	if (is_cmap_hookers_ready) {
+		dlclose(cmap_hookers->cmap_handle);
+		is_cmap_hookers_ready = 0;
+	}
+	if (cmap_hookers)
+		free(cmap_hookers);
+}
 
-        get_string = dlsym(lib_handle, "cmap_get_string");
-        if (!get_string)
-                goto out;
+int get_cluster_name(char **cluster_name)
+{
+        int rv = -1;
+	cmap_handle_t handle;
 
-        finalize = dlsym(lib_handle, "cmap_finalize");
-        if (!finalize)
-                goto out;
+	if (!is_cmap_hookers_ready)
+		return rv;
 
-        rv = initialize(&handle);
+        rv = cmap_hookers->initialize(&handle);
         if (rv != CS_OK)
                 goto out;
 
-        rv = get_string(handle, "totem.cluster_name", cluster_name);
+        rv = cmap_hookers->get_string(handle, "totem.cluster_name", cluster_name);
         if (rv != CS_OK) {
                 free(*cluster_name);
                 rv = -1;
@@ -2168,9 +2174,8 @@ int get_cluster_name(char **cluster_name)
 
         rv = 0;
 name_err:
-        finalize(handle);
+        cmap_hookers->finalize(handle);
 out:
-        dlclose(lib_handle);
         return rv;
 }
 
@@ -2208,3 +2213,15 @@ void free_dlm_hookers(void)
 	if (dlm_hookers)
 		free(dlm_hookers);
 }
+
+void set_hookers(void)
+{
+	set_dlm_hookers();
+	set_cmap_hookers();
+}
+
+void free_hookers(void)
+{
+	free_dlm_hookers();
+	free_cmap_hookers();
+}
-- 
1.7.12.4

Re: [PATCH 1/2] Safeguard against writing to an active device of another node

[Goldwyn Rodrigues]

On 08/03/2015 06:31 AM, Guoqing Jiang wrote:
> Modifying an exiting device's superblock or creating a new superblock
> on an existing device needs to be checked because the device could be
> in use by another node in another array. So, we check this by taking
> all superblock locks in userspace so that we don't  step onto an active
> device used by another node and safeguard against accidental edits.
> After the edit is complete, we release all locks and the lockspace so
> that it can be used by the kernel space.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
>   Makefile |   3 +-
>   mdadm.c  |  11 +++++
>   mdadm.h  |  46 ++++++++++++++++++
>   super1.c |  51 ++++++++++++++++++++
>   util.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 275 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index c189279..370ef75 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -81,11 +81,12 @@ FAILED_SLOTS_DIR = $(RUN_DIR)/failed-slots
>   SYSTEMD_DIR=/lib/systemd/system
>
>   COROSYNC:=$(shell [ -d /usr/include/corosync ] || echo -DNO_COROSYNC)
> +DLM:=$(shell [ -f /usr/include/libdlm.h ] || echo -DNO_DLM)
>
>   DIRFLAGS = -DMAP_DIR=\"$(MAP_DIR)\" -DMAP_FILE=\"$(MAP_FILE)\"
>   DIRFLAGS += -DMDMON_DIR=\"$(MDMON_DIR)\"
>   DIRFLAGS += -DFAILED_SLOTS_DIR=\"$(FAILED_SLOTS_DIR)\"
> -CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(DIRFLAGS) $(COROSYNC)
> +CFLAGS = $(CWFLAGS) $(CXFLAGS) -DSendmail=\""$(MAILCMD)"\" $(CONFFILEFLAGS) $(DIRFLAGS) $(COROSYNC) $(DLM)
>
>   VERSION = $(shell [ -d .git ] && git describe HEAD | sed 's/mdadm-//')
>   VERS_DATE = $(shell [ -d .git ] && date --date="`git log -n1 --format=format:%cd --date=short`" '+%0dth %B %Y' | sed -e 's/1th/1st/' -e 's/2th/2nd/' -e 's/11st/11th/' -e 's/12nd/12th/')
> diff --git a/mdadm.c b/mdadm.c
> index c4daf25..d8032c1 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1322,6 +1322,8 @@ int main(int argc, char *argv[])
>   		/* --scan implied --brief unless -vv */
>   		c.brief = 1;
>
> +	set_dlm_hookers(); /* get dlm funcs from libdlm_lt.so.3 */
> +

Universal Comment: Let call it set_dlm_hooks as opposed to hookers.


>   	rv = 0;
>   	switch(mode) {
>   	case MANAGE:
> @@ -1362,10 +1364,12 @@ int main(int argc, char *argv[])
>   		else if (devs_found > 0) {
>   			if (c.update && devs_found > 1) {
>   				pr_err("can only update a single array at a time\n");
> +				free_dlm_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			if (c.backup_file && devs_found > 1) {
>   				pr_err("can only assemble a single array when providing a backup file.\n");
> +				free_dlm_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			for (dv = devlist ; dv ; dv=dv->next) {
> @@ -1384,10 +1388,12 @@ int main(int argc, char *argv[])
>   		} else {
>   			if (c.update) {
>   				pr_err("--update not meaningful with a --scan assembly.\n");
> +				free_dlm_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			if (c.backup_file) {
>   				pr_err("--backup_file not meaningful with a --scan assembly.\n");
> +				free_dlm_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			rv = scan_assemble(ss, &c, &ident);
> @@ -1455,12 +1461,14 @@ int main(int argc, char *argv[])
>   		if (devmode == 'E') {
>   			if (devlist == NULL && !c.scan) {
>   				pr_err("No devices to examine\n");
> +				free_dlm_hookers(); /* close dlm stuffs */
>   				exit(2);
>   			}
>   			if (devlist == NULL)
>   				devlist = conf_get_devs();
>   			if (devlist == NULL) {
>   				pr_err("No devices listed in %s\n", configfile?configfile:DefaultConfFile);
> +				free_dlm_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			rv = Examine(devlist, &c, ss);
> @@ -1477,6 +1485,7 @@ int main(int argc, char *argv[])
>   				rv = Write_rules(udev_filename);
>   			else {
>   				pr_err("No devices given.\n");
> +				free_dlm_hookers(); /* close dlm stuffs */
>   				exit(2);
>   			}
>   		} else
> @@ -1615,6 +1624,8 @@ int main(int argc, char *argv[])
>   		autodetect();
>   		break;
>   	}
> +
> +	free_dlm_hookers(); /* close dlm stuffs */
>   	exit(rv);
>   }
>
> diff --git a/mdadm.h b/mdadm.h
> index 97892e6..c53adc5 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -35,6 +35,7 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
>
>   #include	<sys/types.h>
>   #include	<sys/stat.h>
> +#include	<stdint.h>
>   #include	<stdlib.h>
>   #include	<time.h>
>   #include	<sys/time.h>
> @@ -51,6 +52,25 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
>   #define srandom srand
>   #endif
>
> +#ifndef NO_DLM
> +#include	<libdlm.h>
> +#include	<errno.h>
> +#else
> +#define LKF_NOQUEUE	0x00000001
> +#define LKF_CONVERT	0x00000004
> +#define LKM_PWMODE	4
> +#define EUNLOCK		0x10002
> +
> +typedef void *dlm_lshandle_t;
> +
> +struct dlm_lksb {
> +	int sb_status;
> +	uint32_t sb_lkid;
> +	char sb_flags;
> +	char *sb_lvbptr;
> +};
> +#endif
> +
>   #include	<linux/kdev_t.h>
>   /*#include	<linux/fs.h> */
>   #include	<sys/mount.h>
> @@ -1428,7 +1448,33 @@ extern char *stat2devnm(struct stat *st);
>   extern char *fd2devnm(int fd);
>
>   extern int in_initrd(void);
> +
> +struct dlm_hookers {
> +	void *dlm_handle;	/* dlm lib related */
> +
> +	dlm_lshandle_t (*create_lockspace)(const char *name,
> +					   unsigned int mode);
> +	int (*release_lockspace)(const char *name, dlm_lshandle_t ls,
> +				 int force);
> +	int (*ls_lock)(dlm_lshandle_t lockspace, uint32_t mode,
> +		       struct dlm_lksb *lksb, uint32_t flags,
> +		       const void *name, unsigned int namelen,
> +		       uint32_t parent, void (*astaddr) (void *astarg),
> +		       void *astarg, void (*bastaddr) (void *astarg),
> +		       void *range);
> +	int (*ls_unlock)(dlm_lshandle_t lockspace, uint32_t lkid,
> +			 uint32_t flags, struct dlm_lksb *lksb,
> +			 void *astarg);
> +	int (*ls_get_fd)(dlm_lshandle_t ls);
> +	int (*dispatch)(int fd);
> +};
> +
>   extern int get_cluster_name(char **name);
> +extern int is_clustered(struct supertype *st);
> +extern int cluster_get_dlmlock(struct supertype *st, int *lockid);
> +extern int cluster_release_dlmlock(struct supertype *st, int lockid);
> +extern void set_dlm_hookers(void);
> +extern void free_dlm_hookers(void);
>
>   #define _ROUND_UP(val, base)	(((val) + (base) - 1) & ~(base - 1))
>   #define ROUND_UP(val, base)	_ROUND_UP(val, (typeof(val))(base))
> diff --git a/super1.c b/super1.c
> index fda71e3..bd88c36 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1072,8 +1072,18 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>   	 * ignored.
>   	 */
>   	int rv = 0;
> +	int lockid;
>   	struct mdp_superblock_1 *sb = st->sb;
>
> +	if (is_clustered(st)) {
> +		rv = cluster_get_dlmlock(st, &lockid);
> +		if (rv) {
> +			pr_err("Cannot get dlmlock in %s return %d\n", __func__, rv);
> +			cluster_release_dlmlock(st, lockid);
> +			return rv;
> +		}
> +	}
> +
>   	if (strcmp(update, "homehost") == 0 &&
>   	    homehost) {
>   		/* Note that 'homehost' is special as it is really
> @@ -1330,6 +1340,9 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>   		rv = -1;
>
>   	sb->sb_csum = calc_sb_1_csum(sb);
> +	if (is_clustered(st))
> +		cluster_release_dlmlock(st, lockid);
> +
>   	return rv;
>   }
>
> @@ -1433,6 +1446,16 @@ static int add_to_super1(struct supertype *st, mdu_disk_info_t *dk,
>   	struct mdp_superblock_1 *sb = st->sb;
>   	__u16 *rp = sb->dev_roles + dk->number;
>   	struct devinfo *di, **dip;
> +	int rv, lockid;
> +
> +	if (is_clustered(st)) {
> +		rv = cluster_get_dlmlock(st, &lockid);
> +		if (rv) {
> +			pr_err("Cannot get dlmlock in %s return %d\n", __func__, rv);
> +			cluster_release_dlmlock(st, lockid);
> +			return rv;
> +		}
> +	}
>
>   	if ((dk->state & 6) == 6) /* active, sync */
>   		*rp = __cpu_to_le16(dk->raid_disk);
> @@ -1460,6 +1483,9 @@ static int add_to_super1(struct supertype *st, mdu_disk_info_t *dk,
>   	di->next = NULL;
>   	*dip = di;
>
> +	if (is_clustered(st))
> +		cluster_release_dlmlock(st, lockid);
> +
>   	return 0;
>   }
>   #endif
> @@ -1473,6 +1499,16 @@ static int store_super1(struct supertype *st, int fd)
>   	struct align_fd afd;
>   	int sbsize;
>   	unsigned long long dsize;
> +	int rv, lockid;
> +
> +	if (is_clustered(st)) {
> +		rv = cluster_get_dlmlock(st, &lockid);
> +		if (rv) {
> +			pr_err("Cannot get dlmlock in %s return %d\n", __func__, rv);
> +			cluster_release_dlmlock(st, lockid);
> +			return rv;
> +		}
> +	}
>
>   	if (!get_dev_size(fd, NULL, &dsize))
>   		return 1;
> @@ -1533,6 +1569,9 @@ static int store_super1(struct supertype *st, int fd)
>   		}
>   	}
>   	fsync(fd);
> +	if (is_clustered(st))
> +		cluster_release_dlmlock(st, lockid);
> +
>   	return 0;
>   }
>
> @@ -2282,6 +2321,16 @@ static int write_bitmap1(struct supertype *st, int fd, enum bitmap_update update
>
>   static void free_super1(struct supertype *st)
>   {
> +	int rv, lockid;
> +	if (is_clustered(st)) {
> +		rv = cluster_get_dlmlock(st, &lockid);
> +		if (rv) {
> +			pr_err("Cannot get dlmlock in %s return %d\n", __func__, rv);
> +			cluster_release_dlmlock(st, lockid);
> +			return;
> +		}
> +	}
> +
>   	if (st->sb)
>   		free(st->sb);
>   	while (st->info) {
> @@ -2292,6 +2341,8 @@ static void free_super1(struct supertype *st)
>   		free(di);
>   	}
>   	st->sb = NULL;
> +	if (is_clustered(st))
> +		cluster_release_dlmlock(st, lockid);
>   }
>
>   #ifndef MDASSEMBLE
> diff --git a/util.c b/util.c
> index ea6e688..19ecf9f 100644
> --- a/util.c
> +++ b/util.c
> @@ -24,6 +24,7 @@
>
>   #include	"mdadm.h"
>   #include	"md_p.h"
> +#include	<sys/poll.h>
>   #include	<sys/socket.h>
>   #include	<sys/utsname.h>
>   #include	<sys/wait.h>
> @@ -88,6 +89,135 @@ struct blkpg_partition {
>      aren't permitted). */
>   #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>
> +static struct dlm_hookers *dlm_hookers = NULL;
> +static int is_dlm_hookers_ready = 0;

This should not be required, just checking for dlm_hooks == NULL should 
be enough. This needs to be set accordingly in set_dlm_hooks.

> +static struct dlm_lock_resource *dlm_lock_res = NULL;
> +static int ast_called = 0;
> +
> +struct dlm_lock_resource {
> +	dlm_lshandle_t *ls;
> +	struct dlm_lksb lksb;
> +};
> +
> +int is_clustered(struct supertype *st)
> +{
> +	/* is it a cluster md or not */
> +	if (is_dlm_hookers_ready && st->cluster_name)
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +/* Using poll(2) to wait for and dispatch ASTs */
> +static int poll_for_ast(dlm_lshandle_t ls)
> +{
> +	struct pollfd pfd;

Shouldn't you check dlm_hooks is NULL here? and starting of every 
function which requires dlm_hooks.

Also, a return value from these functions do not mean an error, it means 
the library is not present.


> +
> +	pfd.fd = dlm_hookers->ls_get_fd(ls);
> +	pfd.events = POLLIN;
> +
> +	while (!ast_called)
> +	{
> +		if (poll(&pfd, 1, 0) < 0)
> +		{
> +			perror("poll");
> +			return -1;
> +		}
> +		dlm_hookers->dispatch(dlm_hookers->ls_get_fd(ls));
> +	}
> +	ast_called = 0;
> +
> +	return 0;
> +}
> +
> +static void dlm_ast(void *arg)
> +{
> +	ast_called = 1;
> +}
> +
> +/* Create the lockspace, take bitmapXXX locks on all the bitmaps. */
> +int cluster_get_dlmlock(struct supertype *st, int *lockid)
> +{
> +	int ret = -1;
> +	char str[64];
> +	int flags = LKF_NOQUEUE;
> +
> +	dlm_lock_res = xmalloc(sizeof(struct dlm_lock_resource));
> +	if (!dlm_lock_res)
> +                goto out;
> +
> +	dlm_lock_res->ls = dlm_hookers->create_lockspace(st->cluster_name, O_RDWR);
> +	if (!dlm_lock_res->ls) {
> +		pr_err("%s failed to create lockspace\n", st->cluster_name);
> +                goto out;
> +	}
> +
> +	/* Conversions need the lockid in the LKSB */
> +	if (flags & LKF_CONVERT)
> +		dlm_lock_res->lksb.sb_lkid = *lockid;
> +
> +	snprintf(str, 64, "bitmap%04d", st->nodes);
> +	/* if flags with LKF_CONVERT causes below return ENOENT which means
> +	 * "No such file or directory" */
> +	ret = dlm_hookers->ls_lock(dlm_lock_res->ls, LKM_PWMODE, &dlm_lock_res->lksb,
> +			  flags, str, strlen(str), 0, dlm_ast,
> +			  dlm_lock_res, NULL, NULL);
> +	if (ret) {
> +		pr_err("error %d when get PW mode on lock %s\n", errno, str);
> +                goto out;
> +	}
> +
> +	/* Wait for it to complete */
> +	poll_for_ast(dlm_lock_res->ls);
> +	*lockid = dlm_lock_res->lksb.sb_lkid;
> +
> +	errno =	dlm_lock_res->lksb.sb_status;
> +	if (errno) {
> +		pr_err("error %d happened in ast with lock %s\n", errno, str);
> +		goto out;
> +	}
> +
> +out:
> +	return ret;
> +}
> +
> +int cluster_release_dlmlock(struct supertype *st, int lockid)
> +{
> +	int ret = -1;
> +
> +	/* if flags with LKF_CONVERT causes below return EINVAL which means
> +	 * "Invalid argument" */
> +	ret = dlm_hookers->ls_unlock(dlm_lock_res->ls, lockid, 0,
> +				     &dlm_lock_res->lksb, dlm_lock_res);
> +	if (ret) {
> +		pr_err("error %d happened when unlock\n", errno);
> +		/* XXX make sure the lock is unlocked eventually */
> +                goto out;
> +	}
> +
> +	/* Wait for it to complete */
> +	poll_for_ast(dlm_lock_res->ls);
> +
> +	errno =	dlm_lock_res->lksb.sb_status;
> +	if (errno != EUNLOCK) {
> +		pr_err("error %d happened in ast when unlock lockspace\n", errno);
> +		/* XXX make sure the lockspace is unlocked eventually */
> +                goto out;
> +	}
> +
> +	ret = dlm_hookers->release_lockspace(st->cluster_name, dlm_lock_res->ls, 1);
> +	if (ret) {
> +		pr_err("error %d happened when release lockspace\n", errno);
> +		/* XXX make sure the lockspace is released eventually */
> +                goto out;
> +	}
> +	free(dlm_lock_res);
> +
> +out:
> +	return ret;
> +}
> +
> +
>   /*
>    * Parse a 128 bit uuid in 4 integers
>    * format is 32 hexx nibbles with options :.<space> separator
> @@ -2043,3 +2173,38 @@ out:
>           dlclose(lib_handle);
>           return rv;
>   }
> +
> +void set_dlm_hookers(void)
> +{
> +	dlm_hookers = xmalloc(sizeof(struct dlm_hookers));
> +	if (!dlm_hookers)
> +		return;
> +
> +	dlm_hookers->dlm_handle = dlopen("libdlm_lt.so.3", RTLD_NOW | RTLD_LOCAL);
> +	if (!dlm_hookers->dlm_handle)
> +		return;
> +
> +	dlm_hookers->create_lockspace = dlsym(dlm_hookers->dlm_handle, "dlm_create_lockspace");
> +	dlm_hookers->release_lockspace = dlsym(dlm_hookers->dlm_handle, "dlm_release_lockspace");
> +	dlm_hookers->ls_lock = dlsym(dlm_hookers->dlm_handle, "dlm_ls_lock");
> +	dlm_hookers->ls_unlock = dlsym(dlm_hookers->dlm_handle, "dlm_ls_unlock");
> +	dlm_hookers->ls_get_fd = dlsym(dlm_hookers->dlm_handle, "dlm_ls_get_fd");
> +	dlm_hookers->dispatch = dlsym(dlm_hookers->dlm_handle, "dlm_dispatch");
> +
> +	if (!dlm_hookers->create_lockspace || !dlm_hookers->ls_lock ||
> +	    !dlm_hookers->ls_unlock || !dlm_hookers->release_lockspace ||
> +	    !dlm_hookers->ls_get_fd || !dlm_hookers->dispatch)
> +		dlclose(dlm_hookers->dlm_handle);
> +	else
> +		is_dlm_hookers_ready = 1;
> +}
> +
> +void free_dlm_hookers(void)
> +{
> +	if (is_dlm_hookers_ready) {
> +		dlclose(dlm_hookers->dlm_handle);
> +		is_dlm_hookers_ready = 0;
> +	}
> +	if (dlm_hookers)
> +		free(dlm_hookers);
> +}
>

-- 
Goldwyn

Re: [PATCH 2/2] Make cmap_* also has same policy as dlm_*

[Goldwyn Rodrigues]

On 08/03/2015 06:31 AM, Guoqing Jiang wrote:
> Let libcmap lib and related funs also only need one-time
> setup during mdadm running period.
>
> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> ---
>   mdadm.c | 23 +++++++++++--------
>   mdadm.h | 20 ++++++++++++++++
>   util.c  | 81 +++++++++++++++++++++++++++++++++++++++--------------------------
>   3 files changed, 82 insertions(+), 42 deletions(-)
>
> diff --git a/mdadm.c b/mdadm.c
> index d8032c1..4812fc5 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1293,18 +1293,22 @@ int main(int argc, char *argv[])
>   		c.require_homehost = 0;
>   	}
>
> +	set_hookers(); /* set hookers from libs */
> +
>   	if (c.homecluster == NULL && (c.nodes > 0)) {
>   		c.homecluster = conf_get_homecluster();
>   		if (c.homecluster == NULL)
>   			rv = get_cluster_name(&c.homecluster);
>   		if (rv != 0) {
>   			pr_err("The md can't get cluster name\n");
> +			free_hookers();
>   			exit(1);
>   		}
>   	}
>
>   	if (c.backup_file && data_offset != INVALID_SECTORS) {
>   		pr_err("--backup-file and --data-offset are incompatible\n");
> +		free_hookers();
>   		exit(2);
>   	}
>
> @@ -1313,6 +1317,7 @@ int main(int argc, char *argv[])
>   		/* Anyone may try this */;
>   	else if (geteuid() != 0) {
>   		pr_err("must be super-user to perform this action\n");
> +		free_hookers();
>   		exit(1);
>   	}
>
> @@ -1322,8 +1327,6 @@ int main(int argc, char *argv[])
>   		/* --scan implied --brief unless -vv */
>   		c.brief = 1;
>
> -	set_dlm_hookers(); /* get dlm funcs from libdlm_lt.so.3 */
> -
>   	rv = 0;
>   	switch(mode) {
>   	case MANAGE:
> @@ -1364,12 +1367,12 @@ int main(int argc, char *argv[])
>   		else if (devs_found > 0) {
>   			if (c.update && devs_found > 1) {
>   				pr_err("can only update a single array at a time\n");
> -				free_dlm_hookers(); /* close dlm stuffs */
> +				free_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			if (c.backup_file && devs_found > 1) {
>   				pr_err("can only assemble a single array when providing a backup file.\n");
> -				free_dlm_hookers(); /* close dlm stuffs */
> +				free_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			for (dv = devlist ; dv ; dv=dv->next) {
> @@ -1388,12 +1391,12 @@ int main(int argc, char *argv[])
>   		} else {
>   			if (c.update) {
>   				pr_err("--update not meaningful with a --scan assembly.\n");
> -				free_dlm_hookers(); /* close dlm stuffs */
> +				free_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			if (c.backup_file) {
>   				pr_err("--backup_file not meaningful with a --scan assembly.\n");
> -				free_dlm_hookers(); /* close dlm stuffs */
> +				free_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			rv = scan_assemble(ss, &c, &ident);
> @@ -1461,14 +1464,14 @@ int main(int argc, char *argv[])
>   		if (devmode == 'E') {
>   			if (devlist == NULL && !c.scan) {
>   				pr_err("No devices to examine\n");
> -				free_dlm_hookers(); /* close dlm stuffs */
> +				free_hookers(); /* close dlm stuffs */
>   				exit(2);
>   			}
>   			if (devlist == NULL)
>   				devlist = conf_get_devs();
>   			if (devlist == NULL) {
>   				pr_err("No devices listed in %s\n", configfile?configfile:DefaultConfFile);
> -				free_dlm_hookers(); /* close dlm stuffs */
> +				free_hookers(); /* close dlm stuffs */
>   				exit(1);
>   			}
>   			rv = Examine(devlist, &c, ss);
> @@ -1485,7 +1488,7 @@ int main(int argc, char *argv[])
>   				rv = Write_rules(udev_filename);
>   			else {
>   				pr_err("No devices given.\n");
> -				free_dlm_hookers(); /* close dlm stuffs */
> +				free_hookers(); /* close dlm stuffs */
>   				exit(2);
>   			}
>   		} else
> @@ -1625,7 +1628,7 @@ int main(int argc, char *argv[])
>   		break;
>   	}
>
> -	free_dlm_hookers(); /* close dlm stuffs */
> +	free_hookers(); /* close dlm stuffs */
>   	exit(rv);
>   }
>
> diff --git a/mdadm.h b/mdadm.h
> index c53adc5..dd7fef4 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -52,6 +52,12 @@ extern __off64_t lseek64 __P ((int __fd, __off64_t __offset, int __whence));
>   #define srandom srand
>   #endif
>
> +#ifdef NO_COROSYNC
> +#define CS_OK 1
> +#else
> +#include	<corosync/cmap.h>
> +#endif
> +
>   #ifndef NO_DLM
>   #include	<libdlm.h>
>   #include	<errno.h>
> @@ -1449,6 +1455,16 @@ extern char *fd2devnm(int fd);
>
>   extern int in_initrd(void);
>
> +struct cmap_hookers {
> +	void *cmap_handle;	/* corosync lib related */
> +
> +	int (*initialize)(cmap_handle_t *handle);
> +	int (*get_string)(cmap_handle_t handle,
> +			  const char *string,
> +			  char **name);
> +	int (*finalize)(cmap_handle_t handle);
> +};
> +
>   struct dlm_hookers {
>   	void *dlm_handle;	/* dlm lib related */
>
> @@ -1475,6 +1491,10 @@ extern int cluster_get_dlmlock(struct supertype *st, int *lockid);
>   extern int cluster_release_dlmlock(struct supertype *st, int lockid);
>   extern void set_dlm_hookers(void);
>   extern void free_dlm_hookers(void);
> +extern void set_cmap_hookers(void);
> +extern void free_cmap_hookers(void);
> +extern void set_hookers(void);
> +extern void free_hookers(void);
>
>   #define _ROUND_UP(val, base)	(((val) + (base) - 1) & ~(base - 1))
>   #define ROUND_UP(val, base)	_ROUND_UP(val, (typeof(val))(base))
> diff --git a/util.c b/util.c
> index 19ecf9f..1c87eb5 100644
> --- a/util.c
> +++ b/util.c
> @@ -36,13 +36,6 @@
>   #include	<dirent.h>
>   #include	<signal.h>
>   #include	<dlfcn.h>
> -#include	<stdint.h>
> -#ifdef NO_COROSYNC
> - typedef uint64_t cmap_handle_t;
> - #define CS_OK 1
> -#else
> - #include	<corosync/cmap.h>
> -#endif
>
>
>   /*
> @@ -2126,40 +2119,53 @@ void reopen_mddev(int mdfd)
>   		dup2(fd, mdfd);
>   }
>
> -int get_cluster_name(char **cluster_name)
> +static struct cmap_hookers *cmap_hookers = NULL;
> +static int is_cmap_hookers_ready = 0;
> +
> +void set_cmap_hookers(void)
>   {
> -        void *lib_handle = NULL;
> -        int rv = -1;
> +	cmap_hookers = xmalloc(sizeof(struct cmap_hookers));
> +	if (!cmap_hookers)
> +		return;
>
> -        cmap_handle_t handle;
> -        static int (*initialize)(cmap_handle_t *handle);
> -        static int (*get_string)(cmap_handle_t handle,
> -				 const char *string,
> -				 char **name);
> -        static int (*finalize)(cmap_handle_t handle);
> +	cmap_hookers->cmap_handle = dlopen("libcmap.so.4", RTLD_NOW | RTLD_LOCAL);
> +	if (!cmap_hookers->cmap_handle)
> +		return;
>
> +	cmap_hookers->initialize = dlsym(cmap_hookers->cmap_handle, "cmap_initialize");
> +	cmap_hookers->get_string = dlsym(cmap_hookers->cmap_handle, "cmap_get_string");
> +	cmap_hookers->finalize = dlsym(cmap_hookers->cmap_handle, "cmap_finalize");
>
> -        lib_handle = dlopen("libcmap.so.4", RTLD_NOW | RTLD_LOCAL);
> -        if (!lib_handle)
> -                return rv;
> +	if (!cmap_hookers->initialize || !cmap_hookers->get_string ||
> +	    !cmap_hookers->finalize)
> +		dlclose(cmap_hookers->cmap_handle);
> +	else
> +		is_cmap_hookers_ready = 1;
> +}
>
> -        initialize = dlsym(lib_handle, "cmap_initialize");
> -        if (!initialize)
> -                goto out;
> +void free_cmap_hookers(void)
> +{
> +	if (is_cmap_hookers_ready) {
> +		dlclose(cmap_hookers->cmap_handle);
> +		is_cmap_hookers_ready = 0;
> +	}
> +	if (cmap_hookers)
> +		free(cmap_hookers);
> +}
>
> -        get_string = dlsym(lib_handle, "cmap_get_string");
> -        if (!get_string)
> -                goto out;
> +int get_cluster_name(char **cluster_name)
> +{
> +        int rv = -1;
> +	cmap_handle_t handle;
>
> -        finalize = dlsym(lib_handle, "cmap_finalize");
> -        if (!finalize)
> -                goto out;
> +	if (!is_cmap_hookers_ready)
> +		return rv;
>
> -        rv = initialize(&handle);
> +        rv = cmap_hookers->initialize(&handle);
>           if (rv != CS_OK)
>                   goto out;
>
> -        rv = get_string(handle, "totem.cluster_name", cluster_name);
> +        rv = cmap_hookers->get_string(handle, "totem.cluster_name", cluster_name);
>           if (rv != CS_OK) {
>                   free(*cluster_name);
>                   rv = -1;
> @@ -2168,9 +2174,8 @@ int get_cluster_name(char **cluster_name)
>
>           rv = 0;
>   name_err:
> -        finalize(handle);
> +        cmap_hookers->finalize(handle);
>   out:
> -        dlclose(lib_handle);
>           return rv;
>   }
>
> @@ -2208,3 +2213,15 @@ void free_dlm_hookers(void)
>   	if (dlm_hookers)
>   		free(dlm_hookers);
>   }
> +
> +void set_hookers(void)
> +{
> +	set_dlm_hookers();
> +	set_cmap_hookers();
> +}
> +
> +void free_hookers(void)
> +{
> +	free_dlm_hookers();
> +	free_cmap_hookers();
> +}
>

You could put both functions in a single structure called cluster_hooks.

-- 
Goldwyn

Re: [PATCH 1/2] Safeguard against writing to an active device of another node

[Guoqing Jiang]

Hi Goldwyn,

Thanks for review.

Goldwyn Rodrigues wrote:
>>
>> +    set_dlm_hookers(); /* get dlm funcs from libdlm_lt.so.3 */
>> +
>
> Universal Comment: Let call it set_dlm_hooks as opposed to hookers.
>
Not sure I understood correctly, the second patch used set_hookers to
call set_dlm_hookers.
>>
>>   #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>>
>> +static struct dlm_hookers *dlm_hookers = NULL;
>> +static int is_dlm_hookers_ready = 0;
>
> This should not be required, just checking for dlm_hooks == NULL
> should be enough. This needs to be set accordingly in set_dlm_hooks.
>
is_dlm_hookers_ready is introduced to check the dlm_* functions is
appeared in libdlm_lt.so.3
or not, in case there is problem within the dlm lib.

>> +static struct dlm_lock_resource *dlm_lock_res = NULL;
>> +static int ast_called = 0;
>> +
>> +struct dlm_lock_resource {
>> +    dlm_lshandle_t *ls;
>> +    struct dlm_lksb lksb;
>> +};
>> +
>> +int is_clustered(struct supertype *st)
>> +{
>> +    /* is it a cluster md or not */
>> +    if (is_dlm_hookers_ready && st->cluster_name)
>> +        return 1;
>> +    else
>> +        return 0;
>> +}
>> +
>> +/* Using poll(2) to wait for and dispatch ASTs */
>> +static int poll_for_ast(dlm_lshandle_t ls)
>> +{
>> +    struct pollfd pfd;
>
> Shouldn't you check dlm_hooks is NULL here? and starting of every
> function which requires dlm_hooks.
>
> Also, a return value from these functions do not mean an error, it
> means the library is not present.
>
I don't think so, because is_dlm_hookers_ready not only ensures 
libdlm_lt.so.3 existed and  it also make sure
all the needed dlm hookers are set. And
cluster_get_dlmlock/cluster_release_dlmlock/dlm_ast/poll_for_ast
could only be execute while is_dlm_hookers_ready is set to 1.

Thanks,
Guoqing

Re: [PATCH 2/2] Make cmap_* also has same policy as dlm_*

[Guoqing Jiang]

Hi Goldwyn,

Goldwyn Rodrigues wrote:
> On 08/03/2015 06:31 AM, Guoqing Jiang wrote:
>> Let libcmap lib and related funs also only need one-time
>> setup during mdadm running period.
>>
>> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
>> ---
>>   mdadm.c | 23 +++++++++++--------
>>   mdadm.h | 20 ++++++++++++++++
>>   util.c  | 81
>> +++++++++++++++++++++++++++++++++++++++--------------------------
>>   3 files changed, 82 insertions(+), 42 deletions(-)
>>
[snip]
>> +
>> +void set_hookers(void)
>> +{
>> +    set_dlm_hookers();
>> +    set_cmap_hookers();
>> +}
>> +
>> +void free_hookers(void)
>> +{
>> +    free_dlm_hookers();
>> +    free_cmap_hookers();
>> +}
>>
>
> You could put both functions in a single structure called cluster_hooks.
>
Yes, it could be. If there more libs are needed in future, so I prefer
separate them according to different libraries.

Thanks,
Guoqing

Re: [PATCH 1/2] Safeguard against writing to an active device of another node

[Adam Goryachev]

On 04/08/15 20:05, Guoqing Jiang wrote:
> Hi Goldwyn,
>
> Thanks for review.
>
> Goldwyn Rodrigues wrote:
>>> +    set_dlm_hookers(); /* get dlm funcs from libdlm_lt.so.3 */
>>> +
>> Universal Comment: Let call it set_dlm_hooks as opposed to hookers.
>>
> Not sure I understood correctly, the second patch used set_hookers to
> call set_dlm_hookers.

I think Neil meant to ask you to please do a global s/hookers/hooks/
because hooker has a particular meaning in english which is different to
what you intended.
http://www.urbandictionary.com/define.php?term=hooker

Hope that helps.

Regards,
Adam

>>>   #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>>>
>>> +static struct dlm_hookers *dlm_hookers = NULL;
>>> +static int is_dlm_hookers_ready = 0;
>> This should not be required, just checking for dlm_hooks == NULL
>> should be enough. This needs to be set accordingly in set_dlm_hooks.
>>
> is_dlm_hookers_ready is introduced to check the dlm_* functions is
> appeared in libdlm_lt.so.3
> or not, in case there is problem within the dlm lib.
>
>>> +static struct dlm_lock_resource *dlm_lock_res = NULL;
>>> +static int ast_called = 0;
>>> +
>>> +struct dlm_lock_resource {
>>> +    dlm_lshandle_t *ls;
>>> +    struct dlm_lksb lksb;
>>> +};
>>> +
>>> +int is_clustered(struct supertype *st)
>>> +{
>>> +    /* is it a cluster md or not */
>>> +    if (is_dlm_hookers_ready && st->cluster_name)
>>> +        return 1;
>>> +    else
>>> +        return 0;
>>> +}
>>> +
>>> +/* Using poll(2) to wait for and dispatch ASTs */
>>> +static int poll_for_ast(dlm_lshandle_t ls)
>>> +{
>>> +    struct pollfd pfd;
>> Shouldn't you check dlm_hooks is NULL here? and starting of every
>> function which requires dlm_hooks.
>>
>> Also, a return value from these functions do not mean an error, it
>> means the library is not present.
>>
> I don't think so, because is_dlm_hookers_ready not only ensures 
> libdlm_lt.so.3 existed and  it also make sure
> all the needed dlm hookers are set. And
> cluster_get_dlmlock/cluster_release_dlmlock/dlm_ast/poll_for_ast
> could only be execute while is_dlm_hookers_ready is set to 1.
>
> Thanks,
> Guoqing
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Adam Goryachev
Website Managers
www.websitemanagers.com.au

Re: [PATCH 1/2] Safeguard against writing to an active device of another node

[Guoqing Jiang]

Adam Goryachev wrote:
> On 04/08/15 20:05, Guoqing Jiang wrote:
>   
>> Hi Goldwyn,
>>
>> Thanks for review.
>>
>> Goldwyn Rodrigues wrote:
>>     
>>>> +    set_dlm_hookers(); /* get dlm funcs from libdlm_lt.so.3 */
>>>> +
>>>>         
>>> Universal Comment: Let call it set_dlm_hooks as opposed to hookers.
>>>
>>>       
>> Not sure I understood correctly, the second patch used set_hookers to
>> call set_dlm_hookers.
>>     
>
> I think Neil meant to ask you to please do a global s/hookers/hooks/
> because hooker has a particular meaning in english which is different to
> what you intended.
> http://www.urbandictionary.com/define.php?term=hooker
>
> Hope that helps.
>   
Got it, thanks for the website, will change it.

Thanks,
Guoqing