[PATCH v2 0/9] speeding up mpathpersist

[PATCH v2 0/9] speeding up mpathpersist

[Martin Wilck]

We've had reports that mpathpersist doesn't scale well in environments
with lots of IO devices. This patch set tries to address this problem
with a few optimizations:

 - instead of doing a full path discovery at startup, only look at
   paths that are part of the maps being worked on
 - allow multiple PR commands in a single run using "batch files",
   without re-discovering everything between PR commands
 - avoid looking at priorities

I've done some basic testing and it seems to work. It remains to be
seen what the effect on performance really is, but the amount of system
calls to be performed for a given set of mpathpersist actions should
be substantially reduced in any case, especially when using batching.

---
v2:
 - avoid memory leak with --batch-file (Ben Marzinski)
 - add documentation to usage() output and mpathpersist man page
 - integrate "mpathpersist.8: fix examples in man page" which I'd sent
   separately before


Martin Wilck (9):
  mpathpersist: call usage() just once on return
  mpathpersist: add option --batch-file (-f)
  mpathpersist: no need to treat error close() as fatal
  libmpathpersist: updatepaths: deal with missing pp->udev
  libmpathpersist: factor out initialization and teardown
  mpathpersist: initialize data structures only once
  libmpathpersist: don't bother with priorities
  mpathpersist.8: fix examples in man page
  mpathpersist.8: add documentation for --batch-file (-f)

 libmpathpersist/mpath_persist.c | 250 +++++++++++++++-----------------
 libmpathpersist/mpath_persist.h |  40 +++++
 mpathpersist/main.c             | 227 ++++++++++++++++++++++-------
 mpathpersist/main.h             |   1 +
 mpathpersist/mpathpersist.8     | 121 ++++++++++++++--
 5 files changed, 440 insertions(+), 199 deletions(-)

-- 
2.21.0

[PATCH v2 1/9] mpathpersist: call usage() just once on return

[Martin Wilck]

This simplifies further changes.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/main.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 10cba452..94e89c13 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -265,7 +265,6 @@ int main (int argc, char * argv[])
 
 			default:
 				fprintf(stderr, "unrecognised switch " "code 0x%x ??\n", c);
-				usage ();
 				ret = MPATH_PR_SYNTAX_ERROR;
 				goto out;
 		}
@@ -283,7 +282,6 @@ int main (int argc, char * argv[])
 		{
 			for (; optind < argc; ++optind)
 				fprintf (stderr, "Unexpected extra argument: %s\n", argv[optind]);
-			usage ();
 			ret = MPATH_PR_SYNTAX_ERROR;
 			goto out;
 		}
@@ -296,14 +294,12 @@ int main (int argc, char * argv[])
 	if ((prout_flag + prin_flag) == 0)
 	{
 		fprintf (stderr, "choose either '--in' or '--out' \n");
-		usage ();
 		ret = MPATH_PR_SYNTAX_ERROR;
 		goto out;
 	}
 	if ((prout_flag + prin_flag) > 1)
 	{
 		fprintf (stderr, "choose either '--in' or '--out' \n");
-		usage ();
 		ret = MPATH_PR_SYNTAX_ERROR;
 		goto out;
 	}
@@ -334,20 +330,17 @@ int main (int argc, char * argv[])
 		{
 			fprintf (stderr,
 					" No service action given for Persistent Reserve IN\n");
-			usage();
 			ret = MPATH_PR_SYNTAX_ERROR;
 		}
 		else if (num_prin_sa > 1)
 		{
 			fprintf (stderr, " Too many service actions given; choose "
 					"one only\n");
-			usage();
 			ret = MPATH_PR_SYNTAX_ERROR;
 		}
 	}
 	else
 	{
-		usage ();
 		ret = MPATH_PR_SYNTAX_ERROR;
 		goto out;
 	}
@@ -356,7 +349,6 @@ int main (int argc, char * argv[])
 	{
 		fprintf (stderr, " --relative-target-port"
 				" only useful with --register-move\n");
-		usage ();
 		ret = MPATH_PR_SYNTAX_ERROR;
 		goto out;
 	}
@@ -378,7 +370,6 @@ int main (int argc, char * argv[])
 	if (device_name == NULL)
 	{
 		fprintf (stderr, "No device name given \n");
-		usage ();
 		ret = MPATH_PR_SYNTAX_ERROR;
 		goto out;
 	}
@@ -496,6 +487,8 @@ int main (int argc, char * argv[])
 	}
 
 out :
+	if (ret == MPATH_PR_SYNTAX_ERROR)
+		usage();
 	mpath_lib_exit(conf);
 	udev_unref(udev);
 	return (ret >= 0) ? ret : MPATH_PR_OTHER;
-- 
2.21.0

[PATCH v2 2/9] mpathpersist: add option --batch-file (-f)

[Martin Wilck]

Add the option --batch-file (-f) to mpathpersist. The option argument
is a text file that is parsed line-by-line. Every line of the file is
interpreted as an additional input line for mpathpersist. The initial
"mpathpersist" on each line is optional. The '#' character denotes
a comment. '#' is only recognized after whitespace. Empty lines,
or comment lines, are allowed.

If -f is given, other command line options are parsed as usual and
commands (if any) are run before running the batch file. Inside the
batch file, the option -f is forbidden, and -v is ignored. If a command
fails, the batch processing is not aborted. The return status of
mpathpersist is 0 if all commands succeeded, and the status of the
first failed command otherwise.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/main.c | 204 ++++++++++++++++++++++++++++++++++++--------
 mpathpersist/main.h |   1 +
 2 files changed, 168 insertions(+), 37 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 94e89c13..2e19f29f 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -15,6 +15,7 @@
 #include <pthread.h>
 #include <ctype.h>
 #include <string.h>
