[PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Zorro Lang]

Darrick found generic/411 golden output mismatch if use
TEST_DIR=/mnt. Because g/411 use some test path named
/mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
"/mnt" things to "TEST_DIR".

For stop this failure, change all directory names to be
"$seq-XXX", that's less likely to be mistaken for TEST_*
and SCRATCH_*.

Reported-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 tests/generic/411     | 10 +++++-----
 tests/generic/411.out |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/generic/411 b/tests/generic/411
index 414d3a5..8a45f14 100755
--- a/tests/generic/411
+++ b/tests/generic/411
@@ -123,18 +123,18 @@ crash_test()
 	start_test shared
 
 	_get_mount $SCRATCH_DEV $mpA
-	mkdir $mpA/mnt1
+	mkdir $mpA/${seq}-mnt1
 	$MOUNT_PROG --make-shared $mpA
 	_get_mount --bind $mpA $mpB
 	_get_mount --bind $mpA $mpC
 	$MOUNT_PROG --make-slave $mpB
 	$MOUNT_PROG --make-slave $mpC
-	_get_mount $SCRATCH_DEV $mpA/mnt1
-	mkdir $mpA/mnt1/mnt2
+	_get_mount $SCRATCH_DEV $mpA/${seq}-mnt1
+	mkdir $mpA/${seq}-mnt1/${seq}-mnt2
 
-	_get_mount $SCRATCH_DEV $mpB/mnt1/mnt2
+	_get_mount $SCRATCH_DEV $mpB/${seq}-mnt1/${seq}-mnt2
 	find_mnt
-	fs_stress $mpB/mnt1/mnt2
+	fs_stress $mpB/${seq}-mnt1/${seq}-mnt2
 
 	end_test
 	echo "crash test passed"
diff --git a/tests/generic/411.out b/tests/generic/411.out
index 16dadaf..01a0cdd 100644
--- a/tests/generic/411.out
+++ b/tests/generic/411.out
@@ -2,11 +2,11 @@ QA output created by 411
 ------
 TEST_DIR/411 SCRATCH_DEV
 mpA SCRATCH_DEV
-mpA/mnt1 SCRATCH_DEV
+mpA/411-mnt1 SCRATCH_DEV
 mpB SCRATCH_DEV
-mpB/mnt1 SCRATCH_DEV
-mpB/mnt1/mnt2 SCRATCH_DEV
+mpB/411-mnt1 SCRATCH_DEV
+mpB/411-mnt1/411-mnt2 SCRATCH_DEV
 mpC SCRATCH_DEV
-mpC/mnt1 SCRATCH_DEV
+mpC/411-mnt1 SCRATCH_DEV
 ======
 crash test passed
-- 
2.7.4

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Amir Goldstein]

On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@redhat.com> wrote:
> Darrick found generic/411 golden output mismatch if use
> TEST_DIR=/mnt. Because g/411 use some test path named
> /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
> "/mnt" things to "TEST_DIR".
>
> For stop this failure, change all directory names to be
> "$seq-XXX", that's less likely to be mistaken for TEST_*
> and SCRATCH_*.
>

Although you have a right to choose whichever names you
want top use for your test, this is papering over a bug.

I re-read the docuemtnation for \B:
http://www.rexegg.com/regex-boundaries.html#bengines

To my understanding, the expression "\B$TEST_DIR" will
match every instance of $TEST_DIR, where preceding character
is NOT a letter, number or underscore.
This is because $TEST_DIR must start with '/', which is not
a letter, number or underscore.

I think it should be safe to fix _filter_test_dir and _filter_scratch.

Eryu?

> Reported-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>  tests/generic/411     | 10 +++++-----
>  tests/generic/411.out |  8 ++++----
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/tests/generic/411 b/tests/generic/411
> index 414d3a5..8a45f14 100755
> --- a/tests/generic/411
> +++ b/tests/generic/411
> @@ -123,18 +123,18 @@ crash_test()
>         start_test shared
>
>         _get_mount $SCRATCH_DEV $mpA
> -       mkdir $mpA/mnt1
> +       mkdir $mpA/${seq}-mnt1
>         $MOUNT_PROG --make-shared $mpA
>         _get_mount --bind $mpA $mpB
>         _get_mount --bind $mpA $mpC
>         $MOUNT_PROG --make-slave $mpB
>         $MOUNT_PROG --make-slave $mpC
> -       _get_mount $SCRATCH_DEV $mpA/mnt1
> -       mkdir $mpA/mnt1/mnt2
> +       _get_mount $SCRATCH_DEV $mpA/${seq}-mnt1
> +       mkdir $mpA/${seq}-mnt1/${seq}-mnt2
>
> -       _get_mount $SCRATCH_DEV $mpB/mnt1/mnt2
> +       _get_mount $SCRATCH_DEV $mpB/${seq}-mnt1/${seq}-mnt2
>         find_mnt
> -       fs_stress $mpB/mnt1/mnt2
> +       fs_stress $mpB/${seq}-mnt1/${seq}-mnt2
>
>         end_test
>         echo "crash test passed"
> diff --git a/tests/generic/411.out b/tests/generic/411.out
> index 16dadaf..01a0cdd 100644
> --- a/tests/generic/411.out
> +++ b/tests/generic/411.out
> @@ -2,11 +2,11 @@ QA output created by 411
>  ------
>  TEST_DIR/411 SCRATCH_DEV
>  mpA SCRATCH_DEV
> -mpA/mnt1 SCRATCH_DEV
> +mpA/411-mnt1 SCRATCH_DEV
>  mpB SCRATCH_DEV
> -mpB/mnt1 SCRATCH_DEV
> -mpB/mnt1/mnt2 SCRATCH_DEV
> +mpB/411-mnt1 SCRATCH_DEV
> +mpB/411-mnt1/411-mnt2 SCRATCH_DEV
>  mpC SCRATCH_DEV
> -mpC/mnt1 SCRATCH_DEV
> +mpC/411-mnt1 SCRATCH_DEV
>  ======
>  crash test passed
> --
> 2.7.4
>
> --
> 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

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Eryu Guan]

On Wed, Mar 08, 2017 at 12:01:33PM +0200, Amir Goldstein wrote:
> On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@redhat.com> wrote:
> > Darrick found generic/411 golden output mismatch if use
> > TEST_DIR=/mnt. Because g/411 use some test path named
> > /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
> > "/mnt" things to "TEST_DIR".
> >
> > For stop this failure, change all directory names to be
> > "$seq-XXX", that's less likely to be mistaken for TEST_*
> > and SCRATCH_*.
> >
> 
> Although you have a right to choose whichever names you
> want top use for your test, this is papering over a bug.
> 
> I re-read the docuemtnation for \B:
> http://www.rexegg.com/regex-boundaries.html#bengines
> 
> To my understanding, the expression "\B$TEST_DIR" will
> match every instance of $TEST_DIR, where preceding character
> is NOT a letter, number or underscore.
> This is because $TEST_DIR must start with '/', which is not
> a letter, number or underscore.
> 
> I think it should be safe to fix _filter_test_dir and _filter_scratch.

