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

[Tejun Heo <tj@kernel.org>]

Hello,

On Thu, Jun 27, 2024 at 05:08:56PM +0800, Li Lingfeng wrote:
> 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.

I find this paragraph a bit confusing. At the time, it was broken, right?
And if we were to memset() now, it'd break again.

> 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.

Ah, okay, this is because we made the stats blk-throtl specific but didn't
implement ->pd_reset_stat_fn(), right?

> 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").

Hmm... I'm still having a bit of trouble following this line of argument
given that all the patch does is dropping stat clearing.

> @@ -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];

The patch looks fine to me although it'd be nice to follow up with a patch
to implement ->pd_reset_stat_fn() for blk-throtl. I'm not quite following
the list corruption part of argument.

Thanks.

-- 
tejun