[PATCH v2] xfs_io: support -c "open foo" command

[PATCH v2] xfs_io: support -c "open foo" command

[Amir Goldstein]

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

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

Re: [PATCH v2] xfs_io: support -c "open foo" command

[Eric Sandeen]

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
> 

Re: [PATCH v2] xfs_io: support -c "open foo" command

[Amir Goldstein]

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.

Re: [PATCH v2] xfs_io: support -c "open foo" command

[Eric Sandeen]

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
> 

Re: [PATCH v2] xfs_io: support -c "open foo" command

[Amir Goldstein]

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.

Re: [PATCH v2] xfs_io: support -c "open foo" command

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] xfs_io: support -c "open foo" command

[Amir Goldstein]

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.

Re: [PATCH v2] xfs_io: support -c "open foo" command

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] xfs_io: support -c "open foo" command

[Amir Goldstein]

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.