Re: [PATCH 4/5] writeback: fix dirtied pages accounting on redirty

[Wu Fengguang <fengguang.wu@intel.com>]

On Tue, Nov 22, 2011 at 05:51:46AM +0800, Andrew Morton wrote:
> On Mon, 21 Nov 2011 21:03:46 +0800
> Wu Fengguang <fengguang.wu@intel.com> wrote:
> 
> > De-account the accumulative dirty counters on page redirty.
> > 
> > Page redirties (very common in ext4) will introduce mismatch between
> > counters (a) and (b)
> > 
> > a) NR_DIRTIED, BDI_DIRTIED, tsk->nr_dirtied
> > b) NR_WRITTEN, BDI_WRITTEN
> > 
> > This will introduce systematic errors in balanced_rate and result in
> > dirty page position errors (ie. the dirty pages are no longer balanced
> > around the global/bdi setpoints).
> > 
> > ...
> >
> > --- linux-next.orig/mm/page-writeback.c	2011-11-17 20:57:15.000000000 +0800
> > +++ linux-next/mm/page-writeback.c	2011-11-17 20:57:16.000000000 +0800
> > @@ -1792,6 +1792,17 @@ int __set_page_dirty_nobuffers(struct pa
> >  }
> >  EXPORT_SYMBOL(__set_page_dirty_nobuffers);
> >  
> > +void account_page_redirty(struct page *page)
> > +{
> > +	struct address_space *mapping = page->mapping;
> > +	if (mapping && mapping_cap_account_dirty(mapping)) {
> > +		current->nr_dirtied--;
> > +		dec_zone_page_state(page, NR_DIRTIED);
> > +		dec_bdi_stat(mapping->backing_dev_info, BDI_DIRTIED);
> > +	}
> > +}
> 
> Again, writeback doesn't seem to be the best place to be adding
> undocumented code.

Sorry, hope this comment can explain when to call the function and why.

/*
 * Call this whenever redirtying a page, to de-account the dirty counters
 * (NR_DIRTIED, BDI_DIRTIED, tsk->nr_dirtied), so that they match the written
 * counters (NR_WRITTEN, BDI_WRITTEN) in long term. The mismatches will lead to
 * systematic errors in balanced_dirty_ratelimit and the dirty pages position
 * control.
 */

> > +EXPORT_SYMBOL(account_page_redirty);
> 
> The export is unneeded?

btrfs will need to call into it. Here is the patch.  I'll include it
in the v2 series.

Thanks,
Fengguang
---

Subject: btrfs: fix dirtied pages accounting on sub-page writes
Date: Mon Aug 08 15:19:47 CST 2011

When doing 1KB sequential writes to the same page,
balance_dirty_pages_ratelimited_nr() should be called once instead of 4
times, the latter makes the dirtier tasks be throttled much too heavy.

Fix it with proper de-accounting on clear_page_dirty_for_io().

CC: Chris Mason <chris.mason@oracle.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 fs/btrfs/file.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-next.orig/fs/btrfs/file.c	2011-11-17 20:13:47.000000000 +0800
+++ linux-next/fs/btrfs/file.c	2011-11-17 20:18:51.000000000 +0800
@@ -1136,7 +1136,8 @@ again:
 				     GFP_NOFS);
 	}
 	for (i = 0; i < num_pages; i++) {
-		clear_page_dirty_for_io(pages[i]);
+		if (clear_page_dirty_for_io(pages[i]))
+			account_page_redirty(pages[i]);
 		set_page_extent_mapped(pages[i]);
 		WARN_ON(!PageLocked(pages[i]));
 	}