PATCH 0/4] multipath-tools: Ceph rbd support v2

PATCH 0/4] multipath-tools: Ceph rbd support v2

[Mike Christie]

The following patches made over Christophe's tree today,
add Ceph rbd support for handling blacklisted devices.


This is not general support for rbd and multipath. There is
no boot/root support and it does not support features like
multibus. My use is for HA configurations, specifically for exporting
rbd images through multiple LIO instances. In this case, we have one
rbd instance that has the ceph rbd exclusive lock and it can send
WRITE requests. If that host becomes unreachable, then another host
will grab the lock, and blacklist the original host to prevent it from
sending stale IO (when blacklisted IO will be failed by the OSD).

To recover from that type of scenario, this patchset adds a repair()
callout to the checker. If the path is in the PATH_DOWN state this
callout can be used to fix it up. For my case, I am remapping
the device to flush stale IO and cleanup the old lock,
and then unblacklisting the path, so it can be used again.

Changes since v1:
- Drop ID_UID use and implemented sysfs getuid support.
- Drop settings that were defaults and follow template.
- Fix ceph auth/user.

[PATCH 1/4] libmultipath: add rbd discovery

[Mike Christie]

rbd is a block device interface for Ceph. It does not support
any SCSI commands, so this patch adds bus detection and virtual
vendor/product pathinfo.

Changes since v1:
1. Drop ID_UID use and implemented sysfs getuid support.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 libmultipath/checkers.h  |  1 +
 libmultipath/discovery.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/structs.h   |  1 +
 3 files changed, 72 insertions(+)

diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index ac382d7..8fc8616 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -84,6 +84,7 @@ enum path_check_state {
 #define EMC_CLARIION "emc_clariion"
 #define READSECTOR0  "readsector0"
 #define CCISS_TUR    "cciss_tur"
+#define RBD          "rbd"
 
 #define DEFAULT_CHECKER TUR
 
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 1fb4db4..61b389f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1170,6 +1170,21 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable)
 }
 
 static int
+rbd_sysfs_pathinfo (struct path * pp, vector hwtable)
+{
+	sprintf(pp->vendor_id, "Ceph");
+	sprintf(pp->product_id, "RBD");
+
+	condlog(3, "%s: vendor = %s product = %s", pp->dev, pp->vendor_id,
+		pp->product_id);
+	/*
+	 * set the hwe configlet pointer
+	 */
+	pp->hwe = find_hwe(hwtable, pp->vendor_id, pp->product_id, NULL);
+	return 0;
+}
+
+static int
 ccw_sysfs_pathinfo (struct path * pp, vector hwtable)
 {
 	struct udev_device *parent;
@@ -1371,6 +1386,8 @@ sysfs_pathinfo(struct path * pp, vector hwtable)
 		pp->bus = SYSFS_BUS_CCW;
 	if (!strncmp(pp->dev,"sd", 2))
 		pp->bus = SYSFS_BUS_SCSI;
+	if (!strncmp(pp->dev,"rbd", 3))
+		pp->bus = SYSFS_BUS_RBD;
 
 	if (pp->bus == SYSFS_BUS_UNDEF)
 		return 0;
@@ -1383,6 +1400,9 @@ sysfs_pathinfo(struct path * pp, vector hwtable)
 	} else if (pp->bus == SYSFS_BUS_CCISS) {
 		if (cciss_sysfs_pathinfo(pp, hwtable))
 			return 1;
+	} else if (pp->bus == SYSFS_BUS_RBD) {
+		if (rbd_sysfs_pathinfo(pp, hwtable))
+			return 1;
 	}
 	return 0;
 }
@@ -1540,6 +1560,53 @@ get_udev_uid(struct path * pp, char *uid_attribute)
 }
 
 static int
+get_rbd_uid(struct path * pp)
+{
+	struct udev_device *rbd_bus_dev;
+	int ret, rbd_bus_id;
+	const char *pool, *image, *snap;
+	char sysfs_path[PATH_SIZE];
+	uint64_t snap_id, max_snap_id = -3;
+
+	ret = sscanf(pp->dev, "rbd%d", &rbd_bus_id);
+	if (ret != 1)
+		return -EINVAL;
+
+	snprintf(sysfs_path, sizeof(sysfs_path), "/sys/bus/rbd/devices/%d",
+		 rbd_bus_id);
+	rbd_bus_dev = udev_device_new_from_syspath(udev, sysfs_path);
+	if (!rbd_bus_dev)
+		return -ENODEV;
+
+	ret = -EINVAL;
+	pool = udev_device_get_sysattr_value(rbd_bus_dev, "pool_id");
+	if (!pool)
+		goto free_dev;
+
+	image = udev_device_get_sysattr_value(rbd_bus_dev, "image_id");
+	if (!image)
+		goto free_dev;
+
+	snap = udev_device_get_sysattr_value(rbd_bus_dev, "snap_id");
+	if (!snap)
+		goto free_dev;
+	snap_id = strtoull(snap, NULL, 19);
+	if (snap_id >= max_snap_id)
+		ret = snprintf(pp->wwid, WWID_SIZE, "%s-%s", pool, image);
+	else
+		ret = snprintf(pp->wwid, WWID_SIZE, "%s-%s-%s", pool,
+			       image, snap);
+	if (ret >= WWID_SIZE) {
+		condlog(0, "%s: wwid overflow", pp->dev);
+		ret = -EOVERFLOW;
+	}
+
+free_dev:
+	udev_device_unref(rbd_bus_dev);
+	return ret;
+}
+
+static int
 get_vpd_uid(struct path * pp)
 {
 	struct udev_device *parent = pp->udev;
@@ -1591,6 +1658,9 @@ get_uid (struct path * pp, int path_state)
 		} else
 			len = strlen(pp->wwid);
 		origin = "callout";
