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@xxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxx>
>
> --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
> >
>
