On Thu, Oct 26, 2017 at 04:57:40PM +0800, Eryu Guan wrote:
> On Tue, Oct 17, 2017 at 10:20:49PM -0700, Shaohua Li wrote:
> > From: Shaohua Li <shli@xxxxxx>
> >
> > If filesystem supports cgroup2 writeback, the writeback IO should belong
> > to the cgroup which writes the file. To verify this, we set a write
> > bandwidth limit for a cgroup using block-throttling, the writeback IO
> > should be throttled according to the write bandwidth.
> >
> > Thanks Dave Chinner's idea to use syncfs to wait for writeback
> > completion.
> >
> > Signed-off-by: Shaohua Li <shli@xxxxxx>
> > ---
Ping..
Shaohua,
Were you planning to incorporate Eryu's feedback and repost this (and
the associated xfs writeback patch)?
Brian
> > common/cgroup2 | 18 +++++++++++++
> > tests/generic/463 | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > tests/generic/463.out | 3 +++
> > tests/generic/group | 1 +
> > 4 files changed, 97 insertions(+)
> > create mode 100644 common/cgroup2
> > create mode 100755 tests/generic/463
> > create mode 100644 tests/generic/463.out
> >
> > diff --git a/common/cgroup2 b/common/cgroup2
> > new file mode 100644
> > index 00000000..bf935fee
> > --- /dev/null
> > +++ b/common/cgroup2
> > @@ -0,0 +1,18 @@
> > +#!/bin/bash
>
> No need to use "#!/bin/bash" for a common file to be sourced by other
> scripts.
>
> > +# cgroup2 specific common functions
> > +
> > +export CGROUP2_PATH="/sys/fs/cgroup"
>
> Make it configurable? e.g.
> export CGROUP2_PATH="${CGROUP2_PATH:-"/sys/fs/cgroup"}
>
> Because on my fedora test host, cgroup is mounted at
> /sys/fs/cgroup/unified by default.
>
> > +
> > +_require_cgroup2()
> > +{
> > + if [ ! -f ${CGROUP2_PATH}/cgroup.subtree_control ]; then
> > + _notrun "Test requires cgroup2 enabled"
> > + fi
>
> Please use tab for indention.
>
> > +}
> > +
> > +_get_scratch_dev_devt()
> > +{
> > + ls -l $SCRATCH_DEV | awk '{printf("%s:%s", substr($5, 1, length($5)-1), 0)}'
> > +}
>
> stat -c %t for major and stat -c %T for minor? And seems that the minor
> device id returned here is always 0, is that intentional?
>
> Also, you need to call _real_dev first in case $SCRATCH_DEV is a
> symlink, e.g. device mapper device. And this helper can be moved to
> common/rc, it's not cgroup2 specific.
>
> I think you can factor out "get the device id" part of _sysfs_dev() into
> a new helper, and use the new one in _sysfs_dev() and this test.
>
> > +
> > +/bin/true
> > diff --git a/tests/generic/463 b/tests/generic/463
> > new file mode 100755
> > index 00000000..620a14a4
> > --- /dev/null
> > +++ b/tests/generic/463
> > @@ -0,0 +1,75 @@
> > +#! /bin/bash
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +. ./common/cgroup2
> > +
> > +status=1 # failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +cgname=`mktemp -du ${CGROUP2_PATH}/test.XXXXXX`
> > +_cleanup()
> > +{
> > + cd /
> > + _scratch_unmount
>
> check will do this automatically :)
>
> > + sync
> > + rmdir $cgname
> > +}
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_supported_os Linux
> > +_require_scratch
> > +_require_cgroup2
> > +
> > +# Setup Filesystem
> > +_scratch_mkfs >/dev/null 2>&1 || _fail "mkfs failed"
> > +
> > +_scratch_mount || _fail "mount failed"
> > +
> > +test_speed()
> > +{
> > + start=$(date +%s)
> > + $XFS_IO_PROG -f -d -c "pwrite -b 1m 0 500m" "$SCRATCH_MNT/image" \
> > + > /dev/null 2>&1
> > + echo $(($(date +%s) - $start))
> > +}
> > +time=$(test_speed)
> > +
> > +if [ $time -gt 25 ]; then
> > + echo "Disk is too slow, make sure disk can do 20MB/s at least"
> > + status=1
> > + exit 1
>
> We should call _notrun here if device speed is too slow, that's not a
> failure, but a un-met test requirement.
>
> > +fi
> > +
> > +echo "Disk speed is ok, start throttled writeback test"
> > +
> > +echo +io > ${CGROUP2_PATH}/cgroup.subtree_control
>
> Make sure if io is available first in cgroup.controllers? Because on my
> fedora rawhide test host, cgroup.controllers is empty, because all io,
> cpu and mem controls are bound to cgroup v1 by default. I think this
> check could be integrated with _require_cgroup2, e.g.
>
> _require_cgroup2 io
>
> > +mkdir $cgname
> > +echo "$(_get_scratch_dev_devt) wbps=$((5*1024*1024))" > $cgname/io.max
> > +
> > +run_writeback()
> > +{
> > + start=$(date +%s)
> > + $XFS_IO_PROG -f -c "pwrite 0 500m" -c "syncfs" "$SCRATCH_MNT/image" \
> > + > /dev/null 2>&1
> > + echo $(($(date +%s) - $start))
> > +}
> > +time=$(
> > +echo $BASHPID > $cgname/cgroup.procs;
> > +run_writeback
> > +)
> > +_within_tolerance "Throttled writeback runtime" $time 100 10% -v
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/463.out b/tests/generic/463.out
> > new file mode 100644
> > index 00000000..c69f87c2
> > --- /dev/null
> > +++ b/tests/generic/463.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 463
> > +Disk speed is ok, start throttled writeback test
> > +Throttled writeback runtime is in range
> > diff --git a/tests/generic/group b/tests/generic/group
> > index f2a6cdad..ea7b6956 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -465,3 +465,4 @@
> > 460 auto quick rw
> > 461 auto shutdown stress
> > 462 auto quick dax
> > +463 cgroup writeback test
>
> s/test/auto/, I don't see the meaning of a 'test' group :)
>
> Thanks,
> Eryu
>
> > --
> > 2.11.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
