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

From: Brian Foster <bfoster@redhat.com>
To: bxue@redhat.com
Cc: fstests@vger.kernel.org, minlei@redhat.com, lczerner@redhat.com
Subject: Re: [PATCH v2] generic/563: tolerate small reads in "write -> read/write" sub-test
Date: Fri, 23 Apr 2021 07:07:17 -0400	[thread overview]
Message-ID: <YIKqZZTRZPFjtQwh@bfoster> (raw)
In-Reply-To: <20210422153147.1049666-1-bxue@redhat.com>

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
> 