Re: [PATCH] overlay: test ro/rw fd data inconsistecies

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Amir Goldstein]

On Thu, Nov 24, 2016 at 10:12 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Introduce a new test to demonstrates a known issue with overlayfs:
> - process A opens file F for read
> - process B writes new data to file F
> - process A reads old data from file F
>
> This issue is about to be fixed with a patch set by Miklos Szeredi.

Eryu and all,

I wanted to ask what is the common practice for introducing tests for
know issues
that are *about* to be solved.

What is the preferred timing for merging these sort of tests?
Is it productive to have these tests merged before a fix is merged to master?
Before a fix is queued for next?
Before a fix is available?

I am asking because there are several known issues for overlayfs
whose fixes are in several different states of maturity and I would like
to know how to treat the tests I write for them.

FYI, the fix for the test in this patch (test ro/rw fd data inconsistencies)
is not queued for next yet, but I am hoping it will be.
Miklos?

>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  tests/overlay/016     | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/overlay/016.out | 12 +++++++
>  tests/overlay/group   |  1 +
>  3 files changed, 111 insertions(+)
>  create mode 100755 tests/overlay/016
>  create mode 100644 tests/overlay/016.out
>
> diff --git a/tests/overlay/016 b/tests/overlay/016
> new file mode 100755
> index 0000000..6d3e339
> --- /dev/null
> +++ b/tests/overlay/016
> @@ -0,0 +1,98 @@
> +#! /bin/bash
> +# FSQA Test No. 016
> +#
> +# Test ro/rw fd data inconsistecies
> +#
> +# This simple test demonstrates a known issue with overlayfs:
> +# - process A opens file F for read
> +# - process B writes new data to file F
> +# - process A reads old data from file F
> +#
> +#-----------------------------------------------------------------------
> +#
> +# Copyright (C) 2016 CTERA Networks. All Rights Reserved.
> +# Author: Amir Goldstein <amir73il@gmail.com>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +tmp=/tmp/$$
> +status=1       # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +       cd /
> +       rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs overlay
> +_supported_os Linux
> +_require_scratch
> +
> +rm -f $seqres.full
> +
> +_scratch_mkfs >>$seqres.full 2>&1
> +
> +# Create our test files.
> +lowerdir=$SCRATCH_DEV/$OVERLAY_LOWER_DIR
> +mkdir -p $lowerdir
> +echo "This is old news" > $lowerdir/foo
> +echo "This is old news" > $lowerdir/bar
> +
> +_scratch_mount
> +
> +cd $SCRATCH_MNT
> +
> +#
> +# case #1:
> +# open file for read (rofd)
> +# open file for write (rwfd)
> +# write to rwfd
> +# read from rofd
> +#
> +$XFS_IO_PROG << EOF | _filter_xfs_io
> +open -r foo
> +open foo
> +pwrite -S 0x61 0 16
> +file 0
> +pread -v 0 16
> +EOF
> +
> +#
> +# case #2:
> +# mmap MAP_SHARED|PROT_READ of rofd
> +# write to rwfd
> +# read from mapped memory
> +#
> +$XFS_IO_PROG << EOF | _filter_xfs_io
> +open -r bar
> +mmap -r 0 16
> +open bar
> +pwrite -S 0x61 0 16
> +mread -v 0 16
> +EOF
> +
> +status=0
> +exit
> diff --git a/tests/overlay/016.out b/tests/overlay/016.out
> new file mode 100644
> index 0000000..52b8cd7
> --- /dev/null
> +++ b/tests/overlay/016.out
> @@ -0,0 +1,12 @@
> +QA output created by 016
> +xfs_io> xfs_io> xfs_io> wrote 16/16 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +xfs_io> [000] foo            (foreign,non-sync,non-direct,read-only)
> + 001  foo            (foreign,non-sync,non-direct,read-write)
> +xfs_io> 00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
> +read 16/16 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +xfs_io> xfs_io> xfs_io> xfs_io> xfs_io> wrote 16/16 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +xfs_io> 00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
> +xfs_io>
> \ No newline at end of file
> diff --git a/tests/overlay/group b/tests/overlay/group
> index 84850b1..5740d2a 100644
> --- a/tests/overlay/group
> +++ b/tests/overlay/group
> @@ -18,3 +18,4 @@
>  013 auto quick
>  014 auto quick copyup
>  015 auto quick whiteout
> +016 auto quick
> --
> 2.7.4
>

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Miklos Szeredi]

