Re: [PATCH v3] generic/139: require 512 bytes to be the minimum dio size

[Zorro Lang <zlang@redhat.com>]

On Thu, Jun 02, 2022 at 09:40:45AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 02, 2022 at 01:17:16PM +0800, Zorro Lang wrote:
> > Due to generic/139 tests base on 512 bytes aligned, so skip this test
> > if the minimum dio write size >512. This patch also change the
> > common/rc::_require_dio helper, supports a minimum aligned size
> > argument.
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> > 
> > Thanks the review from Darrick on v2, this v3 remove some duplicate code
> > which I forgot.
> > 
> > Thanks,
> > Zorro
> > 
> >  common/rc         | 13 ++++++++++---
> >  tests/generic/139 |  2 +-
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 2f31ca46..9823e3a1 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2721,9 +2721,12 @@ _require_xfs_io_command()
> >  	fi
> >  }
> >  
> > -# check that kernel and filesystem support direct I/O
> > +# check that kernel and filesystem support direct I/O, and check if "$1" size
> > +# aligned (optional) is supported
> >  _require_odirect()
> >  {
> > +	local alignment=${1:+"-b $1"}
> 
> This might be a nit, but you might want to do this instead:
> 
> 	local blocksize=$1
> 	local align_args=${1:+"-b $1"}
> 
> So that there's only one "$1" to change if the arguments ever get
> rearranged.  But that might never happen, and this feels nearly like
> pointless navelgazing.
> 
> If you're happy with things the way they are, the logic looks ok so:

OK, I respect the meticulous attitude, will do that when I merge it :)

> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
> > +
> >  	if [ $FSTYP = "ext4" ] || [ $FSTYP = "f2fs" ] ; then
> >  		if echo "$MOUNT_OPTIONS" | grep -q "test_dummy_encryption"; then
> >  			_notrun "$FSTYP encryption doesn't support O_DIRECT"
> > @@ -2735,9 +2738,13 @@ _require_odirect()
> >  		fi
> >  	fi
> >  	local testfile=$TEST_DIR/$$.direct
> > -	$XFS_IO_PROG -F -f -d -c "pwrite 0 20k" $testfile > /dev/null 2>&1
> > +	$XFS_IO_PROG -F -f -d -c "pwrite $alignment 0 20k" $testfile > /dev/null 2>&1
> >  	if [ $? -ne 0 ]; then
> > -		_notrun "O_DIRECT is not supported"
> > +		if [ -n "$alignment" ]; then
> > +			_notrun "O_DIRECT aligned to $1 bytes is not supported"
> > +		else
> > +			_notrun "O_DIRECT is not supported"
> > +		fi
> >  	fi
> >  	rm -f $testfile 2>&1 > /dev/null
> >  }
> > diff --git a/tests/generic/139 b/tests/generic/139
> > index 0bbc222c..3eb1519d 100755
> > --- a/tests/generic/139
> > +++ b/tests/generic/139
> > @@ -26,7 +26,7 @@ _cleanup()
> >  # real QA test starts here
> >  _require_test_reflink
> >  _require_cp_reflink
> > -_require_odirect
> > +_require_odirect 512
> >  
> >  testdir=$TEST_DIR/test-$seq
> >  rm -rf $testdir
> > -- 
> > 2.31.1
> > 
>