| Message ID | 20220602051716.2050004-1-zlang@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] generic/139: require 512 bytes to be the minimum dio size | expand |
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: 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 >
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 > > >
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"} + 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
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(-)