+	} else if (pp->bus == SYSFS_BUS_RBD) {
+		len = get_rbd_uid(pp);
+		origin = "sysfs";
 	} else {
 		int retrigger;
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index a87cda6..cb5d532 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -52,6 +52,7 @@ enum sysfs_buses {
 	SYSFS_BUS_IDE,
 	SYSFS_BUS_CCW,
 	SYSFS_BUS_CCISS,
+	SYSFS_BUS_RBD,
 };
 
 enum pathstates {
-- 
2.7.2

[PATCH 2/4] multipath-tools: add checker callout to repair path

[Mike Christie]

This patch adds a callback which can be used to repair a path
if check() has determined it is in the PATH_DOWN state.

The next patch that adds rbd checker support which will use this to
handle the case where a rbd device is blacklisted.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 libmultipath/checkers.c              | 23 +++++++++++++++++++++++
 libmultipath/checkers.h              |  4 ++++
 libmultipath/checkers/cciss_tur.c    |  5 +++++
 libmultipath/checkers/directio.c     |  5 +++++
 libmultipath/checkers/emc_clariion.c |  5 +++++
 libmultipath/checkers/hp_sw.c        |  5 +++++
 libmultipath/checkers/rdac.c         |  5 +++++
 libmultipath/checkers/readsector0.c  |  5 +++++
 libmultipath/checkers/tur.c          |  5 +++++
 multipathd/main.c                    |  9 +++++++++
 10 files changed, 71 insertions(+)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 42baa86..8976c89 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -138,6 +138,14 @@ struct checker * add_checker (char *multipath_dir, char * name)
 	if (!c->free)
 		goto out;
 
+	c->repair = (void (*)(struct checker *)) dlsym(c->handle,
+						       "libcheck_repair");
+	errstr = dlerror();
+	if (errstr != NULL)
+		condlog(0, "A dynamic linking error occurred: (%s)", errstr);
+	if (!c->repair)
+		goto out;
+
 	c->fd = 0;
 	c->sync = 1;
 	list_add(&c->node, &checkers);
@@ -203,6 +211,20 @@ void checker_put (struct checker * dst)
 	free_checker(src);
 }
 
+void checker_repair (struct checker * c)
+{
+	if (!c)
+		return;
+
+	c->message[0] = '\0';
+	if (c->disable) {
+		MSG(c, "checker disabled");
+		return;
+	}
+
+	c->repair(c);
+}
+
 int checker_check (struct checker * c)
 {
 	int r;
@@ -272,6 +294,7 @@ void checker_get (char *multipath_dir, struct checker * dst, char * name)
 	dst->sync = src->sync;
 	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
 	strncpy(dst->message, src->message, CHECKER_MSG_LEN);
+	dst->repair = src->repair;
 	dst->check = src->check;
 	dst->init = src->init;
 	dst->free = src->free;
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 8fc8616..502d2d9 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -113,6 +113,9 @@ struct checker {
 						multipath-wide. Use MALLOC if
 						you want to stuff data in. */
 	int (*check)(struct checker *);
+	void (*repair)(struct checker *);     /* called if check returns
+					        PATH_DOWN to bring path into
+						usable state */
 	int (*init)(struct checker *);       /* to allocate the context */
 	void (*free)(struct checker *);      /* to free the context */
 };
@@ -132,6 +135,7 @@ void checker_set_async (struct checker *);
 void checker_set_fd (struct checker *, int);
 void checker_enable (struct checker *);
 void checker_disable (struct checker *);
+void checker_repair (struct checker *);
 int checker_check (struct checker *);
 int checker_selected (struct checker *);
 char * checker_name (struct checker *);
diff --git a/libmultipath/checkers/cciss_tur.c b/libmultipath/checkers/cciss_tur.c
index 81500dc..a0ffffe 100644
--- a/libmultipath/checkers/cciss_tur.c
+++ b/libmultipath/checkers/cciss_tur.c
@@ -59,6 +59,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 extern int
 libcheck_check (struct checker * c)
 {
diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index 94bf8f7..eec12d5 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -118,6 +118,11 @@ void libcheck_free (struct checker * c)
 	free(ct);
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 static int
 check_state(int fd, struct directio_context *ct, int sync, int timeout_secs)
 {
diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
index 5e416a0..a7b9f86 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -91,6 +91,11 @@ void libcheck_free (struct checker * c)
 	free(c->context);
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 int libcheck_check (struct checker * c)
 {
 	unsigned char sense_buffer[128] = { 0, };
diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
index fe5e0f9..0cc1111 100644
--- a/libmultipath/checkers/hp_sw.c
+++ b/libmultipath/checkers/hp_sw.c
@@ -44,6 +44,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 static int
 do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
        void *resp, int mx_resp_len, int noisy, unsigned int timeout)
diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
index 00e3c44..68682c8 100644
--- a/libmultipath/checkers/rdac.c
+++ b/libmultipath/checkers/rdac.c
@@ -139,6 +139,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 static int
 do_inq(int sg_fd, unsigned int pg_op, void *resp, int mx_resp_len,
        unsigned int timeout)
diff --git a/libmultipath/checkers/readsector0.c b/libmultipath/checkers/readsector0.c
index 1c2a868..8fccb46 100644
--- a/libmultipath/checkers/readsector0.c
+++ b/libmultipath/checkers/readsector0.c
@@ -23,6 +23,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 int libcheck_check (struct checker * c)
 {
 	unsigned char buf[4096];
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index c4ef8a8..219afc3 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -97,6 +97,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 #define TUR_MSG(msg, fmt, args...) snprintf(msg, CHECKER_MSG_LEN, fmt, ##args);
 
 int
diff --git a/multipathd/main.c b/multipathd/main.c
index 20589a7..f34500c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1725,6 +1725,14 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	return 1;
 }
 
+void repair_path(struct vectors * vecs, struct path * pp)
+{
+	if (pp->state != PATH_DOWN)
+		return;
+
+	checker_repair(&pp->checker);
+}
+
 static void *
 checkerloop (void *ap)
 {
@@ -1796,6 +1804,7 @@ checkerloop (void *ap)
 					i--;
 				} else
 					num_paths += rc;
+				repair_path(vecs, pp);
 			}
 			lock_cleanup_pop(vecs->lock);
 		}
-- 
2.7.2

[PATCH 3/4] multipath-tools: Add rbd checker.

[Mike Christie]

This checker currently only handles the case where a path is failed
due to it being blacklisted by the ceph cluster. The specific use
case for me is when LIO exports rbd images through multiple LIO
instances.

The problem it handles is when rbd instance1 has the exclusive lock,
but becomes unreachable another host in the cluster will take over
and blacklist the instance1. This prevents it from sending stale IO
and corrupting data.

Later, when the host is reachable, we will want to failback to it.
To this, the checker will detect we were blacklisted, unmap the old
image which will make sure old IO is failed, and then remap the image
and unblacklist the host. multipathd will then handle this like a
path being removed and re-added.

Changes since v1:
1. Call the rbd command to remap images instead of trying to interact
with sysfs directly. This fixes ceph auth issues hit when using the
sysfs interface directly.

We currently do not support rbd root/boot. When that is supported we
will have a lib to interact with the kernel and we can then convert
to it.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 libmultipath/checkers/Makefile |   6 +-
 libmultipath/checkers/rbd.c    | 639 +++++++++++++++++++++++++++++++++++++++++
 multipath/multipath.conf.5     |   3 +
 3 files changed, 647 insertions(+), 1 deletion(-)
 create mode 100644 libmultipath/checkers/rbd.c

diff --git a/libmultipath/checkers/Makefile b/libmultipath/checkers/Makefile
index fb8fff0..5fd2099 100644
--- a/libmultipath/checkers/Makefile
+++ b/libmultipath/checkers/Makefile
@@ -11,12 +11,16 @@ LIBS= \
 	libcheckdirectio.so \
 	libcheckemc_clariion.so \
 	libcheckhp_sw.so \
-	libcheckrdac.so
+	libcheckrdac.so \
+	libcheckrbd.so
 
 CFLAGS += -I..
 
 all: $(LIBS)
 
+libcheckrbd.so: rbd.o
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ -lrados -ludev
+
 libcheckdirectio.so: libsg.o directio.o
 	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ -laio
 
diff --git a/libmultipath/checkers/rbd.c b/libmultipath/checkers/rbd.c
new file mode 100644
index 0000000..6f1b53a
--- /dev/null
+++ b/libmultipath/checkers/rbd.c
@@ -0,0 +1,639 @@
+/*
+ * Copyright (c) 2016 Red Hat
+ * Copyright (c) 2004 Christophe Varoqui
+ *
+ * Code based off of tur.c and ceph's krbd.cc
+ */
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <pthread.h>
+#include <libudev.h>
+#include <ifaddrs.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+
+#include "rados/librados.h"
+
+#include "structs.h"
+#include "checkers.h"
+
+#include "../libmultipath/debug.h"
+#include "../libmultipath/uevent.h"
+
+struct rbd_checker_context;
+typedef int (thread_fn)(struct rbd_checker_context *ct, char *msg);
+
+#define RBD_MSG(msg, fmt, args...) snprintf(msg, CHECKER_MSG_LEN, fmt, ##args);
+
+struct rbd_checker_context {
+	int rbd_bus_id;
+	char *client_addr;
+	char *config_info;
+	char *snap;
+	char *pool;
+	char *image;
+	char *username;
+	int remapped;
+	int blacklisted;
+
+	rados_t cluster;
+
+	int state;
+	int running;
+	time_t time;
+	thread_fn *fn;
+	pthread_t thread;
+	pthread_mutex_t lock;
+	pthread_cond_t active;
+	pthread_spinlock_t hldr_lock;
+	int holders;
+	char message[CHECKER_MSG_LEN];
+};
+
+int libcheck_init(struct checker * c)
+{
+	struct rbd_checker_context *ct;
+	struct udev_device *block_dev;
+	struct udev_device *bus_dev;
+	struct udev *udev;
+	struct stat sb;
+	const char *block_name, *addr, *config_info;
+	const char *image, *pool, *snap, *username;
+	char sysfs_path[PATH_SIZE];
+	int ret;
+
+	ct = malloc(sizeof(struct rbd_checker_context));
+	if (!ct)
+		return 1;
+	memset(ct, 0, sizeof(struct rbd_checker_context));
+	ct->holders = 1;
+	pthread_cond_init(&ct->active, NULL);
+	pthread_mutex_init(&ct->lock, NULL);
+	pthread_spin_init(&ct->hldr_lock, PTHREAD_PROCESS_PRIVATE);
+	c->context = ct;
+
+	/*
+	 * The rbd block layer sysfs device is not linked to the rbd bus
+	 * device that we interact with, so figure that out now.
+	 */
+	if (fstat(c->fd, &sb) != 0)
+		goto free_ct;
+
+	udev = udev_new();
+	if (!udev)
+		goto free_ct;
+
+	block_dev = udev_device_new_from_devnum(udev, 'b', sb.st_rdev);
+	if (!block_dev)
+		goto free_udev;
+
+	block_name  = udev_device_get_sysname(block_dev);
+	ret = sscanf(block_name, "rbd%d", &ct->rbd_bus_id);
+
+	udev_device_unref(block_dev);
+	if (ret != 1)
+		goto free_udev;
+
+	snprintf(sysfs_path, sizeof(sysfs_path), "/sys/bus/rbd/devices/%d",
+		 ct->rbd_bus_id);
+	bus_dev = udev_device_new_from_syspath(udev, sysfs_path);
+	if (!bus_dev)
+		goto free_udev;
+
+	addr = udev_device_get_sysattr_value(bus_dev, "client_addr");
+	if (!addr) {
+		condlog(0, "Could not find client_addr in rbd sysfs. Try "
+			"updating kernel");
+		goto free_dev;
+	}
+
+	ct->client_addr = strdup(addr);
+	if (!ct->client_addr)
+		goto free_dev;
+
+	config_info = udev_device_get_sysattr_value(bus_dev, "config_info");
+	if (!config_info)
+		goto free_addr;
+
+	ct->config_info = strdup(config_info);
+	if (!ct->config_info)
+		goto free_addr;
+
+	username = strstr(config_info, "name=");
+	if (username) {
+		char *end;
+		int len;
+
+		username += 5;
+		end = strchr(username, ',');
+		if (!end)
+			goto free_info;
+		len = end - username;
+
+		ct->username = malloc(len + 1);
+		if (!ct->username)
+			goto free_info;
+		strncpy(ct->username, username, len);
+		ct->username[len] = '\0';
+	}
+
+	image = udev_device_get_sysattr_value(bus_dev, "name");
+	if (!image)
+		goto free_username;
+
+	ct->image = strdup(image);
+	if (!ct->image)
+		goto free_info;
+
+	pool = udev_device_get_sysattr_value(bus_dev, "pool");
+	if (!pool)
+		goto free_image;
+
+	ct->pool = strdup(pool);
+	if (!ct->pool)
+		goto free_image;
+
+	snap = udev_device_get_sysattr_value(bus_dev, "current_snap");
+	if (!snap)
+		goto free_pool;
+
+	if (strcmp("-", snap)) {
+		ct->snap = strdup(snap);
+		if (!ct->snap)
+			goto free_pool;
+	}
+
+	if (rados_create(&ct->cluster, NULL) < 0) {
+		condlog(0, "Could not create rados cluster");
+		goto free_snap;
+	}
+
+	if (rados_conf_read_file(ct->cluster, NULL) < 0) {
+		condlog(0, "Could not read rados conf");
+		goto shutdown_rados;
+	}
+
+	ret = rados_connect(ct->cluster);
+	if (ret < 0) {
+		condlog(0, "Could not connect to rados cluster");
+		goto shutdown_rados;
+	}
+
+	udev_device_unref(bus_dev);
+	udev_unref(udev);
+
+	condlog(3, "rbd%d checker init %s %s/%s@%s %s", ct->rbd_bus_id,
+		ct->client_addr, ct->pool, ct->image, ct->snap ? ct->snap : "-",
+		ct->username ? ct->username : "none");
+	return 0;
+
+shutdown_rados:
+	rados_shutdown(ct->cluster);
+free_snap:
+	if (ct->snap)
+		free(ct->snap);
+free_pool:
+	free(ct->pool);
+free_image:
+	free(ct->image);
+free_username:
+	if (ct->username)
+		free(ct->username);
+free_info:
+	free(ct->config_info);
+free_addr:
+	free(ct->client_addr);
+free_dev:
+	udev_device_unref(bus_dev);
+free_udev:
+	udev_unref(udev);
+free_ct:
+	free(ct);
+	return 1;
+}
+
+void cleanup_context(struct rbd_checker_context *ct)
+{
+	pthread_mutex_destroy(&ct->lock);
+	pthread_cond_destroy(&ct->active);
+	pthread_spin_destroy(&ct->hldr_lock);
+
+	rados_shutdown(ct->cluster);
+
+	if (ct->username)
+		free(ct->username);
+	if (ct->snap)
+		free(ct->snap);
+	free(ct->pool);
+	free(ct->image);
+	free(ct->config_info);
+	free(ct->client_addr);
+	free(ct);
+}
+
+void libcheck_free(struct checker * c)
+{
+	if (c->context) {
+		struct rbd_checker_context *ct = c->context;
+		int holders;
+		pthread_t thread;
+
+		pthread_spin_lock(&ct->hldr_lock);
+		ct->holders--;
+		holders = ct->holders;
+		thread = ct->thread;
+		pthread_spin_unlock(&ct->hldr_lock);
+		if (holders)
+			pthread_cancel(thread);
+		else
+			cleanup_context(ct);
+		c->context = NULL;
+	}
+}
+
+static int rbd_is_blacklisted(struct rbd_checker_context *ct, char *msg)
+{
+	char *addr_tok, *start, *save;
+	char *cmd[2];
+	char *blklist, *stat;
+	size_t blklist_len, stat_len;
+	int ret;
+	char *end;
+
+	cmd[0] = "{\"prefix\": \"osd blacklist ls\"}";
+	cmd[1] = NULL;
+
+	ret = rados_mon_command(ct->cluster, (const char **)cmd, 1, "", 0,
+				&blklist, &blklist_len, &stat, &stat_len);
+	if (ret < 0) {
+		RBD_MSG(msg, "rbd checker failed: mon command failed %d",
+			ret);
+		return ret;
+	}
+
+	if (!blklist || !blklist_len)
+		goto free_bufs;
+
+	/*
+	 * parse list of addrs with the format
+	 * ipv4:port/nonce date time\n
+	 * or
+	 * [ipv6]:port/nonce date time\n
+	 */
+	ret = 0;
+	for (start = blklist; ; start = NULL) {
+		addr_tok = strtok_r(start, "\n", &save);
+		if (!addr_tok || !strlen(addr_tok))
+			break;
+
+		end = strchr(addr_tok, ' ');
+		if (!end) {
+			RBD_MSG(msg, "rbd%d checker failed: invalid blacklist %s",
+				 ct->rbd_bus_id, addr_tok);
+			break;
+		}
+		*end = '\0';
+
+		if (!strcmp(addr_tok, ct->client_addr)) {
+			ct->blacklisted = 1;
+			RBD_MSG(msg, "rbd%d checker: %s is blacklisted",
+				ct->rbd_bus_id, ct->client_addr);
+			ret = 1;
+			break;
+		}
+	}
+
+free_bufs:
+	rados_buffer_free(blklist);
+	rados_buffer_free(stat);
+	return ret;
+}
+
+int rbd_check(struct rbd_checker_context *ct, char *msg)
+{
+	if (ct->blacklisted || rbd_is_blacklisted(ct, msg) == 1)
+		return PATH_DOWN;
+
+	RBD_MSG(msg, "rbd checker reports path is up");
+	/*
+	 * Path may have issues, but the ceph cluster is at least
+	 * accepting IO, so we can attempt to do IO.
+	 *
+	 * TODO: in future versions, we can run other tests to
+	 * verify OSDs and networks.
+	 */
+	return PATH_UP;
+}
+
+int safe_write(int fd, const void *buf, size_t count)
+{
+	while (count > 0) {
+		ssize_t r = write(fd, buf, count);
+		if (r < 0) {
+			if (errno == EINTR)
+				continue;
+			return -errno;
+		}
+		count -= r;
+		buf = (char *)buf + r;
+	}
+	return 0;
+}
+
+static int sysfs_write_rbd_bus(const char *which, const char *buf,
+			       size_t buf_len)
+{
+	char sysfs_path[PATH_SIZE];
+	int fd;
+	int r;
+
+	/* we require newer kernels so single_major should alwayws be there */
+	snprintf(sysfs_path, sizeof(sysfs_path),
+		 "/sys/bus/rbd/%s_single_major", which);
+	fd = open(sysfs_path, O_WRONLY);
+	if (fd < 0)
+		return -errno;
+
+	r = safe_write(fd, buf, buf_len);
+	close(fd);
+	return r;
+}
+
+static int rbd_remap(struct rbd_checker_context *ct)
+{
+	char *argv[11];
+	pid_t pid;
+	int ret = 0, i = 0;
+	int status;
+
+	pid = fork();
+	switch (pid) {
+	case 0:
+		argv[i++] = "rbd";
+		argv[i++] = "map";
+		argv[i++] = "-o noshare";
+		if (ct->username) {
+			argv[i++] = "--id";
+			argv[i++] = ct->username;
+		}
+		argv[i++] = "--pool";
+		argv[i++] = ct->pool;
+		if (ct->snap) {
+			argv[i++] = "--snap";
+			argv[i++] = ct->snap;
+		}
+		argv[i++] = ct->image;
+		argv[i] = NULL;
+
+		ret = execvp(argv[0], argv);
+		condlog(0, "Error executing rbd: %s", strerror(errno));
+		exit(-1);
+	case -1:
+		condlog(0, "fork failed: %s", strerror(errno));
+		return -1;
+	default:
+		ret = -1;
+		wait(&status);
+		if (WIFEXITED(status)) {
+			status = WEXITSTATUS(status);
+			if (status == 0)
+				ret = 0;
+			else
+				condlog(0, "rbd failed with %d", status);
+		}
+	}
+
+	return ret;
+}
+
+static int sysfs_write_rbd_remove(const char *buf, int buf_len)
+{
+	return sysfs_write_rbd_bus("remove", buf, buf_len);
+}
+
+static int rbd_rm_blacklist(struct rbd_checker_context *ct)
+{
+	char *cmd[2];
+	char *stat, *cmd_str;
+	size_t stat_len;
+	int ret;
+
+	ret = asprintf(&cmd_str, "{\"prefix\": \"osd blacklist\", \"blacklistop\": \"rm\", \"addr\": \"%s\"}",
+		       ct->client_addr);
+	if (ret == -1)
+		return -ENOMEM;
+
+	cmd[0] = cmd_str;
+	cmd[1] = NULL;
+
+	ret = rados_mon_command(ct->cluster, (const char **)cmd, 1, "", 0,
+				NULL, 0, &stat, &stat_len);
+	if (ret < 0) {
+		condlog(1, "rbd%d repair failed to remove blacklist for %s %d",
+			ct->rbd_bus_id, ct->client_addr, ret);
+		goto free_cmd;
+	}
+
+	condlog(1, "rbd%d repair rm blacklist for %s",
+	       ct->rbd_bus_id, ct->client_addr);
+	free(stat);
+free_cmd:
+	free(cmd_str);
+	return ret;
+}
+
+static int rbd_repair(struct rbd_checker_context *ct, char *msg)
+{
+	char del[17];
+	int ret;
+
+	if (!ct->blacklisted)
+		return PATH_UP;
+
+	if (!ct->remapped) {
+		ret = rbd_remap(ct);
+		if (ret) {
+			RBD_MSG(msg, "rbd%d repair failed to remap. Err %d",
+				ct->rbd_bus_id, ret);
+			return PATH_DOWN;
+		}
+	}
+	ct->remapped = 1;
+
+	snprintf(del, sizeof(del), "%d force", ct->rbd_bus_id);
+	ret = sysfs_write_rbd_remove(del, strlen(del) + 1);
+	if (ret) {
+		RBD_MSG(msg, "rbd%d repair failed to clean up. Err %d",
+			ct->rbd_bus_id, ret);
+		return PATH_DOWN;
+	}
+
+	ret = rbd_rm_blacklist(ct);
+	if (ret) {
+		RBD_MSG(msg, "rbd%d repair could not remove blacklist entry. Err %d",
+			ct->rbd_bus_id, ret);
+		return PATH_DOWN;
+	}
+
+	ct->remapped = 0;
+	ct->blacklisted = 0;
+
+	RBD_MSG(msg, "rbd%d has been repaired", ct->rbd_bus_id);
+	return PATH_UP;
+}
+
+#define rbd_thread_cleanup_push(ct) pthread_cleanup_push(cleanup_func, ct)
+#define rbd_thread_cleanup_pop(ct) pthread_cleanup_pop(1)
+
+void cleanup_func(void *data)
+{
+	int holders;
+	struct rbd_checker_context *ct = data;
+	pthread_spin_lock(&ct->hldr_lock);
+	ct->holders--;
+	holders = ct->holders;
+	ct->thread = 0;
+	pthread_spin_unlock(&ct->hldr_lock);
+	if (!holders)
+		cleanup_context(ct);
+}
+
+void *rbd_thread(void *ctx)
+{
+	struct rbd_checker_context *ct = ctx;
+	int state;
+
+	condlog(3, "rbd%d thread starting up", ct->rbd_bus_id);
+
+	ct->message[0] = '\0';
+	/* This thread can be canceled, so setup clean up */
+	rbd_thread_cleanup_push(ct)
+
+	/* checker start up */
+	pthread_mutex_lock(&ct->lock);
+	ct->state = PATH_PENDING;
+	pthread_mutex_unlock(&ct->lock);
+
+	state = ct->fn(ct, ct->message);
+
+	/* checker done */
+	pthread_mutex_lock(&ct->lock);
+	ct->state = state;
+	pthread_mutex_unlock(&ct->lock);
+	pthread_cond_signal(&ct->active);
+
+	condlog(3, "rbd%d thead finished, state %s", ct->rbd_bus_id,
+		checker_state_name(state));
+	rbd_thread_cleanup_pop(ct);
+	return ((void *)0);
+}
+
+static void rbd_timeout(struct timespec *tsp)
+{
+	struct timeval now;
+
+	gettimeofday(&now, NULL);
+	tsp->tv_sec = now.tv_sec;
+	tsp->tv_nsec = now.tv_usec * 1000;
+	tsp->tv_nsec += 1000000; /* 1 millisecond */
+}
+
+static int rbd_exec_fn(struct checker *c, thread_fn *fn)
+{
+	struct rbd_checker_context *ct = c->context;
+	struct timespec tsp;
+	pthread_attr_t attr;
+	int rbd_status, r;
+
+	if (c->sync)
+		return rbd_check(ct, c->message);
+	/*
+	 * Async mode
+	 */
+	r = pthread_mutex_lock(&ct->lock);
+	if (r != 0) {
+		condlog(2, "rbd%d mutex lock failed with %d", ct->rbd_bus_id,
+			r);
+		MSG(c, "rbd%d thread failed to initialize", ct->rbd_bus_id);
+		return PATH_WILD;
+	}
+
+	if (ct->running) {
+		/* Check if checker is still running */
+		if (ct->thread) {
+			condlog(3, "rbd%d thread not finished", ct->rbd_bus_id);
+			rbd_status = PATH_PENDING;
+		} else {
+			/* checker done */
+			ct->running = 0;
+			rbd_status = ct->state;
+			strncpy(c->message, ct->message, CHECKER_MSG_LEN);
+			c->message[CHECKER_MSG_LEN - 1] = '\0';
+		}
+		pthread_mutex_unlock(&ct->lock);
+	} else {
+		/* Start new checker */
+		ct->state = PATH_UNCHECKED;
+		ct->fn = fn;
+		pthread_spin_lock(&ct->hldr_lock);
+		ct->holders++;
+		pthread_spin_unlock(&ct->hldr_lock);
+		setup_thread_attr(&attr, 32 * 1024, 1);
+		r = pthread_create(&ct->thread, &attr, rbd_thread, ct);
+		if (r) {
+			pthread_mutex_unlock(&ct->lock);
+			ct->thread = 0;
+			ct->holders--;
+			condlog(3, "rbd%d failed to start rbd thread, using sync mode",
+				ct->rbd_bus_id);
+			return fn(ct, c->message);
+		}
+		pthread_attr_destroy(&attr);
+		rbd_timeout(&tsp);
+		r = pthread_cond_timedwait(&ct->active, &ct->lock, &tsp);
+		rbd_status = ct->state;
+		strncpy(c->message, ct->message,CHECKER_MSG_LEN);
+		c->message[CHECKER_MSG_LEN -1] = '\0';
+		pthread_mutex_unlock(&ct->lock);
+
+		if (ct->thread &&
+		    (rbd_status == PATH_PENDING || rbd_status == PATH_UNCHECKED)) {
+			condlog(3, "rbd%d thread still running",
+				ct->rbd_bus_id);
+			ct->running = 1;
+			rbd_status = PATH_PENDING;
+		}
+	}
+
+	return rbd_status;
+}
+
+void libcheck_repair(struct checker * c)
+{
+	struct rbd_checker_context *ct = c->context;
+
+	if (!ct || !ct->blacklisted)
+		return;
+	rbd_exec_fn(c, rbd_repair);
+}
+
+int libcheck_check(struct checker * c)
+{
+	struct rbd_checker_context *ct = c->context;
+
+	if (!ct)
+		return PATH_UNCHECKED;
+
+	if (ct->blacklisted)
+		return PATH_DOWN;
+
+	return rbd_exec_fn(c, rbd_check);
+}
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index aaf2705..8fd61f9 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -344,6 +344,9 @@ Please use \fItur\fR instead.
 (Hardware-dependent)
 Check the path state for HP/COMPAQ Smart Array(CCISS) controllers.
 .TP
+.I rbd
+Check if the path is in the Ceph blacklist and remap the path if it is.
+.TP
 Default value is: \fBtur\fR
 .RE
 .TP
-- 
2.7.2

[PATCH 4/4] multipath-tools: Add rbd to the hwtable

[Mike Christie]

Add rbd to hwtable. These defaults are for the HA type of setup
supported by the checker. We do no support features like multibus
at the dm-multipath level yet.

Changes since v1:
1. Drop settings that were defaults and follow template.
2. Drop ID_UID use.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 libmultipath/hwtable.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
index 8c074f9..c307477 100644
--- a/libmultipath/hwtable.c
+++ b/libmultipath/hwtable.c
@@ -839,6 +839,21 @@ static struct hwentry default_hw[] = {
 		.flush_on_last_del = FLUSH_ENABLED,
 		.dev_loss      = 30,
 	},
+	{
+	/*
+	 * Red Hat
+	 *
+	 * Maintainer: Mike Christie
+	 * Mail: mchristi@redhat.com
+	 */
+		.vendor        = "Ceph",
+		.product       = "RBD",
+		.pgpolicy      = FAILOVER,
+		.no_path_retry = NO_PATH_RETRY_FAIL,
+		.checker_name  = RBD,
+		.deferred_remove = DEFERRED_REMOVE_ON,
+	},
+
 	/*
 	 * Tegile Systems
 	 */
-- 
2.7.2

Re: PATCH 0/4] multipath-tools: Ceph rbd support v2

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 1324 bytes --]

