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

Brian Foster
2021-04-23T11:07:23Z

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%
>
> # 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%?

> +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.

Brian

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