I agreed, according to the document above, \B matches all positions
where \b doesn't match. And \b matches positions where "one side is a
word character and the other side is not", so \B matches "neither side
is a word character" and "both sides are a word character".

This is also because we canonicalized all mount points, there's no path
like //mnt/mnt1 is allowed in fstests. And this leads me to wonder if we
should canonicalize all test devices (if they're block device), to avoid
something like //dev/sda5? The double "/" will break the \B match.

Further more, if we decide to use \B to improve _filter_test_dir and
_filter_scratch, it appears to me that the fix from commit 4e965d8
("fstests: fix test and scratch filters for overlapping DEV/MNT paths")
can be discarded, the order is not a problem anymore.

Thanks for looking at this!

Eryu

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 3539 bytes --]

On Wed, Mar 8, 2017 at 1:51 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Mar 08, 2017 at 12:01:33PM +0200, Amir Goldstein wrote:
>> On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@redhat.com> wrote:
>> > Darrick found generic/411 golden output mismatch if use
>> > TEST_DIR=/mnt. Because g/411 use some test path named
>> > /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
>> > "/mnt" things to "TEST_DIR".
>> >
>> > For stop this failure, change all directory names to be
>> > "$seq-XXX", that's less likely to be mistaken for TEST_*
>> > and SCRATCH_*.
>> >
>>
>> Although you have a right to choose whichever names you
>> want top use for your test, this is papering over a bug.
>>
>> I re-read the docuemtnation for \B:
>> http://www.rexegg.com/regex-boundaries.html#bengines
>>
>> To my understanding, the expression "\B$TEST_DIR" will
>> match every instance of $TEST_DIR, where preceding character
>> is NOT a letter, number or underscore.
>> This is because $TEST_DIR must start with '/', which is not
>> a letter, number or underscore.
>>
>> I think it should be safe to fix _filter_test_dir and _filter_scratch.
>
> I agreed, according to the document above, \B matches all positions
> where \b doesn't match. And \b matches positions where "one side is a
> word character and the other side is not", so \B matches "neither side
> is a word character" and "both sides are a word character".
>
> This is also because we canonicalized all mount points, there's no path
> like //mnt/mnt1 is allowed in fstests. And this leads me to wonder if we
> should canonicalize all test devices (if they're block device), to avoid
> something like //dev/sda5? The double "/" will break the \B match.
>

//dev/sda5 does to break \B match.
\B$TEST_DEV match, as you wrote, means that character before
$TEST_DEV *is not* a word character, because $TEST_DEV begins
with a non-word character. it doesn't matter the the second character
is a non-word as well.

Attached a patch that I tested -g quick on xfs and overlayfs
and did not see any regressions, but you better test it with a wider
variety of fs types and a larger group of tests.

> Further more, if we decide to use \B to improve _filter_test_dir and
> _filter_scratch, it appears to me that the fix from commit 4e965d8
> ("fstests: fix test and scratch filters for overlapping DEV/MNT paths")
> can be discarded, the order is not a problem anymore.
>

Well, not exactly.

The case that commit 4e965d8 fixes is:
TEST_DEV=/mnt
TEST_DIR=/mnt/ovl-mnt

So \B$TEST_DEV will still match the prefix of $TEST_DIR and this is
wrong.

However, while $TEST_DEV can really be a prefix of $TEST_DIR
(and will always be a prefix with new -overlay)
I doubt that $TEST_DIR can ever be a prefix of $TEST_DEV.
So instead of the full blown test in commit 4e965d8, it would
suffice to just match $TEST_DIR before $TEST_DEV
(i.e. keep the if case and drop the else case).

So before you test \B with all fs types, you may revert commit 4e965d8,
but please make sure that the resulting filters are in the correct order:

_filter_test_dir()
{
        # TEST_DEV may be a prefix of TEST_DIR (e.g. /mnt and /mnt/ovl-mnt)
        # substitute TEST_DIR first
        sed -e "s,\B$TEST_DIR,TEST_DIR,g" \
               -e "s,\B$TEST_DEV,TEST_DEV,g"
}

_filter_scratch()
{
        # SCRATCH_DEV may be a prefix of SCRATCH_MNT
        # substitute SCRATCH_MNT first
        sed -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
               -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
               -e "/.use_space/d"
}

[-- Attachment #2: 0001-filter-match-TEST_-SCRATCH_-in-beginning-of-path-str.patch --]
[-- Type: text/x-patch, Size: 1879 bytes --]

From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Wed, 8 Mar 2017 12:38:22 +0200
Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string

For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
are in the beginning of a path string, e.g.:

"/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/filter | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/common/filter b/common/filter
index 1ceb346..2509483 100644
--- a/common/filter
+++ b/common/filter
@@ -283,13 +283,13 @@ _filter_test_dir()
 	if ( echo $TEST_DIR | grep -q $TEST_DEV ); then
 		# TEST_DEV is substr of TEST_DIR (e.g. /mnt and /mnt/ovl-mnt)
 		# substitute TEST_DIR first
-		sed -e "s,$TEST_DIR,TEST_DIR,g" \
-		    -e "s,$TEST_DEV,TEST_DEV,g"
+		sed -e "s,\B$TEST_DIR,TEST_DIR,g" \
+		    -e "s,\B$TEST_DEV,TEST_DEV,g"
 	else
 		# TEST_DIR maybe a substr of TEST_DIR (e.g. /vdc and /dev/vdc)
 		# substitute TEST_DEV first
-		sed -e "s,$TEST_DEV,TEST_DEV,g" \
-		    -e "s,$TEST_DIR,TEST_DIR,g"
+		sed -e "s,\B$TEST_DEV,TEST_DEV,g" \
+		    -e "s,\B$TEST_DIR,TEST_DIR,g"
 	fi
 }
 
@@ -297,13 +297,13 @@ _filter_scratch()
 {
 	if ( echo $SCRATCH_MNT | grep -q $SCRATCH_DEV ); then
 		# SCRATCH_DEV is substr of SCRATCH_MNT
-		sed -e "s,$SCRATCH_MNT,SCRATCH_MNT,g" \
-		    -e "s,$SCRATCH_DEV,SCRATCH_DEV,g" \
+		sed -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
+		    -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
 		    -e "/.use_space/d"
 	else
 		# SCRATCH_MNT maybe a substr of SCRATCH_DEV
-		sed -e "s,$SCRATCH_DEV,SCRATCH_DEV,g" \
-		    -e "s,$SCRATCH_MNT,SCRATCH_MNT,g" \
+		sed -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
+		    -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
 		    -e "/.use_space/d"
 	fi
 }
-- 
2.7.4

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Zorro Lang]

On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
> On Wed, Mar 8, 2017 at 1:51 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Wed, Mar 08, 2017 at 12:01:33PM +0200, Amir Goldstein wrote:
> >> On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@redhat.com> wrote:
> >> > Darrick found generic/411 golden output mismatch if use
> >> > TEST_DIR=/mnt. Because g/411 use some test path named
> >> > /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
> >> > "/mnt" things to "TEST_DIR".
> >> >
> >> > For stop this failure, change all directory names to be
> >> > "$seq-XXX", that's less likely to be mistaken for TEST_*
> >> > and SCRATCH_*.
> >> >
> >>
> >> Although you have a right to choose whichever names you
> >> want top use for your test, this is papering over a bug.
> >>
> >> I re-read the docuemtnation for \B:
> >> http://www.rexegg.com/regex-boundaries.html#bengines