Merged.

I think it is worth advertizing here that the build now requires rados.h.

Thanks.

On Mon, Aug 8, 2016 at 2:01 PM, Mike Christie <mchristi@redhat.com> wrote:

> The following patches made over Christophe's tree today,
> add Ceph rbd support for handling blacklisted devices.
>
>
> This is not general support for rbd and multipath. There is
> no boot/root support and it does not support features like
> multibus. My use is for HA configurations, specifically for exporting
> rbd images through multiple LIO instances. In this case, we have one
> rbd instance that has the ceph rbd exclusive lock and it can send
> WRITE requests. If that host becomes unreachable, then another host
> will grab the lock, and blacklist the original host to prevent it from
> sending stale IO (when blacklisted IO will be failed by the OSD).
>
> To recover from that type of scenario, this patchset adds a repair()
> callout to the checker. If the path is in the PATH_DOWN state this
> callout can be used to fix it up. For my case, I am remapping
> the device to flush stale IO and cleanup the old lock,
> and then unblacklisting the path, so it can be used again.
>
> Changes since v1:
> - Drop ID_UID use and implemented sysfs getuid support.
> - Drop settings that were defaults and follow template.
> - Fix ceph auth/user.
>
>

[-- Attachment #1.2: Type: text/html, Size: 1745 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: PATCH 0/4] multipath-tools: Ceph rbd support v2

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

On 08/09/2016 10:36 AM, Christophe Varoqui wrote:
> Merged.
> 

Thanks.

> I think it is worth advertizing here that the build now requires rados.h.
> 

Sorry about adding the silly dependency. How about the attached patch to
make the rbd checker only be built if the user requests it. In the
documentation for my use, I will describe how to build it. This way
majority of users will not be bothered.


> Thanks.
> 
> On Mon, Aug 8, 2016 at 2:01 PM, Mike Christie <mchristi@redhat.com> wrote:
> 
>> The following patches made over Christophe's tree today,
>> add Ceph rbd support for handling blacklisted devices.
>>
>>
>> This is not general support for rbd and multipath. There is
>> no boot/root support and it does not support features like
>> multibus. My use is for HA configurations, specifically for exporting
>> rbd images through multiple LIO instances. In this case, we have one
>> rbd instance that has the ceph rbd exclusive lock and it can send
>> WRITE requests. If that host becomes unreachable, then another host
>> will grab the lock, and blacklist the original host to prevent it from
>> sending stale IO (when blacklisted IO will be failed by the OSD).
>>
>> To recover from that type of scenario, this patchset adds a repair()
>> callout to the checker. If the path is in the PATH_DOWN state this
>> callout can be used to fix it up. For my case, I am remapping
>> the device to flush stale IO and cleanup the old lock,
>> and then unblacklisting the path, so it can be used again.
>>
>> Changes since v1:
>> - Drop ID_UID use and implemented sysfs getuid support.
>> - Drop settings that were defaults and follow template.
>> - Fix ceph auth/user.
>>
>>
> 
> 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 


[-- Attachment #2: 0001-PATCH-checker-do-not-build-rbd-by-default.patch --]
[-- Type: text/x-patch, Size: 913 bytes --]

From 4d0bfdfc032d4fabf0ac08ba105125ded168bd95 Mon Sep 17 00:00:00 2001
From: Mike Christie <mchristi@redhat.com>
Date: Tue, 9 Aug 2016 13:19:41 -0500
Subject: [PATCH 1/1] checker: do not build rbd by default

rbd is not a common driver and users will probably not have the
tools/libs
to use it. Make the rbd checker not built by default.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 libmultipath/checkers/Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libmultipath/checkers/Makefile b/libmultipath/checkers/Makefile
index 5fd2099..aeb425f 100644
--- a/libmultipath/checkers/Makefile
+++ b/libmultipath/checkers/Makefile
@@ -11,8 +11,11 @@ LIBS= \
 	libcheckdirectio.so \
 	libcheckemc_clariion.so \
 	libcheckhp_sw.so \
-	libcheckrdac.so \
-	libcheckrbd.so
+	libcheckrdac.so
+
+ifneq ($(CHECKER_RBD),)
+LIBS += libcheckrbd.so
+endif
 
 CFLAGS += -I..
 
-- 
2.7.2


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: PATCH 0/4] multipath-tools: Ceph rbd support v2

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 2297 bytes --]

