All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2] xfstests: add a simple cgroup2 writeback test
@ 2018-03-22 21:13 Shaohua Li
  2018-03-23 13:56 ` Brian Foster
  0 siblings, 1 reply; 3+ messages in thread
From: Shaohua Li @ 2018-03-22 21:13 UTC (permalink / raw)
  To: fstests; +Cc: darrick.wong, Kernel-team, bfoster, Shaohua Li, Eryu Guan

From: Shaohua Li <shli@fb.com>

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.

Cc: Eryu Guan <eguan@redhat.com>
Signed-off-by: Shaohua Li <shli@fb.com>
---
 common/cgroup2        | 15 +++++++++++
 common/rc             | 14 ++++++++--
 tests/generic/482     | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/482.out |  3 +++
 tests/generic/group   |  1 +
 5 files changed, 104 insertions(+), 2 deletions(-)
 create mode 100644 common/cgroup2
 create mode 100755 tests/generic/482
 create mode 100644 tests/generic/482.out

diff --git a/common/cgroup2 b/common/cgroup2
new file mode 100644
index 00000000..f89825e2
--- /dev/null
+++ b/common/cgroup2
@@ -0,0 +1,15 @@
+# cgroup2 specific common functions
+
+export CGROUP2_PATH="${CGROUP2_PATH:-/sys/fs/cgroup}"
+
+_require_cgroup2()
+{
+	if [ ! -f "${CGROUP2_PATH}/cgroup.subtree_control" ]; then
+		_notrun "Test requires cgroup2 enabled"
+	fi
+	if [[ ! $(cat ${CGROUP2_PATH}/cgroup.controllers) =~ $1 ]]; then
+		_notrun "Cgroup2 doesn't support $1 controller $1"
+	fi
+}
+
+/bin/true
diff --git a/common/rc b/common/rc
index 93176749..e93674f9 100644
--- a/common/rc
+++ b/common/rc
@@ -3613,14 +3613,24 @@ _short_dev()
 	echo `basename $(_real_dev $1)`
 }
 
-_sysfs_dev()
+#blk-throttling requires minor is 0
+_get_dev_devt()
 {
 	local _dev=`_real_dev $1`
 	local _maj=$(stat -c%t $_dev | tr [:lower:] [:upper:])
 	local _min=$(stat -c%T $_dev | tr [:lower:] [:upper:])
 	_maj=$(echo "ibase=16; $_maj" | bc)
 	_min=$(echo "ibase=16; $_min" | bc)
-	echo /sys/dev/block/$_maj:$_min
+	if [ $2 ]; then
+		echo $_maj:0
+	else
+		echo $_maj:$_min
+	fi
+}
+
+_sysfs_dev()
+{
+	echo /sys/dev/block/$(_get_dev_devt $1 false)
 }
 
 # Get the minimum block size of a file.  Usually this is the
diff --git a/tests/generic/482 b/tests/generic/482
new file mode 100755
index 00000000..1f3cbc97
--- /dev/null
+++ b/tests/generic/482
@@ -0,0 +1,73 @@
+#! /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 /
+	sync
+	rmdir $cgname
+}
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_require_cgroup2 io
+_require_xfs_io_command syncfs
+
+# 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
+	_notrun "Disk is too slow, make sure disk can do 20MB/s at least"
+fi
+
+echo "Disk speed is ok, start throttled writeback test"
+
+echo +io > ${CGROUP2_PATH}/cgroup.subtree_control
+mkdir $cgname
+echo "$(_get_dev_devt $SCRATCH_DEV true) wbps=$((5*1024*1024))" > $cgname/io.max
+
+run_writeback()
+{
+    start=$(date +%s)
+    $XFS_IO_PROG -f -c "pwrite 0 500m" -c "syncfs" "$SCRATCH_MNT/image2" \
+        > /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/482.out b/tests/generic/482.out
new file mode 100644
index 00000000..4d1b40a8
--- /dev/null
+++ b/tests/generic/482.out
@@ -0,0 +1,3 @@
+QA output created by 482
+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 e8676062..f707838c 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -484,3 +484,4 @@
 479 auto quick metadata
 480 auto quick metadata
 481 auto quick log metadata
+482 auto cgroup writeback
-- 
2.14.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH V2] xfstests: add a simple cgroup2 writeback test
  2018-03-22 21:13 [PATCH V2] xfstests: add a simple cgroup2 writeback test Shaohua Li
@ 2018-03-23 13:56 ` Brian Foster
  2018-03-24 16:16   ` Eryu Guan
  0 siblings, 1 reply; 3+ messages in thread
From: Brian Foster @ 2018-03-23 13:56 UTC (permalink / raw)
  To: Shaohua Li; +Cc: fstests, darrick.wong, Kernel-team, Shaohua Li, Eryu Guan

On Thu, Mar 22, 2018 at 02:13:23PM -0700, Shaohua Li wrote:
> From: Shaohua Li <shli@fb.com>
> 
> 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.
> 
> Cc: Eryu Guan <eguan@redhat.com>
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---

Thanks for sending this. Firstly, this test isn't passing for me on
ext4. It should, right? I'm running 4.16.0-0.rc6.git2.1.fc29.x86_64 with
cgroup_no_v1=all, set CGROUP2_PATH=/sys/fs/cgroup/unified and my scratch
device is an LVM volume.

# diff -u tests/generic/482.out
# /root/xfstests-dev/results//generic/482.out.bad
--- tests/generic/482.out       2018-03-23 08:54:51.321808466 -0400
+++ /root/xfstests-dev/results//generic/482.out.bad     2018-03-23
09:25:55.359769041 -0400
@@ -1,3 +1,4 @@
 QA output created by 482
 Disk speed is ok, start throttled writeback test
-Throttled writeback runtime is in range
+Throttled writeback runtime has value of 11
+Throttled writeback runtime is NOT in range 90 .. 110

A few more comments...

>  common/cgroup2        | 15 +++++++++++
>  common/rc             | 14 ++++++++--
>  tests/generic/482     | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/482.out |  3 +++
>  tests/generic/group   |  1 +
>  5 files changed, 104 insertions(+), 2 deletions(-)
>  create mode 100644 common/cgroup2
>  create mode 100755 tests/generic/482
>  create mode 100644 tests/generic/482.out
> 
> diff --git a/common/cgroup2 b/common/cgroup2
> new file mode 100644
> index 00000000..f89825e2
> --- /dev/null
> +++ b/common/cgroup2
> @@ -0,0 +1,15 @@
> +# cgroup2 specific common functions
> +
> +export CGROUP2_PATH="${CGROUP2_PATH:-/sys/fs/cgroup}"
> +
> +_require_cgroup2()
> +{
> +	if [ ! -f "${CGROUP2_PATH}/cgroup.subtree_control" ]; then
> +		_notrun "Test requires cgroup2 enabled"
> +	fi
> +	if [[ ! $(cat ${CGROUP2_PATH}/cgroup.controllers) =~ $1 ]]; then
> +		_notrun "Cgroup2 doesn't support $1 controller $1"
> +	fi
> +}
> +
> +/bin/true
> diff --git a/common/rc b/common/rc
> index 93176749..e93674f9 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3613,14 +3613,24 @@ _short_dev()
>  	echo `basename $(_real_dev $1)`
>  }
>  
> -_sysfs_dev()
> +#blk-throttling requires minor is 0

Why (more on this below)?

> +_get_dev_devt()
>  {
>  	local _dev=`_real_dev $1`
>  	local _maj=$(stat -c%t $_dev | tr [:lower:] [:upper:])
>  	local _min=$(stat -c%T $_dev | tr [:lower:] [:upper:])
>  	_maj=$(echo "ibase=16; $_maj" | bc)
>  	_min=$(echo "ibase=16; $_min" | bc)
> -	echo /sys/dev/block/$_maj:$_min
> +	if [ $2 ]; then
> +		echo $_maj:0
> +	else
> +		echo $_maj:$_min
> +	fi
> +}
> +
> +_sysfs_dev()
> +{
> +	echo /sys/dev/block/$(_get_dev_devt $1 false)
>  }
>  
>  # Get the minimum block size of a file.  Usually this is the
> diff --git a/tests/generic/482 b/tests/generic/482
> new file mode 100755
> index 00000000..1f3cbc97
> --- /dev/null
> +++ b/tests/generic/482
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +

The test should have a header with license and a brief description of
what it's doing. The description helps give somebody an idea of what's
wrong if the test fails and we have to refer to this years down the
road. For example, this should explain that this is explicitly testing
cgroup aware writeback support. That means that it requires specific
filesystem support, block I/O throttling is not enough, etc.

That also begs the question of how to handle filesystems that do not
implement cgroup aware writeback. XFS doesn't atm (though hopefully that
changes with your kernel patch), and I'm not sure that should
necessarily result in a test failure. I don't believe we have any
specific way to check at runtime..? Perhaps we should just opt-in to
specific fs' once they support it and do a _notrun for anything else..?

> +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 /
> +	sync
> +	rmdir $cgname
> +}
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_scratch
> +_require_cgroup2 io
> +_require_xfs_io_command syncfs
> +
> +# 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
> +	_notrun "Disk is too slow, make sure disk can do 20MB/s at least"
> +fi
> +
> +echo "Disk speed is ok, start throttled writeback test"
> +

Could use some comments..

# create a cgroup with a 5MB/s I/O limit

> +echo +io > ${CGROUP2_PATH}/cgroup.subtree_control
> +mkdir $cgname
> +echo "$(_get_dev_devt $SCRATCH_DEV true) wbps=$((5*1024*1024))" > $cgname/io.max

FWIW, the test passes on ext4 for me if I hardcode this to use 253:3 for
my scratch device (rather than 253:0). What device were you using that
required a minor of 0?

> +
> +run_writeback()
> +{
> +    start=$(date +%s)
> +    $XFS_IO_PROG -f -c "pwrite 0 500m" -c "syncfs" "$SCRATCH_MNT/image2" \
> +        > /dev/null 2>&1

Comment with the reason this needs to be syncfs?

> +    echo $(($(date +%s) - $start))
> +}

It's cleaner and more consistent to define helper functions all in one
place above. That makes it easier to follow the actual test logic. I'd
also suggest to rename this to test_writeback().

> +time=$(
> +echo $BASHPID > $cgname/cgroup.procs;

Is it necessary to include the above line within the $()? If so, please
document it with a comment. Otherwise it might be cleaner to run it
separately.

Also could use another comment before this hunk:

# perform buffered writes charged to the cgroup and verify writeback
# honors the I/O throttle

> +run_writeback
> +)
> +_within_tolerance "Throttled writeback runtime" $time 100 10% -v
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/482.out b/tests/generic/482.out
> new file mode 100644
> index 00000000..4d1b40a8
> --- /dev/null
> +++ b/tests/generic/482.out
> @@ -0,0 +1,3 @@
> +QA output created by 482
> +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 e8676062..f707838c 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -484,3 +484,4 @@
>  479 auto quick metadata
>  480 auto quick metadata
>  481 auto quick log metadata
> +482 auto cgroup writeback

cgroup makes sense, but I'm not sure about writeback. It seems like
duplication at least since both groups are new. Use the rw group
perhaps? Eh, I'll defer to Eryu's opinion on what group(s) are most
appropriate.

Brian

> -- 
> 2.14.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH V2] xfstests: add a simple cgroup2 writeback test
  2018-03-23 13:56 ` Brian Foster
