Re: [PATCH v3 1/2] common: new helper to alloacate fixed size files

[Zorro Lang <zlang@redhat.com>]

On Tue, Sep 20, 2022 at 07:53:52PM -0700, Darrick J. Wong wrote:
> On Wed, Sep 21, 2022 at 10:48:57AM +0800, Zorro Lang wrote:
> > On Tue, Sep 20, 2022 at 09:35:13AM +0200, Pavel Reichl wrote:
> > > Helper that creates files of specified size using falloc if supported,
> > > otherwise pwrite is used.
> > > 
> > > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > > ---
> > >  common/rc         | 13 +++++++++++++
> > >  tests/generic/694 |  2 +-
> > >  2 files changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index a25cbcd0..77866582 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -4925,6 +4925,19 @@ hexdump()
> > >  	_fail "Use _hexdump(), please!"
> > >  }
> > >  
> > > +# Helper to write a file containing specified number of bytes using
> > > +# falloc if supported, otherwise use pwrite
> > > +_create_sizedfile()
> > > +{
> > > +	length=$1
> > > +	file=$2
> > > +
> > > +	$XFS_IO_PROG -F -fc "falloc 0 $length" $file 2>&1 | grep -q "Operation not supported"
> > > +	if [ $? -eq 0 ]; then
> > > +		$XFS_IO_PROG -F -fc "pwrite -W 0 $length" $file >/dev/null
> > > +	fi
> > > +}
> > 
> > I think about it more, above code might ignore a failed falloc, if it's not failed
> > by "Operation not supported" but really fails, this function won't print or return
> > any valid things.
> > 
> > So how about we write it like below (for reference only):
> > 
> > # Try to create a file which inode->i_blocks = $length (maybe a little bigger
> > # than expect)
> > _create_file_sized()
> > {
> > 	local length=$1
> > 	local file=$2
> > 	local tmp=`mktemp -u`
> > 	local ret=0
> > 
> > 	$XFS_IO_PROG -ft -c "falloc 0 $length" $file >$tmp.out 2>&1
> > 	ret=$?
> > 	if (grep -Eq "Operation not supported|command .* not found" $tmp.out);then
> > 		# fallocate isn't supported, fallback to general buffer write
> > 		$XFS_IO_PROG -ft -c "pwrite 0 $length" $file >$tmp.out 2>&1
> > 		ret=$?
> > 	fi
> > 	[ $ret -ne 0 ] && cat $tmp.out
> > 	rm -f $tmp.out
> > 	return $ret
> > }
> > 
> > Even though, I think this function might still not good, feel free to tell me if
> > anyone has better idea/suggestion about how to get a file with specified
> > inode->i_blocks.
> 
> Looks reasonable enough to /me...

Thanks for your review, Darrick.

Hi Pavel, the [PATCH 2/2] looks good to me. I'll merge this patchset after you
making this change.

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > +
> > >  init_rc
> > >  
> > >  ################################################################################
> > > diff --git a/tests/generic/694 b/tests/generic/694
> > > index dfd988df..64c3dd9a 100755
> > > --- a/tests/generic/694
> > > +++ b/tests/generic/694
> > > @@ -30,7 +30,7 @@ junk_dir=$TEST_DIR/$seq
> > >  junk_file=$junk_dir/junk
> > >  mkdir -p $junk_dir
> > >  
> > > -$XFS_IO_PROG -f -c "pwrite -W 0 4G" $junk_file > /dev/null
> > > +_create_sizedfile 4G $junk_file
> > >  
> > >  iblocks=`stat -c '%b' $junk_file`
> > >  
> > > -- 
> > > 2.37.3
> > > 
> > 
>