[RFC PATCH 0/7] speeding up mpathpersist

[RFC PATCH 0/7] 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.

Regards
Martin

Martin Wilck (7):
  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

 libmpathpersist/mpath_persist.c | 250 +++++++++++++++-----------------
 libmpathpersist/mpath_persist.h |  40 +++++
 mpathpersist/main.c             | 218 +++++++++++++++++++++-------
 mpathpersist/main.h             |   1 +
 4 files changed, 320 insertions(+), 189 deletions(-)

-- 
2.21.0

[RFC PATCH 1/7] mpathpersist: call usage() just once on return

[Martin Wilck]

This simplifies further changes.
---
 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

[RFC PATCH 2/7] 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.
---
 mpathpersist/main.c | 195 +++++++++++++++++++++++++++++++++++---------
 mpathpersist/main.h |   1 +
 2 files changed, 159 insertions(+), 37 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 94e89c13..c1a6d3c8 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,35 @@ 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, "-f option not allowed in batch file\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 +364,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 +420,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 +567,51 @@ int main (int argc, char * argv[])
 	}
 
 out :
-	if (ret == MPATH_PR_SYNTAX_ERROR)
-		usage();
-	mpath_lib_exit(conf);
+	if (ret == MPATH_PR_SYNTAX_ERROR) {
+		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;
 }
 
diff --git a/mpathpersist/main.h b/mpathpersist/main.h
index beb8a21b..c5f53f52 100644
--- a/mpathpersist/main.h
+++ b/mpathpersist/main.h
@@ -23,6 +23,7 @@ static struct option long_options[] = {
 	{"reserve", 0, NULL, 'R'},
 	{"transport-id", 1, NULL, 'X'},
 	{"alloc-length", 1, NULL, 'l'},
+	{"batch-file", 1, NULL, 'f' },
 	{NULL, 0, NULL, 0}
 };
 
-- 
2.21.0

Re: [RFC PATCH 2/7] mpathpersist: add option --batch-file (-f)

[Benjamin Marzinski]

On Sat, May 18, 2019 at 12:56:58AM +0200, Martin Wilck wrote:
> 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.

One small issue. Otherwise, this looks good.

> ---
>  mpathpersist/main.c | 195 +++++++++++++++++++++++++++++++++++---------
>  mpathpersist/main.h |   1 +
>  2 files changed, 159 insertions(+), 37 deletions(-)
> 

<snip>

> @@ -487,10 +567,51 @@ int main (int argc, char * argv[])
>  	}
>  
>  out :
> -	if (ret == MPATH_PR_SYNTAX_ERROR)
> -		usage();
> -	mpath_lib_exit(conf);
> +	if (ret == MPATH_PR_SYNTAX_ERROR) {

It's possible to set batch_fn, and then later fail with
MPATH_PR_SYNTAX_ERROR. In that case, you should still free batch_fn.

-Ben

> +		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;
>  }
>  
> diff --git a/mpathpersist/main.h b/mpathpersist/main.h
> index beb8a21b..c5f53f52 100644
> --- a/mpathpersist/main.h
> +++ b/mpathpersist/main.h
> @@ -23,6 +23,7 @@ static struct option long_options[] = {
>  	{"reserve", 0, NULL, 'R'},
>  	{"transport-id", 1, NULL, 'X'},
>  	{"alloc-length", 1, NULL, 'l'},
> +	{"batch-file", 1, NULL, 'f' },
>  	{NULL, 0, NULL, 0}
>  };
>  
> -- 
> 2.21.0

Re: [RFC PATCH 2/7] mpathpersist: add option --batch-file (-f)

[Benjamin Marzinski]

On Sat, May 18, 2019 at 12:56:58AM +0200, Martin Wilck wrote:
> 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.

One small issue. Otherwise, this looks good.

> ---
>  mpathpersist/main.c | 195 +++++++++++++++++++++++++++++++++++---------
>  mpathpersist/main.h |   1 +
>  2 files changed, 159 insertions(+), 37 deletions(-)
> 

<snip>

> @@ -487,10 +567,51 @@ int main (int argc, char * argv[])
>  	}
>  
>  out :
> -	if (ret == MPATH_PR_SYNTAX_ERROR)
> -		usage();
> -	mpath_lib_exit(conf);
> +	if (ret == MPATH_PR_SYNTAX_ERROR) {

It's possible to set batch_fn, and then later fail with
MPATH_PR_SYNTAX_ERROR. In that case, you should still free batch_fn.

-Ben

> +		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;
>  }
>  
> diff --git a/mpathpersist/main.h b/mpathpersist/main.h
> index beb8a21b..c5f53f52 100644
> --- a/mpathpersist/main.h
> +++ b/mpathpersist/main.h
> @@ -23,6 +23,7 @@ static struct option long_options[] = {
>  	{"reserve", 0, NULL, 'R'},
>  	{"transport-id", 1, NULL, 'X'},
>  	{"alloc-length", 1, NULL, 'l'},
> +	{"batch-file", 1, NULL, 'f' },
>  	{NULL, 0, NULL, 0}
>  };
>  
> -- 
> 2.21.0

Re: [RFC PATCH 2/7] mpathpersist: add option --batch-file (-f)

[Benjamin Marzinski]

On Sat, May 18, 2019 at 12:56:58AM +0200, Martin Wilck wrote:
> 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.

One small issue. Otherwise, this looks good.

> ---
>  mpathpersist/main.c | 195 +++++++++++++++++++++++++++++++++++---------
>  mpathpersist/main.h |   1 +
>  2 files changed, 159 insertions(+), 37 deletions(-)
> 

<snip>

> @@ -487,10 +567,51 @@ int main (int argc, char * argv[])
>  	}
>  
>  out :
> -	if (ret == MPATH_PR_SYNTAX_ERROR)
> -		usage();
> -	mpath_lib_exit(conf);
> +	if (ret == MPATH_PR_SYNTAX_ERROR) {

It's possible to set batch_fn, and then later fail with
MPATH_PR_SYNTAX_ERROR. In that case, you should still free batch_fn.

-Ben

> +		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;
>  }
>  
> diff --git a/mpathpersist/main.h b/mpathpersist/main.h
> index beb8a21b..c5f53f52 100644
> --- a/mpathpersist/main.h
> +++ b/mpathpersist/main.h
> @@ -23,6 +23,7 @@ static struct option long_options[] = {
>  	{"reserve", 0, NULL, 'R'},
>  	{"transport-id", 1, NULL, 'X'},
>  	{"alloc-length", 1, NULL, 'l'},
> +	{"batch-file", 1, NULL, 'f' },
>  	{NULL, 0, NULL, 0}
>  };
>  
> -- 
> 2.21.0

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

[Martin Wilck]

Simplify code a bit.
---
 mpathpersist/main.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index c1a6d3c8..b204647f 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));
 
@@ -558,13 +557,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

[RFC PATCH 4/7] 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.
---
 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

[RFC PATCH 5/7] 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.
---
 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

[RFC PATCH 6/7] 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.
---
 mpathpersist/main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index b204647f..5f871019 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -367,6 +367,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)
@@ -473,7 +476,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");
@@ -533,8 +536,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];
@@ -572,6 +575,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

[RFC PATCH 7/7] 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.
---
 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