[v2] blk-cgroup: don't clear stat in blkcg_reset_stats()

Commit Message

Li Lingfeng Aug. 21, 2024, 2:07 a.m. UTC

From: Li Lingfeng <[e-mail]>

The list corruption described in commit 6da668063279 ("blk-cgroup: fix
list corruption from resetting io stat") has no effect. It's unnecessary
to fix it.

As for cgroup v1, it does not use iostat any more after commit
ad7c3b41e86b("blk-throttle: Fix io statistics for cgroup v1"), so using
memset to clear iostat has no real effect.
As for cgroup v2, it will not call blkcg_reset_stats() to corrupt the
list.

The list of root cgroup can be used by both cgroup v1 and v2 while
non-root cgroup can't since it must be removed before switch between
cgroup v1 and v2.
So it may has effect if the list of root used by cgroup v2 was corrupted
after switching to cgroup v1, and switch back to cgroup v2 to use the
corrupted list again.
However, the root cgroup will not use the list any more after commit
ef45fe470e1e("blk-cgroup: show global disk stats in root cgroup io.stat").

Although this has no negative effect, it is not necessary. Remove the
related code for cleanup. No function change.

Signed-off-by: Li Lingfeng <[e-mail]>
---
  v1->v2: remove the fix tag and mark this as cleanup.
 block/blk-cgroup.c | 24 ------------------------
 1 file changed, 24 deletions(-)

Comments

Michal Koutný Aug. 21, 2024, 3:51 p.m. UTC | #1

Hello.

On Wed, Aug 21, 2024 at 10:07:56AM GMT, Li Lingfeng <[e-mail]> wrote:
> The list of root cgroup can be used by both cgroup v1 and v2 while
> non-root cgroup can't since it must be removed before switch between
> cgroup v1 and v2.
> So it may has effect if the list of root used by cgroup v2 was corrupted
> after switching to cgroup v1, and switch back to cgroup v2 to use the
> corrupted list again.

> However, the root cgroup will not use the list any more after commit
> ef45fe470e1e("blk-cgroup: show global disk stats in root cgroup io.stat").

How come? Before that patch the root file was excluded with
CFTYPE_NOT_ON_ROOT. IOW how has that patch an effect on llist traversal?

(It doesn't matter, your patch doesn't restore memset anyway.)

This is the reasoning how I understand it:

| The removed function clears blkg.iostat structures that is only used on
| v2 whereas the function can only be called with v1 hierarchy attached.
| Zeroing effect could potentially be visible root blkcg "shared" between
| v1 and v2 but v2 actually synthesizes stats differently for root.

> Although this has no negative effect, it is not necessary. Remove the
> related code for cleanup. No function change.

I'm impressed by the amount of analysis you did to potentially remove
the unused function. If you feel like cleaning up more or sectioning,
see also [1] or [2] for inspiration.

Thanks,
Michal

[1] https://lore.kernel.org/all/[e-mail]/
[2] https://lore.kernel.org/all/[e-mail]/

Li Lingfeng Oct. 22, 2024, 3:44 a.m. UTC | #2

在 2024/8/21 23:51, Michal Koutný 写道:
> Hello.
>
> On Wed, Aug 21, 2024 at 10:07:56AM GMT, Li Lingfeng <[e-mail]> wrote:
>> The list of root cgroup can be used by both cgroup v1 and v2 while
>> non-root cgroup can't since it must be removed before switch between
>> cgroup v1 and v2.
>> So it may has effect if the list of root used by cgroup v2 was corrupted
>> after switching to cgroup v1, and switch back to cgroup v2 to use the
>> corrupted list again.
>> However, the root cgroup will not use the list any more after commit
>> ef45fe470e1e("blk-cgroup: show global disk stats in root cgroup io.stat").
> How come? Before that patch the root file was excluded with
> CFTYPE_NOT_ON_ROOT. IOW how has that patch an effect on llist traversal?
>
> (It doesn't matter, your patch doesn't restore memset anyway.)
>
> This is the reasoning how I understand it:
>
> | The removed function clears blkg.iostat structures that is only used on
> | v2 whereas the function can only be called with v1 hierarchy attached.
> | Zeroing effect could potentially be visible root blkcg "shared" between
> | v1 and v2 but v2 actually synthesizes stats differently for root.
Yes, that's what I mean.
>> Although this has no negative effect, it is not necessary. Remove the
>> related code for cleanup. No function change.
> I'm impressed by the amount of analysis you did to potentially remove
> the unused function. If you feel like cleaning up more or sectioning,
> see also [1] or [2] for inspiration.
Thank you for your advice. I'll look at them later.
> Thanks,
> Michal
>
> [1] https://lore.kernel.org/all/[e-mail]/
> [2] https://lore.kernel.org/all/[e-mail]/

Patch

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 69e70964398c..6d091fa55b8c 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -629,29 +629,6 @@  static void blkg_iostat_set(struct blkg_iostat *dst, struct blkg_iostat *src)
 	}
 }
 
-static void __blkg_clear_stat(struct blkg_iostat_set *bis)
-{
-	struct blkg_iostat cur = {0};
-	unsigned long flags;
-
-	flags = u64_stats_update_begin_irqsave(&bis->sync);
-	blkg_iostat_set(&bis->cur, &cur);
-	blkg_iostat_set(&bis->last, &cur);
-	u64_stats_update_end_irqrestore(&bis->sync, flags);
-}
-
-static void blkg_clear_stat(struct blkcg_gq *blkg)
-{
-	int cpu;
-
-	for_each_possible_cpu(cpu) {
-		struct blkg_iostat_set *s = per_cpu_ptr(blkg->iostat_cpu, cpu);
-
-		__blkg_clear_stat(s);
-	}
-	__blkg_clear_stat(&blkg->iostat);
-}
-
 static int blkcg_reset_stats(struct cgroup_subsys_state *css,
 			     struct cftype *cftype, u64 val)
 {
@@ -668,7 +645,6 @@  static int blkcg_reset_stats(struct cgroup_subsys_state *css,
 	 * anyway.  If you get hit by a race, retry.
 	 */
 	hlist_for_each_entry(blkg, &blkcg->blkg_list, blkcg_node) {
-		blkg_clear_stat(blkg);
 		for (i = 0; i < BLKCG_MAX_POLS; i++) {
 			struct blkcg_policy *pol = blkcg_policy[i];