writeback: don't embed root bdi_writeback_congested in bdi_writeback

PATCH In review Tejun Heo 6 participants 10 messages Jul 2, 2015 – Jul 6, 2015 1 message not in the archive

Subsystems (2)

Memory management MM misc

lore

Share & tools

lore

Tejun Heo

author

2015-07-02 00:53 UTC #0 lore raw

PATCH 1/2 writeback: don't embed root bdi_writeback_congested in bdi_writeback

52ebea749aae ("writeback: make backing_dev_info host cgroup-specific bdi_writebacks") made bdi (backing_dev_info) host per-cgroup wb's (bdi_writeback's). As the congested state needs to be per-wb and referenced from blkcg side and multiple wbs, the patch made all non-root cong's (bdi_writeback_congested's) reference counted and indexed on bdi.

When a bdi is destroyed, cgwb_bdi_destroy() tries to drain all non-root cong's; however, this can hang indefinitely because wb's can also be referenced from blkcg_gq's which are destroyed after bdi destruction is complete.

To fix the bug, bdi destruction will be updated to not wait for cong's to drain, which naturally means that cong's may outlive the associated bdi. This is fine for non-root cong's but is problematic for the root cong's which are embedded in their bdi's as they may end up getting dereferenced after the containing bdi's are freed.

This patch makes root cong's behave the same as non-root cong's. They are no longer embedded in their bdi's but allocated separately during bdi initialization, indexed and reference counted the same way.

* As cong handling is the same for all wb's, wb->congested initialization is moved into wb_init().

* When !CONFIG_CGROUP_WRITEBACK, there was no indexing or refcnting. bdi->wb_congested is now a pointer pointing to the root cong allocated during bdi init and minimal refcnting operations are implemented.

* The above makes root wb init paths diverge depending on CONFIG_CGROUP_WRITEBACK. root wb init is moved to cgwb_bdi_init().

This patch in itself shouldn't cause any consequential behavior differences but prepares for the actual fix.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Jon Christopherson <[email protected]>

Link https://bugzilla.kernel.org/show_bug.cgi?id=100681

---
Hello,

These two patches are to fix a deadlock during bdi destruction which was introduced by cgroup writeback changes. Jens, can you please route these two once Jon confirms the fix?

Thanks.

 include/linux/backing-dev-defs.h |    5 +-
 include/linux/backing-dev.h      |    5 +-
 mm/backing-dev.c                 |   87 ++++++++++++++++++++-------------------
 3 files changed, 53 insertions(+), 44 deletions(-)

include/linux/backing-dev-defs.h +3 -2