Hmm, I just read this page. This's the first time for me to learn
about "Regex Boundaries", thanks for your sharing:) So "\B" will
match:

[word]\B[word]
or
[not-word]\B[not-word]

I think both TEST_* or SCRATCH_* begin with "/", which is not a word.
(We never use relative path at here, right? ...)
so if we use "\B$TEST_DIR" we truly expect "[not-word]\B$TEST_DIR"
("^" belong to "not a word").

> >>
> >> To my understanding, the expression "\B$TEST_DIR" will
> >> match every instance of $TEST_DIR, where preceding character
> >> is NOT a letter, number or underscore.
> >> This is because $TEST_DIR must start with '/', which is not
> >> a letter, number or underscore.
> >>
> >> I think it should be safe to fix _filter_test_dir and _filter_scratch.
> >
> > I agreed, according to the document above, \B matches all positions
> > where \b doesn't match. And \b matches positions where "one side is a
> > word character and the other side is not", so \B matches "neither side
> > is a word character" and "both sides are a word character".
> >
> > This is also because we canonicalized all mount points, there's no path
> > like //mnt/mnt1 is allowed in fstests. And this leads me to wonder if we
> > should canonicalize all test devices (if they're block device), to avoid
> > something like //dev/sda5? The double "/" will break the \B match.
> >
> 
> //dev/sda5 does to break \B match.
> \B$TEST_DEV match, as you wrote, means that character before
> $TEST_DEV *is not* a word character, because $TEST_DEV begins
> with a non-word character. it doesn't matter the the second character
> is a non-word as well.
> 
> Attached a patch that I tested -g quick on xfs and overlayfs
> and did not see any regressions, but you better test it with a wider
> variety of fs types and a larger group of tests.
> 
> > Further more, if we decide to use \B to improve _filter_test_dir and
> > _filter_scratch, it appears to me that the fix from commit 4e965d8
> > ("fstests: fix test and scratch filters for overlapping DEV/MNT paths")
> > can be discarded, the order is not a problem anymore.
> >
> 
> Well, not exactly.
> 
> The case that commit 4e965d8 fixes is:
> TEST_DEV=/mnt
> TEST_DIR=/mnt/ovl-mnt
> 
> So \B$TEST_DEV will still match the prefix of $TEST_DIR and this is
> wrong.
> 
> However, while $TEST_DEV can really be a prefix of $TEST_DIR
> (and will always be a prefix with new -overlay)
> I doubt that $TEST_DIR can ever be a prefix of $TEST_DEV.
> So instead of the full blown test in commit 4e965d8, it would
> suffice to just match $TEST_DIR before $TEST_DEV
> (i.e. keep the if case and drop the else case).
> 
> So before you test \B with all fs types, you may revert commit 4e965d8,
> but please make sure that the resulting filters are in the correct order:
> 
> _filter_test_dir()
> {
>         # TEST_DEV may be a prefix of TEST_DIR (e.g. /mnt and /mnt/ovl-mnt)
>         # substitute TEST_DIR first
>         sed -e "s,\B$TEST_DIR,TEST_DIR,g" \
>                -e "s,\B$TEST_DEV,TEST_DEV,g"
> }
> 
> _filter_scratch()
> {
>         # SCRATCH_DEV may be a prefix of SCRATCH_MNT
>         # substitute SCRATCH_MNT first
>         sed -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
>                -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
>                -e "/.use_space/d"
> }

I just tested this method, generic/411 can work well by this change, even if
set TEST_DIR=/mnt. But I haven't tested all xfstests cases. If we choose
this way, it maybe affect lots of cases.

So there're two ways, change single case and ask all future cases to notice
this problem, or change common _filter function and make sure all xfstests
cases can work well. What do you think, Eryu?

Thanks,
Zorro

> From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
> From: Amir Goldstein <amir73il@gmail.com>
> Date: Wed, 8 Mar 2017 12:38:22 +0200
> Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string
> 
> For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
> are in the beginning of a path string, e.g.:
> 
> "/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  common/filter | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/common/filter b/common/filter
> index 1ceb346..2509483 100644
> --- a/common/filter
> +++ b/common/filter
> @@ -283,13 +283,13 @@ _filter_test_dir()
>  	if ( echo $TEST_DIR | grep -q $TEST_DEV ); then
>  		# TEST_DEV is substr of TEST_DIR (e.g. /mnt and /mnt/ovl-mnt)
>  		# substitute TEST_DIR first
> -		sed -e "s,$TEST_DIR,TEST_DIR,g" \
> -		    -e "s,$TEST_DEV,TEST_DEV,g"
> +		sed -e "s,\B$TEST_DIR,TEST_DIR,g" \
> +		    -e "s,\B$TEST_DEV,TEST_DEV,g"
>  	else
>  		# TEST_DIR maybe a substr of TEST_DIR (e.g. /vdc and /dev/vdc)
>  		# substitute TEST_DEV first
> -		sed -e "s,$TEST_DEV,TEST_DEV,g" \
> -		    -e "s,$TEST_DIR,TEST_DIR,g"
> +		sed -e "s,\B$TEST_DEV,TEST_DEV,g" \
> +		    -e "s,\B$TEST_DIR,TEST_DIR,g"
>  	fi
>  }
>  
> @@ -297,13 +297,13 @@ _filter_scratch()
>  {
>  	if ( echo $SCRATCH_MNT | grep -q $SCRATCH_DEV ); then
>  		# SCRATCH_DEV is substr of SCRATCH_MNT
> -		sed -e "s,$SCRATCH_MNT,SCRATCH_MNT,g" \
> -		    -e "s,$SCRATCH_DEV,SCRATCH_DEV,g" \
> +		sed -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
> +		    -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
>  		    -e "/.use_space/d"
>  	else
>  		# SCRATCH_MNT maybe a substr of SCRATCH_DEV
> -		sed -e "s,$SCRATCH_DEV,SCRATCH_DEV,g" \
> -		    -e "s,$SCRATCH_MNT,SCRATCH_MNT,g" \
> +		sed -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
> +		    -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
>  		    -e "/.use_space/d"
>  	fi
>  }
> -- 
> 2.7.4
> 

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Amir Goldstein]

On Wed, Mar 8, 2017 at 4:59 PM, Zorro Lang <zlang@redhat.com> wrote:
> On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
>> On Wed, Mar 8, 2017 at 1:51 PM, Eryu Guan <eguan@redhat.com> wrote:
>> > On Wed, Mar 08, 2017 at 12:01:33PM +0200, Amir Goldstein wrote:
>> >> On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@redhat.com> wrote:
>> >> > Darrick found generic/411 golden output mismatch if use
>> >> > TEST_DIR=/mnt. Because g/411 use some test path named
>> >> > /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
>> >> > "/mnt" things to "TEST_DIR".
>> >> >
>> >> > For stop this failure, change all directory names to be
>> >> > "$seq-XXX", that's less likely to be mistaken for TEST_*
>> >> > and SCRATCH_*.
>> >> >
>> >>
>> >> Although you have a right to choose whichever names you
>> >> want top use for your test, this is papering over a bug.
>> >>
>> >> I re-read the docuemtnation for \B:
>> >> http://www.rexegg.com/regex-boundaries.html#bengines
>
>
>
> Hmm, I just read this page. This's the first time for me to learn
> about "Regex Boundaries", thanks for your sharing:) So "\B" will
> match:

