Re: [PATCH v3] generic: make 17[1-4] work well when btrfs compression is enabled

[Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>]

hi,

On 10/28/2016 01:13 AM, Darrick J. Wong wrote:
> On Wed, Oct 26, 2016 at 05:52:11PM +0800, Wang Xiaoguang wrote:
>> When enabling btrfs compression, original codes can not fill fs
>> correctly, here we introduce _fill_fs() in common/rc, which'll keep
>> creating and writing files until enospc error occurs. Note _fill_fs
>> is copied from tests/generic/256, but with some minor modifications.
>>
>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>> ---
>> V2: In common/, I did't find an existing function suitable for
>>      these 4 test cases to fill fs, so I still use _pwrite_byte() with
>>      a big enough file length fo fill fs. Note, for btrfs, metadata space
>>      still is not full, only data space is full, but it's OK for these
>>      4 test cases.
>>
>>      All these 4 cases pass in xfs and btrfs(without compression), if
>>      btrfs has compression enabled, these 4 cases will fail for false
>>      enospc error, I have sent kernel patches to fix this bug.
>>
>> V3: Introduce  _fill_fs in common/rc to fill fs.
>> ---
>>   common/rc         | 50 ++++++++++++++++++++++++++++++++++++++++++
>>   tests/generic/171 |  4 +---
>>   tests/generic/172 |  4 ++--
>>   tests/generic/173 |  4 +---
>>   tests/generic/174 |  4 +---
>>   tests/generic/256 | 65 +++++--------------------------------------------------
>>   6 files changed, 60 insertions(+), 71 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 7a9fc90..0e1ac5d 100644
>> --- a/common/rc
>> +++ b/common/rc
> Would you mind moving this to common/populate, where I've been (slowly)
> collecting all the "create stuff inside the filesystem" routines?
OK, sure.
>
> (It's part of a slow-moving effort to declutter common/rc.)
>
>> @@ -4003,6 +4003,56 @@ _require_xfs_mkfs_without_validation()
>>   	fi
>>   }
>>   
>> +# Fill a file system by repeatedly creating files in the given folder
>> +# starting with the given file size.  Files are reduced in size when
>> +# they can no longer fit until no more files can be created.
>> +_fill_fs()
>> +{
>> +	local file_size=$1
>> +	local dir=$2
>> +	local block_size=$3
>> +	local switch_user=$4
>> +	local file_count=1
>> +	local bytes_written=0
>> +
>> +	if [ $# -ne 4 ]; then
>> +		echo "Usage: _fill_fs filesize dir blocksize"
>> +		exit 1
>> +	fi
>> +
>> +	if [ $switch_user -eq 0 ]; then
>> +		mkdir -p $dir
>> +	else
>> +		_user_do "mkdir -p $dir"
>> +	fi
>> +
>> +	if [ $file_size -lt $block_size ]; then
>> +		$file_size = $block_size
>> +	fi
>> +
>> +	while [ $file_size -ge $block_size ]; do
>> +		bytes_written=0
>> +		if [ $switch_user -eq 0 ]; then
>> +			$XFS_IO_PROG -f -c "pwrite 0 $file_size" \
>> +				$dir/$file_count > /dev/null
> If you want to speed this up some more you could pass "-b 8388608" in
> the pwrite string so that xfs_io will allocate an 8MB memory buffer
> internally when writing out data.
>
> $XFS_IO_PROG -f -c "pwrite -b 8388608 0 $file_size" $dir/$file_count
Thanks for this info, I'll use it.
>
>> +		else
>> +			_user_do "$XFS_IO_PROG -f -c \"pwrite 0 $file_size\" \
>> +				$dir/$file_count > /dev/null"
> Also, why redirect to /dev/null?  I think helper functions generally let
> the individual test decide where the stdout & stderr output ultimately
> end up.  Most tests seem to dump to $seqres.full or /dev/null, but we
> should let the test decide where potentially useful diagnostic
> information ends up.
Agree :)

> /me also wonders if falloc would be a faster way to consume disk blocks
> than pwrite, but ... <shrug>
Oh, it is. Falloc is not affected by compression.

>> +		fi
>> +
>> +		if [ -f $dir/$file_count ]; then
>> +			bytes_written=$(stat -c '%s' $dir/$file_count)
>> +		fi
>> +
>> +		# If there was no room to make the file, then divide it in
>> +		# half, and keep going
>> +		if [ $bytes_written -lt $file_size ]; then
>> +			file_size=$((file_size / 2))
>> +		fi
>> +		file_count=$((file_count + 1))
>> +	done
>> +}
>> +
>>   init_rc
>>   
>>   ################################################################################
>> diff --git a/tests/generic/171 b/tests/generic/171
>> index a69f798..fde8ef2 100755
>> --- a/tests/generic/171
>> +++ b/tests/generic/171
>> @@ -75,9 +75,7 @@ _cp_reflink $testdir/bigfile $testdir/clonefile
>>   sync
>>   
>>   echo "Allocate the rest of the space"
>> -nr_free=$(stat -f -c '%f' $testdir)
>> -touch $testdir/file0 $testdir/file1
>> -_pwrite_byte 0x61 0 $((blksz * nr_free)) $testdir/eat_my_space >> $seqres.full 2>&1
>> +_fill_fs $((32 * 1024 * 1024)) $testdir/space $blksz 0 >> $seqres.full 2>&1
> Given that _fill_fs has exponential backoff to try to really fill the fs,
> why not call it with $((blksz * nr_free)) instead of a fixed 32MB?
OK, I'll send v4 soon.
Thanks for all your comments.

Regards,
Xiaoguang Wang
>
> --D
>
>>   sync
>>   
>>   echo "CoW the big file"
>> diff --git a/tests/generic/172 b/tests/generic/172
>> index 8192290..3625546 100755
>> --- a/tests/generic/172
>> +++ b/tests/generic/172
>> @@ -57,6 +57,7 @@ testdir=$SCRATCH_MNT/test-$seq
>>   mkdir $testdir
>>   
>>   echo "Reformat with appropriate size"
>> +blksz="$(get_block_size $testdir)"
>>   umount $SCRATCH_MNT
>>   
>>   file_size=$((168 * 1024 * 1024))
>> @@ -72,8 +73,7 @@ _cp_reflink $testdir/bigfile $testdir/clonefile
>>   sync
>>   
>>   echo "Allocate the rest of the space"
>> -touch $testdir/file0 $testdir/file1
>> -_pwrite_byte 0x61 0 $fs_size $testdir/eat_my_space >> $seqres.full 2>&1
>> +_fill_fs $((32 * 1024 * 1024)) $testdir/space $blksz 0 >> $seqres.full 2>&1
>>   sync
>>   
>>   echo "CoW the big file"
>> diff --git a/tests/generic/173 b/tests/generic/173
>> index e35597f..3d583b1 100755
>> --- a/tests/generic/173
>> +++ b/tests/generic/173
>> @@ -75,9 +75,7 @@ _cp_reflink $testdir/bigfile $testdir/clonefile
>>   sync
>>   
>>   echo "Allocate the rest of the space"
>> -nr_free=$(stat -f -c '%f' $testdir)
>> -touch $testdir/file0 $testdir/file1
>> -_pwrite_byte 0x61 0 $((blksz * nr_free)) $testdir/eat_my_space >> $seqres.full 2>&1
>> +_fill_fs $((32 * 1024 * 1024)) $testdir/space $blksz 0 >> $seqres.full 2>&1
>>   sync
>>   
>>   echo "mmap CoW the big file"
>> diff --git a/tests/generic/174 b/tests/generic/174
>> index e58d64b..bd7fc11 100755
>> --- a/tests/generic/174
>> +++ b/tests/generic/174
>> @@ -76,9 +76,7 @@ _cp_reflink $testdir/bigfile $testdir/clonefile
>>   sync
>>   
>>   echo "Allocate the rest of the space"
>> -nr_free=$(stat -f -c '%f' $testdir)
>> -touch $testdir/file0 $testdir/file1
>> -_pwrite_byte 0x61 0 $((blksz * nr_free)) $testdir/eat_my_space >> $seqres.full 2>&1
>> +_fill_fs $((32 * 1024 * 1024)) $testdir/space $blksz 0 >> $seqres.full 2>&1
>>   sync
>>   
>>   echo "CoW the big file"
>> diff --git a/tests/generic/256 b/tests/generic/256
>> index cfbf790..9488648 100755
>> --- a/tests/generic/256
>> +++ b/tests/generic/256
>> @@ -53,64 +53,6 @@ _require_test
>>   
>>   testfile=$TEST_DIR/256.$$
>>   
>> -# _fill_fs()
>> -#
>> -# Fills a file system by repeatedly creating files in the given folder
>> -# starting with the given file size.  Files are reduced in size when
>> -# they can no longer fit untill no more files can be created.
>> -#
>> -# This routine is used by _test_full_fs_punch to test that a hole may
>> -# still be punched when the disk is full by borrowing reserved blocks.
>> -# All files are created as a non root user to prevent reserved blocks
>> -# from being consumed.
>> -#
>> -_fill_fs() {
>> -	local file_size=$1
>> -	local dir=$2
>> -	local block_size=$3
>> -	local file_count=1
>> -	local bytes_written=0
>> -
>> -	if [ $# -ne 3 ]
>> -	then
>> -		echo "USAGE: _fill_fs filesize dir block size"
>> -		exit 1
>> -	fi
>> -
>> -	# Creation of files or folders
>> -	# must not be done as root or
>> -	# reserved blocks will be consumed
>> -	_user_do "mkdir -p $dir &> /dev/null"
>> -	if [ $? -ne 0 ] ; then
>> -		return 0
>> -	fi
>> -
>> -	if [ $file_size -lt $block_size ]
>> -	then
>> -		$file_size = $block_size
>> -	fi
>> -
>> -	while [ $file_size -ge $block_size ]
>> -	do
>> -		bytes_written=0
>> -		_user_do "$XFS_IO_PROG -f -c \"pwrite 0 $file_size\" $dir/$file_count.bin &> /dev/null"
>> -
>> -		if [ -f $dir/$file_count.bin ]
>> -		then
>> -			bytes_written=`$XFS_IO_PROG -c "stat"  $dir/$file_count.bin | grep stat.size | cut -d ' ' -f3`
>> -		fi
>> -
>> -		# If there was no room to make the file,
>> -		# then divide it in half, and keep going
>> -		if [ $bytes_written -lt $file_size ]
>> -		then
>> -			file_size=$(( $file_size / 2 ))
>> -		fi
>> -		file_count=$(( $file_count + 1 ))
>> -
>> -	done
>> -}
>> -
>>   # _test_full_fs_punch()
>>   #
>>   # This function will test that a hole may be punched
>> @@ -144,7 +86,10 @@ _test_full_fs_punch()
>>   		-c "fsync" $file_name &> /dev/null
>>   	chmod 666 $file_name
>>   
>> -	_fill_fs $(( 1024 * 1024 * 1024 )) $path/fill $block_size
>> +	# All files are created as a non root user to prevent reserved blocks
>> +	# from being consumed.
>> +	_fill_fs $(( 1024 * 1024 * 1024 )) $path/fill $block_size 1 \
>> +		> /dev/null 2>&1
>>   
>>   	for (( i=0; i<$iterations; i++ ))
>>   	do
>> @@ -159,7 +104,7 @@ _test_full_fs_punch()
>>   
>>   		hole_offset=$(( $hole_offset + $hole_len + $hole_interval ))
>>   
>> -		_fill_fs $hole_len $path/fill.$i $block_size
>> +		_fill_fs $hole_len $path/fill.$i $block_size 1 > /dev/null 2>&1
>>   
>>   	done
>>   }
>> -- 
>> 2.9.0
>>
>>
>>
>> --
>> 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
>