@ 2018-03-24 16:16   ` Eryu Guan
  0 siblings, 0 replies; 3+ messages in thread
From: Eryu Guan @ 2018-03-24 16:16 UTC (permalink / raw)
  To: Brian Foster; +Cc: Shaohua Li, fstests, darrick.wong, Kernel-team, Shaohua Li

On Fri, Mar 23, 2018 at 09:56:21AM -0400, Brian Foster wrote:
> On Thu, Mar 22, 2018 at 02:13:23PM -0700, Shaohua Li wrote:
> > From: Shaohua Li <shli@fb.com>
> > 
> > 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.
> > 
> > Cc: Eryu Guan <eguan@redhat.com>
> > Signed-off-by: Shaohua Li <shli@fb.com>
> > ---
> 
> Thanks for sending this. Firstly, this test isn't passing for me on
> ext4. It should, right? I'm running 4.16.0-0.rc6.git2.1.fc29.x86_64 with
> cgroup_no_v1=all, set CGROUP2_PATH=/sys/fs/cgroup/unified and my scratch
> device is an LVM volume.
> 
> # diff -u tests/generic/482.out
> # /root/xfstests-dev/results//generic/482.out.bad
> --- tests/generic/482.out       2018-03-23 08:54:51.321808466 -0400
> +++ /root/xfstests-dev/results//generic/482.out.bad     2018-03-23
> 09:25:55.359769041 -0400
> @@ -1,3 +1,4 @@
>  QA output created by 482
>  Disk speed is ok, start throttled writeback test
> -Throttled writeback runtime is in range
> +Throttled writeback runtime has value of 11
> +Throttled writeback runtime is NOT in range 90 .. 110
> 
> A few more comments...
> 
> >  common/cgroup2        | 15 +++++++++++
> >  common/rc             | 14 ++++++++--
> >  tests/generic/482     | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/482.out |  3 +++
> >  tests/generic/group   |  1 +
> >  5 files changed, 104 insertions(+), 2 deletions(-)
> >  create mode 100644 common/cgroup2
> >  create mode 100755 tests/generic/482
> >  create mode 100644 tests/generic/482.out
> > 
> > diff --git a/common/cgroup2 b/common/cgroup2
> > new file mode 100644
> > index 00000000..f89825e2
> > --- /dev/null
> > +++ b/common/cgroup2
> > @@ -0,0 +1,15 @@
> > +# cgroup2 specific common functions
> > +
> > +export CGROUP2_PATH="${CGROUP2_PATH:-/sys/fs/cgroup}"
> > +
> > +_require_cgroup2()
> > +{
> > +	if [ ! -f "${CGROUP2_PATH}/cgroup.subtree_control" ]; then
> > +		_notrun "Test requires cgroup2 enabled"
> > +	fi
> > +	if [[ ! $(cat ${CGROUP2_PATH}/cgroup.controllers) =~ $1 ]]; then
> > +		_notrun "Cgroup2 doesn't support $1 controller $1"
> > +	fi
> > +}
> > +
> > +/bin/true
> > diff --git a/common/rc b/common/rc
> > index 93176749..e93674f9 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3613,14 +3613,24 @@ _short_dev()
> >  	echo `basename $(_real_dev $1)`
> >  }
> >  
> > -_sysfs_dev()
> > +#blk-throttling requires minor is 0
> 
> Why (more on this below)?
> 
> > +_get_dev_devt()
> >  {
> >  	local _dev=`_real_dev $1`
> >  	local _maj=$(stat -c%t $_dev | tr [:lower:] [:upper:])
> >  	local _min=$(stat -c%T $_dev | tr [:lower:] [:upper:])
> >  	_maj=$(echo "ibase=16; $_maj" | bc)
> >  	_min=$(echo "ibase=16; $_min" | bc)
> > -	echo /sys/dev/block/$_maj:$_min
> > +	if [ $2 ]; then
> > +		echo $_maj:0
> > +	else
> > +		echo $_maj:$_min
> > +	fi
> > +}
> > +
> > +_sysfs_dev()
> > +{
> > +	echo /sys/dev/block/$(_get_dev_devt $1 false)
> >  }
> >  
> >  # Get the minimum block size of a file.  Usually this is the
> > diff --git a/tests/generic/482 b/tests/generic/482
> > new file mode 100755
> > index 00000000..1f3cbc97
> > --- /dev/null
> > +++ b/tests/generic/482
> > @@ -0,0 +1,73 @@
> > +#! /bin/bash
> > +
> 
> The test should have a header with license and a brief description of
> what it's doing. The description helps give somebody an idea of what's
> wrong if the test fails and we have to refer to this years down the
> road. For example, this should explain that this is explicitly testing
> cgroup aware writeback support. That means that it requires specific
> filesystem support, block I/O throttling is not enough, etc.

Agreed.

> 
> That also begs the question of how to handle filesystems that do not
> implement cgroup aware writeback. XFS doesn't atm (though hopefully that
> changes with your kernel patch), and I'm not sure that should
> necessarily result in a test failure. I don't believe we have any

Agreed, it should not be a test failure if the filesystem doesn't
support cgroup aware writeback.

> specific way to check at runtime..? Perhaps we should just opt-in to
> specific fs' once they support it and do a _notrun for anything else..?

IMHO, if there's really nothing can be probed from userspace, the only
way I can think of is to move the test to shared dir, and list supported
fs explicitly in _supported_fs.

> 
> > +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 /
> > +	sync
> > +	rmdir $cgname
> > +}
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_supported_os Linux
> > +_require_scratch
> > +_require_cgroup2 io
> > +_require_xfs_io_command syncfs
> > +
> > +# 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
> > +	_notrun "Disk is too slow, make sure disk can do 20MB/s at least"
> > +fi
> > +
> > +echo "Disk speed is ok, start throttled writeback test"
> > +
> 
> Could use some comments..
> 
> # create a cgroup with a 5MB/s I/O limit
> 
> > +echo +io > ${CGROUP2_PATH}/cgroup.subtree_control
> > +mkdir $cgname
> > +echo "$(_get_dev_devt $SCRATCH_DEV true) wbps=$((5*1024*1024))" > $cgname/io.max
> 
> FWIW, the test passes on ext4 for me if I hardcode this to use 253:3 for
> my scratch device (rather than 253:0). What device were you using that
> required a minor of 0?
> 
> > +
> > +run_writeback()
> > +{
> > +    start=$(date +%s)
> > +    $XFS_IO_PROG -f -c "pwrite 0 500m" -c "syncfs" "$SCRATCH_MNT/image2" \
> > +        > /dev/null 2>&1
> 
> Comment with the reason this needs to be syncfs?
> 
> > +    echo $(($(date +%s) - $start))
> > +}
> 
> It's cleaner and more consistent to define helper functions all in one
> place above. That makes it easier to follow the actual test logic. I'd
> also suggest to rename this to test_writeback().
> 
> > +time=$(
> > +echo $BASHPID > $cgname/cgroup.procs;
> 
> Is it necessary to include the above line within the $()? If so, please
> document it with a comment. Otherwise it might be cleaner to run it
> separately.
> 
> Also could use another comment before this hunk:
> 
> # perform buffered writes charged to the cgroup and verify writeback
> # honors the I/O throttle
> 
> > +run_writeback
> > +)
> > +_within_tolerance "Throttled writeback runtime" $time 100 10% -v
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/482.out b/tests/generic/482.out
> > new file mode 100644
> > index 00000000..4d1b40a8
> > --- /dev/null
> > +++ b/tests/generic/482.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 482
> > +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 e8676062..f707838c 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -484,3 +484,4 @@
> >  479 auto quick metadata
> >  480 auto quick metadata
> >  481 auto quick log metadata
> > +482 auto cgroup writeback
> 
> cgroup makes sense, but I'm not sure about writeback. It seems like
> duplication at least since both groups are new. Use the rw group
> perhaps? Eh, I'll defer to Eryu's opinion on what group(s) are most
> appropriate.

I slightly prefer dropping 'writeback' group, 'cgroup' alone should be
fine.

Thanks
Eryu

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-03-24 16:16 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-22 21:13 [PATCH V2] xfstests: add a simple cgroup2 writeback test Shaohua Li
2018-03-23 13:56 ` Brian Foster
2018-03-24 16:16   ` Eryu Guan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.