@@ -50,10 +50,10 @@ enum wb_stat_item {
  */
 struct bdi_writeback_congested {
 	unsigned long state;		/* WB_[a]sync_congested flags */
+	atomic_t refcnt;		/* nr of attached wb's and blkg */
 
 #ifdef CONFIG_CGROUP_WRITEBACK
 	struct backing_dev_info *bdi;	/* the associated bdi */
-	atomic_t refcnt;		/* nr of attached wb's and blkg */
 	int blkcg_id;			/* ID of the associated blkcg */
 	struct rb_node rb_node;		/* on bdi->cgwb_congestion_tree */
 #endif

@@ -150,11 +150,12 @@ struct backing_dev_info {
 	atomic_long_t tot_write_bandwidth;
 
 	struct bdi_writeback wb;  /* the root writeback info for this bdi */
-	struct bdi_writeback_congested wb_congested; /* its congested state */
 #ifdef CONFIG_CGROUP_WRITEBACK
 	struct radix_tree_root cgwb_tree; /* radix tree of active cgroup wbs */
 	struct rb_root cgwb_congested_tree; /* their congested states */
 	atomic_t usage_cnt; /* counts both cgwbs and cgwb_contested's */
+#else
+	struct bdi_writeback_congested *wb_congested;
 #endif
 	wait_queue_head_t wb_waitq;

include/linux/backing-dev.h +4 -1

@@ -465,11 +465,14 @@ static inline bool inode_cgwb_enabled(st
 static inline struct bdi_writeback_congested *
 wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp)
 {
-	return bdi->wb.congested;
+	atomic_inc(&bdi->wb_congested->refcnt);
+	return bdi->wb_congested;
 }
 
 static inline void wb_congested_put(struct bdi_writeback_congested *congested)
 {
+	if (atomic_dec_and_test(&congested->refcnt))
+		kfree(congested);
 }
 
 static inline struct bdi_writeback *wb_find_current(struct backing_dev_info *bdi)

mm/backing-dev.c +46 ~~-41~~

180 lines not on this page · raw message

Tejun Heo

author

2015-07-02 00:53 UTC #1 lore raw

PATCH 2/2 writeback: don't drain bdi_writeback_congested on bdi destruction

This patch fixes the bug by updating bdi destruction to not wait for cong's to drain. A cong is unlinked from bdi->cgwb_congested_tree on bdi destuction regardless of its reference count as the bdi may go away any point after destruction. wb_congested_put() checks whether the cong is already unlinked on release.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Jon Christopherson <[email protected]>

Link https://bugzilla.kernel.org/show_bug.cgi?id=100681

Fixes 52ebea749aae ("writeback: make backing_dev_info host cgroup-specific bdi_writebacks")

---
 mm/backing-dev.c |   22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

mm/backing-dev.c +16 -6

@@ -425,7 +425,6 @@ retry:
 		new_congested = NULL;
 		rb_link_node(&congested->rb_node, parent, node);
 		rb_insert_color(&congested->rb_node, &bdi->cgwb_congested_tree);
-		atomic_inc(&bdi->usage_cnt);
 		goto found;
 	}

@@ -456,7 +455,6 @@ found:
  */
 void wb_congested_put(struct bdi_writeback_congested *congested)
 {
-	struct backing_dev_info *bdi = congested->bdi;
 	unsigned long flags;
 
 	local_irq_save(flags);

@@ -465,12 +463,15 @@ void wb_congested_put(struct bdi_writeba
 		return;
 	}
 
-	rb_erase(&congested->rb_node, &congested->bdi->cgwb_congested_tree);
+	/* bdi might already have been destroyed leaving @congested unlinked */
+	if (congested->bdi) {
+		rb_erase(&congested->rb_node,
+			 &congested->bdi->cgwb_congested_tree);
+		congested->bdi = NULL;
+	}
+
 	spin_unlock_irqrestore(&cgwb_lock, flags);
 	kfree(congested);
-
-	if (atomic_dec_and_test(&bdi->usage_cnt))
-		wake_up_all(&cgwb_release_wait);
 }
 
 static void cgwb_release_workfn(struct work_struct *work)

@@ -675,13 +676,22 @@ static int cgwb_bdi_init(struct backing_
 static void cgwb_bdi_destroy(struct backing_dev_info *bdi)
 {
 	struct radix_tree_iter iter;
+	struct bdi_writeback_congested *congested, *congested_n;
 	void **slot;
 
 	WARN_ON(test_bit(WB_registered, &bdi->wb.state));
 
 	spin_lock_irq(&cgwb_lock);
+
 	radix_tree_for_each_slot(slot, &bdi->cgwb_tree, &iter, 0)
 		cgwb_kill(*slot);
+
+	rbtree_postorder_for_each_entry_safe(congested, congested_n,
+					&bdi->cgwb_congested_tree, rb_node) {
+		rb_erase(&congested->rb_node, &bdi->cgwb_congested_tree);
+		congested->bdi = NULL;	/* mark @congested unlinked */
+	}
+
 	spin_unlock_irq(&cgwb_lock);
 
 	/*

Jon Christopherson

2015-07-02 03:02 UTC #2 lore raw

Re: [PATCH 2/2 block/for-linus] writeback: don't drain bdi_writeback_congested on bdi destruction

Quoted 7 lines · Tejun Heo

52ebea749aae ("writeback: make backing_dev_info host cgroup-specific bdi_writebacks") made bdi (backing_dev_info) host per-cgroup wb's (bdi_writeback's). As the congested state needs to be per-wb and referenced from blkcg side and multiple wbs, the patch made all non-root cong's (bdi_writeback_congested's) reference counted and indexed on bdi.

<snip>

Thanks Tejun,

I have applied your patches and no longer see the behavior
mentioned in the bug report. All is well!

-Jon

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tejun Heo

author

2015-07-02 13:21 UTC #4 lore raw

Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h usable in user-space

(cc'ing Rusty and lkml)

Hello,

Quoted 21 lines · Jon Christopherson

Hello guys,
One last thing .. the recent commit : 02201e3f1 ("Merge tag
'modules-next-for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux") broke perf tools compilation. I know this isnt your area, but its a simple accidental omission of a file. Here is the needed change to correct retrieved from the list:
tools/perf/util/include/linux/rcupdate.h new file +4 -0
new file mode 100644
index 0000000..51c0f45
@@ -0,0 +1,9 @@
+#ifndef PERF_LINUX_RCUPDATE_H_
+#define PERF_LINUX_RCUPDATE_H_
+
+/* Simple trivial wrappers for now, we don't use RCU in perf user-space
(yet): */

tools/perf/util/include/linux/rcupdate.h

Quoted 5 lines

tools/perf/util/include/linux/rcupdate.h new file +5 -0

+#define WRITE_ONCE(var, val) ((var) = (val))
+#define rcu_assign_pointer(ptr, val) WRITE_ONCE(ptr, val)
+
+#endif
+

Rusty?

Thanks.

Signature · 1 line

tejun

Rusty Russell

2015-07-03 00:30 UTC #5 lore raw

Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h usable in user-space

Quoted 1 line · Tejun Heo

(cc'ing Rusty and lkml)

Looks like Peter Zijlstra is the one to take this fix...

Cheers,
Rusty.

Quoted 37 lines

Hello,
Quoted 21 lines · Jon Christopherson
Hello guys,
One last thing .. the recent commit : 02201e3f1 ("Merge tag
'modules-next-for-linus' of
git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux") broke perf tools compilation. I know this isnt your area, but its a simple accidental omission of a file. Here is the needed change to correct retrieved from the list:
tools/perf/util/include/linux/rcupdate.h new file +4 -0
new file mode 100644
index 0000000..51c0f45
@@ -0,0 +1,9 @@
+#ifndef PERF_LINUX_RCUPDATE_H_
+#define PERF_LINUX_RCUPDATE_H_
+
+/* Simple trivial wrappers for now, we don't use RCU in perf user-space
(yet): */
tools/perf/util/include/linux/rcupdate.h
Quoted 5 lines
tools/perf/util/include/linux/rcupdate.h new file +5 -0
+#define WRITE_ONCE(var, val) ((var) = (val))
+#define rcu_assign_pointer(ptr, val) WRITE_ONCE(ptr, val)
+
+#endif
+
Rusty?
Thanks.
Signature · 1 line
tejun

Peter Zijlstra

2015-07-03 07:15 UTC #6 lore raw

Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h usable in user-space

Quoted 1 line · Rusty Russell

Looks like Peter Zijlstra is the one to take this fix...

acme is the steward of tools/perf/

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
tools/perf/util/include/linux/rcupdate.h new file +0 -1
new file mode 100644
index 0000000..51c0f45
--- /dev/null

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
+++ b/tools/perf/util/include/linux/rcupdate.h
tools/perf/util/include/linux/rcupdate.h new file +4 -0
@@ -0,0 +1,9 @@
+#ifndef PERF_LINUX_RCUPDATE_H_
+#define PERF_LINUX_RCUPDATE_H_
+
+/* Simple trivial wrappers for now, we don't use RCU in perf user-space
(yet): */
+#define WRITE_ONCE(var, val) ((var) = (val))

It looks like perf includes linux/compiler.h so it should already have this.

Quoted 1 line

Quoted 1 line
Quoted 1 line
+#define rcu_assign_pointer(ptr, val) WRITE_ONCE(ptr, val)

That's plain wrong, WRITE_ONCE(*(ptr), (val))

Jon Christopherson

2015-07-03 10:00 UTC #7 lore raw

Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h usable in user-space

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Rusty Russell
Looks like Peter Zijlstra is the one to take this fix...
acme is the steward of tools/perf/

This is the full context of the patch mentioned:

https://lkml.org/lkml/2015/6/17/129

Quoted 17 lines

Quoted 14 lines
Quoted 14 lines
Quoted 12 lines
tools/perf/util/include/linux/rcupdate.h new file +4 -0
new file mode 100644
index 0000000..51c0f45
@@ -0,0 +1,9 @@
+#ifndef PERF_LINUX_RCUPDATE_H_
+#define PERF_LINUX_RCUPDATE_H_
+
+/* Simple trivial wrappers for now, we don't use RCU in perf user-space
Quoted 2 lines
(yet): */
+#define WRITE_ONCE(var, val) ((var) = (val))
It looks like perf includes linux/compiler.h so it should already have this.

Quoted 4 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
+#define rcu_assign_pointer(ptr, val) WRITE_ONCE(ptr, val)
That's plain wrong, WRITE_ONCE(*(ptr), (val))

The original author of the patch appears to be Ingo. Syntax aside .. it solves the issue mentioned. Perhaps a corrected version could be included instead.

-Jon

Arnaldo Carvalho de Melo

2015-07-05 08:42 UTC #8 lore raw

Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h usable in user-space

In reply to Peter Zijlstra

Em Fri, Jul 03, 2015 at 09:14:46AM +0200, Peter Zijlstra escreveu:

Quoted 21 lines · Rusty Russell

Quoted 1 line
Looks like Peter Zijlstra is the one to take this fix...
acme is the steward of tools/perf/
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
tools/perf/util/include/linux/rcupdate.h new file +0 -1
new file mode 100644
index 0000000..51c0f45
--- /dev/null
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
+++ b/tools/perf/util/include/linux/rcupdate.h
tools/perf/util/include/linux/rcupdate.h new file +4 -0
@@ -0,0 +1,9 @@
+#ifndef PERF_LINUX_RCUPDATE_H_
+#define PERF_LINUX_RCUPDATE_H_
+
+/* Simple trivial wrappers for now, we don't use RCU in perf user-space
(yet): */
+#define WRITE_ONCE(var, val) ((var) = (val))
It looks like perf includes linux/compiler.h so it should already have this.

Quoted 3 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
+#define rcu_assign_pointer(ptr, val) WRITE_ONCE(ptr, val)
That's plain wrong, WRITE_ONCE(*(ptr), (val))

Are you sure?

In the kernel, we have this sequence:

#define rcu_assign_pointer(p, v) smp_store_release(&p, RCU_INITIALIZER(v))

#define smp_store_release(p, v) \

do {						\
        compiletime_assert_atomic_type(*p);	\
        smp_mb();				\
        ACCESS_ONCE(*p) = (v);			\
} while (0)

So, if you go shortcircuiting things you remove that & and that *, no?

I.e. end up with what Rusty suggested.

So, I am trying to keep as much as the semantics of the kernel not to fall into thse traps...

Will post a RFC soon, if the rain continues preventing me from running...

- Arnaldo

Peter Zijlstra

2015-07-06 10:57 UTC #9 lore raw

Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h usable in user-space

Quoted 3 lines · Arnaldo Carvalho de Melo

Quoted 1 line
That's plain wrong, WRITE_ONCE(*(ptr), (val))
Are you sure?

I'm sure I was having a heat stroke,.. that original was fine.

Jens Axboe

2015-07-02 14:04 UTC #10 lore raw

Re: [PATCH 2/2 block/for-linus] writeback: don't drain bdi_writeback_congested on bdi destruction

In reply to Jon Christopherson

Quoted 10 lines · Jon Christopherson

Quoted 7 lines · Tejun Heo
52ebea749aae ("writeback: make backing_dev_info host cgroup-specific bdi_writebacks") made bdi (backing_dev_info) host per-cgroup wb's (bdi_writeback's). As the congested state needs to be per-wb and referenced from blkcg side and multiple wbs, the patch made all non-root cong's (bdi_writeback_congested's) reference counted and indexed on bdi.

Quoted 6 lines

<snip>
Thanks Tejun,
I have applied your patches and no longer see the behavior
mentioned in the bug report. All is well!

Tejun, I'll pick them up, and Jon, I'll add your tested-by to the commit. Thanks!

Signature · 1 line

Jens Axboe