| Message ID | 20161207034724.1613-7-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,