| Message ID | 20170309095938.GJ14226@eguan.usersys.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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 -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 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) -- 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
--- 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:"