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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux