* [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR @ 2017-03-08 7:26 Zorro Lang 2017-03-08 10:01 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Zorro Lang @ 2017-03-08 7:26 UTC (permalink / raw) To: fstests; +Cc: darrick.wong 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-08 7:26 [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR Zorro Lang @ 2017-03-08 10:01 ` Amir Goldstein 2017-03-08 11:51 ` Eryu Guan 2017-03-08 13:15 ` Zorro Lang 0 siblings, 2 replies; 14+ messages in thread From: Amir Goldstein @ 2017-03-08 10:01 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests, Darrick J. Wong 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-08 10:01 ` Amir Goldstein @ 2017-03-08 11:51 ` Eryu Guan 2017-03-08 14:26 ` Amir Goldstein 2017-03-08 13:15 ` Zorro Lang 1 sibling, 1 reply; 14+ messages in thread From: Eryu Guan @ 2017-03-08 11:51 UTC (permalink / raw) To: Amir Goldstein; +Cc: Zorro Lang, fstests, Darrick J. Wong 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-08 11:51 ` Eryu Guan @ 2017-03-08 14:26 ` Amir Goldstein 2017-03-08 14:59 ` Zorro Lang ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Amir Goldstein @ 2017-03-08 14:26 UTC (permalink / raw) To: Eryu Guan; +Cc: Zorro Lang, fstests, Darrick J. Wong [-- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-08 14:26 ` Amir Goldstein @ 2017-03-08 14:59 ` Zorro Lang 2017-03-08 16:32 ` Amir Goldstein 2017-03-09 3:11 ` Eryu Guan 2017-03-09 9:59 ` Eryu Guan 2 siblings, 1 reply; 14+ messages in thread From: Zorro Lang @ 2017-03-08 14:59 UTC (permalink / raw) To: Amir Goldstein; +Cc: Eryu Guan, fstests, Darrick J. Wong 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-08 14:59 ` Zorro Lang @ 2017-03-08 16:32 ` Amir Goldstein 0 siblings, 0 replies; 14+ messages in thread From: Amir Goldstein @ 2017-03-08 16:32 UTC (permalink / raw) To: Zorro Lang; +Cc: Eryu Guan, fstests, Darrick J. Wong 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 >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-08 14:26 ` Amir Goldstein 2017-03-08 14:59 ` Zorro Lang @ 2017-03-09 3:11 ` Eryu Guan 2017-03-09 9:59 ` Eryu Guan 2 siblings, 0 replies; 14+ messages in thread From: Eryu Guan @ 2017-03-09 3:11 UTC (permalink / raw) To: Amir Goldstein; +Cc: Zorro Lang, fstests, Darrick J. Wong 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-08 14:26 ` Amir Goldstein 2017-03-08 14:59 ` Zorro Lang 2017-03-09 3:11 ` Eryu Guan @ 2017-03-09 9:59 ` Eryu Guan 2017-03-09 11:28 ` Amir Goldstein 2 siblings, 1 reply; 14+ messages in thread From: Eryu Guan @ 2017-03-09 9:59 UTC (permalink / raw) To: Amir Goldstein; +Cc: Zorro Lang, fstests, Darrick J. Wong 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-09 9:59 ` Eryu Guan @ 2017-03-09 11:28 ` Amir Goldstein 2017-03-09 13:16 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2017-03-09 11:28 UTC (permalink / raw) To: Eryu Guan; +Cc: Zorro Lang, fstests, Darrick J. Wong 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-09 11:28 ` Amir Goldstein @ 2017-03-09 13:16 ` Amir Goldstein 2017-03-10 3:52 ` Eryu Guan 0 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2017-03-09 13:16 UTC (permalink / raw) To: Eryu Guan; +Cc: Zorro Lang, fstests, Darrick J. Wong 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-09 13:16 ` Amir Goldstein @ 2017-03-10 3:52 ` Eryu Guan 2017-03-10 6:57 ` Eryu Guan 0 siblings, 1 reply; 14+ messages in thread From: Eryu Guan @ 2017-03-10 3:52 UTC (permalink / raw) To: Amir Goldstein; +Cc: Zorro Lang, fstests, Darrick J. Wong 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-10 3:52 ` Eryu Guan @ 2017-03-10 6:57 ` Eryu Guan 2017-03-10 7:44 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Eryu Guan @ 2017-03-10 6:57 UTC (permalink / raw) To: Amir Goldstein; +Cc: Zorro Lang, fstests, Darrick J. Wong 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-10 6:57 ` Eryu Guan @ 2017-03-10 7:44 ` Amir Goldstein 0 siblings, 0 replies; 14+ messages in thread From: Amir Goldstein @ 2017-03-10 7:44 UTC (permalink / raw) To: Eryu Guan; +Cc: Zorro Lang, fstests, Darrick J. Wong 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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR 2017-03-08 10:01 ` Amir Goldstein 2017-03-08 11:51 ` Eryu Guan @ 2017-03-08 13:15 ` Zorro Lang 1 sibling, 0 replies; 14+ messages in thread From: Zorro Lang @ 2017-03-08 13:15 UTC (permalink / raw) To: Amir Goldstein; +Cc: fstests 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-03-10 7:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-08 7:26 [PATCH] generic/411: change sub-path name that's duplicate of TEST_DIR Zorro Lang 2017-03-08 10:01 ` Amir Goldstein 2017-03-08 11:51 ` Eryu Guan 2017-03-08 14:26 ` Amir Goldstein 2017-03-08 14:59 ` Zorro Lang 2017-03-08 16:32 ` Amir Goldstein 2017-03-09 3:11 ` Eryu Guan 2017-03-09 9:59 ` Eryu Guan 2017-03-09 11:28 ` Amir Goldstein 2017-03-09 13:16 ` Amir Goldstein 2017-03-10 3:52 ` Eryu Guan 2017-03-10 6:57 ` Eryu Guan 2017-03-10 7:44 ` Amir Goldstein 2017-03-08 13:15 ` Zorro Lang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.