Heh! it's first time for me too ;-)

>
> [word]\B[word]
> or
> [not-word]\B[not-word]
>
> I think both TEST_* or SCRATCH_* begin with "/", which is not a word.
> (We never use relative path at here, right? ...)
> so if we use "\B$TEST_DIR" we truly expect "[not-word]\B$TEST_DIR"
> ("^" belong to "not a word").
>

That is correct.

>> >>
>> >> To my understanding, the expression "\B$TEST_DIR" will
>> >> match every instance of $TEST_DIR, where preceding character
>> >> is NOT a letter, number or underscore.
>> >> This is because $TEST_DIR must start with '/', which is not
>> >> a letter, number or underscore.
>> >>
>> >> I think it should be safe to fix _filter_test_dir and _filter_scratch.
>> >
>> > I agreed, according to the document above, \B matches all positions
>> > where \b doesn't match. And \b matches positions where "one side is a
>> > word character and the other side is not", so \B matches "neither side
>> > is a word character" and "both sides are a word character".
>> >
>> > This is also because we canonicalized all mount points, there's no path
>> > like //mnt/mnt1 is allowed in fstests. And this leads me to wonder if we
>> > should canonicalize all test devices (if they're block device), to avoid
>> > something like //dev/sda5? The double "/" will break the \B match.
>> >
>>
>> //dev/sda5 does to break \B match.
>> \B$TEST_DEV match, as you wrote, means that character before
>> $TEST_DEV *is not* a word character, because $TEST_DEV begins
>> with a non-word character. it doesn't matter the the second character
>> is a non-word as well.
>>
>> Attached a patch that I tested -g quick on xfs and overlayfs
>> and did not see any regressions, but you better test it with a wider
>> variety of fs types and a larger group of tests.
>>
>> > Further more, if we decide to use \B to improve _filter_test_dir and
>> > _filter_scratch, it appears to me that the fix from commit 4e965d8
>> > ("fstests: fix test and scratch filters for overlapping DEV/MNT paths")
>> > can be discarded, the order is not a problem anymore.
>> >
>>
>> Well, not exactly.
>>
>> The case that commit 4e965d8 fixes is:
>> TEST_DEV=/mnt
>> TEST_DIR=/mnt/ovl-mnt
>>
>> So \B$TEST_DEV will still match the prefix of $TEST_DIR and this is
>> wrong.
>>
>> However, while $TEST_DEV can really be a prefix of $TEST_DIR
>> (and will always be a prefix with new -overlay)
>> I doubt that $TEST_DIR can ever be a prefix of $TEST_DEV.
>> So instead of the full blown test in commit 4e965d8, it would
>> suffice to just match $TEST_DIR before $TEST_DEV
>> (i.e. keep the if case and drop the else case).
>>
>> So before you test \B with all fs types, you may revert commit 4e965d8,
>> but please make sure that the resulting filters are in the correct order:
>>
>> _filter_test_dir()
>> {
>>         # TEST_DEV may be a prefix of TEST_DIR (e.g. /mnt and /mnt/ovl-mnt)
>>         # substitute TEST_DIR first
>>         sed -e "s,\B$TEST_DIR,TEST_DIR,g" \
>>                -e "s,\B$TEST_DEV,TEST_DEV,g"
>> }
>>
>> _filter_scratch()
>> {
>>         # SCRATCH_DEV may be a prefix of SCRATCH_MNT
>>         # substitute SCRATCH_MNT first
>>         sed -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
>>                -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
>>                -e "/.use_space/d"
>> }
>
> I just tested this method, generic/411 can work well by this change, even if
> set TEST_DIR=/mnt. But I haven't tested all xfstests cases. If we choose
> this way, it maybe affect lots of cases.
>

But can it really affect lots of cases??

If adding \B can break any test, that means that in some test output,
there exists a pattern [word]$TEST_DIR that is supposed to be
replaced with [word]TEST_DIR.
Even though I find it hard to believe that such case exists, I do not have to
resort to faith. I can use a simple test to verify if [word]TEST_DIR is
expected is golden output:

xfstests-dev$ git grep '\BTEST_DIR' tests/*/*.out
xfstests-dev$ git grep '\BTEST_DEV' tests/*/*.out
xfstests-dev$ git grep '\BSCRATCH_DEV' tests/*/*.out
xfstests-dev$ git grep '\BSCRATCH_MNT' tests/*/*.out

Nothing => Q.E.D


> So there're two ways, change single case and ask all future cases to notice
> this problem, or change common _filter function and make sure all xfstests
> cases can work well. What do you think, Eryu?
>
> Thanks,
> Zorro
>
>> From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
>> From: Amir Goldstein <amir73il@gmail.com>
>> Date: Wed, 8 Mar 2017 12:38:22 +0200
>> Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string
>>
>> For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
>> are in the beginning of a path string, e.g.:
>>
>> "/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  common/filter | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/common/filter b/common/filter
>> index 1ceb346..2509483 100644
>> --- a/common/filter
>> +++ b/common/filter
>> @@ -283,13 +283,13 @@ _filter_test_dir()
>>       if ( echo $TEST_DIR | grep -q $TEST_DEV ); then
>>               # TEST_DEV is substr of TEST_DIR (e.g. /mnt and /mnt/ovl-mnt)
>>               # substitute TEST_DIR first
>> -             sed -e "s,$TEST_DIR,TEST_DIR,g" \
>> -                 -e "s,$TEST_DEV,TEST_DEV,g"
>> +             sed -e "s,\B$TEST_DIR,TEST_DIR,g" \
>> +                 -e "s,\B$TEST_DEV,TEST_DEV,g"
>>       else
>>               # TEST_DIR maybe a substr of TEST_DIR (e.g. /vdc and /dev/vdc)
>>               # substitute TEST_DEV first
>> -             sed -e "s,$TEST_DEV,TEST_DEV,g" \
>> -                 -e "s,$TEST_DIR,TEST_DIR,g"
>> +             sed -e "s,\B$TEST_DEV,TEST_DEV,g" \
>> +                 -e "s,\B$TEST_DIR,TEST_DIR,g"
>>       fi
>>  }
>>
>> @@ -297,13 +297,13 @@ _filter_scratch()
>>  {
>>       if ( echo $SCRATCH_MNT | grep -q $SCRATCH_DEV ); then
>>               # SCRATCH_DEV is substr of SCRATCH_MNT
>> -             sed -e "s,$SCRATCH_MNT,SCRATCH_MNT,g" \
>> -                 -e "s,$SCRATCH_DEV,SCRATCH_DEV,g" \
>> +             sed -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
>> +                 -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
>>                   -e "/.use_space/d"
>>       else
>>               # SCRATCH_MNT maybe a substr of SCRATCH_DEV
>> -             sed -e "s,$SCRATCH_DEV,SCRATCH_DEV,g" \
>> -                 -e "s,$SCRATCH_MNT,SCRATCH_MNT,g" \
>> +             sed -e "s,\B$SCRATCH_DEV,SCRATCH_DEV,g" \
>> +                 -e "s,\B$SCRATCH_MNT,SCRATCH_MNT,g" \
>>                   -e "/.use_space/d"
>>       fi
>>  }
>> --
>> 2.7.4
>>
>

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Eryu Guan]

On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
> On Wed, Mar 8, 2017 at 1:51 PM, Eryu Guan <eguan@redhat.com> wrote:
> > On Wed, Mar 08, 2017 at 12:01:33PM +0200, Amir Goldstein wrote:
> >> On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@redhat.com> wrote:
> >> > Darrick found generic/411 golden output mismatch if use
> >> > TEST_DIR=/mnt. Because g/411 use some test path named
> >> > /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
> >> > "/mnt" things to "TEST_DIR".
> >> >
> >> > For stop this failure, change all directory names to be
> >> > "$seq-XXX", that's less likely to be mistaken for TEST_*
> >> > and SCRATCH_*.
> >> >
> >>
> >> Although you have a right to choose whichever names you
> >> want top use for your test, this is papering over a bug.
> >>
> >> I re-read the docuemtnation for \B:
> >> http://www.rexegg.com/regex-boundaries.html#bengines
> >>
> >> To my understanding, the expression "\B$TEST_DIR" will
> >> match every instance of $TEST_DIR, where preceding character
> >> is NOT a letter, number or underscore.
> >> This is because $TEST_DIR must start with '/', which is not
> >> a letter, number or underscore.
> >>
> >> I think it should be safe to fix _filter_test_dir and _filter_scratch.
> >
> > I agreed, according to the document above, \B matches all positions
> > where \b doesn't match. And \b matches positions where "one side is a
> > word character and the other side is not", so \B matches "neither side
> > is a word character" and "both sides are a word character".
> >
> > This is also because we canonicalized all mount points, there's no path
> > like //mnt/mnt1 is allowed in fstests. And this leads me to wonder if we
> > should canonicalize all test devices (if they're block device), to avoid
> > something like //dev/sda5? The double "/" will break the \B match.
> >
> 
> //dev/sda5 does to break \B match.
> \B$TEST_DEV match, as you wrote, means that character before
> $TEST_DEV *is not* a word character, because $TEST_DEV begins
> with a non-word character. it doesn't matter the the second character
> is a non-word as well.
> 
> Attached a patch that I tested -g quick on xfs and overlayfs
> and did not see any regressions, but you better test it with a wider
> variety of fs types and a larger group of tests.

Sure, I'll give it more tests. Thanks!

> 
> > Further more, if we decide to use \B to improve _filter_test_dir and
> > _filter_scratch, it appears to me that the fix from commit 4e965d8
> > ("fstests: fix test and scratch filters for overlapping DEV/MNT paths")
> > can be discarded, the order is not a problem anymore.
> >
> 
> Well, not exactly.
> 
> The case that commit 4e965d8 fixes is:
> TEST_DEV=/mnt
> TEST_DIR=/mnt/ovl-mnt
> 
> So \B$TEST_DEV will still match the prefix of $TEST_DIR and this is
> wrong.

You're right, I missed that.

Thanks,
Eryu

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Eryu Guan]

On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:

[snip]

> From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
> From: Amir Goldstein <amir73il@gmail.com>
> Date: Wed, 8 Mar 2017 12:38:22 +0200
> Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string
> 
> For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
> are in the beginning of a path string, e.g.:
> 
> "/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

I'm testing this patch now, with the following config:

TEST_DEV=/dev/vda5
TEST_DIR=/vda5
SCRATCH_DEV=/dev/vda6
SCRATCH_MNT=/vda6
FSTYP=xfs
MKFS_OPTIONS="-m crc=1,reflink=1"

First I run this test on xfs, and everything went well. Then I ran test
with -overlay option, and found generic/171 generic/172 in this order
confused _check_mounted_on().

./check -overlay generic/17[1-2]

I got this diff

--- tests/generic/172.out       2016-12-30 14:13:24.076000000 +0800
+++ /root/xfstests/results//xfs_4k_reflink/generic/172.out.bad  2017-03-09 17:27:12.203000000 +0800
@@ -1,9 +1,4 @@
 QA output created by 172
-Format and mount
-Reformat with appropriate size
-Create a big file and reflink it
-Allocate the rest of the space
-CoW the big file
-pwrite: No space left on device
-Remount and try CoW again
-pwrite: No space left on device
+SCRATCH_DEV=/vda6 is mounted but not on SCRATCH_MNT=/vda6/ovl-mnt - aborting
+Already mounted result:
+/dev/vda6 on /vda6 type xfs (rw,relatime,context=system_u:object_r:nfs_t:s0,attr2,inode64,noquota)

Seems the 'grep -F "$dev on ' check isn't that accurate either. This
also happens without this patch.

I'm trying a draft patch like below, this seems to fix the problem for
me, and it works for NFS too (both ipv4 and ipv6). I'm testing your
patch + my local fix now.  Will see how it goes, if everything goes well
I'll post it as a seperate patch.

diff --git a/common/rc b/common/rc
index 109325d..e3169d5 100644
--- a/common/rc
+++ b/common/rc
@@ -1440,11 +1440,13 @@ _check_mounted_on()
        # IPv6 server as a regular expression.  Because of that, we cannot use
        # ^$dev so we use "$dev on " to avoid matching $dev to mount point field
        # for overlay case, where $dev is a directory.
-       local mount_rec=`_mount | grep -F "$dev on "`
+#      local mount_rec=`_mount | grep -F "$dev on "`
+       local mount_rec=`findmnt -rnc -S $dev -o SOURCE,TARGET`
        [ -n "$mount_rec" ] || return 1 # 1 = not mounted

        # if it's mounted, make sure its on $mnt
-       if ! (echo $mount_rec | grep -q "$dev on $mnt")
+#      if ! (echo $mount_rec | grep -q "$dev on $mnt")
+       if ! (echo $mount_rec | grep -Fq "$dev $mnt")
        then
                echo "$devname=$dev is mounted but not on $mntname=$mnt - aborti
                echo "Already mounted result:"

I paste it here to see if you have any early comments :)

Thanks,
Eryu

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Amir Goldstein]

On Thu, Mar 9, 2017 at 11:59 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
>
> [snip]
>
>> From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
>> From: Amir Goldstein <amir73il@gmail.com>
>> Date: Wed, 8 Mar 2017 12:38:22 +0200
>> Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string
>>
>> For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
>> are in the beginning of a path string, e.g.:
>>
>> "/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> I'm testing this patch now, with the following config:
>
> TEST_DEV=/dev/vda5
> TEST_DIR=/vda5
> SCRATCH_DEV=/dev/vda6
> SCRATCH_MNT=/vda6
> FSTYP=xfs
> MKFS_OPTIONS="-m crc=1,reflink=1"
>
> First I run this test on xfs, and everything went well. Then I ran test
> with -overlay option, and found generic/171 generic/172 in this order
> confused _check_mounted_on().
>
> ./check -overlay generic/17[1-2]
>
> I got this diff
>
> --- tests/generic/172.out       2016-12-30 14:13:24.076000000 +0800
> +++ /root/xfstests/results//xfs_4k_reflink/generic/172.out.bad  2017-03-09 17:27:12.203000000 +0800
> @@ -1,9 +1,4 @@
>  QA output created by 172
> -Format and mount
> -Reformat with appropriate size
> -Create a big file and reflink it
> -Allocate the rest of the space
> -CoW the big file
> -pwrite: No space left on device
> -Remount and try CoW again
> -pwrite: No space left on device
> +SCRATCH_DEV=/vda6 is mounted but not on SCRATCH_MNT=/vda6/ovl-mnt - aborting
> +Already mounted result:
> +/dev/vda6 on /vda6 type xfs (rw,relatime,context=system_u:object_r:nfs_t:s0,attr2,inode64,noquota)
>
> Seems the 'grep -F "$dev on ' check isn't that accurate either. This
> also happens without this patch.
>

Oops. Good catch.
I have this sort of setup with kvm-xfstests, but only tested it with
old overlay config
(because I need to change kvm-xfstests scripts to new overlay config)

> I'm trying a draft patch like below, this seems to fix the problem for
> me, and it works for NFS too (both ipv4 and ipv6). I'm testing your
> patch + my local fix now.  Will see how it goes, if everything goes well
> I'll post it as a seperate patch.

Are you planning to keep my patch as is or drop the else statement in
the filters?

>
> diff --git a/common/rc b/common/rc
> index 109325d..e3169d5 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1440,11 +1440,13 @@ _check_mounted_on()
>         # IPv6 server as a regular expression.  Because of that, we cannot use
>         # ^$dev so we use "$dev on " to avoid matching $dev to mount point field
>         # for overlay case, where $dev is a directory.
> -       local mount_rec=`_mount | grep -F "$dev on "`
> +#      local mount_rec=`_mount | grep -F "$dev on "`
> +       local mount_rec=`findmnt -rnc -S $dev -o SOURCE,TARGET`
>         [ -n "$mount_rec" ] || return 1 # 1 = not mounted
>
>         # if it's mounted, make sure its on $mnt
> -       if ! (echo $mount_rec | grep -q "$dev on $mnt")
> +#      if ! (echo $mount_rec | grep -q "$dev on $mnt")
> +       if ! (echo $mount_rec | grep -Fq "$dev $mnt")
>         then
>                 echo "$devname=$dev is mounted but not on $mntname=$mnt - aborti
>                 echo "Already mounted result:"
>
> I paste it here to see if you have any early comments :)

Look good.

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Amir Goldstein]

On Thu, Mar 9, 2017 at 1:28 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Mar 9, 2017 at 11:59 AM, Eryu Guan <eguan@redhat.com> wrote:
>> On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
>>
>> [snip]
>>
>>> From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
>>> From: Amir Goldstein <amir73il@gmail.com>
>>> Date: Wed, 8 Mar 2017 12:38:22 +0200
>>> Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string
>>>
>>> For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
>>> are in the beginning of a path string, e.g.:
>>>
>>> "/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>
>> I'm testing this patch now, with the following config:
>>
>> TEST_DEV=/dev/vda5
>> TEST_DIR=/vda5
>> SCRATCH_DEV=/dev/vda6
>> SCRATCH_MNT=/vda6
>> FSTYP=xfs
>> MKFS_OPTIONS="-m crc=1,reflink=1"
>>
>> First I run this test on xfs, and everything went well. Then I ran test
>> with -overlay option, and found generic/171 generic/172 in this order
>> confused _check_mounted_on().
>>
>> ./check -overlay generic/17[1-2]
>>
>> I got this diff
>>
>> --- tests/generic/172.out       2016-12-30 14:13:24.076000000 +0800
>> +++ /root/xfstests/results//xfs_4k_reflink/generic/172.out.bad  2017-03-09 17:27:12.203000000 +0800
>> @@ -1,9 +1,4 @@
>>  QA output created by 172
>> -Format and mount
>> -Reformat with appropriate size
>> -Create a big file and reflink it
>> -Allocate the rest of the space
>> -CoW the big file
>> -pwrite: No space left on device
>> -Remount and try CoW again
>> -pwrite: No space left on device
>> +SCRATCH_DEV=/vda6 is mounted but not on SCRATCH_MNT=/vda6/ovl-mnt - aborting
>> +Already mounted result:
>> +/dev/vda6 on /vda6 type xfs (rw,relatime,context=system_u:object_r:nfs_t:s0,attr2,inode64,noquota)
>>
>> Seems the 'grep -F "$dev on ' check isn't that accurate either. This
>> also happens without this patch.
>>
>
> Oops. Good catch.
> I have this sort of setup with kvm-xfstests, but only tested it with
> old overlay config
> (because I need to change kvm-xfstests scripts to new overlay config)

So I changed kvm-xfstests over to new overlay config and got the error
you reported.
Your fix also works, but see a few minor comments below.

>
>> I'm trying a draft patch like below, this seems to fix the problem for
>> me, and it works for NFS too (both ipv4 and ipv6). I'm testing your
>> patch + my local fix now.  Will see how it goes, if everything goes well
>> I'll post it as a seperate patch.
>
> Are you planning to keep my patch as is or drop the else statement in
> the filters?
>
>>
>> diff --git a/common/rc b/common/rc
>> index 109325d..e3169d5 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -1440,11 +1440,13 @@ _check_mounted_on()
>>         # IPv6 server as a regular expression.  Because of that, we cannot use
>>         # ^$dev so we use "$dev on " to avoid matching $dev to mount point field
>>         # for overlay case, where $dev is a directory.

The comment above is no longer relevant

>> -       local mount_rec=`_mount | grep -F "$dev on "`
>> +#      local mount_rec=`_mount | grep -F "$dev on "`
>> +       local mount_rec=`findmnt -rnc -S $dev -o SOURCE,TARGET`
>>         [ -n "$mount_rec" ] || return 1 # 1 = not mounted
>>
>>         # if it's mounted, make sure its on $mnt
>> -       if ! (echo $mount_rec | grep -q "$dev on $mnt")
>> +#      if ! (echo $mount_rec | grep -q "$dev on $mnt")
>> +       if ! (echo $mount_rec | grep -Fq "$dev $mnt")

Why bother with grep -F (and having to keep the comment above).
I used the following instead:
+       if [ "$mount_rec" != "$dev $mnt" ]


>>         then
>>                 echo "$devname=$dev is mounted but not on $mntname=$mnt - aborti
>>                 echo "Already mounted result:"
>>
>> I paste it here to see if you have any early comments :)
>
> Look good.

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Eryu Guan]

