| Message ID | 1480943017-6175-1-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/5/16 7:03 AM, Amir Goldstein wrote: > There is an undocumented and possibly unused feature in xfs_io > where all commands are executed per file in the file args list. > > This feature creates ambiguity when trying to execute commands > such as "open" and "file" from command line and result in an > endless loop in the command loop code. I may well be dense, but it's not immediately obvious what the old and/or problematic behavior is. Can you spell it out a bit more? What is an example of this loop? > Also, when running xfs_io -c <cmd> without any file args, xfs_io > exits without doing anything. This behavior is also undocumented > and makes very little sense. What /should/ it do if there is no file specified? Is this just for cmds that don't require a file, such as "-c sync?" > Change the behavior of xfs_io to execute each command specified > with -c <cmd> exactly once, regardless of the number of files in > the file args list. So then which file does it act on? I'm not saying this is incorrect, I'm just not quite understanding what this fixes, and what the new behavior is, sorry. Thanks, -Eric > This enables writing proper xfs_io scripts in command line, which > include "open" and "file" commands. > > This change does not modify the behavior of xfs_io in the most > commonly used case of single file argument in command line and > no "open" and "file" commands in command line. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > io/init.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > v2: > - Fix the case of multiple file args > > v1: > - Fix the case of zero file args > > diff --git a/io/init.c b/io/init.c > index a9191cf..82b6700 100644 > --- a/io/init.c > +++ b/io/init.c > @@ -90,14 +90,15 @@ init_commands(void) > cowextsize_init(); > } > > +/* > + * Return true for first call and false for the next call, > + * to execute each command once. > + */ > static int > init_args_command( > int index) > { > - if (index >= filecount) > - return 0; > - file = &filetable[index++]; > - return index; > + return !index; > } > > static int > -- 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
On Mon, Dec 5, 2016 at 4:19 PM, Eric Sandeen <sandeen@sandeen.net> wrote: > On 12/5/16 7:03 AM, Amir Goldstein wrote: >> There is an undocumented and possibly unused feature in xfs_io >> where all commands are executed per file in the file args list. >> >> This feature creates ambiguity when trying to execute commands >> such as "open" and "file" from command line and result in an >> endless loop in the command loop code. > > I may well be dense, but it's not immediately obvious what the > old and/or problematic behavior is. Can you spell it out a bit > more? What is an example of this loop? $ touch foo $ xfs_io -c "open foo" foo you will see the shit storm > >> Also, when running xfs_io -c <cmd> without any file args, xfs_io >> exits without doing anything. This behavior is also undocumented >> and makes very little sense. > > What /should/ it do if there is no file specified? Is this just for > cmds that don't require a file, such as "-c sync?" Execute the commands as if you ran xfs_io and entered those commands in the shell, for example: $ xfs_io -c "open foo" -c "pread -v 0 4" > >> Change the behavior of xfs_io to execute each command specified >> with -c <cmd> exactly once, regardless of the number of files in >> the file args list. > > So then which file does it act on? I'm not saying this is incorrect, > I'm just not quite understanding what this fixes, and what the new > behavior is, sorry. > You are not to be blamed for not knowing what the old behavior is it is undocumented and I have never seen it used in tests. Old behavior of $ xfs_io -c -c "pread -v 0 4" -c "pread -v 4 8" foo bar - print 0..4 from foo - print 4..8 from foo - print 0..4 from bar - print 4..8 from bar New behavior is: - print 0..4 from bar (the "current" file) - print 4..8 from bar Same behavior as when you issue the same command after running $ xfs_io foo bar 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
On 12/5/16 9:19 AM, Amir Goldstein wrote: > On Mon, Dec 5, 2016 at 4:19 PM, Eric Sandeen <sandeen@sandeen.net> wrote: >> On 12/5/16 7:03 AM, Amir Goldstein wrote: >>> There is an undocumented and possibly unused feature in xfs_io >>> where all commands are executed per file in the file args list. >>> >>> This feature creates ambiguity when trying to execute commands >>> such as "open" and "file" from command line and result in an >>> endless loop in the command loop code. >> >> I may well be dense, but it's not immediately obvious what the >> old and/or problematic behavior is. Can you spell it out a bit >> more? What is an example of this loop? > > $ touch foo > $ xfs_io -c "open foo" foo > > you will see the shit storm ;) Ok thanks. (would be nice to have that in the changelog) > >> >>> Also, when running xfs_io -c <cmd> without any file args, xfs_io >>> exits without doing anything. This behavior is also undocumented >>> and makes very little sense. >> >> What /should/ it do if there is no file specified? Is this just for >> cmds that don't require a file, such as "-c sync?" > > Execute the commands as if you ran xfs_io and entered those commands > in the shell, for example: > > $ xfs_io -c "open foo" -c "pread -v 0 4" ok, I see. >> >>> Change the behavior of xfs_io to execute each command specified >>> with -c <cmd> exactly once, regardless of the number of files in >>> the file args list. >> >> So then which file does it act on? I'm not saying this is incorrect, >> I'm just not quite understanding what this fixes, and what the new >> behavior is, sorry. >> > > You are not to be blamed for not knowing what the old behavior is > it is undocumented and I have never seen it used in tests. > > Old behavior of > > $ xfs_io -c -c "pread -v 0 4" -c "pread -v 4 8" foo bar > > - print 0..4 from foo > - print 4..8 from foo > - print 0..4 from bar > - print 4..8 from bar Ok, interesting. yeah, the manpage does not even say more than one file arg is allowed. > New behavior is: > > - print 0..4 from bar (the "current" file) > - print 4..8 from bar > > Same behavior as when you issue the same command after running > > $ xfs_io foo bar Given that neither the manpage nor the short help mention that more than one filename is accepted, should this even succeed at all? If it does succeed, and has a known behavior, and its desirable, then this warrants a documentation update as well. And then probably an xfstests test case to be sure it doesn't drift. ;) -Eric > 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 > -- 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
On Mon, Dec 5, 2016 at 5:26 PM, Eric Sandeen <sandeen@sandeen.net> wrote: > On 12/5/16 9:19 AM, Amir Goldstein wrote: >> On Mon, Dec 5, 2016 at 4:19 PM, Eric Sandeen <sandeen@sandeen.net> wrote: >>> On 12/5/16 7:03 AM, Amir Goldstein wrote: >>>> There is an undocumented and possibly unused feature in xfs_io >>>> where all commands are executed per file in the file args list. >>>> >>>> This feature creates ambiguity when trying to execute commands >>>> such as "open" and "file" from command line and result in an >>>> endless loop in the command loop code. >>> >>> I may well be dense, but it's not immediately obvious what the >>> old and/or problematic behavior is. Can you spell it out a bit >>> more? What is an example of this loop? >> >> $ touch foo >> $ xfs_io -c "open foo" foo >> >> you will see the shit storm > > ;) Ok thanks. (would be nice to have that in the changelog) > Good point. I will add that. > > Given that neither the manpage nor the short help mention that more than > one filename is accepted, should this even succeed at all? > Make sense to me to fail on multiple file args. This is a behavior change that will not go silently if anyone is actually using multiple file args. If there are no objections, I will send out v4 patch. The only documentation change I am going to make is put the [file] arg in usage (and man page) inside optional brackets []. I think the rest of the behavior is pretty much aligned with the existing documentation. 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
On Mon, Dec 05, 2016 at 05:39:17PM +0200, Amir Goldstein wrote: > On Mon, Dec 5, 2016 at 5:26 PM, Eric Sandeen <sandeen@sandeen.net> wrote: > > On 12/5/16 9:19 AM, Amir Goldstein wrote: > >> On Mon, Dec 5, 2016 at 4:19 PM, Eric Sandeen <sandeen@sandeen.net> wrote: > >>> On 12/5/16 7:03 AM, Amir Goldstein wrote: > >>>> There is an undocumented and possibly unused feature in xfs_io > >>>> where all commands are executed per file in the file args list. > >>>> > >>>> This feature creates ambiguity when trying to execute commands > >>>> such as "open" and "file" from command line and result in an > >>>> endless loop in the command loop code. > >>> > >>> I may well be dense, but it's not immediately obvious what the > >>> old and/or problematic behavior is. Can you spell it out a bit > >>> more? What is an example of this loop? > >> > >> $ touch foo > >> $ xfs_io -c "open foo" foo > >> > >> you will see the shit storm > > > > ;) Ok thanks. (would be nice to have that in the changelog) > > > > Good point. I will add that. > > > > > > Given that neither the manpage nor the short help mention that more than > > one filename is accepted, should this even succeed at all? > > > > Make sense to me to fail on multiple file args. It doesn't to me. The fact that xfs_io has an open command, and it has commands that can act on multiple files (e.g. sendfile) indicate that it should be able to handle multiple files. Indeed, in interactive mode, the current file is really just an index into the stack of open files. It works exactly the same way as other XFS tools do with context stacks and accessed object rings (e.g. xfs_db). i.e. you only need to open the file/access the object once and then you can use the stack/ring to jump straight back to it. In this case, it's the xfs_io "file" command. i.e. xfs_io is intended to by able to operate on multiple files at once, and I do use that in interactive mode from time to time. What I haven't ever done is try to access that through the command line, so I didn't know it had this "command loop" iteration problem. It's the command loop iteration problem we need to fix, not neuter the multiple file capability of xfs_io. Cheers, Dave.
On Tue, Dec 6, 2016 at 12:15 AM, Dave Chinner <david@fromorbit.com> wrote: > On Mon, Dec 05, 2016 at 05:39:17PM +0200, Amir Goldstein wrote: >> On Mon, Dec 5, 2016 at 5:26 PM, Eric Sandeen <sandeen@sandeen.net> wrote: >> > On 12/5/16 9:19 AM, Amir Goldstein wrote: >> >> On Mon, Dec 5, 2016 at 4:19 PM, Eric Sandeen <sandeen@sandeen.net> wrote: >> >>> On 12/5/16 7:03 AM, Amir Goldstein wrote: >> >>>> There is an undocumented and possibly unused feature in xfs_io >> >>>> where all commands are executed per file in the file args list. >> >>>> >> >>>> This feature creates ambiguity when trying to execute commands >> >>>> such as "open" and "file" from command line and result in an >> >>>> endless loop in the command loop code. >> >>> >> >>> I may well be dense, but it's not immediately obvious what the >> >>> old and/or problematic behavior is. Can you spell it out a bit >> >>> more? What is an example of this loop? >> >> >> >> $ touch foo >> >> $ xfs_io -c "open foo" foo >> >> >> >> you will see the shit storm >> > >> > ;) Ok thanks. (would be nice to have that in the changelog) >> > >> >> Good point. I will add that. >> >> >> > >> > Given that neither the manpage nor the short help mention that more than >> > one filename is accepted, should this even succeed at all? >> > >> >> Make sense to me to fail on multiple file args. > > It doesn't to me. > > The fact that xfs_io has an open command, and it has > commands that can act on multiple files (e.g. sendfile) indicate > that it should be able to handle multiple files. > > Indeed, in interactive mode, the current file is really just an > index into the stack of open files. It works exactly the same way as > other XFS tools do with context stacks and accessed object rings > (e.g. xfs_db). i.e. you only need to open the file/access the object > once and then you can use the stack/ring to jump straight back to > it. In this case, it's the xfs_io "file" command. > > i.e. xfs_io is intended to by able to operate on multiple files at > once, and I do use that in interactive mode from time to time. > > What I haven't ever done is try to access that through the command > line, so I didn't know it had this "command loop" iteration problem. > > It's the command loop iteration problem we need to fix, not neuter > the multiple file capability of xfs_io. > OK, v4 it is. For some reason you fail to spell out your idea of "fixing", so I keep having to guess what you mean. This time I am guessing that you mean: - open all the files in args list (all using the same -r/-f flags) - iterate all commands exactly once without implicitly changing "file" 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
On Tue, Dec 06, 2016 at 07:24:12AM +0200, Amir Goldstein wrote: > On Tue, Dec 6, 2016 at 12:15 AM, Dave Chinner <david@fromorbit.com> wrote: > > It's the command loop iteration problem we need to fix, not neuter > > the multiple file capability of xfs_io. > > > > OK, v4 it is. > For some reason you fail to spell out your idea of "fixing", > so I keep having to guess what you mean. I outlined the probable fix last week: https://www.spinics.net/lists/fstests/msg04544.html i.e. fix the one-shot commands with CMD_FLAG_GLOBAL, but indicated we'd need to look at the git history to be sure of what is necessary. > This time I am guessing that you mean: > - open all the files in args list (all using the same -r/-f flags) > - iterate all commands exactly once without implicitly changing "file" Nope. I meant "go look at the git history and determine what the historical behaviour was and determine how we ended up with this mess. Then from that analysis, decide what to do." Unfortunately, you haven't shown any indication that you've looked at the git history, nor that you actually listened to me about the CMD_FLAG_GLOBAL. I've now done that code archeology and it indicates that my suggestion was correct. If you didn't understand what I suggested, then say so clearly and unambiguously so I know I need to explain it more clearly. The /worst/ thing you can do is keep sending new patches that take the same path to one I've already NACKed without having looked at the solution I proposed or done the historical analysis I suggested was necessary. Cheers, Dave.
On Wed, Dec 7, 2016 at 1:25 AM, Dave Chinner <david@fromorbit.com> wrote: ... > > Nope. I meant "go look at the git history and determine what the > historical behaviour was and determine how we ended up with this > mess. Then from that analysis, decide what to do." > > Unfortunately, you haven't shown any indication that you've looked > at the git history, nor that you actually listened to me about the > CMD_FLAG_GLOBAL. I've now done that code archeology and it indicates > that my suggestion was correct. > Thank you for doing that digging. I might have gotten the wrong impression that you were going to do that anyway and figured you would do a better job at this than me, having a longer history with libxfs and xfs_io than me. > If you didn't understand what I suggested, then say so clearly and > unambiguously so I know I need to explain it more clearly. Sorry I wasn't clear about that. My problem, or lack of clarity, was revolving around the semantic changes that would have to be made. I did not want to go fix the problem without knowing what those are going to be, so in the mean while, I suggested a few starter options which do not change semantics for existing known users, allow overlay/016 to work properly and leave the multiple file semantics for you to clarify. > The > /worst/ thing you can do is keep sending new patches that take the > same path to one I've already NACKed without having looked at the > solution I proposed or done the historical analysis I suggested was > necessary. > Had I known /this/ was the worst thing in your book, I would have avoided it. I took that path mainly because we seem to have little overlap working hours, so the round trip of this thread makes progress hard. I apologize if this came off as "shut up Dave! and take my patch". The only reason I suggested changing behavior for multiple files and "break existing users" is because Eric indicated he had no idea about this feature and indicated that it is an undocumented feature. With someone like Eric saying that, it grew my suspicion that maybe we really do not need to fix multiple files for xfs_io?? Could you please consider this option for a moment? Perhaps we are better off correcting a wrong turn taken in 2004 then fixing its consequences now? After all, xfs_io is not the Linux kernel. Its a tool used mainly by developers as you yourself indicated. If killing multiple files will break someones use case, you should know about it pretty quick and we can then resort to changing semantics and adding new command line flags. The motivation for taking this path is not just laziness to "fix the real problem". It stands to make the code a lot simpler and to keep the man page correct and coherent with expected behavior rather then adding more obscure semantics which /are/ going to be confusing. For your consideration. Sorry if we started off on the wrong foot. 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
diff --git a/io/init.c b/io/init.c index a9191cf..82b6700 100644 --- a/io/init.c +++ b/io/init.c @@ -90,14 +90,15 @@ init_commands(void) cowextsize_init(); } +/* + * Return true for first call and false for the next call, + * to execute each command once. + */ static int init_args_command( int index) { - if (index >= filecount) - return 0; - file = &filetable[index++]; - return index; + return !index; } static int
There is an undocumented and possibly unused feature in xfs_io where all commands are executed per file in the file args list. This feature creates ambiguity when trying to execute commands such as "open" and "file" from command line and result in an endless loop in the command loop code. Also, when running xfs_io -c <cmd> without any file args, xfs_io exits without doing anything. This behavior is also undocumented and makes very little sense. Change the behavior of xfs_io to execute each command specified with -c <cmd> exactly once, regardless of the number of files in the file args list. This enables writing proper xfs_io scripts in command line, which include "open" and "file" commands. This change does not modify the behavior of xfs_io in the most commonly used case of single file argument in command line and no "open" and "file" commands in command line. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- io/init.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) v2: - Fix the case of multiple file args v1: - Fix the case of zero file args