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
