Hi Brian,
Thanks for the review. Please find my reply inline below.
On Wed, Apr 21, 2021 at 11:53 PM Brian Foster <bfoster@xxxxxxxxxx> wrote:
>
> On Thu, Apr 15, 2021 at 02:27:44PM +0800, bxue@xxxxxxxxxx wrote:
> > From: Boyang Xue <bxue@xxxxxxxxxx>
> >
> > On ext2/ext3, there're small reads when writing to file in the same cgroup.
> > Since this sub-test tests that if read/write from 2nd cgroup is both 0 after
> > writing in 1st cgroup, these small reads from 1st cgroup should not fail the
> > test. This patch fixes the sub-test in order to tolerate small reads in 1st
> > cgroup.
> >
> > Signed-off-by: Boyang Xue <bxue@xxxxxxxxxx>
> > ---
> > Hi,
> >
> > I found generic/563 fails on ext2/ext3 on the latest kernel:
> >
> > [root@kvm109 repo_xfstests]# ./check generic/563
> > FSTYP -- ext3
> > PLATFORM -- Linux/x86_64 kvm109 5.12.0-0.rc3.170.xx.x86_64 #1 SMP Tue Mar
> > 16 12:02:55 EDT 2021
> > MKFS_OPTIONS -- -b 4096 /dev/vda3
> > MOUNT_OPTIONS -- -o rw,relatime,seclabel -o context=system_u:object_r:root_t:s0
> > /dev/vda3 /scratch
> >
> > generic/563 4s ... - output mismatch (see
> > /tmp/tmp.hMWbgkavD4/repo_xfstests/results//generic/563.out.bad)
> > --- tests/generic/563.out 2021-04-01 02:07:16.303329895 -0400
> > +++ /tmp/tmp.hMWbgkavD4/repo_xfstests/results//generic/563.out.bad
> > 2021-04-01 03:06:19.240329895 -0400
> > @@ -3,7 +3,8 @@
> > read is in range
> > write is in range
> > write -> read/write
> > -read is in range
> > +read has value of 12288
> > +read is NOT in range 0 .. 0
> > write is in range
> > ...
> > (Run 'diff -u /tmp/tmp.hMWbgkavD4/repo_xfstests/tests/generic/563.out
> > /tmp/tmp.hMWbgkavD4/repo_xfstests/results//generic/563.out.bad' to see the
> > entire diff)
> > Ran: generic/563
> > Failures: generic/563
> > Failed 1 of 1 tests
> > ```
> >
> > generic/563 code
> > ```
> > ...
> > # Write from one cgroup then read and write from a second. Writes are charged to
> > # the first group and nothing to the second.
> > echo "write -> read/write"
> > reset <== I have injected commands here for check, it turns out it indeed
> > resets rbytes and wbytes both to 0.
> > switch_cg $cgdir/$seq-cg
> > $XFS_IO_PROG -c "pwrite 0 $iosize" $SCRATCH_MNT/file >> $seqres.full 2>&1
> > switch_cg $cgdir/$seq-cg-2
> > $XFS_IO_PROG -c "pread 0 $iosize" -c "pwrite 0 $iosize" $SCRATCH_MNT/file \
> > >> $seqres.full 2>&1
> > switch_cg $cgdir
> > $XFS_IO_PROG -c fsync $SCRATCH_MNT/file
> > check_cg $cgdir/$seq-cg 0 $iosize <== problem here, expected read bytes = 0,
> > but it's 12288
> > check_cg $cgdir/$seq-cg-2 0 0
> > ...
> > ```
> >
> > local.config
> > ```
> > FSTYP="ext3"
> > TEST_DIR="/test"
> > TEST_DEV="/dev/vda2"
> > SCRATCH_MNT="/scratch"
> > SCRATCH_DEV="/dev/vda3"
> > LOGWRITES_MNT="/logwrites"
> > LOGWRITES_DEV="/dev/vda6"
> > MKFS_OPTIONS="-b 4096"
> > MOUNT_OPTIONS="-o rw,relatime,seclabel"
> > TEST_FS_MOUNT_OPTS="-o rw,relatime,seclabel"
> > ```
> >
> > I think the "write -> read/write" sub-test should test if the read/write bytes
> > in 2nd cgroup both are 0, after writing in the 1st cgroup. Given that it writes
> > 8MB in cgroup, dozens of small reads in service of the write (like read
> > metadata) is not part of the goal of the sub-test, and should be tolerate,
> > rather than fail the test.
> >
> > Currently, the expected read bytes in the 1st cgroup is strictly 0. This patch
> > sets a fixed tolerant value, so read bytes in the range of 0-(tolerant value) is
> > tolerant, doesn't fail the test.
> >
> > I have run the original test on ext2/ext3/ext4 with 1k/2k/4k blksize on x86_64,
> > aarch64, s390x, ppc64le, to determine the tolerant value. It turns out the
> > maximum read bytes in the tests is 33792. So I think set the tolerant value
> > to 33800 is adequate.
> >
> > Please help review this patch. Thanks.
> >
> > -Boyang
> >
> > tests/generic/563 | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/generic/563 b/tests/generic/563
> > index b113eacf..83146721 100755
> > --- a/tests/generic/563
> > +++ b/tests/generic/563
> > @@ -60,6 +60,8 @@ check_cg()
> > cgname=$(basename $cgroot)
> > expectedread=$2
> > expectedwrite=$3
> > + readtol=$4
> > + writetol=$5
> > rbytes=0
> > wbytes=0
> >
> > @@ -71,8 +73,8 @@ check_cg()
> > awk -F = '{ print $2 }'`
> > fi
> >
> > - _within_tolerance "read" $rbytes $expectedread 5% -v
> > - _within_tolerance "write" $wbytes $expectedwrite 5% -v
> > + _within_tolerance "read" $rbytes $expectedread $readtol -v
> > + _within_tolerance "write" $wbytes $expectedwrite $writetol -v
> > }
> >
> > # Move current process to another cgroup.
> > @@ -113,7 +115,7 @@ $XFS_IO_PROG -c "pread 0 $iosize" -c "pwrite 0 $iosize" -c fsync \
> > $SCRATCH_MNT/file >> $seqres.full 2>&1
> > switch_cg $cgdir
> > $XFS_IO_PROG -c fsync $SCRATCH_MNT/file
> > -check_cg $cgdir/$seq-cg $iosize $iosize
> > +check_cg $cgdir/$seq-cg $iosize $iosize 5% 5%
> >
> > # Write from one cgroup then read and write from a second. Writes are charged to
> > # the first group and nothing to the second.
> > @@ -126,8 +128,8 @@ $XFS_IO_PROG -c "pread 0 $iosize" -c "pwrite 0 $iosize" $SCRATCH_MNT/file \
> > >> $seqres.full 2>&1
> > switch_cg $cgdir
> > $XFS_IO_PROG -c fsync $SCRATCH_MNT/file
> > -check_cg $cgdir/$seq-cg 0 $iosize
> > -check_cg $cgdir/$seq-cg-2 0 0
> > +check_cg $cgdir/$seq-cg 0 $iosize 33800 5%
>
> Any reason for the 33800 value as opposed to an even 32k? Also a brief
> comment might be useful:
Test results from ext2/3/4 1k/2k/4k blksize shows the largest possible
value of the read bytes is 33792, so I chose this a bit larger value.
In my next version of the patch, I'm thinking fix the value to 33792,
for accuracy. Does it look better?
>
> # Use a fixed value tolerance for the expected value of zero here
> # because filesystems might perform a small number of metadata reads to
> # complete the write.
Thanks. I'm pasting this comment in the next version.
>
> > +check_cg $cgdir/$seq-cg-2 0 0 5% 5%
> >
>
> I'm not sure it ever really makes sense to have a percentage tolerance
> when the expected values are zero. This is the case with the current
> implementation simply because the tolerance is hardcoded in the helper
> function. If we're going to pass the tolerances for each test along with
> the expected values, it might make a bit more sense to pass along a 0
> tolerance where that is expected. Otherwise the rest LGTM. Thanks for
> sending the patch..
Good idea. I'll fix the test in this way.
>
> Brian
>
> > # Read from one cgroup, read & write from a second. Both reads and writes are
> > # charged to the first group and nothing to the second.
> > @@ -140,8 +142,8 @@ $XFS_IO_PROG -c "pread 0 $iosize" -c "pwrite 0 $iosize" $SCRATCH_MNT/file \
> > >> $seqres.full 2>&1
> > switch_cg $cgdir
> > $XFS_IO_PROG -c fsync $SCRATCH_MNT/file
> > -check_cg $cgdir/$seq-cg $iosize $iosize
> > -check_cg $cgdir/$seq-cg-2 0 0
> > +check_cg $cgdir/$seq-cg $iosize $iosize 5% 5%
> > +check_cg $cgdir/$seq-cg-2 0 0 5% 5%
> >
> > echo "-io" > $cgdir/cgroup.subtree_control || _fail "subtree control"
> >
> > --
> > 2.27.0
> >
>
I will fix all these together with more details in the commit log, and
post the next version soon.
Thanks!
-Boyang
