* [PATCH] writeback: fix BDI_WRITTEN accounting disturbing bdi->completions
@ 2011-09-01 14:40 Wu Fengguang
2011-09-01 16:29 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Wu Fengguang @ 2011-09-01 14:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jan Kara, Dave Chinner, Andrew Morton,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]
Peter,
This is an amazing bug. I'm not sure how the accounting goes wrong in
some tricky way. But you can compare the exact bdi proportions pattern
before/after patch. The gray "bdi setpoint" lines are vastly different.
---
When increasing BDI_WRITTEN together with bdi->completions inside the
same local irq disabling block, bdi_thresh is found to go wild in the
1 disk + 1 usb stick writeback test case. Fix it by moving BDI_WRITTEN
accounting out.
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
mm/page-writeback.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux.orig/mm/page-writeback.c 2011-09-01 21:43:45.312000020 +0800
+++ linux/mm/page-writeback.c 2011-09-01 21:46:10.656000006 +0800
@@ -230,7 +230,6 @@ int dirty_bytes_handler(struct ctl_table
*/
static inline void __bdi_writeout_inc(struct backing_dev_info *bdi)
{
- __inc_bdi_stat(bdi, BDI_WRITTEN);
__prop_inc_percpu_max(&vm_completions, &bdi->completions,
bdi->max_prop_frac);
}
@@ -239,6 +238,7 @@ void bdi_writeout_inc(struct backing_dev
{
unsigned long flags;
+ inc_bdi_stat(bdi, BDI_WRITTEN);
local_irq_save(flags);
__bdi_writeout_inc(bdi);
local_irq_restore(flags);
@@ -1577,6 +1577,7 @@ int test_clear_page_writeback(struct pag
PAGECACHE_TAG_WRITEBACK);
if (bdi_cap_account_writeback(bdi)) {
__dec_bdi_stat(bdi, BDI_WRITEBACK);
+ __inc_bdi_stat(bdi, BDI_WRITTEN);
__bdi_writeout_inc(bdi);
}
}
[-- Attachment #2: xfs-1dd-4k-8p-2948M-20:10-3.1.0-rc1+balance_dirty_pages-pages.png --]
[-- Type: image/png, Size: 82188 bytes --]
[-- Attachment #3: xfs-1dd-4k-8p-2948M-20:10-3.1.0-rc4-fixed+balance_dirty_pages-pages.png --]
[-- Type: image/png, Size: 55013 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] writeback: fix BDI_WRITTEN accounting disturbing bdi->completions
2011-09-01 14:40 [PATCH] writeback: fix BDI_WRITTEN accounting disturbing bdi->completions Wu Fengguang
@ 2011-09-01 16:29 ` Jan Kara
2011-09-02 2:52 ` Wu Fengguang
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2011-09-01 16:29 UTC (permalink / raw)
To: Wu Fengguang
Cc: Peter Zijlstra, Jan Kara, Dave Chinner, Andrew Morton,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
On Thu 01-09-11 22:40:41, Wu Fengguang wrote:
> Peter,
>
> This is an amazing bug. I'm not sure how the accounting goes wrong in
> some tricky way. But you can compare the exact bdi proportions pattern
> before/after patch. The gray "bdi setpoint" lines are vastly different.
>
> ---
> When increasing BDI_WRITTEN together with bdi->completions inside the
> same local irq disabling block, bdi_thresh is found to go wild in the
> 1 disk + 1 usb stick writeback test case. Fix it by moving BDI_WRITTEN
> accounting out.
I don't understand this - the patch is just NOP. The change in
test_clear_page_writeback() does absolutely nothing and the change in
bdi_writeout_inc() just changes:
local_irq_save(flags);
__inc_bdi_stat(bdi);
__prop_inc_percpu_max(&vm_completions, &bdi->completions,
bdi->max_prop_frac);
local_irq_restore(flags);
to:
local_irq_save(flags);
__inc_bdi_stat(bdi);
local_irq_restore(flags);
local_irq_save(flags);
__prop_inc_percpu_max(&vm_completions, &bdi->completions,
bdi->max_prop_frac);
local_irq_restore(flags);
So the difference must be in something else...
Honza
>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
> mm/page-writeback.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- linux.orig/mm/page-writeback.c 2011-09-01 21:43:45.312000020 +0800
> +++ linux/mm/page-writeback.c 2011-09-01 21:46:10.656000006 +0800
> @@ -230,7 +230,6 @@ int dirty_bytes_handler(struct ctl_table
> */
> static inline void __bdi_writeout_inc(struct backing_dev_info *bdi)
> {
> - __inc_bdi_stat(bdi, BDI_WRITTEN);
> __prop_inc_percpu_max(&vm_completions, &bdi->completions,
> bdi->max_prop_frac);
> }
> @@ -239,6 +238,7 @@ void bdi_writeout_inc(struct backing_dev
> {
> unsigned long flags;
>
> + inc_bdi_stat(bdi, BDI_WRITTEN);
> local_irq_save(flags);
> __bdi_writeout_inc(bdi);
> local_irq_restore(flags);
> @@ -1577,6 +1577,7 @@ int test_clear_page_writeback(struct pag
> PAGECACHE_TAG_WRITEBACK);
> if (bdi_cap_account_writeback(bdi)) {
> __dec_bdi_stat(bdi, BDI_WRITEBACK);
> + __inc_bdi_stat(bdi, BDI_WRITTEN);
> __bdi_writeout_inc(bdi);
> }
> }
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] writeback: fix BDI_WRITTEN accounting disturbing bdi->completions
2011-09-01 16:29 ` Jan Kara
@ 2011-09-02 2:52 ` Wu Fengguang
0 siblings, 0 replies; 3+ messages in thread
From: Wu Fengguang @ 2011-09-02 2:52 UTC (permalink / raw)
To: Jan Kara
Cc: Peter Zijlstra, Dave Chinner, Andrew Morton,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
On Fri, Sep 02, 2011 at 12:29:50AM +0800, Jan Kara wrote:
> On Thu 01-09-11 22:40:41, Wu Fengguang wrote:
> > Peter,
> >
> > This is an amazing bug. I'm not sure how the accounting goes wrong in
> > some tricky way. But you can compare the exact bdi proportions pattern
> > before/after patch. The gray "bdi setpoint" lines are vastly different.
> >
> > ---
> > When increasing BDI_WRITTEN together with bdi->completions inside the
> > same local irq disabling block, bdi_thresh is found to go wild in the
> > 1 disk + 1 usb stick writeback test case. Fix it by moving BDI_WRITTEN
> > accounting out.
> I don't understand this - the patch is just NOP. The change in
> test_clear_page_writeback() does absolutely nothing and the change in
> bdi_writeout_inc() just changes:
> local_irq_save(flags);
> __inc_bdi_stat(bdi);
> __prop_inc_percpu_max(&vm_completions, &bdi->completions,
> bdi->max_prop_frac);
> local_irq_restore(flags);
>
> to:
> local_irq_save(flags);
> __inc_bdi_stat(bdi);
> local_irq_restore(flags);
> local_irq_save(flags);
> __prop_inc_percpu_max(&vm_completions, &bdi->completions,
> bdi->max_prop_frac);
> local_irq_restore(flags);
>
> So the difference must be in something else...
Yes you are right -- this patch is basically a NOP. I retried without
this patch and it's now working fine again. Something mysterious to
catch..
Thanks,
Fengguang
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-09-02 2:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-01 14:40 [PATCH] writeback: fix BDI_WRITTEN accounting disturbing bdi->completions Wu Fengguang
2011-09-01 16:29 ` Jan Kara
2011-09-02 2:52 ` Wu Fengguang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).