Distributors will surely want to build all checkers to have the docs and
hwtable in sync with the shipped checkers, and cover the widest range of
setups.

I'd rather drop this patch or, if it proves to be a problem, merge a
variant with the rbd checker build by default and not-buildable by explicit
define.

But thanks,
Christophe


On Tue, Aug 9, 2016 at 8:26 PM, Mike Christie <mchristi@redhat.com> wrote:

> On 08/09/2016 10:36 AM, Christophe Varoqui wrote:
> > Merged.
> >
>
> Thanks.
>
> > I think it is worth advertizing here that the build now requires rados.h.
> >
>
> Sorry about adding the silly dependency. How about the attached patch to
> make the rbd checker only be built if the user requests it. In the
> documentation for my use, I will describe how to build it. This way
> majority of users will not be bothered.
>
>
> > Thanks.
> >
> > On Mon, Aug 8, 2016 at 2:01 PM, Mike Christie <mchristi@redhat.com>
> wrote:
> >
> >> The following patches made over Christophe's tree today,
> >> add Ceph rbd support for handling blacklisted devices.
> >>
> >>
> >> This is not general support for rbd and multipath. There is
> >> no boot/root support and it does not support features like
> >> multibus. My use is for HA configurations, specifically for exporting
> >> rbd images through multiple LIO instances. In this case, we have one
> >> rbd instance that has the ceph rbd exclusive lock and it can send
> >> WRITE requests. If that host becomes unreachable, then another host
> >> will grab the lock, and blacklist the original host to prevent it from
> >> sending stale IO (when blacklisted IO will be failed by the OSD).
> >>
> >> To recover from that type of scenario, this patchset adds a repair()
> >> callout to the checker. If the path is in the PATH_DOWN state this
> >> callout can be used to fix it up. For my case, I am remapping
> >> the device to flush stale IO and cleanup the old lock,
> >> and then unblacklisting the path, so it can be used again.
> >>
> >> Changes since v1:
> >> - Drop ID_UID use and implemented sysfs getuid support.
> >> - Drop settings that were defaults and follow template.
> >> - Fix ceph auth/user.
> >>
> >>
> >
> >
> >
> > --
> > dm-devel mailing list
> > dm-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel
> >
>
>

