All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] multipath: alternative reservation_key method
@ 2017-09-08 18:45 Benjamin Marzinski
  2017-09-08 18:45 ` [PATCH 1/5] libmultipath: pull functions into util.c Benjamin Marzinski
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Benjamin Marzinski @ 2017-09-08 18:45 UTC (permalink / raw)
  To: device-mapper development; +Cc: Martin Wilck

The scsi persistent reservation API doesn't force devices to implement any
method to display the mapping from a reservation key to an I_T Nexus (the
READ_FULL_STATUS action is an optional later addition, and a number of devices
don't support it). To allow multipathd to determine the correct reservation key
for a device without support from the device itself, it uses the
reservation_key configuration option. Unfortunately, using this option forces
the multipath configuration to be updated whenever a new scsi registration key
is used.  This isn't acceptable to some users, who want a static configuration
file. I've had multiple requests to allow persistent reservations without
needing to set the reservation_key paramter beforehand.

This patch set provides an alternative method of setting the reservation_key
for the multipath device. The reservation_key configuration option now also
accepts the keyword "file". If this is used, multpath will look in the new
prkeys file (by default "/etc/multipath/prkeys") for a line with the device
wwid and it's associated reservation_key. The patches allow users to manually
set reservation_key to multipath device mappings by using the multipathd
client commands, but mpathpersist will automatically detect when devices are
configured to use the prkeys file, and will set and clear the reservation
keys automatically.

Benjamin Marzinski (5):
  libmultipath: pull functions into util.c
  libmultipath: change reservation_key to a uint64_t
  libmpathpersist: fix update_prflag code
  multipath: add alternate reservation_key method
  mpathpersist: add support for prkeys file

 libmpathpersist/mpath_persist.c  |  70 ++++++++---------
 libmpathpersist/mpath_updatepr.c |  43 ++++++----
 libmpathpersist/mpathpr.h        |   3 +-
 libmultipath/Makefile            |   2 +-
 libmultipath/byteorder.h         |  36 +++++++++
 libmultipath/checkers/rbd.c      |  16 +---
 libmultipath/config.c            |   9 ++-
 libmultipath/config.h            |   9 ++-
 libmultipath/defaults.h          |   1 +
 libmultipath/dict.c              | 106 +++++++++++++------------
 libmultipath/dict.h              |   2 +-
 libmultipath/prkey.c             | 166 +++++++++++++++++++++++++++++++++++++++
 libmultipath/prkey.h             |  19 +++++
 libmultipath/propsel.c           |  35 +++++++--
 libmultipath/structs.h           |  12 ++-
 libmultipath/util.c              |  33 ++++++++
 libmultipath/util.h              |   4 +
 multipathd/cli.c                 |   8 ++
 multipathd/cli.h                 |   8 ++
 multipathd/cli_handlers.c        |  82 +++++++++++++++++++
 multipathd/cli_handlers.h        |   3 +
 multipathd/main.c                |  26 ++----
 22 files changed, 541 insertions(+), 152 deletions(-)
 create mode 100644 libmultipath/byteorder.h
 create mode 100644 libmultipath/prkey.c
 create mode 100644 libmultipath/prkey.h

-- 
2.7.4

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] libmultipath: pull functions into util.c
  2017-09-08 18:45 [PATCH 0/5] multipath: alternative reservation_key method Benjamin Marzinski
@ 2017-09-08 18:45 ` Benjamin Marzinski
  2017-09-08 20:51   ` Martin Wilck
  2017-09-08 18:45 ` [PATCH 2/5] libmultipath: change reservation_key to a uint64_t Benjamin Marzinski
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Benjamin Marzinski @ 2017-09-08 18:45 UTC (permalink / raw)
  To: device-mapper development; +Cc: Martin Wilck

This patch just pulls safe_write out of rbd. and the persistent
reservation key parsing code out of dict.c and puts them in util.c,
so that other functions can make use of them.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/checkers/rbd.c | 16 +---------------
 libmultipath/dict.c         | 26 +++++---------------------
 libmultipath/util.c         | 33 +++++++++++++++++++++++++++++++++
 libmultipath/util.h         |  4 ++++
 4 files changed, 43 insertions(+), 36 deletions(-)

diff --git a/libmultipath/checkers/rbd.c b/libmultipath/checkers/rbd.c
index 9ea0572..2c18009 100644
--- a/libmultipath/checkers/rbd.c
+++ b/libmultipath/checkers/rbd.c
@@ -28,6 +28,7 @@
 #include "../libmultipath/debug.h"
 #include "../libmultipath/util.h"
 #include "../libmultipath/time-util.h"
+#include "../libmultipath/util.h"
 
 struct rbd_checker_context;
 typedef int (thread_fn)(struct rbd_checker_context *ct, char *msg);
@@ -356,21 +357,6 @@ static int rbd_check(struct rbd_checker_context *ct, char *msg)
 	return PATH_UP;
 }
 
-static 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)
 {
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 9dc1090..680b2c5 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -19,6 +19,7 @@
 #include "blacklist.h"
 #include "defaults.h"
 #include "prio.h"
+#include "util.h"
 #include <errno.h>
 #include <inttypes.h>
 #include "mpath_cmd.h"
@@ -963,32 +964,15 @@ set_reservation_key(vector strvec, void *ptr)
 {
 	unsigned char **uchar_ptr = (unsigned char **)ptr;
 	char *buff;
-	char *tbuff;
-	int j, k;
-	int len;
+	int j;
 	uint64_t prkey;
 
 	buff = set_value(strvec);
 	if (!buff)
 		return 1;
 
-	tbuff = buff;
-
-	if (!memcmp("0x",buff, 2))
-		buff = buff + 2;
-
-	len = strlen(buff);
-
-	k = strspn(buff, "0123456789aAbBcCdDeEfF");
-
-	if (len != k) {
-		FREE(tbuff);
-		return 1;
-	}
-
-	if (1 != sscanf (buff, "%" SCNx64 "", &prkey))
-	{
-		FREE(tbuff);
+	if (parse_prkey(buff, &prkey) != 0) {
+		FREE(buff);
 		return 1;
 	}
 
@@ -1002,7 +986,7 @@ set_reservation_key(vector strvec, void *ptr)
 		prkey >>= 8;
 	}
 
-	FREE(tbuff);
+	FREE(buff);
 	return 0;
 }
 
diff --git a/libmultipath/util.c b/libmultipath/util.c
index dff2ed3..0800da5 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -11,6 +11,7 @@
 #include <unistd.h>
 #include <errno.h>
 
+#include "util.h"
 #include "debug.h"
 #include "memory.h"
 #include "checkers.h"
@@ -416,3 +417,35 @@ int get_linux_version_code(void)
 	pthread_once(&_lvc_initialized, _set_linux_version_code);
 	return _linux_version_code;
 }
+
+int parse_prkey(char *ptr, uint64_t *prkey)
+{
+	if (!ptr)
+		return 1;
+	if (*ptr == '0')
+		ptr++;
+	if (*ptr == 'x' || *ptr == 'X')
+		ptr++;
+	if (*ptr == '\0' || strlen(ptr) > 16)
+		return 1;
+	if (strlen(ptr) != strspn(ptr, "0123456789aAbBcCdDeEfF"))
+		return 1;
+	if (sscanf(ptr, "%" SCNx64 "", prkey) != 1)
+		return 1;
+	return 0;
+}
+
+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;
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index 45291be..3dc048e 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -2,6 +2,7 @@
 #define _UTIL_H
 
 #include <sys/types.h>
+#include <inttypes.h>
 
 size_t strchop(char *);
 int basenamecpy (const char * src, char * dst, int);
@@ -16,6 +17,9 @@ char *parse_uid_attribute_by_attrs(char *uid_attrs, char *path_dev);
 void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
 int systemd_service_enabled(const char *dev);
 int get_linux_version_code(void);
+int parse_prkey(char *ptr, uint64_t *prkey);
+int safe_write(int fd, const void *buf, size_t count);
+
 #define KERNEL_VERSION(maj, min, ptc) ((((maj) * 256) + (min)) * 256 + (ptc))
 
 #define safe_sprintf(var, format, args...)	\
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/5] libmultipath: change reservation_key to a uint64_t
  2017-09-08 18:45 [PATCH 0/5] multipath: alternative reservation_key method Benjamin Marzinski
  2017-09-08 18:45 ` [PATCH 1/5] libmultipath: pull functions into util.c Benjamin Marzinski
@ 2017-09-08 18:45 ` Benjamin Marzinski
  2017-09-08 21:09   ` Martin Wilck
  2017-09-08 18:45 ` [PATCH 3/5] libmpathpersist: fix update_prflag code Benjamin Marzinski
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Benjamin Marzinski @ 2017-09-08 18:45 UTC (permalink / raw)
  To: device-mapper development; +Cc: Martin Wilck

The reservation key is currently being stored as any array of 8 unsigned
chars.  This is exactly the same in-memory representation as a big
endian 64 bit integer. However, the code for dealing with a big endian
64 bit integer is much simpler, so switch to use that instead.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist.c | 23 +++++++----------------
 libmultipath/byteorder.h        | 36 ++++++++++++++++++++++++++++++++++++
 libmultipath/config.c           |  3 ---
 libmultipath/config.h           |  6 ++++--
 libmultipath/dict.c             | 31 ++++---------------------------
 libmultipath/propsel.c          |  6 +++---
 libmultipath/structs.h          |  5 ++++-
 multipathd/main.c               | 23 +++++------------------
 8 files changed, 63 insertions(+), 70 deletions(-)
 create mode 100644 libmultipath/byteorder.h

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index aab6d95..960d50d 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -776,7 +776,7 @@ int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	}
 
 	if (mpp->reservation_key ){
-		memcpy (pamp->key, mpp->reservation_key, 8);
+		memcpy (pamp->key, &mpp->reservation_key, 8);
 		condlog (3, "%s: reservation key set.", mpp->wwid);
 	}
 
