Skip to content

block: always associate blkg and refcount cleanup

Dennis Zhou cgroups linux-block lkml 3 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v6 Latest14 patches 1 replyIn review
  2. v514 patches 7 repliesSuperseded
  3. v413 patches 15 repliesSuperseded

Changes in v6 from the cover letter

0005: Address Mike's feedback for the flush bio comment. 0006: Add a WARN_ONCE() for blkg fallback suggested by Josef.

Patches in v6 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/14blkcg: fix ref count issue with bio_blkcg() using task_cssblock/bfq-cgroup.c block/bfq-iosched.c block/bio.c +2 more+102 -140 replies
2/14blkcg: update blkg_lookup_create() to do lockingblock/blk-cgroup.c block/blk-iolatency.c include/linux/blk-cgroup.h+29 -50 replies
3/14blkcg: convert blkg_lookup_create() to find closest blkgblock/bio.c block/blk-cgroup.c block/blk-iolatency.c +2 more+44 -380 replies
4/14blkcg: introduce common blkg association logicblock/bio.c block/blk-iolatency.c block/blk-throttle.c +1 more+62 -210 replies
5/14dm: set the static flush bio device on demandblock/bio.c drivers/md/dm.c+12 -10 replies
6/14blkcg: associate blkg when associating a deviceblock/bio.c block/blk-iolatency.c block/blk-throttle.c +2 more+14 -120 replies
7/14blkcg: consolidate bio_issue_init() to be a part of coreblock/bio.c block/blk-iolatency.c block/blk-throttle.c +2 more+11 -100 replies
8/14blkcg: associate a blkg for pages being evicted by swapblock/bio.c include/linux/bio.h mm/page_io.c+42 -280 replies
9/14blkcg: associate writeback bios with a blkgDocumentation/admin-guide/cgroup-v2.rst block/bio.c fs/buffer.c +3 more+37 -110 replies
10/14blkcg: remove bio->bi_css and instead use bio->bi_blkgblock/bio.c block/bounce.c drivers/block/loop.c +5 more+32 -660 replies
11/14blkcg: remove additional reference to the cssblock/bio.c include/linux/blk-cgroup.h include/linux/cgroup.h +1 more+69 -880 replies
12/14blkcg: remove bio_disassociate_task()block/bio.c include/linux/bio.h+1 -120 replies
13/14blkcg: change blkg reference counting to use percpu_refblock/blk-cgroup.c include/linux/blk-cgroup.h+44 -120 replies
14/14blkcg: rename blkg_try_get() to blkg_tryget()block/bio.c block/blk-cgroup.c block/blk-iolatency.c +1 more+8 -110 replies

Reviews trailers per patch

All 14 patches have a Reviewed-by or Acked-by.

PatchTejun HeoJosef BacikLiu BoMike Snitzer
1/14A-b R-b
2/14A-b R-b R-b
3/14A-b R-b
4/14A-b R-b
5/14A-b
6/14R-b
7/14A-b R-b R-b
8/14A-b R-b
9/14A-b R-b
10/14A-b R-b
11/14A-b R-b
12/14A-b R-b
13/14A-b R-b
14/14A-b R-b

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes. Scroll the table sideways for all 4 reviewers.

Read the v6 thread

Cover letter [PATCH v6 00/14] block: always associate blkg and refcount cleanup

Hi everyone,

This is a minor update addressing the feedback from Mike and Josef.

v6:
0005: Address Mike's feedback for the flush bio comment. 0006: Add a WARN_ONCE() for blkg fallback suggested by Josef.

From v5:
A special case with dm is the flush bio which is statically initialized before the block device is opened and associated with a disk. This caused blkg association to throw a NPE. 0005 addresses this case by moving association to be on the flush path.

With v4 moving association to piggyback off of bio_set_dev(), this caused a NPE to be thrown by the special case above. I was overly cautious with v4 and added the bio_has_queue() check which is now removed in v5.

Also, the addition of bio_set_dev_only() wasn't quite right due to writeback and swap sharing the same bio init paths in many places. The safer thing to do is double association for those paths and in a follow up series split out the bio init paths.

Changes in v5:
All: Fixed minor grammar and syntactic issues.
0004: Removed bio_has_queue() for being overly cautious. 0005: New, properly addressed the static flush_bio in md. 0006: Removed the rcu lock in blkcg_bio_issue_check() as the bio will
own a ref on the blkg so it is unnecessary.
0011: Consolidated bio_associate_blkg_from_css() (removed __ version).

From v4:
This is respin of v3 [1] with fixes for the errors reported in [2] and [3]. v3 was reverted in [4].