[-- Attachment #1.2: Type: text/html, Size: 3347 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: PATCH 0/4] multipath-tools: Ceph rbd support v2

[Bart Van Assche]

On 08/10/2016 12:55 AM, Christophe Varoqui wrote:
> Distributors will surely want to build all checkers to have the docs and
> hwtable in sync with the shipped checkers, and cover the widest range of
> setups.
>
> I'd rather drop this patch or, if it proves to be a problem, merge a
> variant with the rbd checker build by default and not-buildable by
> explicit define.

Hello Mike and Christophe,

How about detecting at compile time whether the rbd header files are 
available? That approach avoids that we have to introduce a configure 
script and also avoids that the user has to enable rbd support 
explicitly. In e.g. the fio makefile there are several tests to check 
which libraries are available and can be used during the fio build.

Bart.

Re: [PATCH 2/4] multipath-tools: add checker callout to repair path

[Bart Van Assche]

On 08/08/2016 05:01 AM, Mike Christie wrote:
> This patch adds a callback which can be used to repair a path
> if check() has determined it is in the PATH_DOWN state.
> 
> The next patch that adds rbd checker support which will use this to
> handle the case where a rbd device is blacklisted.

Hello Mike,

With this patch applied, with the TUR checker enabled in multipath.conf
I see the following crash if I trigger SRP failover and failback:

ion-dev-ib-ini:~ # gdb ~bart/software/multipath-tools/multipathd/multipathd
(gdb) handle SIGPIPE noprint nostop
Signal        Stop      Print   Pass to program Description
SIGPIPE       No        No      Yes             Broken pipe
(gdb) run -d
Aug 11 08:46:27 | sde: remove path (uevent)
Aug 11 08:46:27 | mpathbe: adding map
Aug 11 08:46:27 | 8:64: cannot find block device
Aug 11 08:46:27 | Invalid device number 1
Aug 11 08:46:27 | 1: cannot find block device
Aug 11 08:46:27 | 8:96: cannot find block device
Aug 11 08:46:27 | mpathbe: failed to setup multipath
Aug 11 08:46:27 | dm-0: uev_add_map failed
Aug 11 08:46:27 | uevent trigger error

Thread 4 "multipathd" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff7f8b700 (LWP 8446)]
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00007ffff6c41905 in checker_repair (c=0x7fffdc001ef0) at checkers.c:225
#2  0x000000000040a760 in repair_path (vecs=0x66d7e0, pp=0x7fffdc001a40)
    at main.c:1733