@@ -794,7 +794,7 @@ int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	for (i = 0; i < num; i++){
 		if (mpp->reservation_key &&
 			memcmp(pr_buff->prin_descriptor.prin_readfd.descriptors[i]->key,
-			       mpp->reservation_key, 8)){
+			       &mpp->reservation_key, 8)){
 			/*register with tarnsport id*/
 			memset(pamp, 0, length);
 			pamp->trnptid_list[0] = pptr;
@@ -828,7 +828,7 @@ int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 
 	if (found){
 		memset (pamp, 0, length);
-		memcpy (pamp->sa_key, mpp->reservation_key, 8);
+		memcpy (pamp->sa_key, &mpp->reservation_key, 8);
 		memset (pamp->key, 0, 8);
 		status = mpath_prout_reg(mpp, MPATH_PROUT_REG_SA, rq_scope, rq_type, pamp, noisy);
 	}
@@ -873,9 +873,7 @@ int update_map_pr(struct multipath *mpp)
 {
 	int noisy=0;
 	struct prin_resp *resp;
-	int i,j, ret, isFound;
-	unsigned char *keyp;
-	uint64_t prkey;
+	int i, ret, isFound;
 
 	if (!mpp->reservation_key)
 	{
@@ -906,15 +904,8 @@ int update_map_pr(struct multipath *mpp)
 		return MPATH_PR_SUCCESS;
 	}
 
-	prkey = 0;
-	keyp = mpp->reservation_key;
-	for (j = 0; j < 8; ++j) {
-		if (j > 0)
-			prkey <<= 8;
-		prkey |= *keyp;
-		++keyp;
-	}
-	condlog(2, "%s: Multipath  reservation_key: 0x%" PRIx64 " ", mpp->alias, prkey);
+	condlog(2, "%s: Multipath  reservation_key: 0x%" PRIx64 " ", mpp->alias,
+		be64_to_cpu(mpp->reservation_key));
 
 	isFound =0;
 	for (i = 0; i < resp->prin_descriptor.prin_readkeys.additional_length/8; i++ )
@@ -922,7 +913,7 @@ int update_map_pr(struct multipath *mpp)
 		condlog(2, "%s: PR IN READKEYS[%d]  reservation key:", mpp->alias, i);
 		dumpHex((char *)&resp->prin_descriptor.prin_readkeys.key_list[i*8], 8 , 1);
 
-		if (!memcmp(mpp->reservation_key, &resp->prin_descriptor.prin_readkeys.key_list[i*8], 8))
+		if (!memcmp(&mpp->reservation_key, &resp->prin_descriptor.prin_readkeys.key_list[i*8], 8))
 		{
 			condlog(2, "%s: reservation key found in pr in readkeys response", mpp->alias);
 			isFound =1;
diff --git a/libmultipath/byteorder.h b/libmultipath/byteorder.h
new file mode 100644
index 0000000..5ddb661
--- /dev/null
+++ b/libmultipath/byteorder.h
@@ -0,0 +1,36 @@
+#ifndef BYTEORDER_H_INCLUDED
+#define BYTEORDER_H_INCLUDED
+
+#ifdef __linux__
+#  include <endian.h>
+#  include <byteswap.h>
+#else
+#  error unsupported
+#endif
+
+#if BYTE_ORDER == LITTLE_ENDIAN
+#  define le16_to_cpu(x) (x)
+#  define be16_to_cpu(x) bswap_16(x)
+#  define le32_to_cpu(x) (x)
+#  define le64_to_cpu(x) (x)
+#  define be32_to_cpu(x) bswap_32(x)
+#  define be64_to_cpu(x) bswap_64(x)
+#elif BYTE_ORDER == BIG_ENDIAN
+#  define le16_to_cpu(x) bswap_16(x)
+#  define be16_to_cpu(x) (x)
+#  define le32_to_cpu(x) bswap_32(x)
+#  define le64_to_cpu(x) bswap_64(x)
+#  define be32_to_cpu(x) (x)
+#  define be64_to_cpu(x) (x)
+#else
+#  error unsupported
+#endif
+
+#define cpu_to_le16(x) le16_to_cpu(x)
+#define cpu_to_be16(x) be16_to_cpu(x)
+#define cpu_to_le32(x) le32_to_cpu(x)
+#define cpu_to_be32(x) be32_to_cpu(x)
+#define cpu_to_le64(x) le64_to_cpu(x)
+#define cpu_to_be64(x) be64_to_cpu(x)
+
+#endif				/* BYTEORDER_H_INCLUDED */
diff --git a/libmultipath/config.c b/libmultipath/config.c
index b21a3aa..dba4bc4 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -532,9 +532,6 @@ free_config (struct config * conf)
 	if (conf->config_dir)
 		FREE(conf->config_dir);
 
-	if (conf->reservation_key)
-		FREE(conf->reservation_key);
-
 	free_blacklist(conf->blist_devnode);
 	free_blacklist(conf->blist_wwid);
 	free_blacklist(conf->blist_property);
diff --git a/libmultipath/config.h b/libmultipath/config.h
index ffc69b5..c504570 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -4,6 +4,8 @@
 #include <sys/types.h>
 #include <stdint.h>
 #include <urcu.h>
+#include <inttypes.h>
+#include "byteorder.h"
 
 #define ORIGIN_DEFAULT 0
 #define ORIGIN_CONFIG  1
@@ -90,7 +92,7 @@ struct mpentry {
 
 	char * prio_name;
 	char * prio_args;
-	unsigned char * reservation_key;
+	uint64_t  reservation_key; /* stored in big-endian format */
 	int pgpolicy;
 	int pgfailback;
 	int rr_weight;
@@ -183,7 +185,7 @@ struct config {
 	char * alias_prefix;
 	char * partition_delim;
 	char * config_dir;
-	unsigned char * reservation_key;
+	uint64_t  reservation_key; /* stored in big-endian format */
 
 	vector keywords;
 	vector mptable;
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 680b2c5..f37348b 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -962,9 +962,8 @@ snprint_def_log_checker_err (struct config *conf, char * buff, int len, void * d
 static int
 set_reservation_key(vector strvec, void *ptr)
 {
-	unsigned char **uchar_ptr = (unsigned char **)ptr;
+	uint64_t *be64_ptr = (uint64_t *)ptr;
 	char *buff;
-	int j;
 	uint64_t prkey;
 
 	buff = set_value(strvec);
@@ -976,16 +975,7 @@ set_reservation_key(vector strvec, void *ptr)
 		return 1;
 	}
 
-	if (!*uchar_ptr)
-		*uchar_ptr = (unsigned char *) malloc(8);
-
-	memset(*uchar_ptr, 0, 8);
-
-	for (j = 7; j >= 0; --j) {
-		(*uchar_ptr)[j] = (prkey & 0xff);
-		prkey >>= 8;
-	}
-
+	*be64_ptr = cpu_to_be64(prkey);
 	FREE(buff);
 	return 0;
 }
@@ -993,21 +983,8 @@ set_reservation_key(vector strvec, void *ptr)
 int
 print_reservation_key(char * buff, int len, void * ptr)
 {
-	unsigned char **uchar_ptr = (unsigned char **)ptr;
-	int i;
-	unsigned char *keyp;
-	uint64_t prkey = 0;
-
-	if (!*uchar_ptr)
-		return 0;
-	keyp = (unsigned char *)(*uchar_ptr);
-	for (i = 0; i < 8; i++) {
-		if (i > 0)
-			prkey <<= 8;
-		prkey |= *keyp;
-		keyp++;
-	}
-	return snprintf(buff, len, "0x%" PRIx64, prkey);
+	uint64_t *be64_ptr = (uint64_t *)ptr;
+	return snprintf(buff, len, "0x%" PRIx64, be64_to_cpu(*be64_ptr));
 }
 
 declare_def_handler(reservation_key, set_reservation_key)
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 175fbe1..b55b75b 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -609,14 +609,14 @@ out:
 
 int select_reservation_key(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	char *origin, buff[PRKEY_SIZE];
 
 	mp_set_mpe(reservation_key);
 	mp_set_conf(reservation_key);
-	mp->reservation_key = NULL;
+	mp->reservation_key = 0;
 	return 0;
 out:
-	print_reservation_key(buff, 12, &mp->reservation_key);
+	print_reservation_key(buff, PRKEY_SIZE, &mp->reservation_key);
 	condlog(3, "%s: reservation_key = %s %s", mp->alias, buff, origin);
 	return 0;
 }
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 8ea984d..a32fe12 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -2,8 +2,10 @@
 #define _STRUCTS_H
 
 #include <sys/types.h>
+#include <inttypes.h>
 
 #include "prio.h"
+#include "byteorder.h"
 
 #define WWID_SIZE		128
 #define SERIAL_SIZE		65
@@ -17,6 +19,7 @@
 #define NAME_SIZE		512
 #define HOST_NAME_LEN		16
 #define SLOT_NAME_SIZE		40
+#define PRKEY_SIZE		19
 
 #define SCSI_VENDOR_SIZE	9
 #define SCSI_PRODUCT_SIZE	17
@@ -306,7 +309,7 @@ struct multipath {
 	void * mpcontext;
 
 	/* persistent management data*/
-	unsigned char * reservation_key;
+	uint64_t  reservation_key; /* stored in big-endian format */
 	unsigned char prflag;
 };
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 4be2c57..580d67c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2786,10 +2786,8 @@ main (int argc, char *argv[])
 void *  mpath_pr_event_handler_fn (void * pathp )
 {
 	struct multipath * mpp;
-	int i,j, ret, isFound;
+	int i, ret, isFound;
 	struct path * pp = (struct path *)pathp;
-	unsigned char *keyp;
-	uint64_t prkey;
 	struct prout_param_descriptor *param;
 	struct prin_resp *resp;
 
@@ -2817,22 +2815,15 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 		ret = MPATH_PR_SUCCESS;
 		goto out;
 	}
-	prkey = 0;
-	keyp = (unsigned char *)mpp->reservation_key;
-	for (j = 0; j < 8; ++j) {
-		if (j > 0)
-			prkey <<= 8;
-		prkey |= *keyp;
-		++keyp;
-	}
-	condlog(2, "Multipath  reservation_key: 0x%" PRIx64 " ", prkey);
+	condlog(2, "Multipath  reservation_key: 0x%" PRIx64 " ",
+		be64_to_cpu(mpp->reservation_key));
 
 	isFound =0;
 	for (i = 0; i < resp->prin_descriptor.prin_readkeys.additional_length/8; i++ )
 	{
 		condlog(2, "PR IN READKEYS[%d]  reservation key:",i);
 		dumpHex((char *)&resp->prin_descriptor.prin_readkeys.key_list[i*8], 8 , -1);
-		if (!memcmp(mpp->reservation_key, &resp->prin_descriptor.prin_readkeys.key_list[i*8], 8))
+		if (!memcmp(&mpp->reservation_key, &resp->prin_descriptor.prin_readkeys.key_list[i*8], 8))
 		{
 			condlog(2, "%s: pr key found in prin readkeys response", mpp->alias);
 			isFound =1;
@@ -2849,11 +2840,7 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 
 	param= malloc(sizeof(struct prout_param_descriptor));
 	memset(param, 0 , sizeof(struct prout_param_descriptor));
-
-	for (j = 7; j >= 0; --j) {
-		param->sa_key[j] = (prkey & 0xff);
-		prkey >>= 8;
-	}
+	memcpy(param->sa_key, &mpp->reservation_key, 8);
 	param->num_transportid = 0;
 
 	condlog(3, "device %s:%s", pp->dev, pp->mpp->wwid);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] libmpathpersist: fix update_prflag code
  2017-09-08 18:45 [PATCH 0/5] multipath: alternative reservation_key method Benjamin Marzinski
  2017-09-08 18:45 ` [PATCH 1/5] libmultipath: pull functions into util.c Benjamin Marzinski
  2017-09-08 18:45 ` [PATCH 2/5] libmultipath: change reservation_key to a uint64_t Benjamin Marzinski
@ 2017-09-08 18:45 ` Benjamin Marzinski
  2017-09-08 21:17   ` Martin Wilck
  2017-09-08 18:45 ` [PATCH 4/5] multipath: add alternate reservation_key method Benjamin Marzinski
  2017-09-08 18:45 ` [PATCH 5/5] mpathpersist: add support for prkeys file Benjamin Marzinski
  4 siblings, 1 reply; 14+ messages in thread
From: Benjamin Marzinski @ 2017-09-08 18:45 UTC (permalink / raw)
  To: device-mapper development; +Cc: Martin Wilck

There are multiple problems with the prflag code. First, it doesn't do
anything useful at all currently. update_prflags is called with "set"
and "unset" instead of "setprstatus" and "unsetprstatus", so it doesn't
actually enable persistent reservation tracking in multipathd when a key
is registered. Second, the string is to store the multipathd message is
64 bytes long, while just a WWID, which can be used as an alias, can be
128 bytes long, so it's possible to run out of space in the message.
Finally, it is called by mpath_persistent_reserve_out when doing a
preempt and abort, which doesn't make any sense. This disables
multipathd persistent reservation tracking when a node has just taken
over the reservation on a device.

This patch fixes these issues, cleans up the return codes and variable
names, and splits update_prflag into two functions, so that the bulk of
the work (now in do_update_pr), can be reused by other callers.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist.c  |  9 ++++-----
 libmpathpersist/mpath_updatepr.c | 33 +++++++++++++++++----------------
 libmpathpersist/mpathpr.h        |  2 +-
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 960d50d..33843a4 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -371,13 +371,12 @@ int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
 			++keyp;
 		}
 		if (prkey == 0)
-			update_prflag(alias, "unset", noisy);
+			update_prflag(alias, 0);
 		else
-			update_prflag(alias, "set", noisy);
+			update_prflag(alias, 1);
 	} else {
-		if ((ret == MPATH_PR_SUCCESS) && ((rq_servact == MPATH_PROUT_CLEAR_SA) ||
-					(rq_servact == MPATH_PROUT_PREE_AB_SA ))){
-			update_prflag(alias, "unset", noisy);
+		if ((ret == MPATH_PR_SUCCESS) && (rq_servact == MPATH_PROUT_CLEAR_SA)) {
+			update_prflag(alias, 0);
 		}
 	}
 out1:
diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c
index b3701b2..6992879 100644
--- a/libmpathpersist/mpath_updatepr.c
+++ b/libmpathpersist/mpath_updatepr.c
@@ -18,42 +18,43 @@
 #include "mpathpr.h"
 
 
-int update_prflag(char * arg1, char * arg2, int noisy)
+static int do_update_pr(char *alias, char *arg)
 {
 	int fd;
-	char str[64];
+	char str[256];
 	char *reply;
 	int ret = 0;
 
 	fd = mpath_connect();
 	if (fd == -1) {
 		condlog (0, "ux socket connect error");
-		return 1 ;
+		return -1;
 	}
 
-	snprintf(str,sizeof(str),"map %s %s", arg1, arg2);
-	condlog (2, "%s: pr flag message=%s", arg1, str);
+	snprintf(str,sizeof(str),"map %s %s", alias, arg);
+	condlog (2, "%s: pr message=%s", alias, str);
 	if (send_packet(fd, str) != 0) {
-		condlog(2, "%s: message=%s send error=%d", arg1, str, errno);
+		condlog(2, "%s: message=%s send error=%d", alias, str, errno);
 		mpath_disconnect(fd);
-		return -2;
+		return -1;
 	}
 	ret = recv_packet(fd, &reply, DEFAULT_REPLY_TIMEOUT);
 	if (ret < 0) {
-		condlog(2, "%s: message=%s recv error=%d", arg1, str, errno);
-		ret = -2;
+		condlog(2, "%s: message=%s recv error=%d", alias, str, errno);
+		ret = -1;
 	} else {
-		condlog (2, "%s: message=%s reply=%s", arg1, str, reply);
-		if (!reply || strncmp(reply,"ok", 2) == 0)
+		condlog (2, "%s: message=%s reply=%s", alias, str, reply);
+		if (reply && strncmp(reply,"ok", 2) == 0)
+			ret = 0;
+		else
 			ret = -1;
-		else if (strncmp(reply, "fail", 4) == 0)
-			ret = -2;
-		else{
-			ret = atoi(reply);
-		}
 	}
 
 	free(reply);
 	mpath_disconnect(fd);
 	return ret;
 }
+
+int update_prflag(char *mapname, int set) {
+	return do_update_pr(mapname, (set)? "setprstatus" : "unsetprstatus");
+}
diff --git a/libmpathpersist/mpathpr.h b/libmpathpersist/mpathpr.h
index 99e641b..967ba52 100644
--- a/libmpathpersist/mpathpr.h
+++ b/libmpathpersist/mpathpr.h
@@ -45,7 +45,7 @@ int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 int send_prout_activepath(char * dev, int rq_servact, int rq_scope,
 	unsigned int rq_type,   struct prout_param_descriptor * paramp, int noisy);
 
-int update_prflag(char * arg1, char * arg2, int noisy);
+int update_prflag(char *mapname, int set);
 void * mpath_alloc_prin_response(int prin_sa);
 int update_map_pr(struct multipath *mpp);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] multipath: add alternate reservation_key method
  2017-09-08 18:45 [PATCH 0/5] multipath: alternative reservation_key method Benjamin Marzinski
                   ` (2 preceding siblings ...)
  2017-09-08 18:45 ` [PATCH 3/5] libmpathpersist: fix update_prflag code Benjamin Marzinski
@ 2017-09-08 18:45 ` Benjamin Marzinski
  2017-09-08 21:34   ` Martin Wilck
  2017-09-13 15:22   ` Xose Vazquez Perez
  2017-09-08 18:45 ` [PATCH 5/5] mpathpersist: add support for prkeys file Benjamin Marzinski
  4 siblings, 2 replies; 14+ messages in thread
From: Benjamin Marzinski @ 2017-09-08 18:45 UTC (permalink / raw)
  To: device-mapper development; +Cc: Martin Wilck

The scsi persistent reservation API doesn't force devices to implement
any method to display the mapping from a reservation key to an I_T Nexus
(the READ_FULL_STATUS action is an optional later addition, and a number
of devices don't support it). To allow multipathd to determine the
correct reservation key for a device without support from the device
itself, it uses the reservation_key configuration option. Unfortunately,
using this option forces the multipath configuration to be updated
whenever a new scsi registration key is used.  This isn't acceptable to
some users, who want a static configuration file.

This patch provides an alternate method of setting the reservation_key
for the multipath device. The reservation_key configuration option now
also accepts the keyword "file". If this is used, multpath will look in
the new prkeys file (by default "/etc/multipath/prkeys") for a line with
the device wwid and it's associated reservation_key. Where a device's
reservation key comes from is tracked by the prkey_source variable,
which is set and read through the reservation_key dict.c functions.

There are also new multipathd commands to get, set, and unset the
mappings in the prkesy file. Currently,

"multipathd map $map setprkey key $key" sets the mapping
"multipathd map $map unsetprkey" unsets the mapping
"multipathd map $map getprkey" gets the current reservation_key for a
multipath device.

There is some lack of symmetry here where you are allowed to set and
unset mappings even for devices that aren't configured to use the prkeys
file, but you will only get the mapping from the prkeys file for devices
that are configured to use it. Otherwise you will get the mapping from
multipath.conf. In other words, setprkey and unsetprkey return success
but don't do anything useful unless a device is configured with

reservation_key file

but getprkey will return the device's current reservation_key regardless
of where the key came from.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/Makefile     |   2 +-
 libmultipath/config.c     |   6 +-
 libmultipath/config.h     |   3 +
 libmultipath/defaults.h   |   1 +
 libmultipath/dict.c       |  67 +++++++++++++++----
 libmultipath/dict.h       |   2 +-
 libmultipath/prkey.c      | 166 ++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/prkey.h      |  19 ++++++
 libmultipath/propsel.c    |  31 +++++++--
 libmultipath/structs.h    |   7 ++
 multipathd/cli.c          |   8 +++
 multipathd/cli.h          |   8 +++
 multipathd/cli_handlers.c |  82 +++++++++++++++++++++++
 multipathd/cli_handlers.h |   3 +
 multipathd/main.c         |   3 +
 15 files changed, 388 insertions(+), 20 deletions(-)
 create mode 100644 libmultipath/prkey.c
 create mode 100644 libmultipath/prkey.h

diff --git a/libmultipath/Makefile b/libmultipath/Makefile
index b3244fc..928bc25 100644
--- a/libmultipath/Makefile
+++ b/libmultipath/Makefile
@@ -42,7 +42,7 @@ OBJS = memory.o parser.o vector.o devmapper.o callout.o \
 	pgpolicies.o debug.o defaults.o uevent.o time-util.o \
 	switchgroup.o uxsock.o print.o alias.o log_pthread.o \
 	log.o configure.o structs_vec.o sysfs.o prio.o checkers.o \
-	lock.o waiter.o file.o wwids.o prioritizers/alua_rtpg.o
+	lock.o waiter.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o
 
 all: $(LIBS)
 
diff --git a/libmultipath/config.c b/libmultipath/config.c
index dba4bc4..ea2359a 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -515,6 +515,9 @@ free_config (struct config * conf)
 	if (conf->wwids_file)
 		FREE(conf->wwids_file);
 
+	if (conf->prkeys_file)
+		FREE(conf->prkeys_file);
+
 	if (conf->prio_name)
 		FREE(conf->prio_name);
 
@@ -603,6 +606,7 @@ load_config (char * file)
 	get_sys_max_fds(&conf->max_fds);
 	conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE);
 	conf->wwids_file = set_default(DEFAULT_WWIDS_FILE);
+	conf->prkeys_file = set_default(DEFAULT_PRKEYS_FILE);
 	conf->multipath_dir = set_default(DEFAULT_MULTIPATHDIR);
 	conf->attribute_flags = 0;
 	conf->reassign_maps = DEFAULT_REASSIGN_MAPS;
@@ -728,7 +732,7 @@ load_config (char * file)
 		conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE);
 
 	if (!conf->multipath_dir || !conf->bindings_file ||
-	    !conf->wwids_file)
+	    !conf->wwids_file || !conf->prkeys_file)
 		goto out;
 
 	return conf;
diff --git a/libmultipath/config.h b/libmultipath/config.h
index c504570..2a24d80 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -92,6 +92,7 @@ struct mpentry {
 
 	char * prio_name;
 	char * prio_args;
+	int prkey_source;
 	uint64_t  reservation_key; /* stored in big-endian format */
 	int pgpolicy;
 	int pgfailback;
@@ -179,12 +180,14 @@ struct config {
 	char * hwhandler;
 	char * bindings_file;
 	char * wwids_file;
+	char * prkeys_file;
 	char * prio_name;
 	char * prio_args;
 	char * checker_name;
 	char * alias_prefix;
 	char * partition_delim;
 	char * config_dir;
+	int prkey_source;
 	uint64_t  reservation_key; /* stored in big-endian format */
 
 	vector keywords;
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index db2b756..740ccf4 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -50,6 +50,7 @@
 #define DEFAULT_CONFIGFILE	"/etc/multipath.conf"
 #define DEFAULT_BINDINGS_FILE	"/etc/multipath/bindings"
 #define DEFAULT_WWIDS_FILE	"/etc/multipath/wwids"
+#define DEFAULT_PRKEYS_FILE    "/etc/multipath/prkeys"
 #define DEFAULT_CONFIG_DIR	"/etc/multipath/conf.d"
 
 char * set_default (char * str);
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index f37348b..667237a 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -368,6 +368,9 @@ declare_def_snprint(bindings_file, print_str)
 declare_def_handler(wwids_file, set_str)
 declare_def_snprint(wwids_file, print_str)
 
+declare_def_handler(prkeys_file, set_str)
+declare_def_snprint(prkeys_file, print_str)
+
 declare_def_handler(retain_hwhandler, set_yes_no_undef)
 declare_def_snprint_defint(retain_hwhandler, print_yes_no_undef, YNU_NO)
 declare_ovr_handler(retain_hwhandler, set_yes_no_undef)
@@ -960,37 +963,74 @@ snprint_def_log_checker_err (struct config *conf, char * buff, int len, void * d
 }
 
 static int
-set_reservation_key(vector strvec, void *ptr)
+set_reservation_key(vector strvec, uint64_t *key_ptr, int *source_ptr)
 {
-	uint64_t *be64_ptr = (uint64_t *)ptr;
 	char *buff;
-	uint64_t prkey;
 
 	buff = set_value(strvec);
 	if (!buff)
 		return 1;
 
-	if (parse_prkey(buff, &prkey) != 0) {
+	if (strcmp(buff, "file") == 0) {
+		*source_ptr = PRKEY_SOURCE_FILE;
+		*key_ptr = 0;
 		FREE(buff);
-		return 1;
+		return 0;
 	}
 
-	*be64_ptr = cpu_to_be64(prkey);
+	if (parse_prkey(buff, key_ptr) != 0) {
+		FREE(buff);
+		return 1;
+	}
+	*source_ptr = PRKEY_SOURCE_CONF;
+	*key_ptr = cpu_to_be64(*key_ptr);
 	FREE(buff);
 	return 0;
 }
 
 int
-print_reservation_key(char * buff, int len, void * ptr)
+print_reservation_key(char * buff, int len, uint64_t key, int source)
+{
+	if (source == PRKEY_SOURCE_NONE)
+		return 0;
+	if (source == PRKEY_SOURCE_FILE)
+		return snprintf(buff, len, "file");
+	return snprintf(buff, len, "0x%" PRIx64, be64_to_cpu(key));
+}
+
+static int
+def_reservation_key_handler(struct config *conf, vector strvec)
+{
+	return set_reservation_key(strvec, &conf->reservation_key,
+				   &conf->prkey_source);
+}
+
+static int
+snprint_def_reservation_key (struct config *conf, char * buff, int len,
+			     void * data)
+{
+	return print_reservation_key(buff, len, conf->reservation_key,
+				     conf->prkey_source);
+}
+
+static int
+mp_reservation_key_handler(struct config *conf, vector strvec)
 {
-	uint64_t *be64_ptr = (uint64_t *)ptr;
-	return snprintf(buff, len, "0x%" PRIx64, be64_to_cpu(*be64_ptr));
+	struct mpentry * mpe = VECTOR_LAST_SLOT(conf->mptable);
+	if (!mpe)
+		return 1;
+	return set_reservation_key(strvec, &mpe->reservation_key,
+				   &mpe->prkey_source);
 }
 
-declare_def_handler(reservation_key, set_reservation_key)
-declare_def_snprint(reservation_key, print_reservation_key)
-declare_mp_handler(reservation_key, set_reservation_key)
-declare_mp_snprint(reservation_key, print_reservation_key)
+static int
+snprint_mp_reservation_key (struct config *conf, char * buff, int len,
+			     void * data)
+{
+	struct mpentry * mpe = (struct mpentry *)data;
+	return print_reservation_key(buff, len, mpe->reservation_key,
+				     mpe->prkey_source);
+}
 
 static int
 set_off_int_undef(vector strvec, void *ptr)
@@ -1389,6 +1429,7 @@ init_keywords(vector keywords)
 	install_keyword("dev_loss_tmo", &def_dev_loss_handler, &snprint_def_dev_loss);
 	install_keyword("bindings_file", &def_bindings_file_handler, &snprint_def_bindings_file);
 	install_keyword("wwids_file", &def_wwids_file_handler, &snprint_def_wwids_file);
+	install_keyword("prkeys_file", &def_prkeys_file_handler, &snprint_def_prkeys_file);
 	install_keyword("log_checker_err", &def_log_checker_err_handler, &snprint_def_log_checker_err);
 	install_keyword("reservation_key", &def_reservation_key_handler, &snprint_def_reservation_key);
 	install_keyword("retain_attached_hw_handler", &def_retain_hwhandler_handler, &snprint_def_retain_hwhandler);
diff --git a/libmultipath/dict.h b/libmultipath/dict.h
index 2d6097d..a5784da 100644
--- a/libmultipath/dict.h
+++ b/libmultipath/dict.h
@@ -13,6 +13,6 @@ int print_pgpolicy(char * buff, int len, void *ptr);
 int print_no_path_retry(char * buff, int len, void *ptr);
 int print_fast_io_fail(char * buff, int len, void *ptr);
 int print_dev_loss(char * buff, int len, void *ptr);
-int print_reservation_key(char * buff, int len, void * ptr);
+int print_reservation_key(char * buff, int len, uint64_t key, int source);
 int print_off_int_undef(char * buff, int len, void *ptr);
 #endif /* _DICT_H */
diff --git a/libmultipath/prkey.c b/libmultipath/prkey.c
new file mode 100644
index 0000000..c939801
--- /dev/null
+++ b/libmultipath/prkey.c
@@ -0,0 +1,166 @@
+#include "structs.h"
+#include "file.h"
+#include "debug.h"
+#include "config.h"
+#include "util.h"
+#include "propsel.h"
+#include "prkey.h"
+#include <sys/types.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <string.h>
+#include <inttypes.h>
+#include <errno.h>
+
+#define PRKEY_READ 0
+#define PRKEY_WRITE 1
+
+static int do_prkey(int fd, char *wwid, char *keystr, int cmd)
+{
+	char buf[4097];
+	char *ptr;
+	off_t start = 0;
+	int bytes;
+
+	while (1) {
+		if (lseek(fd, start, SEEK_SET) < 0) {
+			condlog(0, "prkey file read lseek failed : %s",
+				strerror(errno));
+			return 1;
+		}
+		bytes = read(fd, buf, 4096);
+		if (bytes < 0) {
+			if (errno == EINTR || errno == EAGAIN)
+				continue;
+			condlog(0, "failed to read from prkey file : %s",
+				strerror(errno));
+			return 1;
+		}
+		if (!bytes) {
+			ptr = NULL;
+			break;
+		}
+		buf[bytes] = '\0';
+		ptr = strstr(buf, wwid);
+		while (ptr) {
+			if (ptr == buf || *(ptr - 1) != ' ' ||
+			    *(ptr + strlen(wwid)) != '\n')
+				ptr = strstr(ptr + strlen(wwid), wwid);
+			else
+				break;
+		}
+		if (ptr) {
+			condlog(3, "found prkey for '%s'", wwid);
+			ptr[strlen(wwid)] = '\0';
+			if (ptr - PRKEY_SIZE < buf ||
+			    (ptr - PRKEY_SIZE != buf &&
+			     *(ptr - PRKEY_SIZE - 1) != '\n')) {
+				condlog(0, "malformed prkey file line for wwid: '%s'", ptr);
+				return 1;
+			}
+			ptr = ptr - PRKEY_SIZE;
+			break;
+		}
+		ptr = strrchr(buf, '\n');
+		if (ptr == NULL) {
+			condlog(4, "couldn't file newline, assuming end of file");
+			break;
+		}
+		start = start + (ptr - buf) + 1;
+	}
+	if (cmd == PRKEY_READ) {
+		if (!ptr || *ptr == '#')
+			return 1;
+		memcpy(keystr, ptr, PRKEY_SIZE - 1);
+		keystr[PRKEY_SIZE - 1] = '\0';
+		return 0;
+	}
+	if (!ptr && !keystr)
+		return 0;
+	if (ptr) {
+		if (lseek(fd, start + (ptr - buf), SEEK_SET) < 0) {
+			condlog(0, "prkey write lseek failed : %s",
+				strerror(errno));
+			return 1;
+		}
+	}
+	if (!keystr) {
+		if (safe_write(fd, "#", 1) < 0) {
+			condlog(0, "failed to write to prkey file : %s",
+				strerror(errno));
+			return 1;
+		}
+		return 0;
+	}
+	if (!ptr) {
+		if (lseek(fd, 0, SEEK_END) < 0) {
+			condlog(0, "prkey write lseek failed : %s",
+				strerror(errno));
+			return 1;
+		}
+	}
+	bytes = sprintf(buf, "%s %s\n", keystr, wwid);
+	if (safe_write(fd, buf, bytes) < 0) {
+		condlog(0, "failed to write to prkey file: %s",
+			strerror(errno));
+		return 1;
+	}
+	return 0;
+}
+
+int get_prkey(struct config *conf, struct multipath *mpp, uint64_t *prkey)
+{
+	int fd;
+	int unused;
+	int ret = 1;
+	char keystr[PRKEY_SIZE];
+
+	if (!strlen(mpp->wwid))
+		goto out;
+
+	fd = open_file(conf->prkeys_file, &unused, PRKEYS_FILE_HEADER);
+	if (fd < 0)
+		goto out;
+	ret = do_prkey(fd, mpp->wwid, keystr, PRKEY_READ);
+	if (ret)
+		goto out_file;
+	ret = !!parse_prkey(keystr, prkey);
+out_file:
+	close(fd);
+out:
+	return ret;
+}
+
+int set_prkey(struct config *conf, struct multipath *mpp, uint64_t prkey)
+{
+	int fd;
+	int can_write = 1;
+	int ret = 1;
+	char keystr[PRKEY_SIZE];
+
+	if (!strlen(mpp->wwid))
+		goto out;
+
+	fd = open_file(conf->prkeys_file, &can_write, PRKEYS_FILE_HEADER);
+	if (fd < 0)
+		goto out;
+	if (!can_write) {
+		condlog(0, "cannot set prkey, prkeys file is read-only");
+		goto out_file;
+	}
+	if (prkey) {
+		snprintf(keystr, PRKEY_SIZE, "0x%016" PRIx64, prkey);
+		keystr[PRKEY_SIZE - 1] = '\0';
+		ret = do_prkey(fd, mpp->wwid, keystr, PRKEY_WRITE);
+	}
+	else
+		ret = do_prkey(fd, mpp->wwid, NULL, PRKEY_WRITE);
+	if (ret == 0)
+		select_reservation_key(conf, mpp);
+	if (be64_to_cpu(mpp->reservation_key) != prkey)
+		ret = 1;
+out_file:
+	close(fd);
+out:
+	return ret;
+}
diff --git a/libmultipath/prkey.h b/libmultipath/prkey.h
new file mode 100644
index 0000000..4028e70
--- /dev/null
+++ b/libmultipath/prkey.h
@@ -0,0 +1,19 @@
+#ifndef _PRKEY_H
+#define _PRKEY_H
+
+#include "structs.h"
+#include <inttypes.h>
+
+#define PRKEYS_FILE_HEADER \
+"# Multipath persistent reservation keys, Version : 1.0\n" \
+"# NOTE: this file is automatically maintained by the multipathd program.\n" \
+"# You should not need to edit this file in normal circumstances.\n" \
+"#\n" \
+"# Format:\n" \
+"# prkey wwid\n" \
+"#\n"
+
+int set_prkey(struct config *conf, struct multipath *mpp, uint64_t prkey);
+int get_prkey(struct config *conf, struct multipath *mpp, uint64_t *prkey);
+
+#endif /* _PRKEY_H */
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index b55b75b..cdd07b8 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -20,6 +20,7 @@
 #include "dict.h"
 #include "util.h"
 #include "prioritizers/alua_rtpg.h"
+#include "prkey.h"
 #include <inttypes.h>
 
 pgpolicyfn *pgpolicies[] = {
@@ -82,6 +83,16 @@ do_attr_set(var, mp->mpe, shift, "(setting: multipath.conf multipaths section)")
 #define set_attr_conf(var, shift)					\
 do_attr_set(var, conf, shift, "(setting: multipath.conf defaults/devices section)")
 
+#define do_prkey_set(src, msg)						\
+do {									\
+	if (src && src->prkey_source != PRKEY_SOURCE_NONE) {		\
+		mp->prkey_source = src->prkey_source;			\
+		mp->reservation_key = src->reservation_key;		\
+		origin = msg;						\
+		goto out;						\
+	}								\
+} while (0)
+
 int select_mode(struct config *conf, struct multipath *mp)
 {
 	char *origin;
@@ -610,14 +621,26 @@ out:
 int select_reservation_key(struct config *conf, struct multipath *mp)
 {
 	char *origin, buff[PRKEY_SIZE];
+	char *from_file = "";
+	uint64_t prkey = 0;
 
-	mp_set_mpe(reservation_key);
-	mp_set_conf(reservation_key);
+	do_prkey_set(mp->mpe, "(setting: multipath.conf multipaths section)");
+	do_prkey_set(conf, "(setting: multipath.conf defaults/devices section)");
 	mp->reservation_key = 0;
+	mp->prkey_source = PRKEY_SOURCE_NONE;
 	return 0;
 out:
-	print_reservation_key(buff, PRKEY_SIZE, &mp->reservation_key);
-	condlog(3, "%s: reservation_key = %s %s", mp->alias, buff, origin);
+	if (mp->prkey_source == PRKEY_SOURCE_FILE) {
+		from_file = " (from prkeys file)";
+		if (get_prkey(conf, mp, &prkey) != 0)
+			mp->reservation_key = 0;
+		else
+			mp->reservation_key = cpu_to_be64(prkey);
+	}
+	print_reservation_key(buff, PRKEY_SIZE, mp->reservation_key,
+			      mp->prkey_source);
+	condlog(3, "%s: reservation_key = %s %s%s", mp->alias, buff, origin,
+		from_file);
 	return 0;
 }
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index a32fe12..649d0e1 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -174,6 +174,12 @@ enum initialized_states {
 	INIT_OK,
 };
 
+enum prkey_sources {
+	PRKEY_SOURCE_NONE,
+	PRKEY_SOURCE_CONF,
+	PRKEY_SOURCE_FILE,
+};
+
 struct sg_id {
 	int host_no;
 	int channel;
@@ -309,6 +315,7 @@ struct multipath {
 	void * mpcontext;
 
 	/* persistent management data*/
+	int prkey_source;
 	uint64_t  reservation_key; /* stored in big-endian format */
 	unsigned char prflag;
 };
diff --git a/multipathd/cli.c b/multipathd/cli.c
index 32d4976..deb72cb 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -208,6 +208,11 @@ load_keys (void)
 	r += add_key(keys, "unsetprstatus", UNSETPRSTATUS, 0);
 	r += add_key(keys, "format", FMT, 1);
 	r += add_key(keys, "json", JSON, 0);
+	r += add_key(keys, "getprkey", GETPRKEY, 0);
+	r += add_key(keys, "setprkey", SETPRKEY, 0);
+	r += add_key(keys, "unsetprkey", UNSETPRKEY, 0);
+	r += add_key(keys, "key", KEY, 1);
+
 
 	if (r) {
 		free_keys(keys);
@@ -571,6 +576,9 @@ cli_init (void) {
 	add_handler(GETPRSTATUS+MAP, NULL);
 	add_handler(SETPRSTATUS+MAP, NULL);
 	add_handler(UNSETPRSTATUS+MAP, NULL);
+	add_handler(GETPRKEY+MAP, NULL);
+	add_handler(SETPRKEY+MAP+KEY, NULL);
+	add_handler(UNSETPRKEY+MAP, NULL);
 	add_handler(FORCEQ+DAEMON, NULL);
 	add_handler(RESTOREQ+DAEMON, NULL);
 
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 92cb41b..d289167 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -37,6 +37,10 @@ enum {
 	__UNSETPRSTATUS,
 	__FMT,
 	__JSON,
+	__GETPRKEY,
+	__SETPRKEY,
+	__UNSETPRKEY,
+	__KEY,
 };
 
 #define LIST		(1 << __LIST)
@@ -76,6 +80,10 @@ enum {
 #define UNSETPRSTATUS	(1ULL << __UNSETPRSTATUS)
 #define FMT		(1ULL << __FMT)
 #define JSON		(1ULL << __JSON)
+#define GETPRKEY	(1ULL << __GETPRKEY)
+#define SETPRKEY	(1ULL << __SETPRKEY)
+#define UNSETPRKEY	(1ULL << __UNSETPRKEY)
+#define KEY		(1ULL << __KEY)
 
 #define INITIAL_REPLY_LEN	1200
 
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index b4a95e3..7229a27 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -21,6 +21,7 @@
 #include <errno.h>
 #include <libudev.h>
 #include "util.h"
+#include "prkey.h"
 
 #include "main.h"
 #include "cli.h"
@@ -1390,3 +1391,84 @@ cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
 
 	return 0;
 }
+
+int
+cli_getprkey(void * v, char ** reply, int * len, void * data)
+{
+	struct multipath * mpp;
+	struct vectors * vecs = (struct vectors *)data;
+	char *mapname = get_keyparam(v, MAP);
+
+	mapname = convert_dev(mapname, 0);
+	condlog(3, "%s: get persistent reservation key (operator)", mapname);
+	mpp = find_mp_by_str(vecs->mpvec, mapname);
+
+	if (!mpp)
+		return 1;
+
+	*reply = malloc(20);
+
+	if (!mpp->reservation_key) {
+		sprintf(*reply, "none\n");
+		*len = strlen(*reply) + 1;
+		return 0;
+	}
+	snprintf(*reply, 20, "0x%" PRIx64 "\n",
+		 be64_to_cpu(mpp->reservation_key));
+	(*reply)[19] = '\0';
+	*len = strlen(*reply) + 1;
+	return 0;
+}
+
+int
+cli_unsetprkey(void * v, char ** reply, int * len, void * data)
+{
+	struct multipath * mpp;
+	struct vectors * vecs = (struct vectors *)data;
+	char *mapname = get_keyparam(v, MAP);
+	int ret;
+	struct config *conf;
+
+	mapname = convert_dev(mapname, 0);
+	condlog(3, "%s: unset persistent reservation key (operator)", mapname);
+	mpp = find_mp_by_str(vecs->mpvec, mapname);
+
+	if (!mpp)
+		return 1;
+
+	conf = get_multipath_config();
+	ret = set_prkey(conf, mpp, 0);
+	put_multipath_config(conf);
+
+	return ret;
+}
+
+int
+cli_setprkey(void * v, char ** reply, int * len, void * data)
+{
+	struct multipath * mpp;
+	struct vectors * vecs = (struct vectors *)data;
+	char *mapname = get_keyparam(v, MAP);
+	char *keyparam = get_keyparam(v, KEY);
+	uint64_t prkey;
+	int ret;
+	struct config *conf;
+
+	mapname = convert_dev(mapname, 0);
+	condlog(3, "%s: set persistent reservation key (operator)", mapname);
+	mpp = find_mp_by_str(vecs->mpvec, mapname);
+
+	if (!mpp)
+		return 1;
+
+	if (parse_prkey(keyparam, &prkey) != 0) {
+		condlog(0, "%s: invalid prkey : '%s'", mapname, keyparam);
+		return 1;
+	}
+
+	conf = get_multipath_config();
+	ret = set_prkey(conf, mpp, prkey);
+	put_multipath_config(conf);
+
+	return ret;
+}
diff --git a/multipathd/cli_handlers.h b/multipathd/cli_handlers.h
index f4d02cc..78a3a43 100644
--- a/multipathd/cli_handlers.h
+++ b/multipathd/cli_handlers.h
@@ -45,3 +45,6 @@ int cli_reassign (void * v, char ** reply, int * len, void * data);
 int cli_getprstatus(void * v, char ** reply, int * len, void * data);
 int cli_setprstatus(void * v, char ** reply, int * len, void * data);
 int cli_unsetprstatus(void * v, char ** reply, int * len, void * data);
+int cli_getprkey(void * v, char ** reply, int * len, void * data);
+int cli_setprkey(void * v, char ** reply, int * len, void * data);
+int cli_unsetprkey(void * v, char ** reply, int * len, void * data);
diff --git a/multipathd/main.c b/multipathd/main.c
index 580d67c..09e31cb 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1249,6 +1249,9 @@ uxlsnrloop (void * ap)
 	set_handler_callback(UNSETPRSTATUS+MAP, cli_unsetprstatus);
 	set_handler_callback(FORCEQ+DAEMON, cli_force_no_daemon_q);
 	set_handler_callback(RESTOREQ+DAEMON, cli_restore_no_daemon_q);
+	set_handler_callback(GETPRKEY+MAP, cli_getprkey);
+	set_handler_callback(SETPRKEY+MAP+KEY, cli_setprkey);
+	set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
 
 	umask(077);
 	uxsock_listen(&uxsock_trigger, ap);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] mpathpersist: add support for prkeys file
  2017-09-08 18:45 [PATCH 0/5] multipath: alternative reservation_key method Benjamin Marzinski
                   ` (3 preceding siblings ...)
  2017-09-08 18:45 ` [PATCH 4/5] multipath: add alternate reservation_key method Benjamin Marzinski
@ 2017-09-08 18:45 ` Benjamin Marzinski
  2017-09-08 21:35   ` Martin Wilck
  4 siblings, 1 reply; 14+ messages in thread
From: Benjamin Marzinski @ 2017-09-08 18:45 UTC (permalink / raw)
  To: device-mapper development; +Cc: Martin Wilck

When the reservation_key is set to "file", the reservation key needs to
be added to the prkeys file for multipathd to be able to register new
paths as they are added to the device.  This patch adds support to the
mpathpersist command to message multipathd when a key is registered or
unregistered to either add or remove the mapping from the prkeys file.

With these changes, once a device is configured to use the prkeys file,
running mpathpersist commands will automatically update the prkeys file
as necessary.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist.c  | 42 +++++++++++++++++++++++++---------------
 libmpathpersist/mpath_updatepr.c | 10 ++++++++++
 libmpathpersist/mpathpr.h        |  1 +
 3 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 33843a4..2ddfe77 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -261,8 +261,6 @@ int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
 	int map_present;
 	int major, minor;
 	int ret;
-	int j;
-	unsigned char *keyp;
 	uint64_t prkey;
 	struct config *conf;
 
@@ -339,6 +337,26 @@ int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
 	select_reservation_key(conf, mpp);
 	put_multipath_config(conf);
 
+	memcpy(&prkey, paramp->sa_key, 8);
+	if (mpp->prkey_source == PRKEY_SOURCE_FILE && prkey &&
+	    ((!mpp->reservation_key && MPATH_PROUT_REG_SA) ||
+	     MPATH_PROUT_REG_IGN_SA)) {
+		memcpy(&mpp->reservation_key, paramp->sa_key, 8);
+		if (update_prkey(alias, be64_to_cpu(mpp->reservation_key))) {
+			condlog(0, "%s: failed to set prkey for multipathd.",
+				alias);
+			ret = MPATH_PR_DMMP_ERROR;
+			goto out1;
+		}
+	}
+
+	if (memcmp(paramp->key, &mpp->reservation_key, 8) &&
+	    memcmp(paramp->sa_key, &mpp->reservation_key, 8)) {
+		condlog(0, "%s: configured reservation key doesn't match: 0x%" PRIx64, alias, be64_to_cpu(mpp->reservation_key));
+		ret = MPATH_PR_SYNTAX_ERROR;
+		goto out1;
+	}
+
 	switch(rq_servact)
 	{
 	case MPATH_PROUT_REG_SA:
@@ -362,22 +380,14 @@ int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
 	if ((ret == MPATH_PR_SUCCESS) && ((rq_servact == MPATH_PROUT_REG_SA) ||
 				(rq_servact ==  MPATH_PROUT_REG_IGN_SA)))
 	{
-		keyp=paramp->sa_key;
-		prkey = 0;
-		for (j = 0; j < 8; ++j) {
-			if (j > 0)
-				prkey <<= 8;
-			prkey |= *keyp;
-			++keyp;
-		}
-		if (prkey == 0)
+		if (prkey == 0) {
 			update_prflag(alias, 0);
-		else
+			update_prkey(alias, 0);
+		} else
 			update_prflag(alias, 1);
-	} else {
-		if ((ret == MPATH_PR_SUCCESS) && (rq_servact == MPATH_PROUT_CLEAR_SA)) {
-			update_prflag(alias, 0);
-		}
+	} else if ((ret == MPATH_PR_SUCCESS) && (rq_servact == MPATH_PROUT_CLEAR_SA)) {
+		update_prflag(alias, 0);
+		update_prkey(alias, 0);
 	}
 out1:
 	free_multipathvec(curmp, KEEP_PATHS);
diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c
index 6992879..8063e90 100644
--- a/libmpathpersist/mpath_updatepr.c
+++ b/libmpathpersist/mpath_updatepr.c
@@ -58,3 +58,13 @@ static int do_update_pr(char *alias, char *arg)
 int update_prflag(char *mapname, int set) {
 	return do_update_pr(mapname, (set)? "setprstatus" : "unsetprstatus");
 }
+
+int update_prkey(char *mapname, uint64_t prkey) {
+	char str[256];
+
+	if (prkey)
+		sprintf(str, "setprkey key %" PRIx64, prkey);
+	else
+		sprintf(str, "unsetprkey");
+	return do_update_pr(mapname, str);
+}
diff --git a/libmpathpersist/mpathpr.h b/libmpathpersist/mpathpr.h
index 967ba52..72feb60 100644
--- a/libmpathpersist/mpathpr.h
+++ b/libmpathpersist/mpathpr.h
@@ -46,6 +46,7 @@ int send_prout_activepath(char * dev, int rq_servact, int rq_scope,
 	unsigned int rq_type,   struct prout_param_descriptor * paramp, int noisy);
 
 int update_prflag(char *mapname, int set);
+int update_prkey(char *mapname, uint64_t prkey);
 void * mpath_alloc_prin_response(int prin_sa);
 int update_map_pr(struct multipath *mpp);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] libmultipath: pull functions into util.c
  2017-09-08 18:45 ` [PATCH 1/5] libmultipath: pull functions into util.c Benjamin Marzinski
@ 2017-09-08 20:51   ` Martin Wilck
  0 siblings, 0 replies; 14+ messages in thread
From: Martin Wilck @ 2017-09-08 20:51 UTC (permalink / raw)
  To: Benjamin Marzinski, device-mapper development

On Fri, 2017-09-08 at 13:45 -0500, Benjamin Marzinski wrote:
> This patch just pulls safe_write out of rbd. and the persistent
> reservation key parsing code out of dict.c and puts them in util.c,
> so that other functions can make use of them.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/checkers/rbd.c | 16 +---------------
>  libmultipath/dict.c         | 26 +++++---------------------
>  libmultipath/util.c         | 33 +++++++++++++++++++++++++++++++++
>  libmultipath/util.h         |  4 ++++
>  4 files changed, 43 insertions(+), 36 deletions(-)
> 
Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] libmultipath: change reservation_key to a uint64_t
  2017-09-08 18:45 ` [PATCH 2/5] libmultipath: change reservation_key to a uint64_t Benjamin Marzinski
@ 2017-09-08 21:09   ` Martin Wilck
  2017-09-08 22:55     ` Benjamin Marzinski
  0 siblings, 1 reply; 14+ messages in thread
From: Martin Wilck @ 2017-09-08 21:09 UTC (permalink / raw)
  To: Benjamin Marzinski, device-mapper development

On Fri, 2017-09-08 at 13:45 -0500, Benjamin Marzinski wrote:
> The reservation key is currently being stored as any array of 8
> unsigned
> chars.  This is exactly the same in-memory representation as a big
> endian 64 bit integer. However, the code for dealing with a big
> endian
> 64 bit integer is much simpler, so switch to use that instead.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist.c | 23 +++++++----------------
>  libmultipath/byteorder.h        | 36
> ++++++++++++++++++++++++++++++++++++
>  libmultipath/config.c           |  3 ---
>  libmultipath/config.h           |  6 ++++--
>  libmultipath/dict.c             | 31 ++++---------------------------
>  libmultipath/propsel.c          |  6 +++---
>  libmultipath/structs.h          |  5 ++++-
>  multipathd/main.c               | 23 +++++------------------
>  8 files changed, 63 insertions(+), 70 deletions(-)
>  create mode 100644 libmultipath/byteorder.h
> 
>  
>  	char * prio_name;
>  	char * prio_args;
> -	unsigned char * reservation_key;
> +	uint64_t  reservation_key; /* stored in big-endian format */

I would prefer if you could encapsulate this into a type that can't be
implicitly converted to an int, such as

struct res_key {
	uint64_t _v;
} reservation_key;

together with macros like

#define reskey_to_uint64(r) be64_to_cpu((r)->_v)
#define set_reskey_from_uint64(r, x) do { (r)->_v = (x); } while (0)

Using be64_to_cpu is all too easily forgotten, which leads to nasty
bugs.

Apart from that, ack.

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] libmpathpersist: fix update_prflag code
  2017-09-08 18:45 ` [PATCH 3/5] libmpathpersist: fix update_prflag code Benjamin Marzinski
@ 2017-09-08 21:17   ` Martin Wilck
  0 siblings, 0 replies; 14+ messages in thread
From: Martin Wilck @ 2017-09-08 21:17 UTC (permalink / raw)
  To: Benjamin Marzinski, device-mapper development

On Fri, 2017-09-08 at 13:45 -0500, Benjamin Marzinski wrote:
> There are multiple problems with the prflag code. First, it doesn't
> do
> anything useful at all currently. update_prflags is called with "set"
> and "unset" instead of "setprstatus" and "unsetprstatus", so it
> doesn't
> actually enable persistent reservation tracking in multipathd when a
> key
> is registered. Second, the string is to store the multipathd message
> is
> 64 bytes long, while just a WWID, which can be used as an alias, can
> be
> 128 bytes long, so it's possible to run out of space in the message.
> Finally, it is called by mpath_persistent_reserve_out when doing a
> preempt and abort, which doesn't make any sense. This disables
> multipathd persistent reservation tracking when a node has just taken
> over the reservation on a device.
> 
> This patch fixes these issues, cleans up the return codes and
> variable
> names, and splits update_prflag into two functions, so that the bulk
> of
> the work (now in do_update_pr), can be reused by other callers.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist.c  |  9 ++++-----
>  libmpathpersist/mpath_updatepr.c | 33 +++++++++++++++++-------------
> ---
>  libmpathpersist/mpathpr.h        |  2 +-
>  3 files changed, 22 insertions(+), 22 deletions(-)
> 

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] multipath: add alternate reservation_key method
  2017-09-08 18:45 ` [PATCH 4/5] multipath: add alternate reservation_key method Benjamin Marzinski
@ 2017-09-08 21:34   ` Martin Wilck
  2017-09-08 23:00     ` Benjamin Marzinski
  2017-09-13 15:22   ` Xose Vazquez Perez
  1 sibling, 1 reply; 14+ messages in thread
From: Martin Wilck @ 2017-09-08 21:34 UTC (permalink / raw)
  To: Benjamin Marzinski, device-mapper development

On Fri, 2017-09-08 at 13:45 -0500, Benjamin Marzinski wrote:
> The scsi persistent reservation API doesn't force devices to
> implement
> any method to display the mapping from a reservation key to an I_T
> Nexus
> (the READ_FULL_STATUS action is an optional later addition, and a
> number
> of devices don't support it). To allow multipathd to determine the
> correct reservation key for a device without support from the device
> itself, it uses the reservation_key configuration option.
> Unfortunately,
> using this option forces the multipath configuration to be updated
> whenever a new scsi registration key is used.  This isn't acceptable
> to
> some users, who want a static configuration file.
> 
> This patch provides an alternate method of setting the
> reservation_key
> for the multipath device. The reservation_key configuration option
> now
> also accepts the keyword "file". If this is used, multpath will look
> in
> the new prkeys file (by default "/etc/multipath/prkeys") for a line
> with
> the device wwid and it's associated reservation_key. Where a device's
> reservation key comes from is tracked by the prkey_source variable,
> which is set and read through the reservation_key dict.c functions.
> 
> There are also new multipathd commands to get, set, and unset the
> mappings in the prkesy file. Currently,
> 
> "multipathd map $map setprkey key $key" sets the mapping
> "multipathd map $map unsetprkey" unsets the mapping
> "multipathd map $map getprkey" gets the current reservation_key for a
> multipath device.
> 
> There is some lack of symmetry here where you are allowed to set and
> unset mappings even for devices that aren't configured to use the
> prkeys
> file, but you will only get the mapping from the prkeys file for
> devices
> that are configured to use it. Otherwise you will get the mapping
> from
> multipath.conf. In other words, setprkey and unsetprkey return
> success
> but don't do anything useful unless a device is configured with
> 
> reservation_key file
> 
> but getprkey will return the device's current reservation_key
> regardless
> of where the key came from.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/Makefile     |   2 +-
>  libmultipath/config.c     |   6 +-
>  libmultipath/config.h     |   3 +
>  libmultipath/defaults.h   |   1 +
>  libmultipath/dict.c       |  67 +++++++++++++++----
>  libmultipath/dict.h       |   2 +-
>  libmultipath/prkey.c      | 166
> ++++++++++++++++++++++++++++++++++++++++++++++
>  libmultipath/prkey.h      |  19 ++++++
>  libmultipath/propsel.c    |  31 +++++++--
>  libmultipath/structs.h    |   7 ++
>  multipathd/cli.c          |   8 +++
>  multipathd/cli.h          |   8 +++
>  multipathd/cli_handlers.c |  82 +++++++++++++++++++++++
>  multipathd/cli_handlers.h |   3 +
>  multipathd/main.c         |   3 +
>  15 files changed, 388 insertions(+), 20 deletions(-)
>  create mode 100644 libmultipath/prkey.c
>  create mode 100644 libmultipath/prkey.h

Looks ok to me. I suppose you tested that parsing code for the key file.
Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] mpathpersist: add support for prkeys file
  2017-09-08 18:45 ` [PATCH 5/5] mpathpersist: add support for prkeys file Benjamin Marzinski
@ 2017-09-08 21:35   ` Martin Wilck
  0 siblings, 0 replies; 14+ messages in thread
From: Martin Wilck @ 2017-09-08 21:35 UTC (permalink / raw)
  To: Benjamin Marzinski, device-mapper development

On Fri, 2017-09-08 at 13:45 -0500, Benjamin Marzinski wrote:
> When the reservation_key is set to "file", the reservation key needs
> to
> be added to the prkeys file for multipathd to be able to register new
> paths as they are added to the device.  This patch adds support to
> the
> mpathpersist command to message multipathd when a key is registered
> or
> unregistered to either add or remove the mapping from the prkeys
> file.
> 
> With these changes, once a device is configured to use the prkeys
> file,
> running mpathpersist commands will automatically update the prkeys
> file
> as necessary.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist.c  | 42 +++++++++++++++++++++++++-----
> ----------
>  libmpathpersist/mpath_updatepr.c | 10 ++++++++++
>  libmpathpersist/mpathpr.h        |  1 +
>  3 files changed, 37 insertions(+), 16 deletions(-)
> 

Reviewed-by: Martin Wilck <mwilck@suse.com>
-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] libmultipath: change reservation_key to a uint64_t
  2017-09-08 21:09   ` Martin Wilck
@ 2017-09-08 22:55     ` Benjamin Marzinski
  0 siblings, 0 replies; 14+ messages in thread
From: Benjamin Marzinski @ 2017-09-08 22:55 UTC (permalink / raw)
  To: Martin Wilck; +Cc: device-mapper development

On Fri, Sep 08, 2017 at 11:09:17PM +0200, Martin Wilck wrote:
> On Fri, 2017-09-08 at 13:45 -0500, Benjamin Marzinski wrote:
> > The reservation key is currently being stored as any array of 8
> > unsigned
> > chars.  This is exactly the same in-memory representation as a big
> > endian 64 bit integer. However, the code for dealing with a big
> > endian
> > 64 bit integer is much simpler, so switch to use that instead.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist.c | 23 +++++++----------------
> >  libmultipath/byteorder.h        | 36
> > ++++++++++++++++++++++++++++++++++++
> >  libmultipath/config.c           |  3 ---
> >  libmultipath/config.h           |  6 ++++--
> >  libmultipath/dict.c             | 31 ++++---------------------------
> >  libmultipath/propsel.c          |  6 +++---
> >  libmultipath/structs.h          |  5 ++++-
> >  multipathd/main.c               | 23 +++++------------------
> >  8 files changed, 63 insertions(+), 70 deletions(-)
> >  create mode 100644 libmultipath/byteorder.h
> > 
> >  
> >  	char * prio_name;
> >  	char * prio_args;
> > -	unsigned char * reservation_key;
> > +	uint64_t  reservation_key; /* stored in big-endian format */
> 
> I would prefer if you could encapsulate this into a type that can't be
> implicitly converted to an int, such as
> 
> struct res_key {
> 	uint64_t _v;
> } reservation_key;
> 
> together with macros like
> 
> #define reskey_to_uint64(r) be64_to_cpu((r)->_v)
> #define set_reskey_from_uint64(r, x) do { (r)->_v = (x); } while (0)
> 
> Using be64_to_cpu is all too easily forgotten, which leads to nasty
> bugs.

That's a good idea. I'll resend with a structure instead of a straight
uint64_t.

-Ben

> 
> Apart from that, ack.
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] multipath: add alternate reservation_key method
  2017-09-08 21:34   ` Martin Wilck
@ 2017-09-08 23:00     ` Benjamin Marzinski
  0 siblings, 0 replies; 14+ messages in thread
From: Benjamin Marzinski @ 2017-09-08 23:00 UTC (permalink / raw)
  To: Martin Wilck; +Cc: device-mapper development

On Fri, Sep 08, 2017 at 11:34:48PM +0200, Martin Wilck wrote:
> On Fri, 2017-09-08 at 13:45 -0500, Benjamin Marzinski wrote:
> > The scsi persistent reservation API doesn't force devices to
> > implement
> > any method to display the mapping from a reservation key to an I_T
> > Nexus
> > (the READ_FULL_STATUS action is an optional later addition, and a
> > number
> > of devices don't support it). To allow multipathd to determine the
> > correct reservation key for a device without support from the device
> > itself, it uses the reservation_key configuration option.
> > Unfortunately,
> > using this option forces the multipath configuration to be updated
> > whenever a new scsi registration key is used.  This isn't acceptable
> > to
> > some users, who want a static configuration file.
> > 
> > This patch provides an alternate method of setting the
> > reservation_key
> > for the multipath device. The reservation_key configuration option
> > now
> > also accepts the keyword "file". If this is used, multpath will look
> > in
> > the new prkeys file (by default "/etc/multipath/prkeys") for a line
> > with
> > the device wwid and it's associated reservation_key. Where a device's
> > reservation key comes from is tracked by the prkey_source variable,
> > which is set and read through the reservation_key dict.c functions.
> > 
> > There are also new multipathd commands to get, set, and unset the
> > mappings in the prkesy file. Currently,
> > 
> > "multipathd map $map setprkey key $key" sets the mapping
> > "multipathd map $map unsetprkey" unsets the mapping
> > "multipathd map $map getprkey" gets the current reservation_key for a
> > multipath device.
> > 
> > There is some lack of symmetry here where you are allowed to set and
> > unset mappings even for devices that aren't configured to use the
> > prkeys
> > file, but you will only get the mapping from the prkeys file for
> > devices
> > that are configured to use it. Otherwise you will get the mapping
> > from
> > multipath.conf. In other words, setprkey and unsetprkey return
> > success
> > but don't do anything useful unless a device is configured with
> > 
> > reservation_key file
> > 
> > but getprkey will return the device's current reservation_key
> > regardless
> > of where the key came from.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/Makefile     |   2 +-
> >  libmultipath/config.c     |   6 +-
> >  libmultipath/config.h     |   3 +
> >  libmultipath/defaults.h   |   1 +
> >  libmultipath/dict.c       |  67 +++++++++++++++----
> >  libmultipath/dict.h       |   2 +-
> >  libmultipath/prkey.c      | 166
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  libmultipath/prkey.h      |  19 ++++++
> >  libmultipath/propsel.c    |  31 +++++++--
> >  libmultipath/structs.h    |   7 ++
> >  multipathd/cli.c          |   8 +++
> >  multipathd/cli.h          |   8 +++
> >  multipathd/cli_handlers.c |  82 +++++++++++++++++++++++
> >  multipathd/cli_handlers.h |   3 +
> >  multipathd/main.c         |   3 +
> >  15 files changed, 388 insertions(+), 20 deletions(-)
> >  create mode 100644 libmultipath/prkey.c
> >  create mode 100644 libmultipath/prkey.h
> 
> Looks ok to me. I suppose you tested that parsing code for the key file.
> Reviewed-by: Martin Wilck <mwilck@suse.com>

Yeah. I tested it, both with and without using the prkeys file.
However, I may write up a unit test script for mpathpersist like
test-kpartx, just to allow automated testing of this stuff.

-Ben

> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] multipath: add alternate reservation_key method
  2017-09-08 18:45 ` [PATCH 4/5] multipath: add alternate reservation_key method Benjamin Marzinski
  2017-09-08 21:34   ` Martin Wilck
@ 2017-09-13 15:22   ` Xose Vazquez Perez
  1 sibling, 0 replies; 14+ messages in thread
From: Xose Vazquez Perez @ 2017-09-13 15:22 UTC (permalink / raw)
  To: Benjamin Marzinski, device-mapper development; +Cc: Martin Wilck

On 09/08/2017 08:45 PM, Benjamin Marzinski wrote:

> There are also new multipathd commands to get, set, and unset the
> mappings in the prkesy file. Currently,
> 
> "multipathd map $map setprkey key $key" sets the mapping
> "multipathd map $map unsetprkey" unsets the mapping
> "multipathd map $map getprkey" gets the current reservation_key for a
> multipath device.


multipathd.8 should be updated with these new commands.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-09-13 15:22 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-08 18:45 [PATCH 0/5] multipath: alternative reservation_key method Benjamin Marzinski
2017-09-08 18:45 ` [PATCH 1/5] libmultipath: pull functions into util.c Benjamin Marzinski
2017-09-08 20:51   ` Martin Wilck
2017-09-08 18:45 ` [PATCH 2/5] libmultipath: change reservation_key to a uint64_t Benjamin Marzinski
2017-09-08 21:09   ` Martin Wilck
2017-09-08 22:55     ` Benjamin Marzinski
2017-09-08 18:45 ` [PATCH 3/5] libmpathpersist: fix update_prflag code Benjamin Marzinski
2017-09-08 21:17   ` Martin Wilck
2017-09-08 18:45 ` [PATCH 4/5] multipath: add alternate reservation_key method Benjamin Marzinski
2017-09-08 21:34   ` Martin Wilck
2017-09-08 23:00     ` Benjamin Marzinski
2017-09-13 15:22   ` Xose Vazquez Perez
2017-09-08 18:45 ` [PATCH 5/5] mpathpersist: add support for prkeys file Benjamin Marzinski
2017-09-08 21:35   ` Martin Wilck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.