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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111122135919.GB10545@localhost>
Date:	Tue, 22 Nov 2011 21:59:19 +0800
From:	Wu Fengguang <fengguang.wu@...el.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	Jan Kara <jack@...e.cz>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Christoph Hellwig <hch@....de>,
	LKML <linux-kernel@...r.kernel.org>,
	Chris Mason <chris.mason@...cle.com>
Subject: Re: [PATCH 4/5] writeback: fix dirtied pages accounting on redirty

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@...el.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@...cle.com>
Signed-off-by: Wu Fengguang <fengguang.wu@...el.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]));
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/