+#include <errno.h>
 
 static const char * pr_type_strs[] = {
 	"obsolete [0]",
@@ -59,7 +60,99 @@ void rcu_unregister_thread_memb(void) {}
 
 struct udev *udev;
 
-int main (int argc, char * argv[])
+static int verbose, loglevel, noisy;
+
+static int handle_args(int argc, char * argv[], int line);
+
+static int do_batch_file(const char *batch_fn)
+{
+	char command[] = "mpathpersist";
+	const int ARGV_CHUNK = 2;
+	const char delims[] = " \t\n";
+	size_t len = 0;
+	char *line = NULL;
+	ssize_t n;
+	int nline = 0;
+	int argl = ARGV_CHUNK;
+	FILE *fl;
+	char **argv = calloc(argl, sizeof(*argv));
+	int ret = MPATH_PR_SUCCESS;
+
+	if (argv == NULL)
+		return MPATH_PR_OTHER;
+
+	fl = fopen(batch_fn, "r");
+	if (fl == NULL) {
+		fprintf(stderr, "unable to open %s: %s\n",
+			batch_fn, strerror(errno));
+		free(argv);
+		return MPATH_PR_SYNTAX_ERROR;
+	} else {
+		if (verbose >= 2)
+			fprintf(stderr, "running batch file %s\n",
+				batch_fn);
+	}
+
+	while ((n = getline(&line, &len, fl)) != -1) {
+		char *_token, *token;
+		int argc = 0;
+		int rv;
+
+		nline++;
+		argv[argc++] = command;
+
+		if (line[n-1] == '\n')
+			line[n-1] = '\0';
+		if (verbose >= 3)
+			fprintf(stderr, "processing line %d: %s\n",
+				nline, line);
+
+		for (token = strtok_r(line, delims, &_token);
+		     token != NULL && *token != '#';
+		     token = strtok_r(NULL, delims, &_token)) {
+
+			if (argc >= argl) {
+				int argn = argl + ARGV_CHUNK;
+				char **tmp;
+
+				tmp = realloc(argv, argn * sizeof(*argv));
+				if (tmp == NULL)
+					break;
+				argv = tmp;
+				argl = argn;
+			}
+
+			if (argc == 1 && !strcmp(token, command))
+				continue;
+
+			argv[argc++] = token;
+		}
+
+		if (argc <= 1)
+			continue;
+
+		if (verbose >= 2) {
+			int i;
+
+			fprintf(stderr, "## file %s line %d:", batch_fn, nline);
+			for (i = 0; i < argc; i++)
+				fprintf(stderr, " %s", argv[i]);
+			fprintf(stderr, "\n");
+		}
+
+		optind = 0;
+		rv = handle_args(argc, argv, nline);
+		if (rv != MPATH_PR_SUCCESS)
+			ret = rv;
+	}
+
+	fclose(fl);
+	free(argv);
+	free(line);
+	return ret;
+}
+
+static int handle_args(int argc, char * argv[], int nline)
 {
 	int fd, c, res;
 	const char *device_name = NULL;
@@ -82,51 +175,42 @@ int main (int argc, char * argv[])
 	int prin = 1;
 	int prin_sa = -1;
 	int prout_sa = -1;
-	int verbose = 0;
-	int loglevel = 0;
-	int noisy = 0;
 	int num_transport =0;
+	char *batch_fn = NULL;
 	void *resp = NULL;
 	struct transportid * tmp;
-	struct config *conf;
-
-	if (optind == argc)
-	{
-
-		fprintf (stderr, "No parameter used\n");
-		usage ();
-		exit (1);
-	}
-
-	if (getuid () != 0)
-	{
-		fprintf (stderr, "need to be root\n");
-		exit (1);
-	}
-
-	udev = udev_new();
-	conf = mpath_lib_init();
-	if(!conf) {
-		udev_unref(udev);
-		exit(1);
-	}
+	struct config *conf = multipath_conf;
 
 	memset(transportids, 0, MPATH_MX_TIDS * sizeof(struct transportid));
-	multipath_conf = conf;
 
 	while (1)
 	{
 		int option_index = 0;
 
-		c = getopt_long (argc, argv, "v:Cd:hHioYZK:S:PAT:skrGILcRX:l:",
+		c = getopt_long (argc, argv, "v:Cd:hHioYZK:S:PAT:skrGILcRX:l:f:",
 				long_options, &option_index);
 		if (c == -1)
 			break;
 
 		switch (c)
 		{
+			case 'f':
+				if (nline != 0) {
+					fprintf(stderr,
+						"ERROR: -f option not allowed in batch file\n");
+					ret = MPATH_PR_SYNTAX_ERROR;
+					goto out;
+				}
+				if (batch_fn != NULL) {
+					fprintf(stderr,
+						"ERROR: -f option can be used at most once\n");
+					ret = MPATH_PR_SYNTAX_ERROR;
+					goto out;
+				}
+				batch_fn = strdup(optarg);
+				break;
 			case 'v':
-				if (1 != sscanf (optarg, "%d", &loglevel))
+				if (nline == 0 && 1 != sscanf (optarg, "%d", &loglevel))
 				{
 					fprintf (stderr, "bad argument to '--verbose'\n");
 					return MPATH_PR_SYNTAX_ERROR;
@@ -287,11 +371,13 @@ int main (int argc, char * argv[])
 		}
 	}
 
-	/* set verbosity */
-	noisy = (loglevel >= 3) ? 1 : hex;
-	verbose	= (loglevel >= 3)? 3: loglevel;
+	if (nline == 0) {
+		/* set verbosity */
+		noisy = (loglevel >= 3) ? 1 : hex;
+		verbose	= (loglevel >= 3)? 3: loglevel;
+	}
 
-	if ((prout_flag + prin_flag) == 0)
+	if ((prout_flag + prin_flag) == 0 && batch_fn == NULL)
 	{
 		fprintf (stderr, "choose either '--in' or '--out' \n");
 		ret = MPATH_PR_SYNTAX_ERROR;
@@ -341,7 +427,8 @@ int main (int argc, char * argv[])
 	}
 	else
 	{
-		ret = MPATH_PR_SYNTAX_ERROR;
+		if (batch_fn == NULL)
+			ret = MPATH_PR_SYNTAX_ERROR;
 		goto out;
 	}
 
@@ -487,10 +574,52 @@ int main (int argc, char * argv[])
 	}
 
 out :
-	if (ret == MPATH_PR_SYNTAX_ERROR)
-		usage();
-	mpath_lib_exit(conf);
+	if (ret == MPATH_PR_SYNTAX_ERROR) {
+		free(batch_fn);
+		if (nline == 0)
+			usage();
+		else
+			fprintf(stderr, "syntax error on line %d in batch file\n",
+				nline);
+	} else if (batch_fn != NULL) {
+		int rv = do_batch_file(batch_fn);
+
+		free(batch_fn);
+		ret = ret == 0 ? rv : ret;
+	}
+	return (ret >= 0) ? ret : MPATH_PR_OTHER;
+}
+
+int main(int argc, char *argv[])
+{
+	int ret;
+
+	if (optind == argc)
+	{
+
+		fprintf (stderr, "No parameter used\n");
+		usage ();
+		exit (1);
+	}
+
+	if (getuid () != 0)
+	{
+		fprintf (stderr, "need to be root\n");
+		exit (1);
+	}
+
+	udev = udev_new();
+	multipath_conf = mpath_lib_init();
+	if(!multipath_conf) {
+		udev_unref(udev);
+		exit(1);
+	}
+
+	ret = handle_args(argc, argv, 0);
+
+	mpath_lib_exit(multipath_conf);
 	udev_unref(udev);
+
 	return (ret >= 0) ? ret : MPATH_PR_OTHER;
 }
 
@@ -691,6 +820,7 @@ static void usage(void)
 			"                   4           Informational messages with trace enabled\n"
 			"    --clear|-C                 PR Out: Clear\n"
 			"    --device=DEVICE|-d DEVICE  query or change DEVICE\n"
+			"    --batch-file|-f FILE       run commands from FILE\n"
 			"    --help|-h                  output this usage message\n"
 			"    --hex|-H                   output response in hex\n"
 			"    --in|-i                    request PR In command \n"
diff --git a/mpathpersist/main.h b/mpathpersist/main.h
index beb8a21b..bfbb82e2 100644
--- a/mpathpersist/main.h
+++ b/mpathpersist/main.h
@@ -2,6 +2,7 @@ static struct option long_options[] = {
 	{"verbose", 1, NULL, 'v'},
 	{"clear", 0, NULL, 'C'},
 	{"device", 1, NULL, 'd'},
+	{"batch-file", 1, NULL, 'f' },
 	{"help", 0, NULL, 'h'},
 	{"hex", 0, NULL, 'H'},
 	{"in", 0, NULL, 'i'},
-- 
2.21.0

[PATCH v2 3/9] mpathpersist: no need to treat error close() as fatal

[Martin Wilck]

Simplify code a bit.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/main.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 2e19f29f..6df9762d 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -154,7 +154,7 @@ static int do_batch_file(const char *batch_fn)
 
 static int handle_args(int argc, char * argv[], int nline)
 {
-	int fd, c, res;
+	int fd, c;
 	const char *device_name = NULL;
 	int num_prin_sa = 0;
 	int num_prout_sa = 0;
@@ -179,7 +179,6 @@ static int handle_args(int argc, char * argv[], int nline)
 	char *batch_fn = NULL;
 	void *resp = NULL;
 	struct transportid * tmp;
-	struct config *conf = multipath_conf;
 
 	memset(transportids, 0, MPATH_MX_TIDS * sizeof(struct transportid));
 
@@ -565,13 +564,7 @@ static int handle_args(int argc, char * argv[], int nline)
 		printf("PR out: command failed\n");
 	}
 
-	res = close (fd);
-	if (res < 0)
-	{
-		mpath_lib_exit(conf);
-		udev_unref(udev);
-		return MPATH_PR_FILE_ERROR;
-	}
+	close (fd);
 
 out :
 	if (ret == MPATH_PR_SYNTAX_ERROR) {
-- 
2.21.0

[PATCH v2 4/9] libmpathpersist: updatepaths: deal with missing pp->udev

[Martin Wilck]

We will change the data structure initialization to a lazy
approach, where pp->udev isn't necessarily initialized
when get_mpvec() is called. Deal with it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 6505774f..fee1db72 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -16,6 +16,7 @@
 #include "config.h"
 #include "switchgroup.h"
 #include "discovery.h"
+#include "configure.h"
 #include "dmparser.h"
 #include <ctype.h>
 #include "propsel.h"
@@ -96,6 +97,17 @@ updatepaths (struct multipath * mpp)
 				continue;
 			}
 			pp->mpp = mpp;
+			if (pp->udev == NULL) {
+				pp->udev = get_udev_device(pp->dev_t, DEV_DEVT);
+				if (pp->udev == NULL) {
+					pp->state = PATH_DOWN;
+					continue;
+				}
+				conf = get_multipath_config();
+				pathinfo(pp, conf, DI_SYSFS|DI_CHECKER);
+				put_multipath_config(conf);
+				continue;
+			}
 			if (pp->state == PATH_UNCHECKED ||
 					pp->state == PATH_WILD) {
 				conf = get_multipath_config();
-- 
2.21.0

[PATCH v2 5/9] libmpathpersist: factor out initialization and teardown

[Martin Wilck]

mpath_presistent_reserve_{in,out} share a lot of common code
for initial data structure initialization (discovery) and teardown.
Factor this code out into mpath_persistent_reserve_init_vecs()
(global data structure initialization),
mpath_persistent_reserve_free_vecs (global teardown) and mpath_get_map()
(struct multipath setup for given map device).

Provide __mpath_presistent_reserve_{in,out}, which are the same
as their counterparts without leading underscores, but do not
call the global setup and teardown routines. This allows running
several PR commands in a row without having to re-initialize in
between. Because libmpathpersist is a public API, the previously
known functions don't change behavior.

Don't call path_discovery() any more during global initialization.
We rather do this lazily in the get_mpvec() call chain. dm_get_maps(),
OTOH, is part of the global initialization procedure. In get_mpvec(),
we don't delete non-matching maps any more, because we way want to
act on them later on.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c | 231 ++++++++++++++------------------
 libmpathpersist/mpath_persist.h |  40 ++++++
 2 files changed, 142 insertions(+), 129 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index fee1db72..ce72da67 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -164,48 +164,47 @@ mpath_prin_activepath (struct multipath *mpp, int rq_servact,
 int mpath_persistent_reserve_in (int fd, int rq_servact,
 	struct prin_resp *resp, int noisy, int verbose)
 {
-	struct stat info;
-	vector curmp = NULL;
-	vector pathvec = NULL;
-	char * alias;
-	struct multipath * mpp;
-	int map_present;
-	int major, minor;
-	int ret;
-	struct config *conf;
+	int ret = mpath_persistent_reserve_init_vecs(verbose);
 
-	conf = get_multipath_config();
-	conf->verbosity = verbose;
-	put_multipath_config(conf);
+	if (ret != MPATH_PR_SUCCESS)
+		return ret;
+	ret = __mpath_persistent_reserve_in(fd, rq_servact, resp, noisy);
+	mpath_persistent_reserve_free_vecs();
+	return ret;
+}
 
-	if (fstat( fd, &info) != 0){
-		condlog(0, "stat error %d", fd);
-		return MPATH_PR_FILE_ERROR;
-	}
-	if(!S_ISBLK(info.st_mode)){
-		condlog(0, "Failed to get major:minor. fd = %d", fd);
-		return MPATH_PR_FILE_ERROR;
-	}
+int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
+	unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy, int verbose)
+{
+	int ret = mpath_persistent_reserve_init_vecs(verbose);
 
-	major = major(info.st_rdev);
-	minor = minor(info.st_rdev);
-	condlog(4, "Device %d:%d:  ", major, minor);
+	if (ret != MPATH_PR_SUCCESS)
+		return ret;
+	ret = __mpath_persistent_reserve_out(fd, rq_servact, rq_scope, rq_type,
+					     paramp, noisy);
+	mpath_persistent_reserve_free_vecs();
+	return ret;
+}
 
-	/* get alias from major:minor*/
-	alias = dm_mapname(major, minor);
-	if (!alias){
-		condlog(0, "%d:%d failed to get device alias.", major, minor);
-		return MPATH_PR_DMMP_ERROR;
-	}
+static vector curmp;
+static vector pathvec;
 
-	condlog(3, "alias = %s", alias);
-	map_present = dm_map_present(alias);
-	if (map_present && dm_is_mpath(alias) != 1){
-		condlog( 0, "%s: not a multipath device.", alias);
-		ret = MPATH_PR_DMMP_ERROR;
-		goto out;
-	}
+void mpath_persistent_reserve_free_vecs(void)
+{
+	free_multipathvec(curmp, KEEP_PATHS);
+	free_pathvec(pathvec, FREE_PATHS);
+	curmp = pathvec = NULL;
+}
+
+int mpath_persistent_reserve_init_vecs(int verbose)
+{
+	struct config *conf = get_multipath_config();
 
+	conf->verbosity = verbose;
+	put_multipath_config(conf);
+
+	if (curmp)
+		return MPATH_PR_SUCCESS;
 	/*
 	 * allocate core vectors to store paths and multipaths
 	 */
@@ -213,70 +212,32 @@ int mpath_persistent_reserve_in (int fd, int rq_servact,
 	pathvec = vector_alloc ();
 
 	if (!curmp || !pathvec){
-		condlog (0, "%s: vector allocation failed.", alias);
-		ret = MPATH_PR_DMMP_ERROR;
-		if (curmp)
-			vector_free(curmp);
-		if (pathvec)
-			vector_free(pathvec);
-		goto out;
-	}
-
-	if (path_discovery(pathvec, DI_SYSFS | DI_CHECKER) < 0) {
-		ret = MPATH_PR_DMMP_ERROR;
-		goto out1;
+		condlog (0, "vector allocation failed.");
+		goto err;
 	}
 
-	/* get info of all paths from the dm device	*/
-	if (get_mpvec (curmp, pathvec, alias)){
-		condlog(0, "%s: failed to get device info.", alias);
-		ret = MPATH_PR_DMMP_ERROR;
-		goto out1;
-	}
+	if (dm_get_maps(curmp))
+		goto err;
 
-	mpp = find_mp_by_alias(curmp, alias);
-	if (!mpp){
-		condlog(0, "%s: devmap not registered.", alias);
-		ret = MPATH_PR_DMMP_ERROR;
-		goto out1;
-	}
-
-	ret = mpath_prin_activepath(mpp, rq_servact, resp, noisy);
+	return MPATH_PR_SUCCESS;
 
-out1:
-	free_multipathvec(curmp, KEEP_PATHS);
-	free_pathvec(pathvec, FREE_PATHS);
-out:
-	FREE(alias);
-	return ret;
+err:
+	mpath_persistent_reserve_free_vecs();
+	return MPATH_PR_DMMP_ERROR;
 }
 
-int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
-	unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy, int verbose)
+static int mpath_get_map(int fd, char **palias, struct multipath **pmpp)
 {
-
+	int ret = MPATH_PR_DMMP_ERROR;
 	struct stat info;
-
-	vector curmp = NULL;
-	vector pathvec = NULL;
-
-	char * alias;
-	struct multipath * mpp;
-	int map_present;
 	int major, minor;
-	int ret;
-	uint64_t prkey;
-	struct config *conf;
-
-	conf = get_multipath_config();
-	conf->verbosity = verbose;
-	put_multipath_config(conf);
+	char *alias;
+	struct multipath *mpp;
 
-	if (fstat( fd, &info) != 0){
+	if (fstat(fd, &info) != 0){
 		condlog(0, "stat error fd=%d", fd);
 		return MPATH_PR_FILE_ERROR;
 	}
-
 	if(!S_ISBLK(info.st_mode)){
 		condlog(3, "Failed to get major:minor. fd=%d", fd);
 		return MPATH_PR_FILE_ERROR;
@@ -286,57 +247,73 @@ int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
 	minor = minor(info.st_rdev);
 	condlog(4, "Device  %d:%d", major, minor);
 
-	/* get WWN of the device from major:minor*/
+	/* get alias from major:minor*/
 	alias = dm_mapname(major, minor);
 	if (!alias){
+		condlog(0, "%d:%d failed to get device alias.", major, minor);
 		return MPATH_PR_DMMP_ERROR;
 	}
 
 	condlog(3, "alias = %s", alias);
-	map_present = dm_map_present(alias);
 
-	if (map_present && dm_is_mpath(alias) != 1){
+	if (dm_map_present(alias) && dm_is_mpath(alias) != 1){
 		condlog(3, "%s: not a multipath device.", alias);
-		ret = MPATH_PR_DMMP_ERROR;
-		goto out;
-	}
-
-	/*
-	 * allocate core vectors to store paths and multipaths
-	 */
-	curmp = vector_alloc ();
-	pathvec = vector_alloc ();
-
-	if (!curmp || !pathvec){
-		condlog (0, "%s: vector allocation failed.", alias);
-		ret = MPATH_PR_DMMP_ERROR;
-		if (curmp)
-			vector_free(curmp);
-		if (pathvec)
-			vector_free(pathvec);
 		goto out;
 	}
 
-	if (path_discovery(pathvec, DI_SYSFS | DI_CHECKER) < 0) {
-		ret = MPATH_PR_DMMP_ERROR;
-		goto out1;
-	}
-
 	/* get info of all paths from the dm device     */
 	if (get_mpvec(curmp, pathvec, alias)){
 		condlog(0, "%s: failed to get device info.", alias);
-		ret = MPATH_PR_DMMP_ERROR;
-		goto out1;
+		goto out;
 	}
 
 	mpp = find_mp_by_alias(curmp, alias);
 
 	if (!mpp) {
 		condlog(0, "%s: devmap not registered.", alias);
-		ret = MPATH_PR_DMMP_ERROR;
-		goto out1;
+		goto out;
 	}
 
+	ret = MPATH_PR_SUCCESS;
+	if (pmpp)
+		*pmpp = mpp;
+	if (palias) {
+		*palias = alias;
+		alias = NULL;
+	}
+out:
+	FREE(alias);
+	return ret;
+}
+
+int __mpath_persistent_reserve_in (int fd, int rq_servact,
+	struct prin_resp *resp, int noisy)
+{
+	struct multipath *mpp;
+	int ret;
+
+	ret = mpath_get_map(fd, NULL, &mpp);
+	if (ret != MPATH_PR_SUCCESS)
+		return ret;
+
+	ret = mpath_prin_activepath(mpp, rq_servact, resp, noisy);
+
+	return ret;
+}
+
+int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
+	unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy)
+{
+	struct multipath *mpp;
+	char *alias;
+	int ret;
+	uint64_t prkey;
+	struct config *conf;
+
+	ret = mpath_get_map(fd, &alias, &mpp);
+	if (ret != MPATH_PR_SUCCESS)
+		return ret;
+
 	conf = get_multipath_config();
 	select_reservation_key(conf, mpp);
 	select_all_tg_pt(conf, mpp);
@@ -397,10 +374,6 @@ int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
 		update_prkey(alias, 0);
 	}
 out1:
-	free_multipathvec(curmp, KEEP_PATHS);
-	free_pathvec(pathvec, FREE_PATHS);
-
-out:
 	FREE(alias);
 	return ret;
 }
@@ -412,22 +385,22 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
 	struct multipath *mpp;
 	char params[PARAMS_SIZE], status[PARAMS_SIZE];
 
-	if (dm_get_maps (curmp)){
-		return 1;
-	}
-
 	vector_foreach_slot (curmp, mpp, i){
 		/*
 		 * discard out of scope maps
 		 */
-		if (mpp->alias && refwwid &&
-		    strncmp (mpp->alias, refwwid, WWID_SIZE - 1)){
-			free_multipath (mpp, KEEP_PATHS);
-			vector_del_slot (curmp, i);
-			i--;
+		if (!mpp->alias) {
+			condlog(0, "%s: map with empty alias!", __func__);
 			continue;
 		}
 
+		if (mpp->pg != NULL)
+			/* Already seen this one */
+			continue;
+
+		if (refwwid && strncmp (mpp->alias, refwwid, WWID_SIZE - 1))
+			continue;
+
 		dm_get_map(mpp->alias, &mpp->size, params);
 		condlog(3, "params = %s", params);
 		dm_get_status(mpp->alias, status);
diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
index 9a84bc9c..7cf4faf9 100644
--- a/libmpathpersist/mpath_persist.h
+++ b/libmpathpersist/mpath_persist.h
@@ -213,6 +213,15 @@ extern int mpath_lib_exit (struct config *conf);
 extern int mpath_persistent_reserve_in (int fd, int rq_servact, struct prin_resp *resp,
 		int noisy, int verbose);
 
+/*
+ * DESCRIPTION :
+ * This function is like mpath_persistent_reserve_in(), except that it doesn't call
+ * mpath_persistent_reserve_init_vecs() and mpath_persistent_reserve_free_vecs()
+ * before and after the actual PR call.
+ */
+extern int __mpath_persistent_reserve_in(int fd, int rq_servact,
+		struct prin_resp *resp, int noisy);
+
 /*
  * DESCRIPTION :
  * This function sends PROUT command to the DM device and get the response.
@@ -238,6 +247,37 @@ extern int mpath_persistent_reserve_in (int fd, int rq_servact, struct prin_resp
 extern int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,
 		unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy,
 		int verbose);
+/*
+ * DESCRIPTION :
+ * This function is like mpath_persistent_reserve_out(), except that it doesn't call
+ * mpath_persistent_reserve_init_vecs() and mpath_persistent_reserve_free_vecs()
+ * before and after the actual PR call.
+ */
+extern int __mpath_persistent_reserve_out( int fd, int rq_servact, int rq_scope,
+		unsigned int rq_type, struct prout_param_descriptor *paramp,
+		int noisy);
+
+/*
+ * DESCRIPTION :
+ * This function allocates data structures and performs basic initialization and
+ * device discovery for later calls of __mpath_persistent_reserve_in() or
+ * __mpath_persistent_reserve_out().
+ * @verbose: Set verbosity level. Input argument. value:0 to 3. 0->disabled, 3->Max verbose
+ *
+ * RESTRICTIONS:
+ *
+ * RETURNS: MPATH_PR_SUCCESS if successful else returns any of the status specified
+ *       above in RETURN_STATUS.
+ */
+int mpath_persistent_reserve_init_vecs(int verbose);
+
+/*
+ * DESCRIPTION :
+ * This function frees data structures allocated by
+ * mpath_persistent_reserve_init_vecs().
+ */
+void mpath_persistent_reserve_free_vecs(void);
+
 
 #ifdef __cplusplus
 }
-- 
2.21.0

[PATCH v2 6/9] mpathpersist: initialize data structures only once

[Martin Wilck]

We now have the possibility to run several PR commands in a single
mpathpersist invocation. Run initialization / discovery and teardown
only once at program invocation / exit.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 6df9762d..cbb59623 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -374,6 +374,9 @@ static int handle_args(int argc, char * argv[], int nline)
 		/* set verbosity */
 		noisy = (loglevel >= 3) ? 1 : hex;
 		verbose	= (loglevel >= 3)? 3: loglevel;
+		ret = mpath_persistent_reserve_init_vecs(verbose);
+		if (ret != MPATH_PR_SUCCESS)
+			goto out;
 	}
 
 	if ((prout_flag + prin_flag) == 0 && batch_fn == NULL)
@@ -480,7 +483,7 @@ static int handle_args(int argc, char * argv[], int nline)
 			goto out;
 		}
 
-		ret = mpath_persistent_reserve_in (fd, prin_sa, resp, noisy, verbose);
+		ret = __mpath_persistent_reserve_in (fd, prin_sa, resp, noisy);
 		if (ret != MPATH_PR_SUCCESS )
 		{
 			fprintf (stderr, "Persistent Reserve IN command failed\n");
@@ -540,8 +543,8 @@ static int handle_args(int argc, char * argv[], int nline)
 		}
 
 		/* PROUT commands other than 'register and move' */
-		ret = mpath_persistent_reserve_out (fd, prout_sa, 0, prout_type,
-				paramp, noisy, verbose);
+		ret = __mpath_persistent_reserve_out (fd, prout_sa, 0, prout_type,
+				paramp, noisy);
 		for (j = 0 ; j < num_transport; j++)
 		{
 			tmp = paramp->trnptid_list[j];
@@ -580,6 +583,8 @@ out :
 		free(batch_fn);
 		ret = ret == 0 ? rv : ret;
 	}
+	if (nline == 0)
+		mpath_persistent_reserve_free_vecs();
 	return (ret >= 0) ? ret : MPATH_PR_OTHER;
 }
 
-- 
2.21.0

[PATCH v2 7/9] libmpathpersist: don't bother with priorities

[Martin Wilck]

We send our PR commands to every active path, regardless of priority.
Thus we can save the effort to obtain priorities.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index ce72da67..599c5e9e 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -114,12 +114,6 @@ updatepaths (struct multipath * mpp)
 				pathinfo(pp, conf, DI_CHECKER);
 				put_multipath_config(conf);
 			}
-
-			if (pp->priority == PRIO_UNDEF) {
-				conf = get_multipath_config();
-				pathinfo(pp, conf, DI_PRIO);
-				put_multipath_config(conf);
-			}
 		}
 	}
 	return 0;
