Re: [PATCH v2] generic/563: tolerate small reads in "write -> read/write" sub-test

From: Boyang Xue <bxue@redhat.com>
To: Brian Foster <bfoster@redhat.com>
Cc: fstests@vger.kernel.org, Ming Lei <minlei@redhat.com>,
	Lukas Czerner <lczerner@redhat.com>
Subject: Re: [PATCH v2] generic/563: tolerate small reads in "write -> read/write" sub-test
Date: Sun, 25 Apr 2021 12:53:07 +0800	[thread overview]
Message-ID: <CAHLe9YajTPYfwn-K6t2gk=ybyYrOAkqHf1zSzkqbiekbQ2JVcQ@mail.gmail.com> (raw)
In-Reply-To: <YIKqZZTRZPFjtQwh@bfoster>

Hi Brian,

Please find my reply below inline.

On Fri, Apr 23, 2021 at 7:07 PM Brian Foster <bfoster@redhat.com> wrote:
>
> On Thu, Apr 22, 2021 at 11:31:47PM +0800, bxue@redhat.com wrote:
> > From: Boyang Xue <bxue@redhat.com>
> >
> > On ext2/ext3, it's expected that several single block metadata reads can occur
> > when writing to file in the same cgroup (the stack is like below[1]). The
> > purpose of the "write -> read/write" subtest is to make sure the larger pwrite
> > is accounted to the correct cgroup, not necessarily enforce that zero bytes are
> > read in service of the write. This patch fixes the sub-test in order to tolerate
> > small reads in 1st cgroup.
> >
> > [1] Callchain of the read:
> >
> > @ext3_read_bio[
> >     submit_bio+1
> >     submit_bh_wbc+365
> >     ext4_read_bh+72
> >     ext4_get_branch+201
> >     ext4_ind_map_blocks+382
> >     ext4_map_blocks+295
> >     _ext4_get_block+170
> >     __block_write_begin_int+328
> >     ext4_write_begin+541
> >     generic_perform_write+213
> >     ext4_buffered_write_iter+167
> >     new_sync_write+345
> >     vfs_write+438
> >     __x64_sys_pwrite64+140
> >     do_syscall_64+51
> >     entry_SYSCALL_64_after_hwframe+68
> > , 5793, 12]: 3
> >
> > Signed-off-by: Boyang Xue <bxue@redhat.com>
> > ---
> > Hi,
> >
> > This patch fix the "write -> read/write" sub-test in order to tolerate
> > small reads in service of the write (like read metadata).
> >
> > Change from v1:
> > (1) More details in commit log, including example call stack
> > (2) Set the fixed tolerance value to 33792 for accuracy
> > (3) Update percentage tolerance value to fixed value 0, where doesn't
> > fail the test
> >
> > Tested pass on ext2/ext3/ext4 x 1k/2k/4k blksize.
> >
> > Thanks,
> > Boyang
> >
> >  tests/generic/563 | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/generic/563 b/tests/generic/563
> > index b113eacf..44394b4b 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%

Here the write tolerance has to be 5% rather than 0, since testing
with ext2, the write bytes are slightly larger than $iosize (8392704
vs 8388608).

> >
> >  # 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,12 @@ $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
> > +# 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. On ext2/3 with 1k block size, the read bytes is
> > +# as large as 33792.
> > +check_cg $cgdir/$seq-cg 0 $iosize 33792 0
>
> Shouldn't that last parameter (write tolerance) remain as 5%?

In my test on ext2/3/4 with 1k/2k/4k, the write tolerance here always
equals $iosize in this sub-test. So make it 0 rather 5%, I hope it
will gate future behavior change more sensitively.

>
> > +check_cg $cgdir/$seq-cg-2 0 0 0 0
> >
> >  # 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 +146,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% 0
>
> And here too? Otherwise the patch LGTM.

The same reason as above.

If the write tolerance mismatch confuses future readers, I can put a
comment to explain. Maybe a single comment like this?

```
# Read and write from a single group.
echo "read/write"
reset
switch_cg $cgdir/$seq-cg
$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
# write bytes is slightly larger than $iosize on ext2  <=== new comment
check_cg $cgdir/$seq-cg $iosize $iosize 5% 5%
```

Thanks for the review!

-Boyang

> Brian
>
> > +check_cg $cgdir/$seq-cg-2 0 0 0 0
> >
> >  echo "-io" > $cgdir/cgroup.subtree_control || _fail "subtree control"
> >
> > --
> > 2.27.0
> >
>