xfs_io: fix up command iteration

From: Dave Chinner <[email protected]>

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 <[email protected]>

---
 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(-)

io/file.c +1 -1

index 8e3f07122922..349b19cdc420 100644
@@ -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";

io/freeze.c +2 -2

index 3d0d2a4b5601..0305713d99e8 100644
@@ -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) {

io/getrusage.c +2 -1

index bccf94cbc302..cf1f2afd19a8 100644
@@ -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)

io/imap.c +1 -1

index 7123432f411f..f52238e0c450 100644
@@ -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)

io/inject.c +1 -1

index 5d5e4aef3dfc..25c70218a1ef 100644
@@ -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;

io/link.c +1 -1

index ccf8e691bb1d..9b2e8a970942 100644
@@ -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");

io/mmap.c +2 -1

index dc188d0557cf..e2d8d5a92326 100644
@@ -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");

io/open.c +4 -3

index 722d0f9eacf5..a12f4a2ba528 100644
@@ -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;

io/reflink.c +2 -2

index a09e82dca80a..a22b6b4a07e3 100644
@@ -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 =

io/resblks.c +1 -1

index 73318ae03fd2..06903f5bb748 100644
@@ -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");

io/shutdown.c +1 -1

index d8507cc78af7..d9cd520d11e2 100644
@@ -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");

io/sync.c +2 -1

index 28e3a15e0a96..c77263804a35 100644
@@ -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");
 

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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?

Quoted 14 lines · Eric Sandeen

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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:

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 5 lines · Dave Chinner

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.

From: Dave Chinner <[email protected]>

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 <[email protected]>

---
 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(-)

include/command.h +1 -0

index 348002cbe3ed..fb3f5c79b991 100644
@@ -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);
 

io/init.c +0 -0

index 5ce627ef22c9..cf8573f0ecd5 100644

io/init.c +4 -1

                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;

libxcmd/command.c +27 -6

index decc442a9d03..5917aea42611 100644
@@ -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;

man/man8/xfs_io.8 +0 -0

index 885df7f141e2..d6bacaf0438d 100644
@@ -9,10 +9,13 @@ xfs_io \- debug the I/O path of an XFS filesystem
 .B \-c

man/man8/xfs_io.8 +20 -6

+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

Quoted 2 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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

Cheers,
Amir.

Quoted 8 lines

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"

Quoted 23 lines

 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
$

Quoted 5 lines

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

Quoted 43 lines

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 <[email protected]>

---
 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(-)

include/command.h +1 -0

index 348002cbe3ed..fb3f5c79b991 100644
@@ -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);
 

io/init.c +0 -0

index 5ce627ef22c9..cf8573f0ecd5 100644

io/init.c void · line 37

Quoted 5 lines

io/init.c +1 -1

@@ -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]...

Quoted 75 lines

io/init.c +4 -1

                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;

libxcmd/command.c +25 -4

index decc442a9d03..5917aea42611 100644
@@ -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)
 

libxcmd/command.c command_loop(void) · line 240

Quoted 8 lines

libxcmd/command.c +1 -1

        /* 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));

Quoted 13 lines

libxcmd/command.c +1 -1

                        exit(1);
                }
-               done = process_input(input, true);
+               done = process_input(input, cmdline[i].iterate);
        }
        free(cmdline);
        return;

man/man8/xfs_io.8 +0 -0

index 885df7f141e2..d6bacaf0438d 100644
@@ -9,10 +9,13 @@ xfs_io \- debug the I/O path of an XFS filesystem
 .B \-c

man/man8/xfs_io.8 xfs_io \- debug the I/O path of an XFS filesystem · line 18

Quoted 10 lines

man/man8/xfs_io.8 +4 -1

 .I cmd
 ] ... [
+.B \-C
+.I cmd
+] ... [
 .B \-p
 .I prog
 ]
-.I file
+.I [ file ]

Suggest: [ file ] ...

man/man8/xfs_io.8 These code paths include not only the obvious read/write/mmap interfaces · line 36

Quoted 13 lines

man/man8/xfs_io.8 +6 -0

 .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.

Quoted 34 lines

man/man8/xfs_io.8 +20 -6

+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

Quoted 2 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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

Quoted 8 lines · Amir Goldstein

Quoted 2 lines · Dave Chinner

From: Dave Chinner <[email protected]>

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?

On Wed, Dec 07, 2016 at 04:21:31PM +0200, Amir Goldstein wrote:

Quoted 3 lines

1. Do you think that xfs_io should error on -c "open foo" to force explicit -C "open foo"?

No.

Quoted 2 lines

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....

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 1 line · Dave Chinner

On Wed, Dec 07, 2016 at 04:21:31PM +0200, Amir Goldstein wrote:

...

Quoted 11 lines

Quoted 3 lines

1. Do you think that xfs_io should error on -c "open foo" to force explicit -C "open foo"?

No.

Quoted 2 lines

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:

Quoted 8 lines

$ 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.

Quoted 4 lines

Quoted 3 lines

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?

Quoted 5 lines

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)

Quoted 4 lines

man/man8/xfs_io.8 +0 -0

@@ -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

man/man8/xfs_io.8 These code paths include not only the obvious read/write/mmap interfaces · line 36

Quoted 6 lines

man/man8/xfs_io.8 +6 -0

+.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.

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.

Quoted 43 lines · Dave Chinner

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.

Quoted 2 lines

-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.
--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html