#3  0x000000000040ab27 in checkerloop (ap=0x66d7e0) at main.c:1807
#4  0x00007ffff79bb474 in start_thread (arg=0x7ffff7f8b700)
    at pthread_create.c:333
#5  0x00007ffff63243ed in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
(gdb) up
#1  0x00007ffff6c41905 in checker_repair (c=0x7fffdc001ef0) at checkers.c:225
225             c->repair(c);
(gdb) print *c
$1 = {node = {next = 0x0, prev = 0x0}, handle = 0x0, refcount = 0, fd = 0, 
  sync = 0, timeout = 0, disable = 0, name = '\000' <repeats 15 times>, 
  message = '\000' <repeats 255 times>, context = 0x0, mpcontext = 0x0, 
  check = 0x0, repair = 0x0, init = 0x0, free = 0x0}

Can you have a look at this?

Thanks,

Bart.

Re: [PATCH 2/4] multipath-tools: add checker callout to repair path

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

On 08/11/2016 10:50 AM, Bart Van Assche wrote:
> On 08/08/2016 05:01 AM, Mike Christie wrote:
>> This patch adds a callback which can be used to repair a path
>> if check() has determined it is in the PATH_DOWN state.
>>
>> The next patch that adds rbd checker support which will use this to
>> handle the case where a rbd device is blacklisted.
> 
> Hello Mike,
> 
> With this patch applied, with the TUR checker enabled in multipath.conf
> I see the following crash if I trigger SRP failover and failback:
> 
> ion-dev-ib-ini:~ # gdb ~bart/software/multipath-tools/multipathd/multipathd
> (gdb) handle SIGPIPE noprint nostop
> Signal        Stop      Print   Pass to program Description
> SIGPIPE       No        No      Yes             Broken pipe
> (gdb) run -d
> Aug 11 08:46:27 | sde: remove path (uevent)
> Aug 11 08:46:27 | mpathbe: adding map
> Aug 11 08:46:27 | 8:64: cannot find block device
> Aug 11 08:46:27 | Invalid device number 1
> Aug 11 08:46:27 | 1: cannot find block device
> Aug 11 08:46:27 | 8:96: cannot find block device
> Aug 11 08:46:27 | mpathbe: failed to setup multipath
> Aug 11 08:46:27 | dm-0: uev_add_map failed
> Aug 11 08:46:27 | uevent trigger error
> 
> Thread 4 "multipathd" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7ffff7f8b700 (LWP 8446)]
> 0x0000000000000000 in ?? ()
> (gdb) bt
> #0  0x0000000000000000 in ?? ()
> #1  0x00007ffff6c41905 in checker_repair (c=0x7fffdc001ef0) at checkers.c:225
> #2  0x000000000040a760 in repair_path (vecs=0x66d7e0, pp=0x7fffdc001a40)
>     at main.c:1733
> #3  0x000000000040ab27 in checkerloop (ap=0x66d7e0) at main.c:1807
> #4  0x00007ffff79bb474 in start_thread (arg=0x7ffff7f8b700)
>     at pthread_create.c:333
> #5  0x00007ffff63243ed in clone ()
>     at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
> (gdb) up
> #1  0x00007ffff6c41905 in checker_repair (c=0x7fffdc001ef0) at checkers.c:225
> 225             c->repair(c);
> (gdb) print *c
> $1 = {node = {next = 0x0, prev = 0x0}, handle = 0x0, refcount = 0, fd = 0, 
>   sync = 0, timeout = 0, disable = 0, name = '\000' <repeats 15 times>, 
>   message = '\000' <repeats 255 times>, context = 0x0, mpcontext = 0x0, 
>   check = 0x0, repair = 0x0, init = 0x0, free = 0x0}
> 

Sorry about the stupid bug.

Could you try the attached patch. I found two segfaults. If check_path
returns less than 0 then we free the path and so we cannot call repair
on it. If libcheck_init fails it memsets the checker, so we cannot call
repair on it too.

I moved the repair call to the specific paths that the path is down.

[-- Attachment #2: multipathd-only-call-repair-when-failed.patch --]
[-- Type: text/x-patch, Size: 1390 bytes --]

diff --git a/multipathd/main.c b/multipathd/main.c
index f34500c..9f213cc 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1442,6 +1442,16 @@ int update_path_groups(struct multipath *mpp, struct vectors *vecs, int refresh)
 	return 0;
 }
 
