On Thu, May 25, 2023 at 12:35:18PM +0800, Ming Lei <ming.lei@xxxxxxxxxx> wrote: > 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. On v2, io implies memory too. Do you observe the leak on the v2 system too? (Beware that (not only) dirty pages would pin offlined memcg, so the actual mem_cgroup_css_release and cgroup_rstat_flush would be further delayed.) > To prevent this potential memory leak: > > - flush blkcg per-cpu stats list in __blkg_release(), when no new stat > can be added > > - add global blkg_stat_lock for covering concurrent parent blkg stat > update It's bit unfortunate yet another lock is added :-/ IIUC, even Waiman's patch (flush in blkcg_destroy_blkcfs) would need synchronization for different CPU replicas flushes in blkcg_iostat_update, right? > - 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 At one moment, the lhead -> blkcg_gq reference seemed alright to me and consequently blkcg_gq -> blkcg is the one that looks reversed (forming the cycle). But changing its direction would be much more fundamental change, it'd need also kind of blkcg_gq reparenting -- similarly to memcg. Thanks, Michal
Attachment:
signature.asc
Description: PGP signature