On Tue, Nov 29, 2016 at 9:53 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Nov 24, 2016 at 10:12 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> Introduce a new test to demonstrates a known issue with overlayfs:
>> - process A opens file F for read
>> - process B writes new data to file F
>> - process A reads old data from file F
>>
>> This issue is about to be fixed with a patch set by Miklos Szeredi.
>
> Eryu and all,
>
> I wanted to ask what is the common practice for introducing tests for
> know issues
> that are *about* to be solved.
>
> What is the preferred timing for merging these sort of tests?
> Is it productive to have these tests merged before a fix is merged to master?
> Before a fix is queued for next?
> Before a fix is available?

IMO adding a test doesn't hurt, it'll just indicate that the current
version is broken.  It doesn't have to have any synchronization with
the actual fix.

> I am asking because there are several known issues for overlayfs
> whose fixes are in several different states of maturity and I would like
> to know how to treat the tests I write for them.
>
> FYI, the fix for the test in this patch (test ro/rw fd data inconsistencies)
> is not queued for next yet, but I am hoping it will be.
> Miklos?

I think it's in good shape for 4.10.  I'll try to ping Al about the VFS bits.

Thanks,
Miklos

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Eryu Guan]

On Tue, Nov 29, 2016 at 10:53:36AM +0200, Amir Goldstein wrote:
> On Thu, Nov 24, 2016 at 10:12 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > Introduce a new test to demonstrates a known issue with overlayfs:
> > - process A opens file F for read
> > - process B writes new data to file F
> > - process A reads old data from file F
> >
> > This issue is about to be fixed with a patch set by Miklos Szeredi.
> 
> Eryu and all,
> 
> I wanted to ask what is the common practice for introducing tests for
> know issues
> that are *about* to be solved.
> 
> What is the preferred timing for merging these sort of tests?
> Is it productive to have these tests merged before a fix is merged to master?
> Before a fix is queued for next?
> Before a fix is available?

Basically new regression tests will be merged as soon as possible, as
long as there're no objections from reviewers or all comments are
addressed.

One exception is that for tests that could crash latest maintainer's
tree (even there's a known fix), I'd perfer letting the fix go upstream
first, so that the test doesn't break anyone's tests by crashing all the
testing hosts. It's great if the test author could give a notification
on the test to say that the fix has been merged, so the test could be
merged too. I'll watch the patch status too, but maybe not so timely.

> 
> I am asking because there are several known issues for overlayfs
> whose fixes are in several different states of maturity and I would like
> to know how to treat the tests I write for them.

Thanks for writing test cases! You can send them out at anytime :)

> 
> FYI, the fix for the test in this patch (test ro/rw fd data inconsistencies)
> is not queued for next yet, but I am hoping it will be.
> Miklos?

FYI, this test is already in my last pull request to Dave.

Thanks,
Eryu

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Amir Goldstein]

On Tue, Nov 29, 2016 at 1:37 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Nov 29, 2016 at 10:53:36AM +0200, Amir Goldstein wrote:

