[patch 1/2] vfs: add/use update_page_accounting

[Edward Shishkin]

Andrew Morton writes:
 > On Wed, 18 Feb 2009 03:26:16 +0300
 > Edward Shishkin <edward.shishkin@xxxxxxxxx> wrote:
 > 
 > > Andrew Morton wrote:
 > > > On Tue, 17 Feb 2009 01:43:28 +0300
 > > > Edward Shishkin <edward.shishkin@xxxxxxxxx> wrote:
 > > >
 > > >   
 > > >> + */
 > > >> +int set_page_dirty_notag(struct page *page)
 > > >> +{
 > > >> +	struct address_space *mapping = page->mapping;
 > > >> +
 > > >> +	if (!TestSetPageDirty(page)) {
 > > >> +		WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
 > > >> +		if (mapping_cap_account_dirty(mapping)) {
 > > >> +		        /*
 > > >> +			 * The accounting functions rely on
 > > >> +			 * being atomic wrt interrupts.
 > > >> +			 */
 > > >> +		        unsigned long flags;
 > > >> +			local_irq_save(flags);
 > > >> +			__inc_zone_page_state(page, NR_FILE_DIRTY);
 > > >> +			__inc_bdi_stat(mapping->backing_dev_info,
 > > >> +				       BDI_RECLAIMABLE);
 > > >> +			task_dirty_inc(current);
 > > >> +			task_io_account_write(PAGE_CACHE_SIZE);
 > > >> +			local_irq_restore(flags);
 > > >> +		}
 > > >> +		__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
 > > >> +		return 1;
 > > >> +	}
 > > >> +	return 0;
 > > >> +}
 > > >>     
 > > >
 > > > I'll maintain this in -mm, alongside the resier4 patches which need it.
 > > >
 > > > Of course, this rather obviates the purpose - if someone changes, say,
 > > > __set_page_dirty_nobuffers() then they won't similarly update
 > > > set_page_dirty_notag().  Oh well.
 > > >
 > > > This problem would fix itself if those two functions were to
 > > > substantially share code.
 > > 
 > > It will fix the problem only partially:
 > > there is one more friend __set_page_dirty() in buffer.c
 > 
 > But all three functions do the same thing?
 > 
 > 			if (mapping_cap_account_dirty(mapping)) {
 > 				__inc_zone_page_state(page, NR_FILE_DIRTY);
 > 				__inc_bdi_stat(mapping->backing_dev_info,
 > 						BDI_RECLAIMABLE);
 > 				task_dirty_inc(current);
 > 				task_io_account_write(PAGE_CACHE_SIZE);
 > 			}
 > 
 > ?

Yup, exactly the same.

 > 
 > > Maybe it makes sense to add comments with warnings
 > > in all such places, or create a header file with a static inline
 > > function update_page_accounting() ?
 > 
 > Could just uninline the helper function I guess - if you look, those
 > four statements already involve doing a heck of a lot of stuff.
 > 
 > Try it, see how it looks?
 > 

Done.
Please, review.

Add/use a helper function update_page_accounting().

Signed-off-by: Edward Shishkin<edward.shishkin@xxxxxxxxx>
---
 fs/buffer.c         |    9 +--------
 include/linux/mm.h  |    1 +
 mm/page-writeback.c |   22 +++++++++++++++-------
 3 files changed, 17 insertions(+), 15 deletions(-)

--- mmotm.orig/fs/buffer.c
+++ mmotm/fs/buffer.c
@@ -803,14 +803,7 @@ static int __set_page_dirty(struct page 
 	spin_lock_irq(&mapping->tree_lock);
 	if (page->mapping) {	/* Race with truncate? */
 		WARN_ON_ONCE(warn && !PageUptodate(page));
-
-		if (mapping_cap_account_dirty(mapping)) {
-			__inc_zone_page_state(page, NR_FILE_DIRTY);
-			__inc_bdi_stat(mapping->backing_dev_info,
-					BDI_RECLAIMABLE);
-			task_dirty_inc(current);
-			task_io_account_write(PAGE_CACHE_SIZE);
-		}
+		update_page_accounting(page, mapping);
 		radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
 	}
--- mmotm.orig/include/linux/mm.h
+++ mmotm/include/linux/mm.h
@@ -839,6 +839,7 @@ int __set_page_dirty_nobuffers(struct pa
 int __set_page_dirty_no_writeback(struct page *page);
 int redirty_page_for_writepage(struct writeback_control *wbc,
 				struct page *page);
+void update_page_accounting(struct page *page, struct address_space *mapping);
 int set_page_dirty(struct page *page);
 int set_page_dirty_lock(struct page *page);
 int clear_page_dirty_for_io(struct page *page);
--- mmotm.orig/mm/page-writeback.c
+++ mmotm/mm/page-writeback.c
@@ -1198,6 +1198,20 @@ int __set_page_dirty_no_writeback(struct
 }
 
 /*
+ * Helper function for set_page_dirty family.
+ * NOTE: This relies on being atomic wrt interrupts.
+ */
+void update_page_accounting(struct page *page, struct address_space *mapping)
+{
+	if (mapping_cap_account_dirty(mapping)) {
+		__inc_zone_page_state(page, NR_FILE_DIRTY);
+		__inc_bdi_stat(mapping->backing_dev_info, BDI_RECLAIMABLE);
+		task_dirty_inc(current);
+		task_io_account_write(PAGE_CACHE_SIZE);
+	}
+}
+
+/*
  * For address_spaces which do not use buffers.  Just tag the page as dirty in
  * its radix tree.
  *
@@ -1226,13 +1240,7 @@ int __set_page_dirty_nobuffers(struct pa
 		if (mapping2) { /* Race with truncate? */
 			BUG_ON(mapping2 != mapping);
 			WARN_ON_ONCE(!PagePrivate(page) && !PageUptodate(page));
-			if (mapping_cap_account_dirty(mapping)) {
-				__inc_zone_page_state(page, NR_FILE_DIRTY);
-				__inc_bdi_stat(mapping->backing_dev_info,
-						BDI_RECLAIMABLE);
-				task_dirty_inc(current);
-				task_io_account_write(PAGE_CACHE_SIZE);
-			}
+			update_page_accounting(page, mapping);
 			radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
 		}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/