All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] xfs_io: fix up command iteration
@ 2016-12-07  3:47 Dave Chinner
  2016-12-07  3:47 ` [PATCH 1/6] libxcmd: check CMD_FLAG_GLOBAL inside args_command() Dave Chinner
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: Dave Chinner @ 2016-12-07  3:47 UTC (permalink / raw)
  To: linux-xfs; +Cc: fstests, amir73il

Hi folks,

Here are teh changes I've put together to address the command
iteration problems that xfs_io is demonstrating. I described the
history that lead us to the current problems here:

https://www.spinics.net/lists/fstests/msg04572.html

This series cleans up the libxcmd command code and fixes the xfs_io
problems without completely removing the existing iteration
behaviour.

The first three patches address the "args_command" abstraction,
renaming it to a "command iterator" abstraction and clean up the
command loop implementation to make it easier to follow. It is now
clear that the the command loop has an external command iterator
control function, and it has a clean and clear mechanism for the
external commands to prevent iteration from occurring
(CMD_FLAG_ONESHOT).

The fourth patch cleans up a recent change made to the xfs_quota
interface to support foreign filesystems - it still needs to be abel
to run the help and quit commands regardless of other state. The
same requirement exists for xfs_io, it just implemented the command
check that allowed them to be run differently. This patch
centralises that by marking libxcmd functions as "library" functions
and skips the application provided command checking altogether.
Hence the apps no longer need to handle these cases at all.

The fifth patch makes all the xfs_io commands that shouldn't iterate
the file table one-shot only commands. This avoids all the nasty
problems with commands like open that end up completely filling
the open file table with thousands of filedescriptors pointing to
the same file. THis makes those commands usable and predictable.

Finally, the last patch adds CLI support for running xfs_io commands
as one-shot commands rather than iterating commands. This is useful
for being able to set up multiple files and operate on them as a
whole or individually as necessary. This patch also documents in the
man page that some commands may iterate all open files and that the
new "-C" command option can be used to avoid problems arising from
this historical behaviour.

-Dave.


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

* [PATCH 1/6] libxcmd: check CMD_FLAG_GLOBAL inside args_command()
  2016-12-07  3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
@ 2016-12-07  3:47 ` Dave Chinner
  2016-12-07  3:47 ` [PATCH 2/6] libxcmd: rename args_command to command_iterator Dave Chinner
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: Dave Chinner @ 2016-12-07  3:47 UTC (permalink / raw)
  To: linux-xfs; +Cc: fstests, amir73il

From: Dave Chinner <dchinner@redhat.com>

Rather than having multiple methods of executing commands from the
CLI, use CMD_FLAG_GLOBAL to indicate a one-shot command rather than
an iterative command from args_command(). This simplifies the main
loop processing.

To make it more obvious what this CMD_FLAG_GLOBAL flag does, rename
it to CMD_FLAG_ONESHOT to indicate that the command should only ever
be executed once and not iterated.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 include/command.h |  7 ++++++-
 io/file.c         |  2 +-
 io/init.c         |  2 +-
 libxcmd/command.c | 20 +++++++++++++-------
 libxcmd/help.c    |  2 +-
 libxcmd/quit.c    |  2 +-
 quota/path.c      |  4 ++--
 quota/report.c    |  2 +-
 8 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/include/command.h b/include/command.h
index 81d5a4dbb7f3..58bfcaac44a0 100644
--- a/include/command.h
+++ b/include/command.h
@@ -20,7 +20,12 @@
 
 #include <sys/time.h>
 
-#define CMD_FLAG_GLOBAL		(1<<31)	/* don't iterate "args" */
+/*
+ * A "oneshot" command ony runs once per command execution. It does
+ * not iterate the command args function callout and so can be used
+ * for functions like "help" that should only ever be run once.
+ */
+#define CMD_FLAG_ONESHOT	(1<<31)
 #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
 
 typedef int (*cfunc_t)(int argc, char **argv);
diff --git a/io/file.c b/io/file.c
index d4bc4f8fc1d5..8e3f07122922 100644
--- a/io/file.c
+++ b/io/file.c
@@ -104,7 +104,7 @@ file_init(void)
 	print_cmd.argmin = 0;
 	print_cmd.argmax = 0;
 	print_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK | CMD_FOREIGN_OK |
-				CMD_FLAG_GLOBAL;
+				CMD_FLAG_ONESHOT;
 	print_cmd.oneline = _("list current open files and memory mappings");
 
 	add_command(&file_cmd);
diff --git a/io/init.c b/io/init.c
index a9191cfa072d..ab40f3745390 100644
--- a/io/init.c
+++ b/io/init.c
@@ -104,7 +104,7 @@ static int
 init_check_command(
 	const cmdinfo_t	*ct)
 {
-	if (ct->flags & CMD_FLAG_GLOBAL)
+	if (ct->flags & CMD_FLAG_ONESHOT)
 		return 1;
 
 	if (!file && !(ct->flags & CMD_NOFILE_OK)) {
diff --git a/libxcmd/command.c b/libxcmd/command.c
index dd0034cc6d83..dce8361ce3ea 100644
--- a/libxcmd/command.c
+++ b/libxcmd/command.c
@@ -124,10 +124,20 @@ add_user_command(char *optarg)
 	cmdline[ncmdline-1] = optarg;
 }
 
+/*
+ * To detect one-shot commands, they will return a negative index. If we
+ * get a negative index on entry, we've already run the one-shot command,
+ * so we abort straight away.
+ */
 static int
 args_command(
-	int	index)
+	const cmdinfo_t	*ct,
+	int		index)
 {
+	if (index < 0)
+		return 0;
+	if (ct->flags & CMD_FLAG_ONESHOT)
+		return -1;
 	if (args_func)
 		return args_func(index);
 	return 0;
@@ -160,13 +170,9 @@ command_loop(void)
 		if (c) {
 			ct = find_command(v[0]);
 			if (ct) {
-				if (ct->flags & CMD_FLAG_GLOBAL)
+				j = 0;
+				while (!done && (j = args_command(ct, j)))
 					done = command(ct, c, v);
-				else {
-					j = 0;
-					while (!done && (j = args_command(j)))
-						done = command(ct, c, v);
-				}
 			} else
 				fprintf(stderr, _("command \"%s\" not found\n"),
 					v[0]);
diff --git a/libxcmd/help.c b/libxcmd/help.c
index 8894c7931f89..bc31d6df1d8a 100644
--- a/libxcmd/help.c
+++ b/libxcmd/help.c
@@ -89,7 +89,7 @@ help_init(void)
 	help_cmd.cfunc = help_f;
 	help_cmd.argmin = 0;
 	help_cmd.argmax = 1;
-	help_cmd.flags = CMD_FLAG_GLOBAL | CMD_ALL_FSTYPES;
+	help_cmd.flags = CMD_FLAG_ONESHOT | CMD_ALL_FSTYPES;
 	help_cmd.args = _("[command]");
 	help_cmd.oneline = _("help for one or all commands");
 
diff --git a/libxcmd/quit.c b/libxcmd/quit.c
index e0af91629b81..19431015aee2 100644
--- a/libxcmd/quit.c
+++ b/libxcmd/quit.c
@@ -39,7 +39,7 @@ quit_init(void)
 	quit_cmd.cfunc = quit_f;
 	quit_cmd.argmin = -1;
 	quit_cmd.argmax = -1;
-	quit_cmd.flags = CMD_FLAG_GLOBAL | CMD_ALL_FSTYPES;
+	quit_cmd.flags = CMD_FLAG_ONESHOT | CMD_ALL_FSTYPES;
 	quit_cmd.oneline = _("exit the program");
 
 	add_command(&quit_cmd);
diff --git a/quota/path.c b/quota/path.c
index 57d14f0b5511..330a3bef6aa9 100644
--- a/quota/path.c
+++ b/quota/path.c
@@ -141,7 +141,7 @@ path_init(void)
 	path_cmd.cfunc = path_f;
 	path_cmd.argmin = 0;
 	path_cmd.argmax = 1;
-	path_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK;
+	path_cmd.flags = CMD_FLAG_ONESHOT | CMD_FLAG_FOREIGN_OK;
 	path_cmd.oneline = _("set current path, or show the list of paths");
 
 	print_cmd.name = "print";
@@ -149,7 +149,7 @@ path_init(void)
 	print_cmd.cfunc = print_f;
 	print_cmd.argmin = 0;
 	print_cmd.argmax = 0;
-	print_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK;
+	print_cmd.flags = CMD_FLAG_ONESHOT | CMD_FLAG_FOREIGN_OK;
 	print_cmd.oneline = _("list known mount points and projects");
 
 	if (expert)
diff --git a/quota/report.c b/quota/report.c
index 604f50dc6001..ca9d2b2c9564 100644
--- a/quota/report.c
+++ b/quota/report.c
@@ -770,7 +770,7 @@ report_init(void)
 	report_cmd.args = _("[-bir] [-gpu] [-ahnt] [-f file]");
 	report_cmd.oneline = _("report filesystem quota information");
 	report_cmd.help = report_help;
-	report_cmd.flags = CMD_FLAG_GLOBAL | CMD_FLAG_FOREIGN_OK;
+	report_cmd.flags = CMD_FLAG_ONESHOT | CMD_FLAG_FOREIGN_OK;
 
 	if (expert) {
 		add_command(&dump_cmd);
-- 
2.10.2


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

* [PATCH 2/6] libxcmd: rename args_command to command_iterator
  2016-12-07  3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
  2016-12-07  3:47 ` [PATCH 1/6] libxcmd: check CMD_FLAG_GLOBAL inside args_command() Dave Chinner
@ 2016-12-07  3:47 ` Dave Chinner
  2016-12-07  3:47 ` [PATCH 3/6] libxcmd: merge command() and iterate_command() Dave Chinner
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: Dave Chinner @ 2016-12-07  3:47 UTC (permalink / raw)
  To: linux-xfs; +Cc: fstests, amir73il

From: Dave Chinner <dchinner@redhat.com>

It is not particularly easy to understand the function of the
args_command abstraction. it's actually a command iterator interface
that allows callers to specify the target of the command and iterate
the command multiple times over different targets. Rename and
document the abstraction to make this functionality clear.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 include/command.h |  4 ++--
 io/init.c         |  9 +++++++--
 libxcmd/command.c | 16 ++++++++--------
 quota/init.c      |  9 +++++++--
 4 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/include/command.h b/include/command.h
index 58bfcaac44a0..637ee06e6e9a 100644
--- a/include/command.h
+++ b/include/command.h
@@ -50,12 +50,12 @@ extern int		ncmds;
 extern void		help_init(void);
 extern void		quit_init(void);
 
-typedef int (*argsfunc_t)(int index);
+typedef int (*iterfunc_t)(int index);
 typedef int (*checkfunc_t)(const cmdinfo_t *ci);
 
 extern void		add_command(const cmdinfo_t *ci);
 extern void		add_user_command(char *optarg);
-extern void		add_args_command(argsfunc_t af);
+extern void		add_command_iterator(iterfunc_t func);
 extern void		add_check_command(checkfunc_t cf);
 
 extern const cmdinfo_t	*find_command(const char *cmd);
diff --git a/io/init.c b/io/init.c
index ab40f3745390..34009b024833 100644
--- a/io/init.c
+++ b/io/init.c
@@ -90,8 +90,13 @@ init_commands(void)
 	cowextsize_init();
 }
 
+/*
+ * This allows xfs_io commands specified on the command line to be run on every
+ * open file in the file table. Commands that should not be iterated across all
+ * open files need to specify CMD_FLAG_ONESHOT in their command flags.
+ */
 static int