On Thu, Mar 09, 2017 at 03:16:38PM +0200, Amir Goldstein wrote:
> On Thu, Mar 9, 2017 at 1:28 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > On Thu, Mar 9, 2017 at 11:59 AM, Eryu Guan <eguan@redhat.com> wrote:
> >> On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
> >>
> >> [snip]
> >>
> >>> From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
> >>> From: Amir Goldstein <amir73il@gmail.com>
> >>> Date: Wed, 8 Mar 2017 12:38:22 +0200
> >>> Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string
> >>>
> >>> For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
> >>> are in the beginning of a path string, e.g.:
> >>>
> >>> "/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"
> >>>
> >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >>
> >> I'm testing this patch now, with the following config:
> >>
> >> TEST_DEV=/dev/vda5
> >> TEST_DIR=/vda5
> >> SCRATCH_DEV=/dev/vda6
> >> SCRATCH_MNT=/vda6
> >> FSTYP=xfs
> >> MKFS_OPTIONS="-m crc=1,reflink=1"
> >>
> >> First I run this test on xfs, and everything went well. Then I ran test
> >> with -overlay option, and found generic/171 generic/172 in this order
> >> confused _check_mounted_on().
> >>
> >> ./check -overlay generic/17[1-2]
> >>
> >> I got this diff
> >>
> >> --- tests/generic/172.out       2016-12-30 14:13:24.076000000 +0800
> >> +++ /root/xfstests/results//xfs_4k_reflink/generic/172.out.bad  2017-03-09 17:27:12.203000000 +0800
> >> @@ -1,9 +1,4 @@
> >>  QA output created by 172
> >> -Format and mount
> >> -Reformat with appropriate size
> >> -Create a big file and reflink it
> >> -Allocate the rest of the space
> >> -CoW the big file
> >> -pwrite: No space left on device
> >> -Remount and try CoW again
> >> -pwrite: No space left on device
> >> +SCRATCH_DEV=/vda6 is mounted but not on SCRATCH_MNT=/vda6/ovl-mnt - aborting
> >> +Already mounted result:
> >> +/dev/vda6 on /vda6 type xfs (rw,relatime,context=system_u:object_r:nfs_t:s0,attr2,inode64,noquota)
> >>
> >> Seems the 'grep -F "$dev on ' check isn't that accurate either. This
> >> also happens without this patch.
> >>
> >
> > Oops. Good catch.
> > I have this sort of setup with kvm-xfstests, but only tested it with
> > old overlay config
> > (because I need to change kvm-xfstests scripts to new overlay config)
> 
> So I changed kvm-xfstests over to new overlay config and got the error
> you reported.
> Your fix also works, but see a few minor comments below.
> 
> >
> >> I'm trying a draft patch like below, this seems to fix the problem for
> >> me, and it works for NFS too (both ipv4 and ipv6). I'm testing your
> >> patch + my local fix now.  Will see how it goes, if everything goes well
> >> I'll post it as a seperate patch.
> >
> > Are you planning to keep my patch as is or drop the else statement in
> > the filters?

I was testing your attached patch as is, if you can send a formal patch
with the else statement dropped that'd be great.

> >
> >>
> >> diff --git a/common/rc b/common/rc
> >> index 109325d..e3169d5 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -1440,11 +1440,13 @@ _check_mounted_on()
> >>         # IPv6 server as a regular expression.  Because of that, we cannot use
> >>         # ^$dev so we use "$dev on " to avoid matching $dev to mount point field
> >>         # for overlay case, where $dev is a directory.
> 
> The comment above is no longer relevant

Yes, will remove it in final patch, this one is a draft patch.

> 
> >> -       local mount_rec=`_mount | grep -F "$dev on "`
> >> +#      local mount_rec=`_mount | grep -F "$dev on "`
> >> +       local mount_rec=`findmnt -rnc -S $dev -o SOURCE,TARGET`
> >>         [ -n "$mount_rec" ] || return 1 # 1 = not mounted
> >>
> >>         # if it's mounted, make sure its on $mnt
> >> -       if ! (echo $mount_rec | grep -q "$dev on $mnt")
> >> +#      if ! (echo $mount_rec | grep -q "$dev on $mnt")
> >> +       if ! (echo $mount_rec | grep -Fq "$dev $mnt")
> 
> Why bother with grep -F (and having to keep the comment above).
> I used the following instead:
> +       if [ "$mount_rec" != "$dev $mnt" ]

Yes, this looks better, thanks!

Eryu

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Eryu Guan]

On Fri, Mar 10, 2017 at 11:52:40AM +0800, Eryu Guan wrote:
> On Thu, Mar 09, 2017 at 03:16:38PM +0200, Amir Goldstein wrote:
> > On Thu, Mar 9, 2017 at 1:28 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > > On Thu, Mar 9, 2017 at 11:59 AM, Eryu Guan <eguan@redhat.com> wrote:
> > >> On Wed, Mar 08, 2017 at 04:26:22PM +0200, Amir Goldstein wrote:
> > >>
> > >> [snip]
> > >>
> > >>> From f365bd5d60d9d5b82e02db17e615380bf37a74de Mon Sep 17 00:00:00 2001
> > >>> From: Amir Goldstein <amir73il@gmail.com>
> > >>> Date: Wed, 8 Mar 2017 12:38:22 +0200
> > >>> Subject: [PATCH] filter: match $TEST_* $SCRATCH_* in beginning of path string
> > >>>
> > >>> For example, if $TEST_DIR=/mnt, only replace instacnes of /mnt that
> > >>> are in the beginning of a path string, e.g.:
> > >>>
> > >>> "/mnt/mntA/mntB:/mnt/mntC" => "TEST_DIR/mntA/mntB:TEST_DIR/mntC"
> > >>>
> > >>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >>
> > >> I'm testing this patch now, with the following config:
> > >>
> > >> TEST_DEV=/dev/vda5
> > >> TEST_DIR=/vda5
> > >> SCRATCH_DEV=/dev/vda6
> > >> SCRATCH_MNT=/vda6
> > >> FSTYP=xfs
> > >> MKFS_OPTIONS="-m crc=1,reflink=1"
> > >>
> > >> First I run this test on xfs, and everything went well. Then I ran test
> > >> with -overlay option, and found generic/171 generic/172 in this order
> > >> confused _check_mounted_on().
> > >>
> > >> ./check -overlay generic/17[1-2]
> > >>
> > >> I got this diff
> > >>
> > >> --- tests/generic/172.out       2016-12-30 14:13:24.076000000 +0800
> > >> +++ /root/xfstests/results//xfs_4k_reflink/generic/172.out.bad  2017-03-09 17:27:12.203000000 +0800
> > >> @@ -1,9 +1,4 @@
> > >>  QA output created by 172
> > >> -Format and mount
> > >> -Reformat with appropriate size
> > >> -Create a big file and reflink it
> > >> -Allocate the rest of the space
> > >> -CoW the big file
> > >> -pwrite: No space left on device
> > >> -Remount and try CoW again
> > >> -pwrite: No space left on device
> > >> +SCRATCH_DEV=/vda6 is mounted but not on SCRATCH_MNT=/vda6/ovl-mnt - aborting
> > >> +Already mounted result:
> > >> +/dev/vda6 on /vda6 type xfs (rw,relatime,context=system_u:object_r:nfs_t:s0,attr2,inode64,noquota)
> > >>
> > >> Seems the 'grep -F "$dev on ' check isn't that accurate either. This
> > >> also happens without this patch.
> > >>
> > >
> > > Oops. Good catch.
> > > I have this sort of setup with kvm-xfstests, but only tested it with
> > > old overlay config
> > > (because I need to change kvm-xfstests scripts to new overlay config)
> > 
> > So I changed kvm-xfstests over to new overlay config and got the error
> > you reported.
> > Your fix also works, but see a few minor comments below.
> > 
> > >
> > >> I'm trying a draft patch like below, this seems to fix the problem for
> > >> me, and it works for NFS too (both ipv4 and ipv6). I'm testing your
> > >> patch + my local fix now.  Will see how it goes, if everything goes well
> > >> I'll post it as a seperate patch.
> > >
> > > Are you planning to keep my patch as is or drop the else statement in
> > > the filters?
> 
> I was testing your attached patch as is, if you can send a formal patch
> with the else statement dropped that'd be great.