+void repair_path(struct path * pp)
+{
+	if (pp->state != PATH_DOWN)
+		return;
+
+	checker_repair(&pp->checker);
+	if (strlen(checker_message(&pp->checker)))
+		LOG_MSG(1, checker_message(&pp->checker));
+}
+
 /*
  * Returns '1' if the path has been checked, '-1' if it was blacklisted
  * and '0' otherwise
@@ -1606,6 +1616,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 			pp->mpp->failback_tick = 0;
 
 			pp->mpp->stat_path_failures++;
+			repair_path(pp);
 			return 1;
 		}
 
@@ -1700,7 +1711,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	}
 
 	pp->state = newstate;
-
+	repair_path(pp);
 
 	if (pp->mpp->wait_for_udev)
 		return 1;
@@ -1725,14 +1736,6 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	return 1;
 }
 
-void repair_path(struct vectors * vecs, struct path * pp)
-{
-	if (pp->state != PATH_DOWN)
-		return;
-
-	checker_repair(&pp->checker);
-}
-
 static void *
 checkerloop (void *ap)
 {
@@ -1804,7 +1807,6 @@ checkerloop (void *ap)
 					i--;
 				} else
 					num_paths += rc;
-				repair_path(vecs, pp);
 			}
 			lock_cleanup_pop(vecs->lock);
 		}

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/4] multipath-tools: add checker callout to repair path

[Bart Van Assche]

On 08/11/2016 01:33 PM, Mike Christie wrote:
> Could you try the attached patch. I found two segfaults. If check_path
> returns less than 0 then we free the path and so we cannot call repair
> on it. If libcheck_init fails it memsets the checker, so we cannot call
> repair on it too.
>
> I moved the repair call to the specific paths that the path is down.

Hello Mike,

Thanks for the patch. Unfortunately even with this patch applied I can 
still trigger a segfault sporadically:

# valgrind --read-var-info=yes multipathd -d
Aug 11 14:02:21 | mpathbf: load table [0 2097152 multipath 3 
queue_if_no_path pg_init_retries 50 0 2 1 queue-length 0 1 1 8:160 1000 
queue-length 0 1 1 8:64 1000]
Aug 11 14:02:21 | mpathbf: event checker started
Aug 11 14:02:21 | sdk [8:160]: path added to devmap mpathbf
Aug 11 14:02:21 | sdd: add path (uevent)
==2452== Thread 4:
==2452== Jump to the invalid address stated on the next line
==2452==    at 0x0: ???
==2452==    by 0x409BBE: repair_path (main.c:1451)
==2452==    by 0x40A905: check_path (main.c:1715)
==2452==    by 0x40AE72: checkerloop (main.c:1808)
==2452==    by 0x5047473: start_thread (pthread_create.c:333)
==2452==    by 0x671B3EC: clone (clone.S:109)
==2452==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==2452==
==2452==
==2452== Process terminating with default action of signal 11 (SIGSEGV)
==2452==  Bad permissions for mapped region at address 0x0
==2452==    at 0x0: ???
==2452==    by 0x409BBE: repair_path (main.c:1451)
==2452==    by 0x40A905: check_path (main.c:1715)
==2452==    by 0x40AE72: checkerloop (main.c:1808)
==2452==    by 0x5047473: start_thread (pthread_create.c:333)
==2452==    by 0x671B3EC: clone (clone.S:109)
==2452==

(gdb) list main.c:1451
1446    void repair_path(struct path * pp)
1447    {
1448            if (pp->state != PATH_DOWN)
1449                    return;
1450
1451            checker_repair(&pp->checker);
1452            if (strlen(checker_message(&pp->checker)))
1453                    LOG_MSG(1, checker_message(&pp->checker));
1454    }
1455

Re: [PATCH 2/4] multipath-tools: add checker callout to repair path

[Mike Christie]

On 08/11/2016 04:41 PM, Bart Van Assche wrote:
> On 08/11/2016 01:33 PM, Mike Christie wrote:
>> Could you try the attached patch. I found two segfaults. If check_path
>> returns less than 0 then we free the path and so we cannot call repair
>> on it. If libcheck_init fails it memsets the checker, so we cannot call
>> repair on it too.
>>
>> I moved the repair call to the specific paths that the path is down.
> 
> Hello Mike,
> 
> Thanks for the patch. Unfortunately even with this patch applied I can
> still trigger a segfault sporadically:
> 

I can't seem to replicate the problem with my patch and I do not see
anything. Could you send me your multipath.conf/hwtable settings?

For the fo/fb test, dev_loss_tmo is firing causing paths to be
added/deleted right?

Re: [PATCH 2/4] multipath-tools: add checker callout to repair path

[Bart Van Assche]

On 08/12/2016 09:54 AM, Mike Christie wrote:
> On 08/11/2016 04:41 PM, Bart Van Assche wrote:
>> On 08/11/2016 01:33 PM, Mike Christie wrote:
>>> Could you try the attached patch. I found two segfaults. If check_path
>>> returns less than 0 then we free the path and so we cannot call repair
>>> on it. If libcheck_init fails it memsets the checker, so we cannot call
>>> repair on it too.
>>>
>>> I moved the repair call to the specific paths that the path is down.
>>
>> Thanks for the patch. Unfortunately even with this patch applied I can
>> still trigger a segfault sporadically:
> 
> I can't seem to replicate the problem with my patch and I do not see
> anything. Could you send me your multipath.conf/hwtable settings?

Please find that file at the end of this e-mail.
 
> For the fo/fb test, dev_loss_tmo is firing causing paths to be
> added/deleted right?
 
The script that I'm using to simulate path loss writes into /sys/class
/srp_remote_ports/*/delete. That causes the ib_srp driver to call
scsi_remove_host(). That script is available at
https://github.com/bvanassche/srp-test. However, an InfiniBand HCA is
needed to run this script.

Bart.


/etc/multipath.conf:

defaults {
        user_friendly_names     yes
        queue_without_daemon    no
}

blacklist {
        device {
                vendor                  "ATA"
                product                 ".*"
        }
}

devices {
        device {
                vendor                  "SCST_BIO|LIO-ORG"
                product                 ".*"
                features                "3 queue_if_no_path pg_init_retries 50"
                path_grouping_policy    group_by_prio
                path_selector           "queue-length 0"
                path_checker            tur
        }
}

blacklist_exceptions {
        property        ".*"
}

Re: [PATCH 2/4] multipath-tools: add checker callout to repair path

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On 08/11/2016 04:41 PM, Bart Van Assche wrote:
> On 08/11/2016 01:33 PM, Mike Christie wrote:
>> Could you try the attached patch. I found two segfaults. If check_path
>> returns less than 0 then we free the path and so we cannot call repair
>> on it. If libcheck_init fails it memsets the checker, so we cannot call
>> repair on it too.
>>
>> I moved the repair call to the specific paths that the path is down.
> 
> Hello Mike,
> 
> Thanks for the patch. Unfortunately even with this patch applied I can
> still trigger a segfault sporadically:
> 

Ok. This should fix all of them. Attached patch fixes:

1. If check_path returns less than 0 then we free the path and so we
cannot call repair on it of course.
2. If libcheck_init fails it memsets the checker, so we cannot call
repair on it too.
3. We can hit a race where when pathinfo is setting up a path, the path
could have gone down. In the DI_CHECKER chunk we then do not run
get_state and attach a checker. Later when check_path is run
path_offline we could still return PATH_DOWN or PATH_REMOVED and
get_state is again not run so we do not get to attach a checker again. I
was then running repair_path since the state was PATH_DOWN, and kaboom.

Attached patch should fix these issues.