-init_args_command(
+filetable_iterator(
 	int	index)
 {
 	if (index >= filecount)
@@ -214,7 +219,7 @@ init(
 	}
 
 	init_commands();
-	add_args_command(init_args_command);
+	add_command_iterator(filetable_iterator);
 	add_check_command(init_check_command);
 }
 
diff --git a/libxcmd/command.c b/libxcmd/command.c
index dce8361ce3ea..789aeb5c5e5a 100644
--- a/libxcmd/command.c
+++ b/libxcmd/command.c
@@ -23,7 +23,7 @@
 cmdinfo_t	*cmdtab;
 int		ncmds;
 
-static argsfunc_t	args_func;
+static iterfunc_t	iter_func;
 static checkfunc_t	check_func;
 static int		ncmdline;
 static char		**cmdline;
@@ -130,7 +130,7 @@ add_user_command(char *optarg)
  * so we abort straight away.
  */
 static int
-args_command(
+iterate_command(
 	const cmdinfo_t	*ct,
 	int		index)
 {
@@ -138,16 +138,16 @@ args_command(
 		return 0;
 	if (ct->flags & CMD_FLAG_ONESHOT)
 		return -1;
-	if (args_func)
-		return args_func(index);
+	if (iter_func)
+		return iter_func(index);
 	return 0;
 }
 
 void
-add_args_command(
-	argsfunc_t	af)
+add_command_iterator(
+	iterfunc_t	func)
 {
-	args_func = af;
+	iter_func = func;
 }
 
 void
@@ -171,7 +171,7 @@ command_loop(void)
 			ct = find_command(v[0]);
 			if (ct) {
 				j = 0;
-				while (!done && (j = args_command(ct, j)))
+				while (!done && (j = iterate_command(ct, j)))
 					done = command(ct, c, v);
 			} else
 				fprintf(stderr, _("command \"%s\" not found\n"),
diff --git a/quota/init.c b/quota/init.c
index 3bebbb8735f3..193f6421fd59 100644
--- a/quota/init.c
+++ b/quota/init.c
@@ -75,8 +75,13 @@ init_commands(void)
 	state_init();
 }
 
+/*
+ * This function allows xfs_quota commands to iterate across all discovered
+ * quota enabled filesystems. Commands that should not iterate all filesystems
+ * should specify CMD_FLAG_ONESHOT in their command flags.
+ */
 static int
-init_args_command(
+filesystem_iterator(
 	int	index)
 {
 	if (index >= fs_count)
@@ -189,7 +194,7 @@ init(
 	free(projopts);
 
 	init_commands();
-	add_args_command(init_args_command);
+	add_command_iterator(filesystem_iterator);
 	add_check_command(init_check_command);
 
 	/*
-- 
2.10.2


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

* [PATCH 3/6] libxcmd: merge command() and iterate_command()
  2016-12-07  3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
  2016-12-07  3:47 ` [PATCH 1/6] libxcmd: check CMD_FLAG_GLOBAL inside args_command() Dave Chinner
  2016-12-07  3:47 ` [PATCH 2/6] libxcmd: rename args_command to command_iterator Dave Chinner
@ 2016-12-07  3:47 ` Dave Chinner
  2016-12-07  3:47 ` [PATCH 4/6] libxcmd: don't check generic library commands Dave Chinner
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: Dave Chinner @ 2016-12-07  3:47 UTC (permalink / raw)
  To: linux-xfs; +Cc: fstests, amir73il

From: Dave Chinner <dchinner@redhat.com>

Simplify the command loop further by merging the command loop
iteration checks with the command execution function. This removes
all visibility of command iteration from the main command execution
loop, and enables us to factor and clean up the command loop
processing neatly.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 libxcmd/command.c | 110 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 66 insertions(+), 44 deletions(-)

diff --git a/libxcmd/command.c b/libxcmd/command.c
index 789aeb5c5e5a..2c94a3199115 100644
--- a/libxcmd/command.c
+++ b/libxcmd/command.c
@@ -125,22 +125,30 @@ add_user_command(char *optarg)
 }
 
 /*
- * To detect one-shot commands, they will return a negative index. If we
- * get a negative index on entry, we've already run the one-shot command,
- * so we abort straight away.
+ * Run a command, iterating as necessary. Return 0 for success, non-zero
+ * if an error occurred. Errors terminate loop iteration immediately.
  */
 static int
 iterate_command(
 	const cmdinfo_t	*ct,
-	int		index)
+	int		argc,
+	char		**argv)
 {
-	if (index < 0)
+	int		error = 0;
+	int		j;
+
+	/* if there's nothing to iterate, we're done! */
+	if (!iter_func)
 		return 0;
-	if (ct->flags & CMD_FLAG_ONESHOT)
-		return -1;
-	if (iter_func)
-		return iter_func(index);
-	return 0;
+
+	for (j = iter_func(0); j; j = iter_func(j)) {
+		error = command(ct, argc, argv);
+		if (error)
+			break;
+
+	}
+
+	return error;
 }
 
 void
@@ -150,14 +158,55 @@ add_command_iterator(
 	iter_func = func;
 }
 
-void
-command_loop(void)
+static int
+process_input(
+	char	*input,
+	bool	iterate)
 {
-	int		c, i, j = 0, done = 0;
-	char		*input;
 	char		**v;
 	const cmdinfo_t	*ct;
+	int		c = 0;
+	int		error = 0;
+
+	v = breakline(input, &c);
+	if (!c)
+		goto out;
+
+	ct = find_command(v[0]);
+	if (!ct) {
+		fprintf(stderr, _("command \"%s\" not found\n"), v[0]);
+		goto out;
+	}
+
+	/* oneshot commands don't iterate */
+	if (!iterate || (ct->flags & CMD_FLAG_ONESHOT))
+		error = command(ct, c, v);
+	else
+		error = iterate_command(ct, c, v);
+out:
+	doneline(input, v);
+	return error;
+}
+
+void
+command_loop(void)
+{
+	char	*input;
+	int	done = 0;
+	int	i;
+
+	if (!cmdline) {
+		/* interactive mode */
+		while (!done) {
+			input = fetchline();
+			if (!input)
+				break;
+			done = process_input(input, false);
+		}
+		return;
+	}
 
+	/* command line mode */
 	for (i = 0; !done && i < ncmdline; i++) {
 		input = strdup(cmdline[i]);
 		if (!input) {
@@ -166,37 +215,10 @@ command_loop(void)
 				cmdline[i], strerror(errno));
 			exit(1);
 		}
-		v = breakline(input, &c);
-		if (c) {
-			ct = find_command(v[0]);
-			if (ct) {
-				j = 0;
-				while (!done && (j = iterate_command(ct, j)))
-					done = command(ct, c, v);
-			} else
-				fprintf(stderr, _("command \"%s\" not found\n"),
-					v[0]);
-		}
-		doneline(input, v);
-	}
-	if (cmdline) {
-		free(cmdline);
-		return;
-	}
-	while (!done) {
-		if ((input = fetchline()) == NULL)
-			break;
-		v = breakline(input, &c);
-		if (c) {
-			ct = find_command(v[0]);
-			if (ct)
-				done = command(ct, c, v);
-			else
-				fprintf(stderr, _("command \"%s\" not found\n"),
-					v[0]);
-		}
-		doneline(input, v);
+		done = process_input(input, true);
 	}
+	free(cmdline);
+	return;
 }
 
 void
-- 
2.10.2


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

* [PATCH 4/6] libxcmd: don't check generic library commands
  2016-12-07  3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
                   ` (2 preceding siblings ...)
  2016-12-07  3:47 ` [PATCH 3/6] libxcmd: merge command() and iterate_command() Dave Chinner
@ 2016-12-07  3:47 ` Dave Chinner
  2016-12-07  3:47 ` [PATCH 5/6] xfs_io: make various commands one-shot only Dave Chinner
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 20+ messages in thread
From: Dave Chinner @ 2016-12-07  3:47 UTC (permalink / raw)
  To: linux-xfs; +Cc: fstests, amir73il

From: Dave Chinner <dchinner@redhat.com>

The generic "help" and "quit" commands have different methods of
skipping user provided command check functions that may prevent them
from running. xfs_quota use CMD_ALL_FSTYPES and xfs_io uses
CMD_FLAG_ONESHOT.  Add a new CMD_FLAG_LIBRARY to indicate commands
that should not be checked against application specific check
functions so they are always present and can be run regardless of
the context in which they are run.

This gets rid of the CMD_ALL_FSTYPES flag, and enables us to remove
the ONESHOT check in xfs_io so we use only app specific flags for
determining if app commands should run or not.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 include/command.h | 1 +
 io/init.c         | 3 ---
 libxcmd/command.c | 4 ++++
 libxcmd/help.c    | 2 +-
 libxcmd/quit.c    | 2 +-
 quota/init.c      | 4 ----
 6 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/include/command.h b/include/command.h
index 637ee06e6e9a..348002cbe3ed 100644
--- a/include/command.h
+++ b/include/command.h
@@ -27,6 +27,7 @@
  */
 #define CMD_FLAG_ONESHOT	(1<<31)
 #define CMD_FLAG_FOREIGN_OK	(1<<30)	/* command not restricted to XFS */
+#define CMD_FLAG_LIBRARY	(1<<29)	/* command provided by libxcmd */
 
 typedef int (*cfunc_t)(int argc, char **argv);
 typedef void (*helpfunc_t)(void);
diff --git a/io/init.c b/io/init.c
index 34009b024833..5ce627ef22c9 100644
--- a/io/init.c
+++ b/io/init.c
@@ -109,9 +109,6 @@ static int
 init_check_command(
 	const cmdinfo_t	*ct)
 {
-	if (ct->flags & CMD_FLAG_ONESHOT)
-		return 1;
-
 	if (!file && !(ct->flags & CMD_NOFILE_OK)) {
 		fprintf(stderr, _("no files are open, try 'help open'\n"));
 		return 0;
diff --git a/libxcmd/command.c b/libxcmd/command.c
index 2c94a3199115..decc442a9d03 100644
--- a/libxcmd/command.c
+++ b/libxcmd/command.c
@@ -48,6 +48,10 @@ static int
 check_command(
 	const cmdinfo_t	*ci)
 {
+	/* always run internal library supplied commands */
+	if (ci->flags & CMD_FLAG_LIBRARY)
+		return 1;
+
 	if (check_func)
 		return check_func(ci);
 	return 1;
diff --git a/libxcmd/help.c b/libxcmd/help.c
index bc31d6df1d8a..f888377cabf7 100644
--- a/libxcmd/help.c
+++ b/libxcmd/help.c
@@ -89,7 +89,7 @@ help_init(void)
 	help_cmd.cfunc = help_f;
 	help_cmd.argmin = 0;
 	help_cmd.argmax = 1;
-	help_cmd.flags = CMD_FLAG_ONESHOT | CMD_ALL_FSTYPES;
+	help_cmd.flags = CMD_FLAG_ONESHOT | CMD_FLAG_LIBRARY;
 	help_cmd.args = _("[command]");
 	help_cmd.oneline = _("help for one or all commands");
 
diff --git a/libxcmd/quit.c b/libxcmd/quit.c
index 19431015aee2..1e1ba986f2ba 100644
--- a/libxcmd/quit.c
+++ b/libxcmd/quit.c
@@ -39,7 +39,7 @@ quit_init(void)
 	quit_cmd.cfunc = quit_f;
 	quit_cmd.argmin = -1;
 	quit_cmd.argmax = -1;
-	quit_cmd.flags = CMD_FLAG_ONESHOT | CMD_ALL_FSTYPES;
+	quit_cmd.flags = CMD_FLAG_ONESHOT | CMD_FLAG_LIBRARY;
 	quit_cmd.oneline = _("exit the program");
 
 	add_command(&quit_cmd);
diff --git a/quota/init.c b/quota/init.c
index 193f6421fd59..d45dc4c5461e 100644
--- a/quota/init.c
+++ b/quota/init.c
@@ -117,10 +117,6 @@ init_check_command(
 	if (!fs_path)
 		return 1;
 
-	/* Always run commands that are valid for all fs types. */
-	if (ct->flags & CMD_ALL_FSTYPES)
-		return 1;
-
 	/* If it's an XFS filesystem, always run the command. */
 	if (!(fs_path->fs_flags & FS_FOREIGN))
 		return 1;
-- 
2.10.2


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

* [PATCH 5/6] xfs_io: make various commands one-shot only
  2016-12-07  3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
                   ` (3 preceding siblings ...)
  2016-12-07  3:47 ` [PATCH 4/6] libxcmd: don't check generic library commands Dave Chinner
@ 2016-12-07  3:47 ` Dave Chinner
  2016-12-15 18:21   ` Eric Sandeen
  2016-12-07  3:47 ` [PATCH 6/6] libxcmd: add non-iterating user commands Dave Chinner
  2017-01-12  5:14 ` [PATCH 0/6] xfs_io: fix up command iteration Amir Goldstein
  6 siblings, 1 reply; 20+ messages in thread
From: Dave Chinner @ 2016-12-07  3:47 UTC (permalink / raw)
  To: linux-xfs; +Cc: fstests, amir73il

From: Dave Chinner <dchinner@redhat.com>

It makes no sense to iterate the file table for some xfs_io
commands. Some commands are already marked in this way, but lots of
them are not and this leads to bad behaviour. For example, the open
command will run until the process fd table is full and EMFILE is
returned rather than just opening the specified file once.

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 io/file.c      | 2 +-
 io/freeze.c    | 4 ++--
 io/getrusage.c | 3 ++-
 io/imap.c      | 2 +-
 io/inject.c    | 2 +-
 io/link.c      | 2 +-
 io/mmap.c      | 3 ++-
 io/open.c      | 7 ++++---
 io/reflink.c   | 4 ++--
 io/resblks.c   | 2 +-
 io/shutdown.c  | 2 +-
 io/sync.c      | 3 ++-
 12 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/io/file.c b/io/file.c
index 8e3f07122922..349b19cdc420 100644
--- a/io/file.c
+++ b/io/file.c
@@ -95,7 +95,7 @@ file_init(void)
 	file_cmd.cfunc = file_f;
 	file_cmd.argmin = 0;
 	file_cmd.argmax = 1;
-	file_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	file_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	file_cmd.oneline = _("set the current file");
 
 	print_cmd.name = "print";
diff --git a/io/freeze.c b/io/freeze.c
index 3d0d2a4b5601..0305713d99e8 100644
--- a/io/freeze.c
+++ b/io/freeze.c
@@ -65,14 +65,14 @@ freeze_init(void)
 	freeze_cmd.cfunc = freeze_f;
 	freeze_cmd.argmin = 0;
 	freeze_cmd.argmax = 0;
-	freeze_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	freeze_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	freeze_cmd.oneline = _("freeze filesystem of current file");
 
 	thaw_cmd.name = "thaw";
 	thaw_cmd.cfunc = thaw_f;
 	thaw_cmd.argmin = 0;
 	thaw_cmd.argmax = 0;
-	thaw_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	thaw_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	thaw_cmd.oneline = _("unfreeze filesystem of current file");
 
 	if (expert) {
diff --git a/io/getrusage.c b/io/getrusage.c
index bccf94cbc302..cf1f2afd19a8 100644
--- a/io/getrusage.c
+++ b/io/getrusage.c
@@ -113,7 +113,8 @@ getrusage_init(void)
 	getrusage_cmd.argmin = 0;
 	getrusage_cmd.argmax = -1;
 	getrusage_cmd.cfunc = getrusage_f;
-	getrusage_cmd.flags = CMD_NOFILE_OK | CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	getrusage_cmd.flags = CMD_NOFILE_OK | CMD_NOMAP_OK |
+			      CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	getrusage_cmd.oneline = _("report process resource usage");
 
 	if (expert)
diff --git a/io/imap.c b/io/imap.c
index 7123432f411f..f52238e0c450 100644
--- a/io/imap.c
+++ b/io/imap.c
@@ -72,7 +72,7 @@ imap_init(void)
 	imap_cmd.argmin = 0;
 	imap_cmd.argmax = 1;
 	imap_cmd.args = _("[nentries]");
-	imap_cmd.flags = CMD_NOMAP_OK;
+	imap_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
 	imap_cmd.oneline = _("inode map for filesystem of current file");
 
 	if (expert)
diff --git a/io/inject.c b/io/inject.c
index 5d5e4aef3dfc..25c70218a1ef 100644
--- a/io/inject.c
+++ b/io/inject.c
@@ -163,7 +163,7 @@ inject_init(void)
 	inject_cmd.cfunc = inject_f;
 	inject_cmd.argmin = 0;
 	inject_cmd.argmax = -1;
-	inject_cmd.flags = CMD_NOMAP_OK;
+	inject_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
 	inject_cmd.args = _("[tag ...]");
 	inject_cmd.oneline = _("inject errors into a filesystem");
 	inject_cmd.help = inject_help;
diff --git a/io/link.c b/io/link.c
index ccf8e691bb1d..9b2e8a970942 100644
--- a/io/link.c
+++ b/io/link.c
@@ -59,7 +59,7 @@ flink_init(void)
 	flink_cmd.cfunc = flink_f;
 	flink_cmd.argmin = 1;
 	flink_cmd.argmax = 1;
-	flink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	flink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	flink_cmd.args = _("filename");
 	flink_cmd.oneline =
 		_("link the open file descriptor to the supplied filename");
diff --git a/io/mmap.c b/io/mmap.c
index dc188d0557cf..e2d8d5a92326 100644
--- a/io/mmap.c
+++ b/io/mmap.c
@@ -674,7 +674,8 @@ mmap_init(void)
 	mmap_cmd.cfunc = mmap_f;
 	mmap_cmd.argmin = 0;
 	mmap_cmd.argmax = -1;
-	mmap_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK | CMD_FOREIGN_OK;
+	mmap_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK |
+			 CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	mmap_cmd.args = _("[N] | [-rwx] [-s size] [off len]");
 	mmap_cmd.oneline =
 		_("mmap a range in the current file, show mappings");
diff --git a/io/open.c b/io/open.c
index 722d0f9eacf5..a12f4a2ba528 100644
--- a/io/open.c
+++ b/io/open.c
@@ -918,7 +918,8 @@ open_init(void)
 	open_cmd.cfunc = open_f;
 	open_cmd.argmin = 0;
 	open_cmd.argmax = -1;
-	open_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK | CMD_FOREIGN_OK;
+	open_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK |
+			 CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	open_cmd.args = _("[-acdrstxT] [-m mode] [path]");
 	open_cmd.oneline = _("open the file specified by path");
 	open_cmd.help = open_help;
@@ -936,7 +937,7 @@ open_init(void)
 	close_cmd.cfunc = close_f;
 	close_cmd.argmin = 0;
 	close_cmd.argmax = 0;
-	close_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	close_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	close_cmd.oneline = _("close the current open file");
 
 	statfs_cmd.name = "statfs";
@@ -980,7 +981,7 @@ open_init(void)
 	inode_cmd.args = _("[-nv] [num]");
 	inode_cmd.argmin = 0;
 	inode_cmd.argmax = 3;
-	inode_cmd.flags = CMD_NOMAP_OK;
+	inode_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
 	inode_cmd.oneline =
 		_("Query inode number usage in the filesystem");
 	inode_cmd.help = inode_help;
diff --git a/io/reflink.c b/io/reflink.c
index a09e82dca80a..a22b6b4a07e3 100644
--- a/io/reflink.c
+++ b/io/reflink.c
@@ -304,7 +304,7 @@ reflink_init(void)
 	reflink_cmd.cfunc = reflink_f;
 	reflink_cmd.argmin = 4;
 	reflink_cmd.argmax = -1;
-	reflink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	reflink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	reflink_cmd.args =
 _("infile src_off dst_off len");
 	reflink_cmd.oneline =
@@ -318,7 +318,7 @@ _("infile src_off dst_off len");
 	dedupe_cmd.cfunc = dedupe_f;
 	dedupe_cmd.argmin = 4;
 	dedupe_cmd.argmax = -1;
-	dedupe_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
+	dedupe_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	dedupe_cmd.args =
 _("infile src_off dst_off len");
 	dedupe_cmd.oneline =
diff --git a/io/resblks.c b/io/resblks.c
index 73318ae03fd2..06903f5bb748 100644
--- a/io/resblks.c
+++ b/io/resblks.c
@@ -61,7 +61,7 @@ resblks_init(void)
 	resblks_cmd.cfunc = resblks_f;
 	resblks_cmd.argmin = 0;
 	resblks_cmd.argmax = 1;
-	resblks_cmd.flags = CMD_NOMAP_OK;
+	resblks_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
 	resblks_cmd.args = _("[blocks]");
 	resblks_cmd.oneline =
 		_("get and/or set count of reserved filesystem blocks");
diff --git a/io/shutdown.c b/io/shutdown.c
index d8507cc78af7..d9cd520d11e2 100644
--- a/io/shutdown.c
+++ b/io/shutdown.c
@@ -54,7 +54,7 @@ shutdown_init(void)
 	shutdown_cmd.cfunc = shutdown_f;
 	shutdown_cmd.argmin = 0;
 	shutdown_cmd.argmax = 1;
-	shutdown_cmd.flags = CMD_NOMAP_OK;
+	shutdown_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
 	shutdown_cmd.args = _("[-f]");
 	shutdown_cmd.oneline =
 		_("shuts down the filesystem where the current file resides");
diff --git a/io/sync.c b/io/sync.c
index 28e3a15e0a96..c77263804a35 100644
--- a/io/sync.c
+++ b/io/sync.c
@@ -52,7 +52,8 @@ sync_init(void)
 {
 	sync_cmd.name = "sync";
 	sync_cmd.cfunc = sync_f;
-	sync_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK | CMD_FOREIGN_OK;
+	sync_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK |
+			 CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
 	sync_cmd.oneline =
 		_("calls sync(2) to flush all in-core filesystem state to disk");
 
-- 
2.10.2


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

* [PATCH 6/6] libxcmd: add non-iterating user commands
  2016-12-07  3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
                   ` (4 preceding siblings ...)
  2016-12-07  3:47 ` [PATCH 5/6] xfs_io: make various commands one-shot only Dave Chinner
@ 2016-12-07  3:47 ` Dave Chinner
  2016-12-07  4:49   ` Amir Goldstein
  2017-01-12  5:14 ` [PATCH 0/6] xfs_io: fix up command iteration Amir Goldstein
  6 siblings, 1 reply; 20+ messages in thread
From: Dave Chinner @ 2016-12-07  3:47 UTC (permalink / raw)
  To: linux-xfs; +Cc: fstests, amir73il

From: Dave Chinner <dchinner@redhat.com>

Right now command iteration is not directly controllable by the
user; it is controlled entirely by the application command flag
setup. Sometimes we don't want commands to iterate but only operate
on the currently selected object.

For example, the stat command iterates:

$ xfs_io -c "open -r foo" -c "open bar" -C "file" -c "stat" foo
 000  foo            (foreign,non-sync,non-direct,read-write)
 001  foo            (foreign,non-sync,non-direct,read-only)
[002] bar            (foreign,non-sync,non-direct,read-write)
fd.path = "foo"
fd.flags = non-sync,non-direct,read-write
stat.ino = 462399
stat.type = regular file
stat.size = 776508
stat.blocks = 1528
fd.path = "foo"
fd.flags = non-sync,non-direct,read-only
stat.ino = 462399
stat.type = regular file
stat.size = 776508
stat.blocks = 1528
fd.path = "bar"
fd.flags = non-sync,non-direct,read-write
stat.ino = 475227
stat.type = regular file
stat.size = 0
stat.blocks = 0
$

To do this, add a function to supply a "non-iterating" user command
that will execute an iterating-capable command as though it
CMD_FLAG_ONESHOT was set. Add a new command line option to xfs_io to
drive it (-C <command>) and connect it all up. Document it in the
xfs_IO man page, too.

The result of "-C stat":

$ xfs_io -c "open -r foo" -c "open bar" -c "file" -C "stat" foo
 000  foo            (foreign,non-sync,non-direct,read-write)
 001  foo            (foreign,non-sync,non-direct,read-only)
[002] bar            (foreign,non-sync,non-direct,read-write)
fd.path = "bar"
fd.flags = non-sync,non-direct,read-write
stat.ino = 475227
stat.type = regular file
stat.size = 0
stat.blocks = 0
$

Is that we only see the stat output for the active open file
which is "bar".

Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
 include/command.h |  1 +
 io/init.c         |  7 +++++--
 libxcmd/command.c | 33 +++++++++++++++++++++++++++------
 man/man8/xfs_io.8 | 36 ++++++++++++++++++++++++++++++------
 4 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/include/command.h b/include/command.h
index 348002cbe3ed..fb3f5c79b991 100644
--- a/include/command.h
+++ b/include/command.h
@@ -56,6 +56,7 @@ typedef int (*checkfunc_t)(const cmdinfo_t *ci);
 
 extern void		add_command(const cmdinfo_t *ci);
 extern void		add_user_command(char *optarg);
+extern void		add_oneshot_user_command(char *optarg);
 extern void		add_command_iterator(iterfunc_t func);
 extern void		add_check_command(checkfunc_t cf);
 
diff --git a/io/init.c b/io/init.c
index 5ce627ef22c9..cf8573f0ecd5 100644
--- a/io/init.c
+++ b/io/init.c
@@ -34,7 +34,7 @@ void
 usage(void)
 {
 	fprintf(stderr,
-		_("Usage: %s [-adfinrRstVx] [-m mode] [-p prog] [-c cmd]... file\n"),
+_("Usage: %s [-adfinrRstVx] [-m mode] [-p prog] [[-c|-C] cmd]... file\n"),
 		progname);
 	exit(1);
 }
@@ -145,7 +145,7 @@ init(
 	pagesize = getpagesize();
 	gettimeofday(&stopwatch, NULL);
 
-	while ((c = getopt(argc, argv, "ac:dFfim:p:nrRstTVx")) != EOF) {
+	while ((c = getopt(argc, argv, "ac:C:dFfim:p:nrRstTVx")) != EOF) {
 		switch (c) {
 		case 'a':
 			flags |= IO_APPEND;
@@ -153,6 +153,9 @@ init(
 		case 'c':
 			add_user_command(optarg);
 			break;
+		case 'C':
+			add_oneshot_user_command(optarg);
+			break;
 		case 'd':
 			flags |= IO_DIRECT;
 			break;
diff --git a/libxcmd/command.c b/libxcmd/command.c
index decc442a9d03..5917aea42611 100644
--- a/libxcmd/command.c
+++ b/libxcmd/command.c
@@ -25,8 +25,14 @@ int		ncmds;
 
 static iterfunc_t	iter_func;
 static checkfunc_t	check_func;
-static int		ncmdline;
-static char		**cmdline;
+
+struct cmdline {
+	char	*cmdline;
+	bool	iterate;
+};
+
+static int	ncmdline;
+struct cmdline	*cmdline;
 
 static int
 compare(const void *a, const void *b)
@@ -120,12 +126,27 @@ void
 add_user_command(char *optarg)
 {
 	ncmdline++;
-	cmdline = realloc(cmdline, sizeof(char*) * (ncmdline));
+	cmdline = realloc(cmdline, sizeof(struct cmdline) * (ncmdline));
+	if (!cmdline) {
+		perror("realloc");
+		exit(1);
+	}
+	cmdline[ncmdline-1].cmdline = optarg;
+	cmdline[ncmdline-1].iterate = true;
+
+}
+
+void
+add_oneshot_user_command(char *optarg)
+{
+	ncmdline++;
+	cmdline = realloc(cmdline, sizeof(struct cmdline) * (ncmdline));
 	if (!cmdline) {
 		perror("realloc");
 		exit(1);
 	}
-	cmdline[ncmdline-1] = optarg;
+	cmdline[ncmdline-1].cmdline = optarg;
+	cmdline[ncmdline-1].iterate = false;
 }
 
 /*
@@ -212,14 +233,14 @@ command_loop(void)
 
 	/* command line mode */
 	for (i = 0; !done && i < ncmdline; i++) {
-		input = strdup(cmdline[i]);
+		input = strdup(cmdline[i].cmdline);
 		if (!input) {
 			fprintf(stderr,
 				_("cannot strdup command '%s': %s\n"),
 				cmdline[i], strerror(errno));
 			exit(1);
 		}
-		done = process_input(input, true);
+		done = process_input(input, cmdline[i].iterate);
 	}
 	free(cmdline);
 	return;
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index 885df7f141e2..d6bacaf0438d 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -9,10 +9,13 @@ xfs_io \- debug the I/O path of an XFS filesystem
 .B \-c
 .I cmd
 ] ... [
+.B \-C
+.I cmd
+] ... [
 .B \-p
 .I prog
 ]
-.I file
+.I [ file ]
 .br
 .B xfs_io \-V
 .SH DESCRIPTION
@@ -25,14 +28,35 @@ These code paths include not only the obvious read/write/mmap interfaces
 for manipulating files, but also cover all of the XFS extensions (such
 as space preallocation, additional inode flags, etc).
 .SH OPTIONS
+.B xfs_io
+commands may be run interactively (the default) or as arguments on the
+command line.
+Interactive mode always runs commands on the current open file, whilst commands
+run from the command line may operate on all open files rather than the current
+open file.
+Multiple arguments may be given on the command line and they are run in the
+sequence given. The program exits one all commands have
+been run.
 .TP 1.0i
 .BI \-c " cmd"
-.B xfs_io
-commands may be run interactively (the default) or as arguments on
-the command line. Multiple
+Run the specified command on all currently open files.
+Some commands can not be run on all open files, so they will execute on only
+the current open file to maintain compatibility with historical usage.
+Multiple
+.B \-c
+arguments may be given and may be interleaved on the command line in any order
+with
+.B \-C
+commands.
+.TP
+.BI \-C " cmd"
+Run the specified command only on the active open file. 
+Multiple
+.B \-C
+arguments may be given and may be interleaved on the command line in any order
+with
 .B \-c
-arguments may be given. The commands are run in the sequence given,
-then the program exits.
+commands.
 .TP
 .BI \-p " prog"
 Set the program name for prompts and some error messages,
-- 
2.10.2


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

* Re: [PATCH 6/6] libxcmd: add non-iterating user commands
  2016-12-07  3:47 ` [PATCH 6/6] libxcmd: add non-iterating user commands Dave Chinner
@ 2016-12-07  4:49   ` Amir Goldstein
  2016-12-07  4:57     ` Amir Goldstein
                       ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Amir Goldstein @ 2016-12-07  4:49 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs, fstests

On Wed, Dec 7, 2016 at 5:47 AM, Dave Chinner <david@fromorbit.com> wrote:
> From: Dave Chinner <dchinner@redhat.com>
>

Thank you for fixing this!
See some typo fixes below.
I will test the fix later today.

Cheers,
Amir.


> Right now command iteration is not directly controllable by the
> user; it is controlled entirely by the application command flag
> setup. Sometimes we don't want commands to iterate but only operate
> on the currently selected object.
>
> For example, the stat command iterates:
>
> $ xfs_io -c "open -r foo" -c "open bar" -C "file" -c "stat" foo

-c "file"

>  000  foo            (foreign,non-sync,non-direct,read-write)
>  001  foo            (foreign,non-sync,non-direct,read-only)
> [002] bar            (foreign,non-sync,non-direct,read-write)
> fd.path = "foo"
> fd.flags = non-sync,non-direct,read-write
> stat.ino = 462399
> stat.type = regular file
> stat.size = 776508
> stat.blocks = 1528
> fd.path = "foo"
> fd.flags = non-sync,non-direct,read-only
> stat.ino = 462399
> stat.type = regular file
> stat.size = 776508
> stat.blocks = 1528
> fd.path = "bar"
> fd.flags = non-sync,non-direct,read-write
> stat.ino = 475227
> stat.type = regular file
> stat.size = 0
> stat.blocks = 0
> $
>
> To do this, add a function to supply a "non-iterating" user command
> that will execute an iterating-capable command as though it
> CMD_FLAG_ONESHOT was set. Add a new command line option to xfs_io to
> drive it (-C <command>) and connect it all up. Document it in the
> xfs_IO man page, too.

xfs_io

>
> The result of "-C stat":
>
> $ xfs_io -c "open -r foo" -c "open bar" -c "file" -C "stat" foo
>  000  foo            (foreign,non-sync,non-direct,read-write)
>  001  foo            (foreign,non-sync,non-direct,read-only)
> [002] bar            (foreign,non-sync,non-direct,read-write)
> fd.path = "bar"
> fd.flags = non-sync,non-direct,read-write
> stat.ino = 475227
> stat.type = regular file
> stat.size = 0
> stat.blocks = 0
> $
>
> Is that we only see the stat output for the active open file
> which is "bar".
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  include/command.h |  1 +
>  io/init.c         |  7 +++++--
>  libxcmd/command.c | 33 +++++++++++++++++++++++++++------
>  man/man8/xfs_io.8 | 36 ++++++++++++++++++++++++++++++------
>  4 files changed, 63 insertions(+), 14 deletions(-)
>
> diff --git a/include/command.h b/include/command.h
> index 348002cbe3ed..fb3f5c79b991 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -56,6 +56,7 @@ typedef int (*checkfunc_t)(const cmdinfo_t *ci);
>
>  extern void            add_command(const cmdinfo_t *ci);
>  extern void            add_user_command(char *optarg);
> +extern void            add_oneshot_user_command(char *optarg);
>  extern void            add_command_iterator(iterfunc_t func);
>  extern void            add_check_command(checkfunc_t cf);
>
> diff --git a/io/init.c b/io/init.c
> index 5ce627ef22c9..cf8573f0ecd5 100644
> --- a/io/init.c
> +++ b/io/init.c
> @@ -34,7 +34,7 @@ void
>  usage(void)
>  {
>         fprintf(stderr,
> -               _("Usage: %s [-adfinrRstVx] [-m mode] [-p prog] [-c cmd]... file\n"),
> +_("Usage: %s [-adfinrRstVx] [-m mode] [-p prog] [[-c|-C] cmd]... file\n"),

Suggest: [file]...

>                 progname);
>         exit(1);
>  }
> @@ -145,7 +145,7 @@ init(
>         pagesize = getpagesize();
>         gettimeofday(&stopwatch, NULL);
>
> -       while ((c = getopt(argc, argv, "ac:dFfim:p:nrRstTVx")) != EOF) {
> +       while ((c = getopt(argc, argv, "ac:C:dFfim:p:nrRstTVx")) != EOF) {
>                 switch (c) {
>                 case 'a':
>                         flags |= IO_APPEND;
> @@ -153,6 +153,9 @@ init(
>                 case 'c':
>                         add_user_command(optarg);
>                         break;
> +               case 'C':
> +                       add_oneshot_user_command(optarg);
> +                       break;
>                 case 'd':
>                         flags |= IO_DIRECT;
>                         break;
> diff --git a/libxcmd/command.c b/libxcmd/command.c
> index decc442a9d03..5917aea42611 100644
> --- a/libxcmd/command.c
> +++ b/libxcmd/command.c
> @@ -25,8 +25,14 @@ int          ncmds;
>
>  static iterfunc_t      iter_func;
>  static checkfunc_t     check_func;
> -static int             ncmdline;
> -static char            **cmdline;
> +
> +struct cmdline {
> +       char    *cmdline;
> +       bool    iterate;
> +};
> +
> +static int     ncmdline;
> +struct cmdline *cmdline;
>
>  static int
>  compare(const void *a, const void *b)
> @@ -120,12 +126,27 @@ void
>  add_user_command(char *optarg)
>  {
>         ncmdline++;
> -       cmdline = realloc(cmdline, sizeof(char*) * (ncmdline));
> +       cmdline = realloc(cmdline, sizeof(struct cmdline) * (ncmdline));
> +       if (!cmdline) {
> +               perror("realloc");
> +               exit(1);
> +       }
> +       cmdline[ncmdline-1].cmdline = optarg;
> +       cmdline[ncmdline-1].iterate = true;
> +
> +}
> +
> +void
> +add_oneshot_user_command(char *optarg)
> +{
> +       ncmdline++;
> +       cmdline = realloc(cmdline, sizeof(struct cmdline) * (ncmdline));
>         if (!cmdline) {
>                 perror("realloc");
>                 exit(1);
>         }
> -       cmdline[ncmdline-1] = optarg;
> +       cmdline[ncmdline-1].cmdline = optarg;
> +       cmdline[ncmdline-1].iterate = false;
>  }
>
>  /*
> @@ -212,14 +233,14 @@ command_loop(void)
>
>         /* command line mode */
>         for (i = 0; !done && i < ncmdline; i++) {
> -               input = strdup(cmdline[i]);
> +               input = strdup(cmdline[i].cmdline);
>                 if (!input) {
>                         fprintf(stderr,
>                                 _("cannot strdup command '%s': %s\n"),
>                                 cmdline[i], strerror(errno));
>                         exit(1);
>                 }
> -               done = process_input(input, true);
> +               done = process_input(input, cmdline[i].iterate);
>         }
>         free(cmdline);
>         return;
> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
> index 885df7f141e2..d6bacaf0438d 100644
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -9,10 +9,13 @@ xfs_io \- debug the I/O path of an XFS filesystem
>  .B \-c
>  .I cmd
>  ] ... [
> +.B \-C
> +.I cmd
> +] ... [
>  .B \-p
>  .I prog
>  ]
> -.I file
> +.I [ file ]

Suggest: [ file ] ...

>  .br
>  .B xfs_io \-V
>  .SH DESCRIPTION
> @@ -25,14 +28,35 @@ These code paths include not only the obvious read/write/mmap interfaces
>  for manipulating files, but also cover all of the XFS extensions (such
>  as space preallocation, additional inode flags, etc).
>  .SH OPTIONS
> +.B xfs_io
> +commands may be run interactively (the default) or as arguments on the
> +command line.
> +Interactive mode always runs commands on the current open file, whilst commands
> +run from the command line may operate on all open files rather than the current
> +open file.

"operate on all open files" is ambiguous. does the loop go files -> cmds or
cmds -> files? this is part of the confusion IMO.

> +Multiple arguments may be given on the command line and they are run in the
> +sequence given. The program exits one all commands have
> +been run.
>  .TP 1.0i
>  .BI \-c " cmd"
> -.B xfs_io
> -commands may be run interactively (the default) or as arguments on
> -the command line. Multiple
> +Run the specified command on all currently open files.
> +Some commands can not be run on all open files, so they will execute on only
> +the current open file to maintain compatibility with historical usage.
> +Multiple
> +.B \-c
> +arguments may be given and may be interleaved on the command line in any order
> +with
> +.B \-C
> +commands.
> +.TP
> +.BI \-C " cmd"
> +Run the specified command only on the active open file.
> +Multiple
> +.B \-C
> +arguments may be given and may be interleaved on the command line in any order
> +with
>  .B \-c
> -arguments may be given. The commands are run in the sequence given,
> -then the program exits.
> +commands.
>  .TP
>  .BI \-p " prog"
>  Set the program name for prompts and some error messages,
> --
> 2.10.2
>

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

* Re: [PATCH 6/6] libxcmd: add non-iterating user commands
  2016-12-07  4:49   ` Amir Goldstein
@ 2016-12-07  4:57     ` Amir Goldstein
  2016-12-07 14:21     ` Amir Goldstein
  2016-12-15 19:09     ` Eric Sandeen
  2 siblings, 0 replies; 20+ messages in thread
From: Amir Goldstein @ 2016-12-07  4:57 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs, fstests

On Wed, Dec 7, 2016 at 6:49 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Dec 7, 2016 at 5:47 AM, Dave Chinner <david@fromorbit.com> wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>
> Thank you for fixing this!
> See some typo fixes below.
> I will test the fix later today.
>
> Cheers,
> Amir.
>
>
>> Right now command iteration is not directly controllable by the
>> user; it is controlled entirely by the application command flag
>> setup. Sometimes we don't want commands to iterate but only operate
>> on the currently selected object.
>>
>> For example, the stat command iterates:
>>
>> $ xfs_io -c "open -r foo" -c "open bar" -C "file" -c "stat" foo
>
> -c "file"
>
>>  000  foo            (foreign,non-sync,non-direct,read-write)
>>  001  foo            (foreign,non-sync,non-direct,read-only)
>> [002] bar            (foreign,non-sync,non-direct,read-write)
>> fd.path = "foo"
>> fd.flags = non-sync,non-direct,read-write
>> stat.ino = 462399
>> stat.type = regular file
>> stat.size = 776508
>> stat.blocks = 1528
>> fd.path = "foo"
>> fd.flags = non-sync,non-direct,read-only
>> stat.ino = 462399
>> stat.type = regular file
>> stat.size = 776508
>> stat.blocks = 1528
>> fd.path = "bar"
>> fd.flags = non-sync,non-direct,read-write
>> stat.ino = 475227
>> stat.type = regular file
>> stat.size = 0
>> stat.blocks = 0
>> $
>>
>> To do this, add a function to supply a "non-iterating" user command
>> that will execute an iterating-capable command as though it
>> CMD_FLAG_ONESHOT was set. Add a new command line option to xfs_io to
>> drive it (-C <command>) and connect it all up. Document it in the
>> xfs_IO man page, too.
>
> xfs_io
>
>>
>> The result of "-C stat":
>>
>> $ xfs_io -c "open -r foo" -c "open bar" -c "file" -C "stat" foo
>>  000  foo            (foreign,non-sync,non-direct,read-write)
>>  001  foo            (foreign,non-sync,non-direct,read-only)
>> [002] bar            (foreign,non-sync,non-direct,read-write)
>> fd.path = "bar"
>> fd.flags = non-sync,non-direct,read-write
>> stat.ino = 475227
>> stat.type = regular file
>> stat.size = 0
>> stat.blocks = 0
>> $
>>
>> Is that we only see the stat output for the active open file
>> which is "bar".
>>
>> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
>> ---
>>  include/command.h |  1 +
>>  io/init.c         |  7 +++++--
>>  libxcmd/command.c | 33 +++++++++++++++++++++++++++------
>>  man/man8/xfs_io.8 | 36 ++++++++++++++++++++++++++++++------
>>  4 files changed, 63 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/command.h b/include/command.h
>> index 348002cbe3ed..fb3f5c79b991 100644
>> --- a/include/command.h
>> +++ b/include/command.h
>> @@ -56,6 +56,7 @@ typedef int (*checkfunc_t)(const cmdinfo_t *ci);
>>
>>  extern void            add_command(const cmdinfo_t *ci);
>>  extern void            add_user_command(char *optarg);
>> +extern void            add_oneshot_user_command(char *optarg);
>>  extern void            add_command_iterator(iterfunc_t func);
>>  extern void            add_check_command(checkfunc_t cf);
>>
>> diff --git a/io/init.c b/io/init.c
>> index 5ce627ef22c9..cf8573f0ecd5 100644
>> --- a/io/init.c
>> +++ b/io/init.c
>> @@ -34,7 +34,7 @@ void
>>  usage(void)
>>  {
>>         fprintf(stderr,
>> -               _("Usage: %s [-adfinrRstVx] [-m mode] [-p prog] [-c cmd]... file\n"),
>> +_("Usage: %s [-adfinrRstVx] [-m mode] [-p prog] [[-c|-C] cmd]... file\n"),
>
> Suggest: [file]...
>
>>                 progname);
>>         exit(1);
>>  }
>> @@ -145,7 +145,7 @@ init(
>>         pagesize = getpagesize();
>>         gettimeofday(&stopwatch, NULL);
>>
>> -       while ((c = getopt(argc, argv, "ac:dFfim:p:nrRstTVx")) != EOF) {
>> +       while ((c = getopt(argc, argv, "ac:C:dFfim:p:nrRstTVx")) != EOF) {
>>                 switch (c) {
>>                 case 'a':
>>                         flags |= IO_APPEND;
>> @@ -153,6 +153,9 @@ init(
>>                 case 'c':
>>                         add_user_command(optarg);
>>                         break;
>> +               case 'C':
>> +                       add_oneshot_user_command(optarg);
>> +                       break;
>>                 case 'd':
>>                         flags |= IO_DIRECT;
>>                         break;
>> diff --git a/libxcmd/command.c b/libxcmd/command.c
>> index decc442a9d03..5917aea42611 100644
>> --- a/libxcmd/command.c
>> +++ b/libxcmd/command.c
>> @@ -25,8 +25,14 @@ int          ncmds;
>>
>>  static iterfunc_t      iter_func;
>>  static checkfunc_t     check_func;
>> -static int             ncmdline;
>> -static char            **cmdline;
>> +
>> +struct cmdline {
>> +       char    *cmdline;
>> +       bool    iterate;
>> +};
>> +
>> +static int     ncmdline;
>> +struct cmdline *cmdline;
>>
>>  static int
>>  compare(const void *a, const void *b)
>> @@ -120,12 +126,27 @@ void
>>  add_user_command(char *optarg)
>>  {
>>         ncmdline++;
>> -       cmdline = realloc(cmdline, sizeof(char*) * (ncmdline));
>> +       cmdline = realloc(cmdline, sizeof(struct cmdline) * (ncmdline));
>> +       if (!cmdline) {
>> +               perror("realloc");
>> +               exit(1);
>> +       }
>> +       cmdline[ncmdline-1].cmdline = optarg;
>> +       cmdline[ncmdline-1].iterate = true;
>> +
>> +}
>> +
>> +void
>> +add_oneshot_user_command(char *optarg)
>> +{
>> +       ncmdline++;
>> +       cmdline = realloc(cmdline, sizeof(struct cmdline) * (ncmdline));
>>         if (!cmdline) {
>>                 perror("realloc");
>>                 exit(1);
>>         }
>> -       cmdline[ncmdline-1] = optarg;
>> +       cmdline[ncmdline-1].cmdline = optarg;
>> +       cmdline[ncmdline-1].iterate = false;
>>  }
>>
>>  /*
>> @@ -212,14 +233,14 @@ command_loop(void)
>>
>>         /* command line mode */
>>         for (i = 0; !done && i < ncmdline; i++) {
>> -               input = strdup(cmdline[i]);
>> +               input = strdup(cmdline[i].cmdline);
>>                 if (!input) {
>>                         fprintf(stderr,
>>                                 _("cannot strdup command '%s': %s\n"),
>>                                 cmdline[i], strerror(errno));

cmdline[i].cmdline, strerror(errno));

>>                         exit(1);
>>                 }
>> -               done = process_input(input, true);
>> +               done = process_input(input, cmdline[i].iterate);
>>         }
>>         free(cmdline);
>>         return;
>> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
>> index 885df7f141e2..d6bacaf0438d 100644
>> --- a/man/man8/xfs_io.8
>> +++ b/man/man8/xfs_io.8
>> @@ -9,10 +9,13 @@ xfs_io \- debug the I/O path of an XFS filesystem
>>  .B \-c
>>  .I cmd
>>  ] ... [
>> +.B \-C
>> +.I cmd
>> +] ... [
>>  .B \-p
>>  .I prog
>>  ]
>> -.I file
>> +.I [ file ]
>
> Suggest: [ file ] ...
>
>>  .br
>>  .B xfs_io \-V
>>  .SH DESCRIPTION
>> @@ -25,14 +28,35 @@ These code paths include not only the obvious read/write/mmap interfaces
>>  for manipulating files, but also cover all of the XFS extensions (such
>>  as space preallocation, additional inode flags, etc).
>>  .SH OPTIONS
>> +.B xfs_io
>> +commands may be run interactively (the default) or as arguments on the
>> +command line.
>> +Interactive mode always runs commands on the current open file, whilst commands
>> +run from the command line may operate on all open files rather than the current
>> +open file.
>
> "operate on all open files" is ambiguous. does the loop go files -> cmds or
> cmds -> files? this is part of the confusion IMO.
>
>> +Multiple arguments may be given on the command line and they are run in the
>> +sequence given. The program exits one all commands have
>> +been run.
>>  .TP 1.0i
>>  .BI \-c " cmd"
>> -.B xfs_io
>> -commands may be run interactively (the default) or as arguments on
>> -the command line. Multiple
>> +Run the specified command on all currently open files.
>> +Some commands can not be run on all open files, so they will execute on only
>> +the current open file to maintain compatibility with historical usage.
>> +Multiple
>> +.B \-c
>> +arguments may be given and may be interleaved on the command line in any order
>> +with
>> +.B \-C
>> +commands.
>> +.TP
>> +.BI \-C " cmd"
>> +Run the specified command only on the active open file.
>> +Multiple
>> +.B \-C
>> +arguments may be given and may be interleaved on the command line in any order
>> +with
>>  .B \-c
>> -arguments may be given. The commands are run in the sequence given,
>> -then the program exits.
>> +commands.
>>  .TP
>>  .BI \-p " prog"
>>  Set the program name for prompts and some error messages,
>> --
>> 2.10.2
>>

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

* Re: [PATCH 6/6] libxcmd: add non-iterating user commands
  2016-12-07  4:49   ` Amir Goldstein
  2016-12-07  4:57     ` Amir Goldstein
@ 2016-12-07 14:21     ` Amir Goldstein
  2016-12-07 20:16       ` Dave Chinner
  2016-12-15 19:09     ` Eric Sandeen
  2 siblings, 1 reply; 20+ messages in thread
From: Amir Goldstein @ 2016-12-07 14:21 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs, fstests

On Wed, Dec 7, 2016 at 6:49 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Dec 7, 2016 at 5:47 AM, Dave Chinner <david@fromorbit.com> wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>
> Thank you for fixing this!
> See some typo fixes below.
> I will test the fix later today.
>

Short of one compilation warning I commented on,
I tested these changes and found no regression with -g quick run
I also verified that my test can be converted to use the one shot commands,
e.g.:

$XFS_IO_PROG -r foo \
       -C "open foo" \
       -C "pwrite -S 0x61 0 16" \
       -C "file 0" \
       -C "pread -v 0 16" \
| _filter_xfs_io

$XFS_IO_PROG -r bar \
       -C "mmap -r 0 16" \
       -C "open bar" \
       -C "pwrite -S 0x61 0 16" \
       -C "mread -v 0 16" \
| _filter_xfs_io

Notice that I used explicit -C for all commands including the implicit
one shot commands.

1. Do you think that xfs_io should error on -c "open foo"  to force
explicit -C "open foo"?
    it can't be breaking any existing users, because -c "open foo" is
already very broken.
2. You marked mmap ONE_SHOT, but not all the m* commands.
   Stands to reason that they should all be marked ONE_SHOT. because iterating
   the file table has nothing to do with the m* commands. no?

About the fix to overlay/016.
How would you prefer to address the conditional availability of xfs_io -C?

1. new helper _require_xfs_io_one_shot_command
2. _require_xfs_io_command "open" (which internally checks for xfs_io -C "open")
3. third option?

Cheers,
Amir.

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

* Re: [PATCH 6/6] libxcmd: add non-iterating user commands
  2016-12-07 14:21     ` Amir Goldstein
@ 2016-12-07 20:16       ` Dave Chinner
  2016-12-08 10:14         ` Amir Goldstein
  0 siblings, 1 reply; 20+ messages in thread
From: Dave Chinner @ 2016-12-07 20:16 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: linux-xfs, fstests

On Wed, Dec 07, 2016 at 04:21:31PM +0200, Amir Goldstein wrote:
> On Wed, Dec 7, 2016 at 6:49 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Wed, Dec 7, 2016 at 5:47 AM, Dave Chinner <david@fromorbit.com> wrote:
> >> From: Dave Chinner <dchinner@redhat.com>
> >>
> >
> > Thank you for fixing this!
> > See some typo fixes below.
> > I will test the fix later today.
> >
> 
> Short of one compilation warning I commented on,
> I tested these changes and found no regression with -g quick run
> I also verified that my test can be converted to use the one shot commands,
> e.g.:
> 
> $XFS_IO_PROG -r foo \
>        -C "open foo" \
>        -C "pwrite -S 0x61 0 16" \
>        -C "file 0" \
>        -C "pread -v 0 16" \
> | _filter_xfs_io
> 
> $XFS_IO_PROG -r bar \
>        -C "mmap -r 0 16" \
>        -C "open bar" \
>        -C "pwrite -S 0x61 0 16" \
>        -C "mread -v 0 16" \
> | _filter_xfs_io
> 
> Notice that I used explicit -C for all commands including the implicit
> one shot commands.
> 
> 1. Do you think that xfs_io should error on -c "open foo"  to force
> explicit -C "open foo"?

No.

>     it can't be breaking any existing users, because -c "open foo" is
> already very broken.

Maybe so, but there are users of it. e.g:

$ git grep open |grep XFS_IO
tests/overlay/001:      $XFS_IO_PROG -c "open" $f >>$seqres.full
$

This is precisely why I made oneshot commands just work silently
with "-c" - who knows what will break if we start rejecting commands
that otherwise work just fine when there is only one open file....

> 2. You marked mmap ONE_SHOT, but not all the m* commands.
>    Stands to reason that they should all be marked ONE_SHOT. because iterating
>    the file table has nothing to do with the m* commands. no?

It is not clear to me what the correct thing to do here is, I don't
have the time right now to look into it, so I didn't
change mread/mwrite/msync behaviour. If it's broken before it is
still broken now.

> About the fix to overlay/016.
> How would you prefer to address the conditional availability of xfs_io -C?
> 
> 1. new helper _require_xfs_io_one_shot_command
> 2. _require_xfs_io_command "open" (which internally checks for xfs_io -C "open")
> 3. third option?

I don't really care - #2 is probably neatest. If what you do is too
ugly to live then I'll let you know.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

* Re: [PATCH 6/6] libxcmd: add non-iterating user commands
  2016-12-07 20:16       ` Dave Chinner
@ 2016-12-08 10:14         ` Amir Goldstein
  2016-12-08 22:22           ` Dave Chinner
  0 siblings, 1 reply; 20+ messages in thread
From: Amir Goldstein @ 2016-12-08 10:14 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs, fstests

On Wed, Dec 7, 2016 at 10:16 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Wed, Dec 07, 2016 at 04:21:31PM +0200, Amir Goldstein wrote:
...
>>
>> 1. Do you think that xfs_io should error on -c "open foo"  to force
>> explicit -C "open foo"?
>
> No.
>
>>     it can't be breaking any existing users, because -c "open foo" is
>> already very broken.
>
> Maybe so, but there are users of it. e.g:
>
> $ git grep open |grep XFS_IO
> tests/overlay/001:      $XFS_IO_PROG -c "open" $f >>$seqres.full
> $
>
> This is precisely why I made oneshot commands just work silently
> with "-c" - who knows what will break if we start rejecting commands
> that otherwise work just fine when there is only one open file....
>

Interesting example. Here -c "open" is actually an 'alias' for
-c "stat", which could have been non one shot.
Nevertheless, I see your point.

>> 2. You marked mmap ONE_SHOT, but not all the m* commands.
>>    Stands to reason that they should all be marked ONE_SHOT. because iterating
>>    the file table has nothing to do with the m* commands. no?
>
> It is not clear to me what the correct thing to do here is, I don't
> have the time right now to look into it, so I didn't
> change mread/mwrite/msync behaviour. If it's broken before it is
> still broken now.
>

Very well, but I am not sure why mmap should be marked one shot?
Possibly, mmap caught your filters because it is CMD_NOFILE_OK,
but in fact, the only case where mmap with no open files works is
after mmaping files and closing all open files.

Currently, this command:
$ xfs_io -c "mmap 0 4" -C "mmap" foo bar

Results in:
[000] 0x7f17319ae000 - 0x7f17319ae004 rwx             bar (0 : 4)

IMO, it would be more useful if it resulted in:
 000  0x7f27e289d000 - 0x7f27e289d004 rwx             foo (0 : 4)
[001] 0x7f27e289c000 - 0x7f27e289c004 rwx             bar (0 : 4)

Which will allow following up with more specific -C m* commands
and it would be more consistent with the result of:
$ xfs_io -C "file" foo bar
 000  foo            (foreign,non-sync,non-direct,read-write)
[001] bar            (foreign,non-sync,non-direct,read-write)



>> About the fix to overlay/016.
>> How would you prefer to address the conditional availability of xfs_io -C?
>>
>> 1. new helper _require_xfs_io_one_shot_command
>> 2. _require_xfs_io_command "open" (which internally checks for xfs_io -C "open")
>> 3. third option?
>
> I don't really care - #2 is probably neatest. If what you do is too
> ugly to live then I'll let you know.
>

I trust that you will :-)
Will post it soon.

Thanks,
Amir.

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

* Re: [PATCH 6/6] libxcmd: add non-iterating user commands
  2016-12-08 10:14         ` Amir Goldstein
@ 2016-12-08 22:22           ` Dave Chinner
  0 siblings, 0 replies; 20+ messages in thread
From: Dave Chinner @ 2016-12-08 22:22 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: linux-xfs, fstests

On Thu, Dec 08, 2016 at 12:14:24PM +0200, Amir Goldstein wrote:
> On Wed, Dec 7, 2016 at 10:16 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Wed, Dec 07, 2016 at 04:21:31PM +0200, Amir Goldstein wrote:
> ...
> >>
> >> 1. Do you think that xfs_io should error on -c "open foo"  to force
> >> explicit -C "open foo"?
> >
> > No.
> >
> >>     it can't be breaking any existing users, because -c "open foo" is
> >> already very broken.
> >
> > Maybe so, but there are users of it. e.g:
> >
> > $ git grep open |grep XFS_IO
> > tests/overlay/001:      $XFS_IO_PROG -c "open" $f >>$seqres.full
> > $
> >
> > This is precisely why I made oneshot commands just work silently
> > with "-c" - who knows what will break if we start rejecting commands
> > that otherwise work just fine when there is only one open file....
> >
> 
> Interesting example. Here -c "open" is actually an 'alias' for
> -c "stat", which could have been non one shot.
> Nevertheless, I see your point.
> 
> >> 2. You marked mmap ONE_SHOT, but not all the m* commands.
> >>    Stands to reason that they should all be marked ONE_SHOT. because iterating
> >>    the file table has nothing to do with the m* commands. no?
> >
> > It is not clear to me what the correct thing to do here is, I don't
> > have the time right now to look into it, so I didn't
> > change mread/mwrite/msync behaviour. If it's broken before it is
> > still broken now.
> >
> 
> Very well, but I am not sure why mmap should be marked one shot?

Look at the help documentation:

$ xfs_io -c "help mmap"
mmap [N] | [-rwx] [-s size] [off len] -- mmap a range in the current file, show mappings

 maps a range within the current file into memory

 Example:
 'mmap -rw 0 1m' - maps one megabyte from the start of the current file
....

It explicitly and repeatedly says "current file" in the
description of it's operation. Any typical user is going to read
that and think that it means "current open file", not "map a range
on all open files".

> Possibly, mmap caught your filters because it is CMD_NOFILE_OK,
> but in fact, the only case where mmap with no open files works is
> after mmaping files and closing all open files.

Well, yes. We've come across applications that do exactly this
in the past, and had to simulate their behaviours. It's entirely
reasonable to want to list or change the active mappings after all
the open files have been closed.

> Currently, this command:
> $ xfs_io -c "mmap 0 4" -C "mmap" foo bar
> 
> Results in:
> [000] 0x7f17319ae000 - 0x7f17319ae004 rwx             bar (0 : 4)
> 
> IMO, it would be more useful if it resulted in:
>  000  0x7f27e289d000 - 0x7f27e289d004 rwx             foo (0 : 4)
> [001] 0x7f27e289c000 - 0x7f27e289c004 rwx             bar (0 : 4)
> 
> Which will allow following up with more specific -C m* commands
> and it would be more consistent with the result of:
> $ xfs_io -C "file" foo bar
>  000  foo            (foreign,non-sync,non-direct,read-write)
> [001] bar            (foreign,non-sync,non-direct,read-write)

Maybe so but, unfortunately, mapping tables are different to the
file tables.

As I have already said: it's not clear what the correct behaviour
here is because mapping commands /don't iterate the mapping table/.
The exception is the mmap command, and that mapping table iteration
behaviour is the reason I changed mmap to be a oneshot command,
otherwise it does stupid things when multiple files are open
and they are iterated.

If you want to have mmap commands do sane things for iteration, then
you need to address the file vs mapping iteration problem, not hack
special one-off behaviours into the mmap commands.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

* Re: [PATCH 5/6] xfs_io: make various commands one-shot only
  2016-12-07  3:47 ` [PATCH 5/6] xfs_io: make various commands one-shot only Dave Chinner
@ 2016-12-15 18:21   ` Eric Sandeen
  2016-12-16  0:53     ` Dave Chinner
  0 siblings, 1 reply; 20+ messages in thread
From: Eric Sandeen @ 2016-12-15 18:21 UTC (permalink / raw)
  To: Dave Chinner, linux-xfs; +Cc: fstests, amir73il

On 12/6/16 9:47 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It makes no sense to iterate the file table for some xfs_io
> commands. Some commands are already marked in this way, but lots of
> them are not and this leads to bad behaviour. For example, the open
> command will run until the process fd table is full and EMFILE is
> returned rather than just opening the specified file once.

Ok, I'm not quite clear on when we should expect commands to be
"oneshot"

With freeze, for example, this:

xfs_io -x -c freeze mnt/file mnt2/file

will freeze both filesystems today.  With this change,

xfs_io -x -c freeze mnt/file mnt2/file

freezes the mnt2 filesystem but not mnt.  Is that desired?

I guess the command /is/ documented as "freeze fs of /current/ file"
but i wonder if that's just an accident of documentation.

ditto for i.e. the inode command, or resblks - why not
iterate those?

Thanks,
-Eric

> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  io/file.c      | 2 +-
>  io/freeze.c    | 4 ++--
>  io/getrusage.c | 3 ++-
>  io/imap.c      | 2 +-
>  io/inject.c    | 2 +-
>  io/link.c      | 2 +-
>  io/mmap.c      | 3 ++-
>  io/open.c      | 7 ++++---
>  io/reflink.c   | 4 ++--
>  io/resblks.c   | 2 +-
>  io/shutdown.c  | 2 +-
>  io/sync.c      | 3 ++-
>  12 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/io/file.c b/io/file.c
> index 8e3f07122922..349b19cdc420 100644
> --- a/io/file.c
> +++ b/io/file.c
> @@ -95,7 +95,7 @@ file_init(void)
>  	file_cmd.cfunc = file_f;
>  	file_cmd.argmin = 0;
>  	file_cmd.argmax = 1;
> -	file_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	file_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	file_cmd.oneline = _("set the current file");
>  
>  	print_cmd.name = "print";
> diff --git a/io/freeze.c b/io/freeze.c
> index 3d0d2a4b5601..0305713d99e8 100644
> --- a/io/freeze.c
> +++ b/io/freeze.c
> @@ -65,14 +65,14 @@ freeze_init(void)
>  	freeze_cmd.cfunc = freeze_f;
>  	freeze_cmd.argmin = 0;
>  	freeze_cmd.argmax = 0;
> -	freeze_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	freeze_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	freeze_cmd.oneline = _("freeze filesystem of current file");
>  
>  	thaw_cmd.name = "thaw";
>  	thaw_cmd.cfunc = thaw_f;
>  	thaw_cmd.argmin = 0;
>  	thaw_cmd.argmax = 0;
> -	thaw_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	thaw_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	thaw_cmd.oneline = _("unfreeze filesystem of current file");
>  
>  	if (expert) {
> diff --git a/io/getrusage.c b/io/getrusage.c
> index bccf94cbc302..cf1f2afd19a8 100644
> --- a/io/getrusage.c
> +++ b/io/getrusage.c
> @@ -113,7 +113,8 @@ getrusage_init(void)
>  	getrusage_cmd.argmin = 0;
>  	getrusage_cmd.argmax = -1;
>  	getrusage_cmd.cfunc = getrusage_f;
> -	getrusage_cmd.flags = CMD_NOFILE_OK | CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	getrusage_cmd.flags = CMD_NOFILE_OK | CMD_NOMAP_OK |
> +			      CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	getrusage_cmd.oneline = _("report process resource usage");
>  
>  	if (expert)
> diff --git a/io/imap.c b/io/imap.c
> index 7123432f411f..f52238e0c450 100644
> --- a/io/imap.c
> +++ b/io/imap.c
> @@ -72,7 +72,7 @@ imap_init(void)
>  	imap_cmd.argmin = 0;
>  	imap_cmd.argmax = 1;
>  	imap_cmd.args = _("[nentries]");
> -	imap_cmd.flags = CMD_NOMAP_OK;
> +	imap_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
>  	imap_cmd.oneline = _("inode map for filesystem of current file");
>  
>  	if (expert)
> diff --git a/io/inject.c b/io/inject.c
> index 5d5e4aef3dfc..25c70218a1ef 100644
> --- a/io/inject.c
> +++ b/io/inject.c
> @@ -163,7 +163,7 @@ inject_init(void)
>  	inject_cmd.cfunc = inject_f;
>  	inject_cmd.argmin = 0;
>  	inject_cmd.argmax = -1;
> -	inject_cmd.flags = CMD_NOMAP_OK;
> +	inject_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
>  	inject_cmd.args = _("[tag ...]");
>  	inject_cmd.oneline = _("inject errors into a filesystem");
>  	inject_cmd.help = inject_help;
> diff --git a/io/link.c b/io/link.c
> index ccf8e691bb1d..9b2e8a970942 100644
> --- a/io/link.c
> +++ b/io/link.c
> @@ -59,7 +59,7 @@ flink_init(void)
>  	flink_cmd.cfunc = flink_f;
>  	flink_cmd.argmin = 1;
>  	flink_cmd.argmax = 1;
> -	flink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	flink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	flink_cmd.args = _("filename");
>  	flink_cmd.oneline =
>  		_("link the open file descriptor to the supplied filename");
> diff --git a/io/mmap.c b/io/mmap.c
> index dc188d0557cf..e2d8d5a92326 100644
> --- a/io/mmap.c
> +++ b/io/mmap.c
> @@ -674,7 +674,8 @@ mmap_init(void)
>  	mmap_cmd.cfunc = mmap_f;
>  	mmap_cmd.argmin = 0;
>  	mmap_cmd.argmax = -1;
> -	mmap_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK | CMD_FOREIGN_OK;
> +	mmap_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK |
> +			 CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	mmap_cmd.args = _("[N] | [-rwx] [-s size] [off len]");
>  	mmap_cmd.oneline =
>  		_("mmap a range in the current file, show mappings");
> diff --git a/io/open.c b/io/open.c
> index 722d0f9eacf5..a12f4a2ba528 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -918,7 +918,8 @@ open_init(void)
>  	open_cmd.cfunc = open_f;
>  	open_cmd.argmin = 0;
>  	open_cmd.argmax = -1;
> -	open_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK | CMD_FOREIGN_OK;
> +	open_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK |
> +			 CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	open_cmd.args = _("[-acdrstxT] [-m mode] [path]");
>  	open_cmd.oneline = _("open the file specified by path");
>  	open_cmd.help = open_help;
> @@ -936,7 +937,7 @@ open_init(void)
>  	close_cmd.cfunc = close_f;
>  	close_cmd.argmin = 0;
>  	close_cmd.argmax = 0;
> -	close_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	close_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	close_cmd.oneline = _("close the current open file");
>  
>  	statfs_cmd.name = "statfs";
> @@ -980,7 +981,7 @@ open_init(void)
>  	inode_cmd.args = _("[-nv] [num]");
>  	inode_cmd.argmin = 0;
>  	inode_cmd.argmax = 3;
> -	inode_cmd.flags = CMD_NOMAP_OK;
> +	inode_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
>  	inode_cmd.oneline =
>  		_("Query inode number usage in the filesystem");
>  	inode_cmd.help = inode_help;
> diff --git a/io/reflink.c b/io/reflink.c
> index a09e82dca80a..a22b6b4a07e3 100644
> --- a/io/reflink.c
> +++ b/io/reflink.c
> @@ -304,7 +304,7 @@ reflink_init(void)
>  	reflink_cmd.cfunc = reflink_f;
>  	reflink_cmd.argmin = 4;
>  	reflink_cmd.argmax = -1;
> -	reflink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	reflink_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	reflink_cmd.args =
>  _("infile src_off dst_off len");
>  	reflink_cmd.oneline =
> @@ -318,7 +318,7 @@ _("infile src_off dst_off len");
>  	dedupe_cmd.cfunc = dedupe_f;
>  	dedupe_cmd.argmin = 4;
>  	dedupe_cmd.argmax = -1;
> -	dedupe_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> +	dedupe_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	dedupe_cmd.args =
>  _("infile src_off dst_off len");
>  	dedupe_cmd.oneline =
> diff --git a/io/resblks.c b/io/resblks.c
> index 73318ae03fd2..06903f5bb748 100644
> --- a/io/resblks.c
> +++ b/io/resblks.c
> @@ -61,7 +61,7 @@ resblks_init(void)
>  	resblks_cmd.cfunc = resblks_f;
>  	resblks_cmd.argmin = 0;
>  	resblks_cmd.argmax = 1;
> -	resblks_cmd.flags = CMD_NOMAP_OK;
> +	resblks_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
>  	resblks_cmd.args = _("[blocks]");
>  	resblks_cmd.oneline =
>  		_("get and/or set count of reserved filesystem blocks");
> diff --git a/io/shutdown.c b/io/shutdown.c
> index d8507cc78af7..d9cd520d11e2 100644
> --- a/io/shutdown.c
> +++ b/io/shutdown.c
> @@ -54,7 +54,7 @@ shutdown_init(void)
>  	shutdown_cmd.cfunc = shutdown_f;
>  	shutdown_cmd.argmin = 0;
>  	shutdown_cmd.argmax = 1;
> -	shutdown_cmd.flags = CMD_NOMAP_OK;
> +	shutdown_cmd.flags = CMD_NOMAP_OK | CMD_FLAG_ONESHOT;
>  	shutdown_cmd.args = _("[-f]");
>  	shutdown_cmd.oneline =
>  		_("shuts down the filesystem where the current file resides");
> diff --git a/io/sync.c b/io/sync.c
> index 28e3a15e0a96..c77263804a35 100644
> --- a/io/sync.c
> +++ b/io/sync.c
> @@ -52,7 +52,8 @@ sync_init(void)
>  {
>  	sync_cmd.name = "sync";
>  	sync_cmd.cfunc = sync_f;
> -	sync_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK | CMD_FOREIGN_OK;
> +	sync_cmd.flags = CMD_NOMAP_OK | CMD_NOFILE_OK |
> +			 CMD_FOREIGN_OK | CMD_FLAG_ONESHOT;
>  	sync_cmd.oneline =
>  		_("calls sync(2) to flush all in-core filesystem state to disk");
>  
> 

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

* Re: [PATCH 6/6] libxcmd: add non-iterating user commands
  2016-12-07  4:49   ` Amir Goldstein
  2016-12-07  4:57     ` Amir Goldstein
  2016-12-07 14:21     ` Amir Goldstein
@ 2016-12-15 19:09     ` Eric Sandeen
  2 siblings, 0 replies; 20+ messages in thread
From: Eric Sandeen @ 2016-12-15 19:09 UTC (permalink / raw)
  To: Amir Goldstein, Dave Chinner; +Cc: linux-xfs, fstests

On 12/6/16 10:49 PM, Amir Goldstein wrote:
>> @@ -25,14 +28,35 @@ These code paths include not only the obvious read/write/mmap interfaces
>>  for manipulating files, but also cover all of the XFS extensions (such
>>  as space preallocation, additional inode flags, etc).
>>  .SH OPTIONS
>> +.B xfs_io
>> +commands may be run interactively (the default) or as arguments on the
>> +command line.
>> +Interactive mode always runs commands on the current open file, whilst commands
>> +run from the command line may operate on all open files rather than the current
>> +open file.
> "operate on all open files" is ambiguous. does the loop go files -> cmds or
> cmds -> files? this is part of the confusion IMO.

How about adding something like:

Files specified at the end of the command line are added to the file
table first, then files specified in -c and -C commands are added.
The last file in the file table is the current open file.

-Eric 

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

* Re: [PATCH 5/6] xfs_io: make various commands one-shot only
  2016-12-15 18:21   ` Eric Sandeen
@ 2016-12-16  0:53     ` Dave Chinner
  2016-12-16  1:50       ` Eric Sandeen
  0 siblings, 1 reply; 20+ messages in thread
From: Dave Chinner @ 2016-12-16  0:53 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs, fstests, amir73il

On Thu, Dec 15, 2016 at 12:21:43PM -0600, Eric Sandeen wrote:
> On 12/6/16 9:47 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > It makes no sense to iterate the file table for some xfs_io
> > commands. Some commands are already marked in this way, but lots of
> > them are not and this leads to bad behaviour. For example, the open
> > command will run until the process fd table is full and EMFILE is
> > returned rather than just opening the specified file once.
> 
> Ok, I'm not quite clear on when we should expect commands to be
> "oneshot"
> 
> With freeze, for example, this:
> 
> xfs_io -x -c freeze mnt/file mnt2/file
> 
> will freeze both filesystems today.  With this change,

xfs_freeze will ignore multiple filesystem options, so it only ever
passes a single file to xfs_io.  IOWs, it's behaviour will be
completely unchanged by making the xfs_io freeze command a one-shot
command.

And right now, I think that's the only case we have to care about.
unless someone is actually using xfs_io directly to freeze multiple
filesystems like this, then I think it's better to make it a
one-shot command.

> xfs_io -x -c freeze mnt/file mnt2/file
> 
> freezes the mnt2 filesystem but not mnt.  Is that desired?

It's exactly what the man page says the freeze command does.

> I guess the command /is/ documented as "freeze fs of /current/ file"
> but i wonder if that's just an accident of documentation.

If the man page documents it as operating on the current file,
but instead it freezes all the open files, then that's a bug that
needs fixing.

> ditto for i.e. the inode command, or resblks - why not
> iterate those?

Because they are aimed at single, specific filesystem operations
only. It just doesn't make sense to iterate them across all open
files inside xfs_io. If you have multiple filesystems youneed to
query/modify, then do an xfs_io call for each. 

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

* Re: [PATCH 5/6] xfs_io: make various commands one-shot only
  2016-12-16  0:53     ` Dave Chinner
@ 2016-12-16  1:50       ` Eric Sandeen
  2016-12-16  4:21         ` Dave Chinner
  0 siblings, 1 reply; 20+ messages in thread
From: Eric Sandeen @ 2016-12-16  1:50 UTC (permalink / raw)
  To: Dave Chinner; +Cc: linux-xfs, fstests, amir73il

On 12/15/16 6:53 PM, Dave Chinner wrote:
> On Thu, Dec 15, 2016 at 12:21:43PM -0600, Eric Sandeen wrote:
>> On 12/6/16 9:47 PM, Dave Chinner wrote:
>>> From: Dave Chinner <dchinner@redhat.com>
>>>
>>> It makes no sense to iterate the file table for some xfs_io
>>> commands. Some commands are already marked in this way, but lots of
>>> them are not and this leads to bad behaviour. For example, the open
>>> command will run until the process fd table is full and EMFILE is
>>> returned rather than just opening the specified file once.
>>
>> Ok, I'm not quite clear on when we should expect commands to be
>> "oneshot"
>>
>> With freeze, for example, this:
>>
>> xfs_io -x -c freeze mnt/file mnt2/file
>>
>> will freeze both filesystems today.  With this change,
> 
> xfs_freeze will ignore multiple filesystem options, so it only ever
> passes a single file to xfs_io.  IOWs, it's behaviour will be
> completely unchanged by making the xfs_io freeze command a one-shot
> command.

Sure, but xfs_freeze is not xfs_io, it's a wrapper which by design
presents /simple/ single-fs arguments to xfs_io.
 
> And right now, I think that's the only case we have to care about.
> unless someone is actually using xfs_io directly to freeze multiple
> filesystems like this, then I think it's better to make it a
> one-shot command.
> 
>> xfs_io -x -c freeze mnt/file mnt2/file
>>
>> freezes the mnt2 filesystem but not mnt.  Is that desired?
> 
> It's exactly what the man page says the freeze command does.
> 
>> I guess the command /is/ documented as "freeze fs of /current/ file"
>> but i wonder if that's just an accident of documentation.
> 
> If the man page documents it as operating on the current file,
> but instead it freezes all the open files, then that's a bug that
> needs fixing.

yes, in either the man page /or/ the code...
 
>> ditto for i.e. the inode command, or resblks - why not
>> iterate those?
> 
> Because they are aimed at single, specific filesystem operations
> only. It just doesn't make sense to iterate them across all open
> files inside xfs_io. If you have multiple filesystems youneed to
> query/modify, then do an xfs_io call for each. 

Ok, I guess this finally clicked for me; a very easily described
test for whether the flag gets set:

  Only operations which specifically operate on /files/ will iterate*.

  System-wide and fs-wide operations (even if they happen to take a
  file as an argument, as i.e. freeze can) do /not/ iterate.

*and open doesn't iterate because recursion :)

I'm happy with that, though I think it should be documented clearly.

Sorry if I was slow getting to your point.

-Eric

> Cheers,
> 
> Dave.
> 

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

* Re: [PATCH 5/6] xfs_io: make various commands one-shot only
  2016-12-16  1:50       ` Eric Sandeen
@ 2016-12-16  4:21         ` Dave Chinner
  0 siblings, 0 replies; 20+ messages in thread
From: Dave Chinner @ 2016-12-16  4:21 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs, fstests, amir73il

On Thu, Dec 15, 2016 at 07:50:33PM -0600, Eric Sandeen wrote:
> On 12/15/16 6:53 PM, Dave Chinner wrote:
> > On Thu, Dec 15, 2016 at 12:21:43PM -0600, Eric Sandeen wrote:
> > Because they are aimed at single, specific filesystem operations
> > only. It just doesn't make sense to iterate them across all open
> > files inside xfs_io. If you have multiple filesystems youneed to
> > query/modify, then do an xfs_io call for each. 
> 
> Ok, I guess this finally clicked for me; a very easily described
> test for whether the flag gets set:
> 
>   Only operations which specifically operate on /files/ will iterate*.
> 
>   System-wide and fs-wide operations (even if they happen to take a
>   file as an argument, as i.e. freeze can) do /not/ iterate.
> 
> *and open doesn't iterate because recursion :)
> 
> I'm happy with that, though I think it should be documented clearly.
> 
> Sorry if I was slow getting to your point.

No, you're not slow, I simply couldn't explain my rationale as
clearly and obviously as you've just done. I'll add that to my man
page updates, and repost the series for you.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

* Re: [PATCH 0/6] xfs_io: fix up command iteration
  2016-12-07  3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
                   ` (5 preceding siblings ...)
  2016-12-07  3:47 ` [PATCH 6/6] libxcmd: add non-iterating user commands Dave Chinner
@ 2017-01-12  5:14 ` Amir Goldstein
  2017-01-12 12:52   ` Eric Sandeen
  6 siblings, 1 reply; 20+ messages in thread
From: Amir Goldstein @ 2017-01-12  5:14 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs, fstests, Dave Chinner, Christoph Hellwig

On Wed, Dec 7, 2016 at 5:47 AM, Dave Chinner <david@fromorbit.com> wrote:
> Hi folks,
>
> Here are teh changes I've put together to address the command
> iteration problems that xfs_io is demonstrating. I described the
> history that lead us to the current problems here:
>
> https://www.spinics.net/lists/fstests/msg04572.html
>
> This series cleans up the libxcmd command code and fixes the xfs_io
> problems without completely removing the existing iteration
> behaviour.
>
> The first three patches address the "args_command" abstraction,
> renaming it to a "command iterator" abstraction and clean up the
> command loop implementation to make it easier to follow. It is now
> clear that the the command loop has an external command iterator
> control function, and it has a clean and clear mechanism for the
> external commands to prevent iteration from occurring
> (CMD_FLAG_ONESHOT).
>
> The fourth patch cleans up a recent change made to the xfs_quota
> interface to support foreign filesystems - it still needs to be abel
> to run the help and quit commands regardless of other state. The
> same requirement exists for xfs_io, it just implemented the command
> check that allowed them to be run differently. This patch
> centralises that by marking libxcmd functions as "library" functions
> and skips the application provided command checking altogether.
> Hence the apps no longer need to handle these cases at all.
>
> The fifth patch makes all the xfs_io commands that shouldn't iterate
> the file table one-shot only commands. This avoids all the nasty
> problems with commands like open that end up completely filling
> the open file table with thousands of filedescriptors pointing to
> the same file. THis makes those commands usable and predictable.
>
> Finally, the last patch adds CLI support for running xfs_io commands
> as one-shot commands rather than iterating commands. This is useful
> for being able to set up multiple files and operate on them as a
> whole or individually as necessary. This patch also documents in the
> man page that some commands may iterate all open files and that the
> new "-C" command option can be used to avoid problems arising from
> this historical behaviour.
>
> -Dave.
>

Eric,

I wanted to make sure that this work by Dave is on your radar for next.
Please note that most of the patches got Reviewed-by Christoph on v1,
but Dave did not add the tag on v2 posting.

Thanks,
Amir.

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

* Re: [PATCH 0/6] xfs_io: fix up command iteration
  2017-01-12  5:14 ` [PATCH 0/6] xfs_io: fix up command iteration Amir Goldstein
@ 2017-01-12 12:52   ` Eric Sandeen
  0 siblings, 0 replies; 20+ messages in thread
From: Eric Sandeen @ 2017-01-12 12:52 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: linux-xfs, fstests, Dave Chinner, Christoph Hellwig

On 1/11/17 11:14 PM, Amir Goldstein wrote:
> On Wed, Dec 7, 2016 at 5:47 AM, Dave Chinner <david@fromorbit.com> wrote:
>> Hi folks,
>>
>> Here are teh changes I've put together to address the command
>> iteration problems that xfs_io is demonstrating. I described the
>> history that lead us to the current problems here:
>>
>> https://www.spinics.net/lists/fstests/msg04572.html
>>
>> This series cleans up the libxcmd command code and fixes the xfs_io
>> problems without completely removing the existing iteration
>> behaviour.
>>
>> The first three patches address the "args_command" abstraction,
>> renaming it to a "command iterator" abstraction and clean up the
>> command loop implementation to make it easier to follow. It is now
>> clear that the the command loop has an external command iterator
>> control function, and it has a clean and clear mechanism for the
>> external commands to prevent iteration from occurring
>> (CMD_FLAG_ONESHOT).
>>
>> The fourth patch cleans up a recent change made to the xfs_quota
>> interface to support foreign filesystems - it still needs to be abel
>> to run the help and quit commands regardless of other state. The
>> same requirement exists for xfs_io, it just implemented the command
>> check that allowed them to be run differently. This patch
>> centralises that by marking libxcmd functions as "library" functions
>> and skips the application provided command checking altogether.
>> Hence the apps no longer need to handle these cases at all.
>>
>> The fifth patch makes all the xfs_io commands that shouldn't iterate
>> the file table one-shot only commands. This avoids all the nasty
>> problems with commands like open that end up completely filling
>> the open file table with thousands of filedescriptors pointing to
>> the same file. THis makes those commands usable and predictable.
>>
>> Finally, the last patch adds CLI support for running xfs_io commands
>> as one-shot commands rather than iterating commands. This is useful
>> for being able to set up multiple files and operate on them as a
>> whole or individually as necessary. This patch also documents in the
>> man page that some commands may iterate all open files and that the
>> new "-C" command option can be used to avoid problems arising from
>> this historical behaviour.
>>
>> -Dave.
>>
> 
> Eric,
> 
> I wanted to make sure that this work by Dave is on your radar for next.
> Please note that most of the patches got Reviewed-by Christoph on v1,
> but Dave did not add the tag on v2 posting.

Yes, it's in my queue.

I wanted to do the libxfs sync first, then add the bits outside that.

Thanks,
-Eric
 
> Thanks,
> Amir.
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

end of thread, other threads:[~2017-01-12 12:52 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-07  3:47 [PATCH 0/6] xfs_io: fix up command iteration Dave Chinner
2016-12-07  3:47 ` [PATCH 1/6] libxcmd: check CMD_FLAG_GLOBAL inside args_command() Dave Chinner
2016-12-07  3:47 ` [PATCH 2/6] libxcmd: rename args_command to command_iterator Dave Chinner
2016-12-07  3:47 ` [PATCH 3/6] libxcmd: merge command() and iterate_command() Dave Chinner
2016-12-07  3:47 ` [PATCH 4/6] libxcmd: don't check generic library commands Dave Chinner
2016-12-07  3:47 ` [PATCH 5/6] xfs_io: make various commands one-shot only Dave Chinner
2016-12-15 18:21   ` Eric Sandeen
2016-12-16  0:53     ` Dave Chinner
2016-12-16  1:50       ` Eric Sandeen
2016-12-16  4:21         ` Dave Chinner
2016-12-07  3:47 ` [PATCH 6/6] libxcmd: add non-iterating user commands Dave Chinner
2016-12-07  4:49   ` Amir Goldstein
2016-12-07  4:57     ` Amir Goldstein
2016-12-07 14:21     ` Amir Goldstein
2016-12-07 20:16       ` Dave Chinner
2016-12-08 10:14         ` Amir Goldstein
2016-12-08 22:22           ` Dave Chinner
2016-12-15 19:09     ` Eric Sandeen
2017-01-12  5:14 ` [PATCH 0/6] xfs_io: fix up command iteration Amir Goldstein
2017-01-12 12:52   ` Eric Sandeen

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.