Re: [PATCH blktests] tests/throtl: add a new test 009

[Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

On Aug 20, 2026 / 16:43, Tao Cui wrote:
> 
> Hi Shin'ichiro,
> 
> 在 2026/8/18 20:42, Shin'ichiro Kawasaki 写道:
> > On Aug 13, 2026 / 22:40, Tao Cui wrote:
> >> From: Tao Cui <cuitao@kylinos.cn>
> >>
> >> Test the boundary condition of removing a cgroup while it still has IO
> >> queued in the blk-throttle queue.  The queued bios must be flushed through
> >> without any blk-cgroup or blk-throttle warning.
> >>
> >> The dmesg check matches any blk-cgroup / blk-throttle warning rather than a
> >> single call site, so it is not tied to one symbol.
> > 
> > Tao, thanks for the patch. Does this test case have corresponding kernel side
> > fix? I'm guessing that your recent kernel patch is the fix. If so, I suggest
> > to mention the kernel commit in here and the test case header comment below.
> > 
> 
> 
> Thanks for the review.
> 
> No.  Christoph asked me to add the reproducer to blktests in that RFC
> thread [1], that's how this patch came about.
> 
> The WARN it catches came from yukuai's "store blkcg in bio" RFC v1, which
> is not merged: bio_pinned_blkg() re-looked up a blkg that blkg_destroy()
> had already removed from the radix tree, while a queued throttled bio
> still pinned it.  RFC v2 fixed it with blkg_lookup_any(), and mainline
> passes this test too, since the blkg stays findable until queued bios
> drain.

I see, thanks for the clarification. I suggest to add a "Link:" tag to this
patch's commit message, to refer to the background discussion [*].

[*] https://lore.kernel.org/all/20260804133208.GB8078@lst.de/

> 
> >>
> >> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> >> ---
> >> See also the related blkcg "store blkcg in bio" RFC:
> >> https://lore.kernel.org/r/20260804065313.2092022-1-yukuai@kernel.org
> >>
> >>  tests/throtl/009     | 67 ++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/throtl/009.out |  2 ++
> >>  2 files changed, 69 insertions(+)
> >>  create mode 100755 tests/throtl/009
> >>  create mode 100644 tests/throtl/009.out
> >>
> >> diff --git a/tests/throtl/009 b/tests/throtl/009
> >> new file mode 100755
> >> index 0000000..1f4ed67
> >> --- /dev/null
> >> +++ b/tests/throtl/009
> >> @@ -0,0 +1,67 @@
> >> +#!/bin/bash
> >> +# SPDX-License-Identifier: GPL-3.0+
> >> +# Copyright (C) 2026 Tao Cui <cuitao@kylinos.cn>
> >> +#
> >> +# Regression test for removing a cgroup while IO is still queued in the
> >> +# blk-throttle queue.  The queued bios must be flushed without any
> >> +# blk-cgroup or blk-throttle warning, and without leaking the blkg
> >> +# reference they pin.
> >> +
> >> +. tests/throtl/rc
> >> +
> >> +THROTL_BLKDEV_TYPES="nullb"
> > 
> > Is there any reason to run this test case only for nullb? If not, I suggest to
> > remove the line above to run it for both nullb and scsi_debug.
> 
> > Is there any reason to run this test case only for nullb?
> 
> The test just exercises the blk-throttle/blk-cgroup path on cgroup
> removal and doesn't depend on the device type, nullb is simply the most
> convenient.  Dropping THROTL_BLKDEV_TYPES is fine with me if you want
> scsi_debug covered too.

I see, thanks. The other tests cases in throtl group run for both nullb and
scsi_debug, so I suggest to do the same for this test case also for consistency.

Once there was a failure in throtl group that was observed only with scsi_debug.
IIRC, the test case does not depend on the device type, but the failure was
timing dependent and observed only with scsi_debug. Then, I think it is useful
to run the throtl test cases for both null_blk and scsi_debug in general.