The issue in [3] was that bio->bi_disk->queue and blkg->q were out of sync. So when I changed blk_get_rl() to use blkg->q, the wrong queue was returned and elevator from q->elevator->type threw a NPE. Note, with v4.21, the old block stack was removed and so this patch was dropped. I did backport this to v4.20 and verified this series does not encounter the error.

The biggest changes in v4 are when association occurs and clearly defining the cases where association should happen.
1. Association is now done when the device is set to keep blkg->q and

     bio->bi_disk->queue in sync.

2. When a bio is submitted directly to the device, it will not be associated with a blkg. This is because a blkg represents the relationship between a blkcg and a request_queue. Going directly to the device means the request_queue may not exist meaning no blkg will exist.

The patch updating blk_get_rl() was dropped (v3 10/12). The patch to always associate a blkg from v3 (v3 04/12) was fixed and split into patches 0004 and 0005. 0011 is new removing bio_disassociate_task().

Summarizing the ideas of this series:
1. Gracefully handle blkg failure to create by walking up the blkg tree rather than fall through to root.
2. Associate a bio with a blkg in core logic rather than per controller logic.
3. Rather than have a css and blkg reference, hold just a blkg ref as it also holds a css ref.
4. Switch to percpu ref counting for blkg.

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://marc.info/?l=linux-cgroups&m=154110436103723
[4] https://lore.kernel.org/lkml/[email protected]/

This patchset contains the following 14 patches:
0001-blkcg-fix-ref-count-issue-with-bio_blkcg-using-task_.patch 0002-blkcg-update-blkg_lookup_create-to-do-locking.patch 0003-blkcg-convert-blkg_lookup_create-to-find-closest-blk.patch 0004-blkcg-introduce-common-blkg-association-logic.patch 0005-dm-set-flush-bio-device-on-demand.patch 0006-blkcg-associate-blkg-when-associating-a-device.patch 0007-blkcg-consolidate-bio_issue_init-to-be-a-part-of-cor.patch 0008-blkcg-associate-a-blkg-for-pages-being-evicted-by-sw.patch 0009-blkcg-associate-writeback-bios-with-a-blkg.patch 0010-blkcg-remove-bio-bi_css-and-instead-use-bio-bi_blkg.patch 0011-blkcg-remove-additional-reference-to-the-css.patch 0012-blkcg-remove-bio_disassociate_task.patch 0013-blkcg-change-blkg-reference-counting-to-use-percpu_r.patch 0014-blkcg-rename-blkg_try_get-to-blkg_tryget.patch

This patchset is on top of linux-block#for-4.21/block 154989e45fd8.

diffstats below:

Dennis Zhou (14):
blkcg: fix ref count issue with bio_blkcg() using task_css
blkcg: update blkg_lookup_create() to do locking
blkcg: convert blkg_lookup_create() to find closest blkg
blkcg: introduce common blkg association logic
dm: set the static flush bio device on demand
blkcg: associate blkg when associating a device
blkcg: consolidate bio_issue_init() to be a part of core
blkcg: associate a blkg for pages being evicted by swap
blkcg: associate writeback bios with a blkg
blkcg: remove bio->bi_css and instead use bio->bi_blkg
blkcg: remove additional reference to the css
blkcg: remove bio_disassociate_task()
blkcg: change blkg reference counting to use percpu_ref
blkcg: rename blkg_try_get() to blkg_tryget()

 Documentation/admin-guide/cgroup-v2.rst |   8 +-
 block/bfq-cgroup.c                      |   4 +-
 block/bfq-iosched.c                     |   2 +-
 block/bio.c                             | 156 +++++++++++++++---------
 block/blk-cgroup.c                      |  95 ++++++++++++---
 block/blk-iolatency.c                   |  24 +---
 block/blk-throttle.c                    |  11 --
 block/bounce.c                          |   3 +-
 drivers/block/loop.c                    |   5 +-
 drivers/md/dm.c                         |  12 +-
 drivers/md/raid0.c                      |   2 +-
 fs/buffer.c                             |  10 +-
 fs/ext4/page-io.c                       |   2 +-
 include/linux/bio.h                     |  29 +++--
 include/linux/blk-cgroup.h              | 125 +++++++++++++------
 include/linux/blk_types.h               |   7 +-
 include/linux/cgroup.h                  |   2 +
 include/linux/writeback.h               |   5 +-
 kernel/cgroup/cgroup.c                  |  48 ++++++--
 kernel/trace/blktrace.c                 |   4 +-
 mm/page_io.c                            |   2 +-
 21 files changed, 367 insertions(+), 189 deletions(-)

Thanks,
Dennis