Re: [PATCH] xfs/014: try a few times to create speculative preallocations

[Zorro Lang <zlang@redhat.com>]

On Wed, Jan 05, 2022 at 11:09:57AM -0800, Darrick J. Wong wrote:
> On Thu, Jan 06, 2022 at 12:19:05AM +0800, Zorro Lang wrote:
> > On Mon, Jan 03, 2022 at 06:04:17PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > This test checks that speculative file preallocations are transferred to
> > > threads writing other files when space is low.  Since we have background
> > > threads to clear those preallocations, it's possible that the test
> > > program might not get a speculative preallocation on the first try.
> > > 
> > > This problem has become more pronounced since the introduction of
> > > background inode inactivation since userspace no longer has direct
> > > control over the timing of file blocks being released from unlinked
> > > files.  As a result, the author has seen an increase in sporadic
> > > warnings from this test about speculative preallocations not appearing.
> > > 
> > > Therefore, modify the function to try up to five times to create the
> > > speculative preallocation before emitting warnings that then cause
> > > golden output failures.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/xfs/014 |   41 +++++++++++++++++++++++++----------------
> > >  1 file changed, 25 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/tests/xfs/014 b/tests/xfs/014
> > > index a605b359..1f0ebac3 100755
> > > --- a/tests/xfs/014
> > > +++ b/tests/xfs/014
> > > @@ -33,27 +33,36 @@ _cleanup()
> > >  # failure.
> > >  _spec_prealloc_file()
> > >  {
> > > -	file=$1
> > > +	local file=$1
> > > +	local prealloc_size=0
> > > +	local i=0
> > >  
> > > -	rm -f $file
> > > +	# Now that we have background garbage collection processes that can be
> > > +	# triggered by low space/quota conditions, it's possible that we won't
> > > +	# succeed in creating a speculative preallocation on the first try.
> > > +	for ((tries = 0; tries < 5 && prealloc_size == 0; tries++)); do
> > > +		rm -f $file
> > >  
> > > -	# a few file extending open-write-close cycles should be enough to
> > > -	# trigger the fs to retain preallocation. write 256k in 32k intervals to
> > > -	# be sure
> > > -	for i in $(seq 0 32768 262144); do
> > > -		$XFS_IO_PROG -f -c "pwrite $i 32k" $file >> $seqres.full
> > > +		# a few file extending open-write-close cycles should be enough
> > > +		# to trigger the fs to retain preallocation. write 256k in 32k
> > > +		# intervals to be sure
> > > +		for i in $(seq 0 32768 262144); do
> > > +			$XFS_IO_PROG -f -c "pwrite $i 32k" $file >> $seqres.full
> > > +		done
> > > +
> > > +		# write a 4k aligned amount of data to keep the calculations
> > > +		# simple
> > > +		$XFS_IO_PROG -c "pwrite 0 128m" $file >> $seqres.full
> > > +
> > > +		size=`_get_filesize $file`
> > > +		blocks=`stat -c "%b" $file`
> > > +		blocksize=`stat -c "%B" $file`
> > > +
> > > +		prealloc_size=$((blocks * blocksize - size))
> > 
> > So we only try same pwrite operations 5 times, and only check the prealloc_size after 5
> > times done? Should we break from this loop once prealloc_size > 0?
> 
> The second clause of the for loop tests for that, does it not?

Oh, yes, sorry I missed the "&& prealloc_size == 0", I thought you just gave it 5 tries :)
So this patch is good to me.

Reviewed-by: Zorro Lang <zlang@redhat.com>

> 
> --D
> 
> > 
> > Thanks,
> > Zorro
> > 
> > >  	done
> > >  
> > > -	# write a 4k aligned amount of data to keep the calculations simple
> > > -	$XFS_IO_PROG -c "pwrite 0 128m" $file >> $seqres.full
> > > -
> > > -	size=`_get_filesize $file`
> > > -	blocks=`stat -c "%b" $file`
> > > -	blocksize=`stat -c "%B" $file`
> > > -
> > > -	prealloc_size=$((blocks * blocksize - size))
> > >  	if [ $prealloc_size -eq 0 ]; then
> > > -		echo "Warning: No speculative preallocation for $file." \
> > > +		echo "Warning: No speculative preallocation for $file after $tries iterations." \
> > >  			"Check use of the allocsize= mount option."
> > >  	fi
> > >  
> > > 
> > 
>