Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1175638 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2 block/for-linus] writeback: don't drain  bdi_writeback_congested on bdi destruction Tejun Heo <tj@kernel.org> - 2015-07-02 03:00 +0200
    Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h  usable in user-space Tejun Heo <tj@kernel.org> - 2015-07-02 15:30 +0200
      Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h usable in user-space Rusty Russell <rusty@rustcorp.com.au> - 2015-07-03 02:40 +0200
        Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h  usable in user-space Jon Christopherson <jon@jons.org> - 2015-07-03 12:10 +0200
        Re: [PATCH] tools/perf, rbtree: Add RCU wrappers to make rbtree.h  usable in user-space Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-07-05 10:50 +0200
    Re: [PATCH 2/2 block/for-linus] writeback: don't drain bdi_writeback_congested  on bdi destruction Jens Axboe <axboe@kernel.dk> - 2015-07-02 16:10 +0200

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

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.

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 <tj@kernel.org>
Reported-by: Jon Christopherson <jon@jons.org>
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(-)

--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -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);
 
 	/*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

(cc'ing Rusty and lkml)

Hello,

On Wed, Jul 01, 2015 at 10:18:48PM -0500, Jon Christopherson wrote:
> 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:
> 
> diff --git a/tools/perf/util/include/linux/rcupdate.h
> b/tools/perf/util/include/linux/rcupdate.h
> new file mode 100644
> index 0000000..51c0f45
> --- /dev/null
> +++ b/tools/perf/util/include/linux/rcupdate.h
> @@ -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))
> +#define rcu_assign_pointer(ptr, val) WRITE_ONCE(ptr, val)
> +
> +#endif
> +

Rusty?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

Tejun Heo <tj@kernel.org> writes:
> (cc'ing Rusty and lkml)

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

Cheers,
Rusty.

>
> Hello,
>
> On Wed, Jul 01, 2015 at 10:18:48PM -0500, Jon Christopherson wrote:
>> 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:
>> 
>> diff --git a/tools/perf/util/include/linux/rcupdate.h
>> b/tools/perf/util/include/linux/rcupdate.h
>> new file mode 100644
>> index 0000000..51c0f45
>> --- /dev/null
>> +++ b/tools/perf/util/include/linux/rcupdate.h
>> @@ -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))
>> +#define rcu_assign_pointer(ptr, val) WRITE_ONCE(ptr, val)
>> +
>> +#endif
>> +
>
> Rusty?
>
> Thanks.
>
> -- 
> tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On 07/03/2015 02:14 AM, Peter Zijlstra wrote:
> On Fri, Jul 03, 2015 at 06:21:12AM +0930, Rusty Russell wrote:
>> 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

>
>>>> diff --git a/tools/perf/util/include/linux/rcupdate.h
>>>> b/tools/perf/util/include/linux/rcupdate.h
>>>> new file mode 100644
>>>> index 0000000..51c0f45
>>>> --- /dev/null
>>>> +++ b/tools/perf/util/include/linux/rcupdate.h
>>>> @@ -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.
>
>>>> +#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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

Em Fri, Jul 03, 2015 at 09:14:46AM +0200, Peter Zijlstra escreveu:
> On Fri, Jul 03, 2015 at 06:21:12AM +0930, Rusty Russell wrote:
> > Looks like Peter Zijlstra is the one to take this fix...
> 
> acme is the steward of tools/perf/
> 
> > >> diff --git a/tools/perf/util/include/linux/rcupdate.h
> > >> b/tools/perf/util/include/linux/rcupdate.h
> > >> new file mode 100644
> > >> index 0000000..51c0f45
> > >> --- /dev/null
> > >> +++ b/tools/perf/util/include/linux/rcupdate.h
> > >> @@ -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.
> 
> > >> +#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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On 07/01/2015 09:02 PM, Jon Christopherson wrote:
>
>
> On 07/01/2015 07:53 PM, Tejun Heo wrote:
>> 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!

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


-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web