>>
>> Eryu and all,
>>
>> I wanted to ask what is the common practice for introducing tests for
>> know issues
>> that are *about* to be solved.
>>
>> What is the preferred timing for merging these sort of tests?
>> Is it productive to have these tests merged before a fix is merged to master?
>> Before a fix is queued for next?
>> Before a fix is available?
>
> Basically new regression tests will be merged as soon as possible, as
> long as there're no objections from reviewers or all comments are
> addressed.
>
> One exception is that for tests that could crash latest maintainer's
> tree (even there's a known fix), I'd perfer letting the fix go upstream
> first, so that the test doesn't break anyone's tests by crashing all the
> testing hosts. It's great if the test author could give a notification
> on the test to say that the fix has been merged, so the test could be
> merged too. I'll watch the patch status too, but maybe not so timely.
>

Nothing of a sort lurking with the tests I am planning to write.
Just tests that check for "Non-standard behavior" of overlayfs,
some of it described in Documentation/filesystems/overlayfs.txt.

>
>>
>> FYI, the fix for the test in this patch (test ro/rw fd data inconsistencies)
>> is not queued for next yet, but I am hoping it will be.
>> Miklos?
>
> FYI, this test is already in my last pull request to Dave.
>

Eryu,

I am getting this error when running my test with an older xfs_io (4.3.0).
I generated the good output with xfs_io from Dave's for-next branch (4.8.0).

Have you any idea why in one setup I see the commands echoed
to output and not in the other?

I realize that the use of redirecting commands from here document
to xfs_io has not been used in xfstests before, but I could not find
another way to use 'open' commands, which are needed for this test.

Amir.

overlay/016      - output mismatch (see
/home/amir/src/xfstests-dev/results//overlay/016.out.bad)
    --- tests/overlay/016.out   2016-12-01 12:19:02.710370574 +0200
    +++ /home/amir/src/xfstests-dev/results//overlay/016.out.bad
 2016-12-01 18:29:23.684327009 +0200
    @@ -1,12 +1,22 @@
     QA output created by 016
    -xfs_io> xfs_io> xfs_io> wrote 16/16 bytes at offset 0
    +xfs_io> open -r foo
    +xfs_io> open foo
    +xfs_io> pwrite -S 0x61 0 16
    +wrote 16/16 bytes at offset 0
     XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    ...

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Eryu Guan]

On Thu, Dec 01, 2016 at 06:42:00PM +0200, Amir Goldstein wrote:
> On Tue, Nov 29, 2016 at 1:37 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Tue, Nov 29, 2016 at 10:53:36AM +0200, Amir Goldstein wrote:
> 
> >>
> >> Eryu and all,
> >>
> >> I wanted to ask what is the common practice for introducing tests for
> >> know issues
> >> that are *about* to be solved.
> >>
> >> What is the preferred timing for merging these sort of tests?
> >> Is it productive to have these tests merged before a fix is merged to master?
> >> Before a fix is queued for next?
> >> Before a fix is available?
> >
> > Basically new regression tests will be merged as soon as possible, as
> > long as there're no objections from reviewers or all comments are
> > addressed.
> >
> > One exception is that for tests that could crash latest maintainer's
> > tree (even there's a known fix), I'd perfer letting the fix go upstream
> > first, so that the test doesn't break anyone's tests by crashing all the
> > testing hosts. It's great if the test author could give a notification
> > on the test to say that the fix has been merged, so the test could be
> > merged too. I'll watch the patch status too, but maybe not so timely.
> >
> 
> Nothing of a sort lurking with the tests I am planning to write.
> Just tests that check for "Non-standard behavior" of overlayfs,
> some of it described in Documentation/filesystems/overlayfs.txt.
> 
> >
> >>
> >> FYI, the fix for the test in this patch (test ro/rw fd data inconsistencies)
> >> is not queued for next yet, but I am hoping it will be.
> >> Miklos?
> >
> > FYI, this test is already in my last pull request to Dave.
> >
> 
> Eryu,
> 
> I am getting this error when running my test with an older xfs_io (4.3.0).
> I generated the good output with xfs_io from Dave's for-next branch (4.8.0).
> 
> Have you any idea why in one setup I see the commands echoed
> to output and not in the other?

I'm also using latest for-next branch, so I didn't notice this issue
either. I guess xfs_io changed its behavior on when to print \n in
interactive mode, but I didn't dig into the history.

> 
> I realize that the use of redirecting commands from here document
> to xfs_io has not been used in xfstests before, but I could not find
> another way to use 'open' commands, which are needed for this test.
> 
> Amir.
> 
> overlay/016      - output mismatch (see
> /home/amir/src/xfstests-dev/results//overlay/016.out.bad)
>     --- tests/overlay/016.out   2016-12-01 12:19:02.710370574 +0200
>     +++ /home/amir/src/xfstests-dev/results//overlay/016.out.bad
>  2016-12-01 18:29:23.684327009 +0200
>     @@ -1,12 +1,22 @@
>      QA output created by 016
>     -xfs_io> xfs_io> xfs_io> wrote 16/16 bytes at offset 0
>     +xfs_io> open -r foo
>     +xfs_io> open foo
>     +xfs_io> pwrite -S 0x61 0 16
>     +wrote 16/16 bytes at offset 0
>      XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>     ...

I did find a better way to call open either, but if all we care about is
the output from reading the file, then we can check that explicitly and
ignore other outputs. e.g. 

--- a/tests/overlay/016
+++ b/tests/overlay/016
@@ -61,6 +61,8 @@ mkdir -p $lowerdir
 echo "This is old news" > $lowerdir/foo
 echo "This is old news" > $lowerdir/bar
 
+echo "Silence is golden"
+
 _scratch_mount
 
 cd $SCRATCH_MNT
@@ -72,7 +74,7 @@ cd $SCRATCH_MNT
 # write to rwfd
 # read from rofd
 #
-$XFS_IO_PROG << EOF | _filter_xfs_io
+$XFS_IO_PROG << EOF | grep "old"
 open -r foo
 open foo
 pwrite -S 0x61 0 16
@@ -86,7 +88,7 @@ EOF
 # write to rwfd
 # read from mapped memory
 #
-$XFS_IO_PROG << EOF | _filter_xfs_io
+$XFS_IO_PROG << EOF | grep "old"
 open -r bar
 mmap -r 0 16
 open bar
diff --git a/tests/overlay/016.out b/tests/overlay/016.out
index 52b8cd7..aa2526b 100644
--- a/tests/overlay/016.out
+++ b/tests/overlay/016.out
@@ -1,12 +1,2 @@
 QA output created by 016
-xfs_io> xfs_io> xfs_io> wrote 16/16 bytes at offset 0
-XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-xfs_io> [000] foo            (foreign,non-sync,non-direct,read-only)
- 001  foo            (foreign,non-sync,non-direct,read-write)
-xfs_io> 00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
-read 16/16 bytes at offset 0
-XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-xfs_io> xfs_io> xfs_io> xfs_io> xfs_io> wrote 16/16 bytes at offset 0
-XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-xfs_io> 00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
-xfs_io> 
\ No newline at end of file
+Silence is golden

This should work for both old and new version of xfs_io.

Thanks,
Eryu

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Dave Chinner]

On Thu, Dec 01, 2016 at 06:42:00PM +0200, Amir Goldstein wrote:
> On Tue, Nov 29, 2016 at 1:37 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Tue, Nov 29, 2016 at 10:53:36AM +0200, Amir Goldstein wrote:
> 
> >>
> >> Eryu and all,
> >>
> >> I wanted to ask what is the common practice for introducing tests for
> >> know issues
> >> that are *about* to be solved.
> >>
> >> What is the preferred timing for merging these sort of tests?
> >> Is it productive to have these tests merged before a fix is merged to master?
> >> Before a fix is queued for next?
> >> Before a fix is available?
> >
> > Basically new regression tests will be merged as soon as possible, as
> > long as there're no objections from reviewers or all comments are
> > addressed.
> >
> > One exception is that for tests that could crash latest maintainer's
> > tree (even there's a known fix), I'd perfer letting the fix go upstream
> > first, so that the test doesn't break anyone's tests by crashing all the
> > testing hosts. It's great if the test author could give a notification
> > on the test to say that the fix has been merged, so the test could be
> > merged too. I'll watch the patch status too, but maybe not so timely.
> >
> 
> Nothing of a sort lurking with the tests I am planning to write.
> Just tests that check for "Non-standard behavior" of overlayfs,
> some of it described in Documentation/filesystems/overlayfs.txt.
> 
> >
> >>
> >> FYI, the fix for the test in this patch (test ro/rw fd data inconsistencies)
> >> is not queued for next yet, but I am hoping it will be.
> >> Miklos?
> >
> > FYI, this test is already in my last pull request to Dave.
> >
> 
> Eryu,
> 
> I am getting this error when running my test with an older xfs_io (4.3.0).
> I generated the good output with xfs_io from Dave's for-next branch (4.8.0).
> 
> Have you any idea why in one setup I see the commands echoed
> to output and not in the other?

I think there's a bug somewhere in xfs_io to do with command line
repetition of the open command. That is:

# xfs_io
xfs_io> open foo
xfs_io> open -r foo
xfs_io> print
 000  foo            (foreign,non-sync,non-direct,read-write)
 [001] foo            (foreign,non-sync,non-direct,read-only)
xfs_io> 

Shows we have two open files, the second being read only.

Fromteh command line, opening a read-only file:

# xfs_io -r -c file foo
[000] foo            (foreign,non-sync,non-direct,read-only)
#

But if we try to open a second file from the CLI:

sudo xfs_io -r -c "open -f bar" -c print foo
bar: Too many open files
[000] foo            (foreign,non-sync,non-direct,read-only)
 001  bar            (foreign,non-sync,non-direct,read-write)
 002  bar            (foreign,non-sync,non-direct,read-write)
 003  bar            (foreign,non-sync,non-direct,read-write)
 004  bar            (foreign,non-sync,non-direct,read-write)
 005  bar            (foreign,non-sync,non-direct,read-write)
 006  bar            (foreign,non-sync,non-direct,read-write)
.....

It just falls into an endless loop opening the file until we run out
fd space.

Oh, there's bugs all over the place here - Ok, I think the command
loop handling is broken - it's making my head hurt right now.
Functions that don't set CMD_FLAG_GLOBAL have problems with not
breaking out of the args processing loop. I'll deal with this on
Monday, not at beer o'clock on Friday afternoon.

> I realize that the use of redirecting commands from here document
> to xfs_io has not been used in xfstests before, but I could not find
> another way to use 'open' commands, which are needed for this test.

Let's fix xfs_io rather than hack yet another whacky way to execute
xfs_io into xfstests...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Amir Goldstein]

On Fri, Dec 2, 2016 at 8:49 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Dec 01, 2016 at 06:42:00PM +0200, Amir Goldstein wrote:
...
>
> I think there's a bug somewhere in xfs_io to do with command line
> repetition of the open command. That is:
>
> # xfs_io
> xfs_io> open foo
> xfs_io> open -r foo
> xfs_io> print
>  000  foo            (foreign,non-sync,non-direct,read-write)
>  [001] foo            (foreign,non-sync,non-direct,read-only)
> xfs_io>
>
> Shows we have two open files, the second being read only.
>
> From the command line, opening a read-only file:
>
> # xfs_io -r -c file foo
> [000] foo            (foreign,non-sync,non-direct,read-only)
> #
>
> But if we try to open a second file from the CLI:
>
> sudo xfs_io -r -c "open -f bar" -c print foo
> bar: Too many open files
> [000] foo            (foreign,non-sync,non-direct,read-only)
>  001  bar            (foreign,non-sync,non-direct,read-write)
>  002  bar            (foreign,non-sync,non-direct,read-write)
>  003  bar            (foreign,non-sync,non-direct,read-write)
>  004  bar            (foreign,non-sync,non-direct,read-write)
>  005  bar            (foreign,non-sync,non-direct,read-write)
>  006  bar            (foreign,non-sync,non-direct,read-write)
> .....
>
> It just falls into an endless loop opening the file until we run out
> fd space.
>
> Oh, there's bugs all over the place here - Ok, I think the command
> loop handling is broken - it's making my head hurt right now.
> Functions that don't set CMD_FLAG_GLOBAL have problems with not
> breaking out of the args processing loop. I'll deal with this on
> Monday, not at beer o'clock on Friday afternoon.
>
>> I realize that the use of redirecting commands from here document
>> to xfs_io has not been used in xfstests before, but I could not find
>> another way to use 'open' commands, which are needed for this test.
>
> Let's fix xfs_io rather than hack yet another whacky way to execute
> xfs_io into xfstests...
>

I have started going down that path, because I initially mistaken
xfs_io -c to be similar to bash -c, but then I realized it is quite different.
xfs_io -c CMD1 -c CMD2 FILE1 FILE2 iterates CMD1, CMD2 on FILE1
and then on FILE2.
As long as this semantic holds, it is going to difficult be to support
-c "open file", without resorting to spaghetti code and confusing users.

OTOH, is there anymore more natural for an program that has an
interactive shell mode to get a script as input and work in non interactive
mode?

So I wrote this test by redirecting input into an interactive shell, which
is not clean.

One alternative is xfs_io -F <script>, similar to debugfs -f, sed -f.
Another is xfs_io -s to read from stdio, similar to bash -s, then
prompt and echo can be silent.
And last, there is the option to treat -c CMD1 -c CMD2 exactly the
same as you would treat interactive commands, meaning that
first FILE1 FILE2 are open as then execute all commands just
instead of "per file in the file table", which creates the endless loop.

Incidentally, xfs_io does not advertise the "run all command per file"
behavior, so not sure why it was done for and if anybody relies on it.
I did not notice any tests that make use of multiple file args for xfs_io,
but it wasn't easy to grep for this practice.

If you have a preference for solution I can execute the work.

Amir.

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Dave Chinner]

On Fri, Dec 02, 2016 at 10:13:33AM +0200, Amir Goldstein wrote:
> On Fri, Dec 2, 2016 at 8:49 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Dec 01, 2016 at 06:42:00PM +0200, Amir Goldstein wrote:
> ...
> >
> > I think there's a bug somewhere in xfs_io to do with command line
> > repetition of the open command. That is:
> >
> > # xfs_io
> > xfs_io> open foo
> > xfs_io> open -r foo
> > xfs_io> print
> >  000  foo            (foreign,non-sync,non-direct,read-write)
> >  [001] foo            (foreign,non-sync,non-direct,read-only)
> > xfs_io>
> >
> > Shows we have two open files, the second being read only.
> >
> > From the command line, opening a read-only file:
> >
> > # xfs_io -r -c file foo
> > [000] foo            (foreign,non-sync,non-direct,read-only)
> > #
> >
> > But if we try to open a second file from the CLI:
> >
> > sudo xfs_io -r -c "open -f bar" -c print foo
> > bar: Too many open files
> > [000] foo            (foreign,non-sync,non-direct,read-only)
> >  001  bar            (foreign,non-sync,non-direct,read-write)
> >  002  bar            (foreign,non-sync,non-direct,read-write)
> >  003  bar            (foreign,non-sync,non-direct,read-write)
> >  004  bar            (foreign,non-sync,non-direct,read-write)
> >  005  bar            (foreign,non-sync,non-direct,read-write)
> >  006  bar            (foreign,non-sync,non-direct,read-write)
> > .....
> >
> > It just falls into an endless loop opening the file until we run out
> > fd space.
> >
> > Oh, there's bugs all over the place here - Ok, I think the command
> > loop handling is broken - it's making my head hurt right now.
> > Functions that don't set CMD_FLAG_GLOBAL have problems with not
> > breaking out of the args processing loop. I'll deal with this on
> > Monday, not at beer o'clock on Friday afternoon.
> >
> >> I realize that the use of redirecting commands from here document
> >> to xfs_io has not been used in xfstests before, but I could not find
> >> another way to use 'open' commands, which are needed for this test.
> >
> > Let's fix xfs_io rather than hack yet another whacky way to execute
> > xfs_io into xfstests...
> >
> 
> I have started going down that path, because I initially mistaken
> xfs_io -c to be similar to bash -c, but then I realized it is quite different.
> xfs_io -c CMD1 -c CMD2 FILE1 FILE2 iterates CMD1, CMD2 on FILE1
> and then on FILE2.
> As long as this semantic holds, it is going to difficult be to support
> -c "open file", without resorting to spaghetti code and confusing users.

That's what I mean - that functionality is busted in xfs_io. the
CMD_FLAG_GLOBAL set on various commands controls this iteration
functionality, and it's not set on the open command so it tries to
run iteratively when run from the CLI and so behaves differently
(and incorrectly) comapred to interactive mode.  That's /one/ of the
bugs that needs fixing.

The other problem is that the function return values for almost all
commands are wrong for the multi-iterator case, and that is what is
leading to the endless repeat loop problem. The issue here is that
the interactive/CMD_FLAG_GLOBAL mode requires 0 for "command
completed, prompt for next command", and 1 for "error, terminate".
In comparison, the iterative mode which calls the same functions
requires 0 for "process next command arg" and 1 for "process next
command". All of the functions return values required by the
interactive/CMD_FLAG_GLOBAL mode, and so don't so the right thing in
iterative mode....

> Incidentally, xfs_io does not advertise the "run all command per file"
> behavior, so not sure why it was done for and if anybody relies on it.

Not everything in xfs_io is documented in the man page as it's
always been a developer tool and the historic, sharp "cut you into
little pieces" bleedy bits that aren't useful to typical users
simply never got documented. This may be one of them, but I need to
go look at the history to determine if it was added intentionally,
or...

> I did not notice any tests that make use of multiple file args for xfs_io,
> but it wasn't easy to grep for this practice.

... it's just an unexpected interaction with common functionality
provided by libxcmd that is present to support xfs_quota
requirements, not xfs_io. Hence there's every chance that it's use
in xfs_io is simply incorrect and never intended, and you're the
first person to ever trip over it.

The solution may simply be that we add CMD_FLAG_GLOBAL to
every command in xfs_io, but I'm not sure yet.

> If you have a preference for solution I can execute the work.

Always fix the broken tool first.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] overlay: test ro/rw fd data inconsistecies

[Amir Goldstein]

On Mon, Dec 5, 2016 at 1:10 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Dec 02, 2016 at 10:13:33AM +0200

...
>
> ... it's just an unexpected interaction with common functionality
> provided by libxcmd that is present to support xfs_quota
> requirements, not xfs_io. Hence there's every chance that it's use
> in xfs_io is simply incorrect and never intended, and you're the
> first person to ever trip over it.
>
> The solution may simply be that we add CMD_FLAG_GLOBAL to
> every command in xfs_io, but I'm not sure yet.
>
>> If you have a preference for solution I can execute the work.
>
> Always fix the broken tool first.
>

The question was if I need to fix the run command per file or kill it.

Anyway, I came up with a very simple patch that enabled running
xfs_io -c <cmd> without any <file> args.
In that case, it is easy to understand what the user wants and commands
are just run once as if they where input in the interactive shell.

This is good enough for my test case and should be a good change
for xfs_io all in all.

Patch on its way.

I will let you decide how to handle the undocumented/unused/broken
case of multiple files and "open" command.

Amir.