@@ -413,7 +407,6 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
 		 * about them
 		 */
 		updatepaths(mpp);
-		mpp->bestpg = select_path_group (mpp);
 		disassemble_status (status, mpp);
 
 	}
-- 
2.21.0

[PATCH v2 8/9] mpathpersist.8: fix examples in man page

[Martin Wilck]

--prout-type is ignored in the REGISTER service action. The RESERVE service
action takes a Reservation Key, not a Service action Reservation Key, as
argument. The text mentions "Service Action Reservation Key" in several places
where it means the Reservation Key. Add examples for unregistering the current
key, and the CLEAR service action. Fix formatting with longer text in the
description lines (the .TP formatting would produce bad results if the
first line needs to be broken).

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/mpathpersist.8 | 43 +++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/mpathpersist/mpathpersist.8 b/mpathpersist/mpathpersist.8
index 885491dd..cd602e40 100644
--- a/mpathpersist/mpathpersist.8
+++ b/mpathpersist/mpathpersist.8
@@ -167,18 +167,43 @@ PR In: maximum allocation length. LEN is a decimal number between 0 and 8192.
 .SH EXAMPLE
 .\" ----------------------------------------------------------------------------
 .
-.TP
-Register the Service Action Reservation Key for the /dev/mapper/mpath9 device:
-\fBmpathpersist --out --register --param-sark=\fI123abc \fB--prout-type=\fI5 /dev/mapper/mpath9\fR
-.TP
-Read the Service Action Reservation Key for the /dev/mapper/mpath9 device:
+.PP
+Register the key \(dq123abc\(dq for the /dev/mapper/mpath9 device:
+.RS
+\fBmpathpersist --out --register --param-sark=\fI123abc /dev/mapper/mpath9\fR
+.RE
+.PP
+Read registered reservation keys for the /dev/mapper/mpath9 device:
+.RS
 \fBmpathpersist -i -k \fI/dev/mapper/mpath9\fR
-.TP
-Reserve the Service Action Reservation Key for the /dev/mapper/mpath9 device:
-\fBmpathpersist --out --reserve --param-sark=\fI123abc \fB--prout-type=\fI8 \fB-d \fI/dev/mapper/mpath9\fR
-.TP
+.RE
+.PP
+Create a reservation for the /dev/mapper/mpath9 device with the given
+reservation key:
+.RS
+\fBmpathpersist --out --reserve --param-rk=\fI123abc \fB--prout-type=\fI8 \fB-d \fI/dev/mapper/mpath9\fR
+.RE
+.PP
 Read the reservation status of the /dev/mapper/mpath9 device:
+.RS
 \fBmpathpersist -i -s -d \fI/dev/mapper/mpath9\fR
+.RE
+.PP
+Release the previously created reservation (note that the prout-type needs to
+be the same as above):
+.RS
+\fBmpathpersist --out --release --param-rk=\fI123abc \fB--prout-type=\fI8 \fB-d \fI/dev/mapper/mpath9\fR
+.RE
+.PP
+Remove the current key registered for this host (i.e. reset it to 0):
+.RS
+\fBmpathpersist --out --register-ignore -K \fI123abc\fB -S \fI0\fB \fI/dev/mapper/mpath9\fR
+.RE
+.PP
+Remove current reservation, and unregister all registered keys from all I_T nexuses:
+.RS
+\fBmpathpersist -oCK \fI123abc \fI/dev/mapper/mpath9\fR
+.RE
 .
 .
 .\" ----------------------------------------------------------------------------
-- 
2.21.0

[PATCH v2 9/9] mpathpersist.8: add documentation for --batch-file (-f)

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/mpathpersist.8 | 78 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 77 insertions(+), 1 deletion(-)

diff --git a/mpathpersist/mpathpersist.8 b/mpathpersist/mpathpersist.8
index cd602e40..882043ae 100644
--- a/mpathpersist/mpathpersist.8
+++ b/mpathpersist/mpathpersist.8
@@ -5,7 +5,7 @@
 .\"
 .\" ----------------------------------------------------------------------------
 .
-.TH MPATHPERSIST 8 2016-10-30 "Linux"
+.TH MPATHPERSIST 8 2019-05-27 "Linux"
 .
 .
 .\" ----------------------------------------------------------------------------
@@ -71,6 +71,11 @@ Informational messages with trace enabled.
 Query or change DEVICE.
 .
 .TP
+.BI \--batch-file=\fIDEVICE\fB|\-f " FILE"
+Read commands from \fIFILE\fR. See section \(dqBATCH FILES\(dq below. This
+option can be given at most once.
+.
+.TP
 .B \--help|\-h
 Output this usage message.
 .
@@ -207,6 +212,77 @@ Remove current reservation, and unregister all registered keys from all I_T nexu
 .
 .
 .\" ----------------------------------------------------------------------------
+.SH BATCH FILES
+.\" ----------------------------------------------------------------------------
+.
+.PP
+The option \fI--batch-file\fR (\fI-f\fR) sets an input file to be processed
+by \fBmpathpersist\fR. Grouping commands in batch files can provide a speed
+improvement in particular on large installments, because \fBmpathpersist\fR
+needs to scan existing paths and maps only once during startup.
+.
+.PP
+The input file is a text file that is parsed
+line by line. Every line of the file is interpreted as a command line
+(i.e. list of options and parameters) for \fBmpathpersist\fR. Options
+and parameters are separated by one or more whitespace characters (space or TAB).
+Lines can, but do not have to, begin with the word \(dqmpathpersist\(dq.
+The \(dq#\(dq character, either at the beginning of the line or following
+some whitespace, denotes the start of a comment that lasts until the end of the
+line. Empty lines are allowed. Continuation of mpathpersist commands over
+multiple lines is not supported.
+.
+.PP
+All options listed in this man page, except \fI-f\fR and
+\fI-v\fR, are allowed in batch files. Both short and long option formats may be used.
+Using the  \fI-f\fR option inside the batch file is an error. The \fI-v\fR
+option is ignored in batch files.
+.
+.PP
+The multipath map on which to act must be specified on every input line, e.g. using the \fI-d\fR option.
+Commands acting on different multipath maps may be combined in a
+batch file, and multiple commands may act on the same multipath
+map. Commands are executed one by one, so
+that commands further down in the file see status changes caused by previous
+commands.
+If \fBmpathpersist\fR encounters an error while processing a line in the
+batch file, batch file processing is \fBnot\fR aborted; subsequent commands
+are executed nonetheless. The exit status of \fBmpathpersist\fR is the status
+of the first failed command, or 0 if all commands succeeded.
+.
+.PP
+If other options and parameters are used along with
+\fI-f\fR on the \fBmpathpersist\fR command line, the command line will be executed first, followed
+by the commands from the the batch file.
+.
+.PP
+Below is an example of a valid batch input file.
+.
+.PP
+.RS
+.EX
+# This is an mpathpersist input file.
+# Short and long forms of the same command
+-i -k /dev/dm-1 # short form, this comment is ignored
+mpathpersist --in --read-keys --device=/dev/dm-1
+
+# Mixing of long and short options, variable white space
+  --out  --register    -S  abcde     /dev/dm-1
+
+# Mixing of commands for different maps
+-ir /dev/dm-0
+-ir /dev/dm-1
+
+mpathpersist --out --param-rk abcde --reserve --prout-type 5 /dev/dm-1
+# This should now show a reservation
+-ir /dev/dm-1
+-oCK abcde /dev/dm-1
+--in --read-reservation /dev/dm-1
+.EE
+.RE
+.
+.
+.\" ----------------------------------------------------------------------------
 .SH "SEE ALSO"
 .\" ----------------------------------------------------------------------------
 .
-- 
2.21.0

Re: [PATCH v2 0/9] speeding up mpathpersist

[Benjamin Marzinski]

On Mon, May 27, 2019 at 02:59:33PM +0200, Martin Wilck wrote:
> We've had reports that mpathpersist doesn't scale well in environments
> with lots of IO devices. This patch set tries to address this problem
> with a few optimizations:
> 
>  - instead of doing a full path discovery at startup, only look at
>    paths that are part of the maps being worked on
>  - allow multiple PR commands in a single run using "batch files",
>    without re-discovering everything between PR commands
>  - avoid looking at priorities
> 
> I've done some basic testing and it seems to work. It remains to be
> seen what the effect on performance really is, but the amount of system
> calls to be performed for a given set of mpathpersist actions should
> be substantially reduced in any case, especially when using batching.
> 
> ---
> v2:
>  - avoid memory leak with --batch-file (Ben Marzinski)
>  - add documentation to usage() output and mpathpersist man page
>  - integrate "mpathpersist.8: fix examples in man page" which I'd sent
>    separately before
> 

ACK for the set.

-Ben

> 
> Martin Wilck (9):
>   mpathpersist: call usage() just once on return
>   mpathpersist: add option --batch-file (-f)
>   mpathpersist: no need to treat error close() as fatal
>   libmpathpersist: updatepaths: deal with missing pp->udev
>   libmpathpersist: factor out initialization and teardown
>   mpathpersist: initialize data structures only once
>   libmpathpersist: don't bother with priorities
>   mpathpersist.8: fix examples in man page
>   mpathpersist.8: add documentation for --batch-file (-f)
> 
>  libmpathpersist/mpath_persist.c | 250 +++++++++++++++-----------------
>  libmpathpersist/mpath_persist.h |  40 +++++
>  mpathpersist/main.c             | 227 ++++++++++++++++++++++-------
>  mpathpersist/main.h             |   1 +
>  mpathpersist/mpathpersist.8     | 121 ++++++++++++++--
>  5 files changed, 440 insertions(+), 199 deletions(-)
> 
> -- 
> 2.21.0