[-- Attachment #2: multipathd-fix-segfault.patch --]
[-- Type: text/x-patch, Size: 1731 bytes --]

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 8976c89..fd999b0 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -213,7 +213,7 @@ void checker_put (struct checker * dst)
 
 void checker_repair (struct checker * c)
 {
-	if (!c)
+	if (!c || !checker_selected(c))
 		return;
 
 	c->message[0] = '\0';
diff --git a/multipathd/main.c b/multipathd/main.c
index f5e9a01..c4ffe6f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1442,6 +1442,16 @@ int update_path_groups(struct multipath *mpp, struct vectors *vecs, int refresh)
 	return 0;
 }
 
+void repair_path(struct path * pp)
+{
+	if (pp->state != PATH_DOWN)
+		return;
+
+	checker_repair(&pp->checker);
+	if (strlen(checker_message(&pp->checker)))
+		LOG_MSG(1, checker_message(&pp->checker));
+}
+
 /*
  * Returns '1' if the path has been checked, '-1' if it was blacklisted
  * and '0' otherwise
@@ -1606,6 +1616,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 			pp->mpp->failback_tick = 0;
 
 			pp->mpp->stat_path_failures++;
+			repair_path(pp);
 			return 1;
 		}
 
@@ -1700,7 +1711,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	}
 
 	pp->state = newstate;
-
+	repair_path(pp);
 
 	if (pp->mpp->wait_for_udev)
 		return 1;
@@ -1725,14 +1736,6 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	return 1;
 }
 
-void repair_path(struct vectors * vecs, struct path * pp)
-{
-	if (pp->state != PATH_DOWN)
-		return;
-
-	checker_repair(&pp->checker);
-}
-
 static void *
 checkerloop (void *ap)
 {
@@ -1804,7 +1807,6 @@ checkerloop (void *ap)
 					i--;
 				} else
 					num_paths += rc;
-				repair_path(vecs, pp);
 			}
 			lock_cleanup_pop(vecs->lock);
 		}

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/4] multipath-tools: add checker callout to repair path

[Bart Van Assche]

On 08/14/2016 01:41 AM, Mike Christie wrote:
> Attached patch should fix these issues.

Hello Mike,

With that patch applied multipathd works again reliable on my test setup.

Thanks!

Bart.

[PATCH 2/4] multipath-tools: add checker callout to repair path

[Mike Christie]

This patch adds a callback which can be used to repair a path
if check() has determined it is in the PATH_DOWN state.

The next patch that adds rbd checker support which will use this to
handle the case where a rbd device is blacklisted.

Signed-off-by: Mike Christie <mchristi@redhat.com>
---
 libmultipath/checkers.c              | 23 +++++++++++++++++++++++
 libmultipath/checkers.h              |  4 ++++
 libmultipath/checkers/cciss_tur.c    |  5 +++++
 libmultipath/checkers/directio.c     |  5 +++++
 libmultipath/checkers/emc_clariion.c |  5 +++++
 libmultipath/checkers/hp_sw.c        |  5 +++++
 libmultipath/checkers/rdac.c         |  5 +++++
 libmultipath/checkers/readsector0.c  |  5 +++++
 libmultipath/checkers/tur.c          |  5 +++++
 multipathd/main.c                    | 14 +++++++++++++-
 10 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index ef1d099..de6a973 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -139,6 +139,14 @@ struct checker * add_checker (char * name)
 	if (!c->free)
 		goto out;
 
+	c->repair = (void (*)(struct checker *)) dlsym(c->handle,
+						       "libcheck_repair");
+	errstr = dlerror();
+	if (errstr != NULL)
+		condlog(0, "A dynamic linking error occurred: (%s)", errstr);
+	if (!c->repair)
+		goto out;
+
 	c->fd = 0;
 	c->sync = 1;
 	list_add(&c->node, &checkers);
@@ -204,6 +212,20 @@ void checker_put (struct checker * dst)
 	free_checker(src);
 }
 
+void checker_repair (struct checker * c)
+{
+	if (!c)
+		return;
+
+	c->message[0] = '\0';
+	if (c->disable) {
+		MSG(c, "checker disabled");
+		return;
+	}
+
+	c->repair(c);
+}
+
 int checker_check (struct checker * c)
 {
 	int r;
@@ -268,6 +290,7 @@ void checker_get (struct checker * dst, char * name)
 	dst->sync = src->sync;
 	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
 	strncpy(dst->message, src->message, CHECKER_MSG_LEN);
+	dst->repair = src->repair;
 	dst->check = src->check;
 	dst->init = src->init;
 	dst->free = src->free;
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index ea59c94..d665736 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -113,6 +113,9 @@ struct checker {
 						multipath-wide. Use MALLOC if
 						you want to stuff data in. */
 	int (*check)(struct checker *);
+	void (*repair)(struct checker *);     /* called if check returns
+					        PATH_DOWN to bring path into
+						usable state */
 	int (*init)(struct checker *);       /* to allocate the context */
 	void (*free)(struct checker *);      /* to free the context */
 };
@@ -132,6 +135,7 @@ void checker_set_async (struct checker *);
 void checker_set_fd (struct checker *, int);
 void checker_enable (struct checker *);
 void checker_disable (struct checker *);
+void checker_repair (struct checker *);
 int checker_check (struct checker *);
 int checker_selected (struct checker *);
 char * checker_name (struct checker *);
diff --git a/libmultipath/checkers/cciss_tur.c b/libmultipath/checkers/cciss_tur.c
index 4c26901..7e4eb81 100644
--- a/libmultipath/checkers/cciss_tur.c
+++ b/libmultipath/checkers/cciss_tur.c
@@ -63,6 +63,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 extern int
 libcheck_check (struct checker * c)
 {
diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index 94bf8f7..eec12d5 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -118,6 +118,11 @@ void libcheck_free (struct checker * c)
 	free(ct);
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 static int
 check_state(int fd, struct directio_context *ct, int sync, int timeout_secs)
 {
diff --git a/libmultipath/checkers/emc_clariion.c b/libmultipath/checkers/emc_clariion.c
index a797734..53db066 100644
--- a/libmultipath/checkers/emc_clariion.c
+++ b/libmultipath/checkers/emc_clariion.c
@@ -91,6 +91,11 @@ void libcheck_free (struct checker * c)
 	free(c->context);
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 int libcheck_check (struct checker * c)
 {
 	unsigned char sense_buffer[128] = { 0, };
diff --git a/libmultipath/checkers/hp_sw.c b/libmultipath/checkers/hp_sw.c
index fe5e0f9..0cc1111 100644
--- a/libmultipath/checkers/hp_sw.c
+++ b/libmultipath/checkers/hp_sw.c
@@ -44,6 +44,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 static int
 do_inq(int sg_fd, int cmddt, int evpd, unsigned int pg_op,
        void *resp, int mx_resp_len, int noisy, unsigned int timeout)
diff --git a/libmultipath/checkers/rdac.c b/libmultipath/checkers/rdac.c
index 00e3c44..68682c8 100644
--- a/libmultipath/checkers/rdac.c
+++ b/libmultipath/checkers/rdac.c
@@ -139,6 +139,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 static int
 do_inq(int sg_fd, unsigned int pg_op, void *resp, int mx_resp_len,
        unsigned int timeout)
diff --git a/libmultipath/checkers/readsector0.c b/libmultipath/checkers/readsector0.c
index 0550fb6..b3ed1f3 100644
--- a/libmultipath/checkers/readsector0.c
+++ b/libmultipath/checkers/readsector0.c
@@ -23,6 +23,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 int libcheck_check (struct checker * c)
 {
 	unsigned char buf[4096];
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index 2edc8ad..338d4a3 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -97,6 +97,11 @@ void libcheck_free (struct checker * c)
 	return;
 }
 
+void libcheck_repair (struct checker * c)
+{
+	return;
+}
+
 #define TUR_MSG(msg, fmt, args...) snprintf(msg, CHECKER_MSG_LEN, fmt, ##args);
 
 int
diff --git a/multipathd/main.c b/multipathd/main.c
index c0ca571..14728d5 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1635,6 +1635,14 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	return 1;
 }
 
+void repair_path(struct vectors * vecs, struct path * pp)
+{
+	if (pp->state != PATH_DOWN)
+		return;
+
+	checker_repair(&pp->checker);
+}
+
 static void *
 checkerloop (void *ap)
 {
@@ -1665,6 +1673,7 @@ checkerloop (void *ap)
 	while (1) {
 		struct timeval diff_time, start_time, end_time;
 		int num_paths = 0, ticks = 0, signo, strict_timing, rc = 0;
+		int checked;
 		sigset_t mask;
 
 		if (gettimeofday(&start_time, NULL) != 0)
@@ -1695,7 +1704,10 @@ checkerloop (void *ap)
 			lock(vecs->lock);
 			pthread_testcancel();
 			vector_foreach_slot (vecs->pathvec, pp, i) {
-				num_paths += check_path(vecs, pp, ticks);
+				checked = check_path(vecs, pp, ticks);
+				if (checked)
+					repair_path(vecs, pp);
+				num_paths += checked;
 			}
 			lock_cleanup_pop(vecs->lock);
 		}
-- 
2.5.5