Your test patch worked fine in my testing. I tested the following
configs with both reflink enabled xfs (which could cover the most test
cases) and overlayfs on top of xfs (both old and new config)

# kvm-xfstests config
TEST_DEV=/dev/sdc1
TEST_DIR=/sdc1
SCRATCH_DEV=/dev/sdc2
SCRATCH_MNT=/sdc2

# djwong config
TEST_DEV=/dev/sdc1
TEST_DIR=/mnt
SCRATCH_DEV=/dev/sdc2
SCRATCH_MNT=/opt

Thanks,
Eryu

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Amir Goldstein]

On Fri, Mar 10, 2017 at 8:57 AM, Eryu Guan <eguan@redhat.com> wrote:
> On Fri, Mar 10, 2017 at 11:52:40AM +0800, Eryu Guan wrote:
[...]
>> > > Are you planning to keep my patch as is or drop the else statement in
>> > > the filters?
>>
>> I was testing your attached patch as is, if you can send a formal patch
>> with the else statement dropped that'd be great.
>
> Your test patch worked fine in my testing. I tested the following
> configs with both reflink enabled xfs (which could cover the most test
> cases) and overlayfs on top of xfs (both old and new config)
>
> # kvm-xfstests config
> TEST_DEV=/dev/sdc1
> TEST_DIR=/sdc1
> SCRATCH_DEV=/dev/sdc2
> SCRATCH_MNT=/sdc2
>
> # djwong config
> TEST_DEV=/dev/sdc1
> TEST_DIR=/mnt
> SCRATCH_DEV=/dev/sdc2
> SCRATCH_MNT=/opt
>

Cool. I sent you the formal patch without the else statement.
It passed my kvm-xfstests run with new overlay config (along with your patch)

Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR

[Zorro Lang]

On Wed, Mar 08, 2017 at 12:01:33PM +0200, Amir Goldstein wrote:
> On Wed, Mar 8, 2017 at 9:26 AM, Zorro Lang <zlang@redhat.com> wrote:
> > Darrick found generic/411 golden output mismatch if use
> > TEST_DIR=/mnt. Because g/411 use some test path named
> > /mnt/XXXX/mnt1/mnt2, _filter_test_dir will replace all
> > "/mnt" things to "TEST_DIR".
> >
> > For stop this failure, change all directory names to be
> > "$seq-XXX", that's less likely to be mistaken for TEST_*
> > and SCRATCH_*.
> >
> 
> Although you have a right to choose whichever names you
> want top use for your test, this is papering over a bug.
> 
> I re-read the docuemtnation for \B:
> http://www.rexegg.com/regex-boundaries.html#bengines

Oh, sorry, I sent this patch before I saw your reply about "\B".
I don't think this's the best way to fix this problem, just sent
a demo patch which follow the first round discussion result with
Eryu. We can NACK this patch and keep looking for a better one
anytime :)

Let me read above documentation at first :)

Thanks,
Zorro


> 
> To my understanding, the expression "\B$TEST_DIR" will
> match every instance of $TEST_DIR, where preceding character
> is NOT a letter, number or underscore.
> This is because $TEST_DIR must start with '/', which is not
> a letter, number or underscore.
> 
> I think it should be safe to fix _filter_test_dir and _filter_scratch.
> 
> Eryu?
> 
> > Reported-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> >  tests/generic/411     | 10 +++++-----
> >  tests/generic/411.out |  8 ++++----
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/tests/generic/411 b/tests/generic/411
> > index 414d3a5..8a45f14 100755
> > --- a/tests/generic/411
> > +++ b/tests/generic/411
> > @@ -123,18 +123,18 @@ crash_test()
> >         start_test shared
> >
> >         _get_mount $SCRATCH_DEV $mpA
> > -       mkdir $mpA/mnt1
> > +       mkdir $mpA/${seq}-mnt1
> >         $MOUNT_PROG --make-shared $mpA
> >         _get_mount --bind $mpA $mpB
> >         _get_mount --bind $mpA $mpC
> >         $MOUNT_PROG --make-slave $mpB
> >         $MOUNT_PROG --make-slave $mpC
> > -       _get_mount $SCRATCH_DEV $mpA/mnt1
> > -       mkdir $mpA/mnt1/mnt2
> > +       _get_mount $SCRATCH_DEV $mpA/${seq}-mnt1
> > +       mkdir $mpA/${seq}-mnt1/${seq}-mnt2
> >
> > -       _get_mount $SCRATCH_DEV $mpB/mnt1/mnt2
> > +       _get_mount $SCRATCH_DEV $mpB/${seq}-mnt1/${seq}-mnt2
> >         find_mnt
> > -       fs_stress $mpB/mnt1/mnt2
> > +       fs_stress $mpB/${seq}-mnt1/${seq}-mnt2
> >
> >         end_test
> >         echo "crash test passed"
> > diff --git a/tests/generic/411.out b/tests/generic/411.out
> > index 16dadaf..01a0cdd 100644
> > --- a/tests/generic/411.out
> > +++ b/tests/generic/411.out
> > @@ -2,11 +2,11 @@ QA output created by 411
> >  ------
> >  TEST_DIR/411 SCRATCH_DEV
> >  mpA SCRATCH_DEV
> > -mpA/mnt1 SCRATCH_DEV
> > +mpA/411-mnt1 SCRATCH_DEV
> >  mpB SCRATCH_DEV
> > -mpB/mnt1 SCRATCH_DEV
> > -mpB/mnt1/mnt2 SCRATCH_DEV
> > +mpB/411-mnt1 SCRATCH_DEV
> > +mpB/411-mnt1/411-mnt2 SCRATCH_DEV
> >  mpC SCRATCH_DEV
> > -mpC/mnt1 SCRATCH_DEV
> > +mpC/411-mnt1 SCRATCH_DEV
> >  ======
> >  crash test passed
> > --
> > 2.7.4
> >
> > --
> > 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
> --
> 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