Re: [PATCH V2] blk-cgroup: Flush stats before releasing blkcg_gq

[Waiman Long <longman@redhat.com>]

On 5/23/23 23:51, Ming Lei wrote:
> As noted by Michal, the blkg_iostat_set's in the lockless list hold
> reference to blkg's to protect against their removal. Those blkg's
> hold reference to blkcg. When a cgroup is being destroyed,
> cgroup_rstat_flush() is only called at css_release_work_fn() which
> is called when the blkcg reference count reaches 0. This circular
> dependency will prevent blkcg and some blkgs from being freed after
> they are made offline.
>
> It is less a problem if the cgroup to be destroyed also has other
> controllers like memory that will call cgroup_rstat_flush() which will
> clean up the reference count. If block is the only controller that uses
> rstat, these offline blkcg and blkgs may never be freed leaking more
> and more memory over time.
>
> To prevent this potential memory leak:
>
> - flush blkcg per-cpu stats list in __blkg_release(), when no new stat
> can be added
>
> - don't grab bio->bi_blkg reference when adding the stats into blkcg's
> per-cpu stat list since all stats are guaranteed to be consumed before
> releasing blkg instance, and grabbing blkg reference for stats was the
> most fragile part of original patch
>
> Based on Waiman's patch:
>
> https://lore.kernel.org/linux-block/20221215033132.230023-3-longman@redhat.com/
>
> Fixes: 3b8cc6298724 ("blk-cgroup: Optimize blkcg_rstat_flush()")
> Cc: Waiman Long <longman@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: mkoutny@suse.com
> Cc: Yosry Ahmed <yosryahmed@google.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> V2:
> 	- remove kernel/cgroup change, and call blkcg_rstat_flush()
> 	to flush stat directly
>
>   block/blk-cgroup.c | 29 +++++++++++++++++++++--------
>   1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 0ce64dd73cfe..ed0eb8896972 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -34,6 +34,8 @@
>   #include "blk-ioprio.h"
>   #include "blk-throttle.h"
>   
> +static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu);
> +
>   /*
>    * blkcg_pol_mutex protects blkcg_policy[] and policy [de]activation.
>    * blkcg_pol_register_mutex nests outside of it and synchronizes entire
> @@ -163,10 +165,21 @@ static void blkg_free(struct blkcg_gq *blkg)
>   static void __blkg_release(struct rcu_head *rcu)
>   {
>   	struct blkcg_gq *blkg = container_of(rcu, struct blkcg_gq, rcu_head);
> +	struct blkcg *blkcg = blkg->blkcg;
> +	int cpu;
>   
>   #ifdef CONFIG_BLK_CGROUP_PUNT_BIO
>   	WARN_ON(!bio_list_empty(&blkg->async_bios));
>   #endif
> +	/*
> +	 * Flush all the non-empty percpu lockless lists before releasing
> +	 * us, given these stat belongs to us.
> +	 *
> +	 * cgroup locks aren't needed here since __blkcg_rstat_flush just
> +	 * propagates delta into blkg parent, which is live now.
> +	 */
> +	for_each_possible_cpu(cpu)
> +		__blkcg_rstat_flush(blkcg, cpu);
>   
>   	/* release the blkcg and parent blkg refs this blkg has been holding */
>   	css_put(&blkg->blkcg->css);
> @@ -951,17 +964,12 @@ static void blkcg_iostat_update(struct blkcg_gq *blkg, struct blkg_iostat *cur,
>   	u64_stats_update_end_irqrestore(&blkg->iostat.sync, flags);
>   }
>   
> -static void blkcg_rstat_flush(struct cgroup_subsys_state *css, int cpu)
> +static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu)
>   {
> -	struct blkcg *blkcg = css_to_blkcg(css);
>   	struct llist_head *lhead = per_cpu_ptr(blkcg->lhead, cpu);
>   	struct llist_node *lnode;
>   	struct blkg_iostat_set *bisc, *next_bisc;
>   
> -	/* Root-level stats are sourced from system-wide IO stats */
> -	if (!cgroup_parent(css->cgroup))
> -		return;
> -
>   	rcu_read_lock();
>   
>   	lnode = llist_del_all(lhead);
> @@ -991,13 +999,19 @@ static void blkcg_rstat_flush(struct cgroup_subsys_state *css, int cpu)
>   		if (parent && parent->parent)
>   			blkcg_iostat_update(parent, &blkg->iostat.cur,
>   					    &blkg->iostat.last);
> -		percpu_ref_put(&blkg->refcnt);
>   	}
>   
>   out:
>   	rcu_read_unlock();
>   }
>   
> +static void blkcg_rstat_flush(struct cgroup_subsys_state *css, int cpu)
> +{
> +	/* Root-level stats are sourced from system-wide IO stats */
> +	if (cgroup_parent(css->cgroup))
> +		__blkcg_rstat_flush(css_to_blkcg(css), cpu);
> +}
> +

I think it may not safe to call __blkcg_rstat_flus() directly without 
taking the cgroup_rstat_cpu_lock. That is why I added a helper to 
kernel/cgroup/rstat.c in my patch to meet the locking requirement.

Cheers,
Longman