lkml.org 
[lkml]   [2008]   [Jul]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] fix task dirty balancing
From
Date
On Sat, 2008-07-05 at 15:04 +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 02 Jul 2008 22:27:18 +0200
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
>
> > On Wed, 2008-07-02 at 17:26 +0900, YAMAMOTO Takashi wrote:
> > > hi,
> > >
> > > task_dirty_inc doesn't seem to be called properly for
> > > filesystems which don't use set_page_dirty for write(2).
> > > eg. ext2 w/o nobh option.
> >
> > I'm thinking this is an ext2 bug. So I'd rather it'd just call
> > set_page_dirty() like a proper filesystem instead of doing things like
> > this.
> >
> > And I certainly don't like exporting task_dirty_inc() - filesystems and
> > the like should not have to know about things like that.
> >
> Hmm, a bit complicated for me.
>
> At first, there are 2 __set_page_dirty() in the kernel.
> - mm/page-writeback.c: __set_page_dirty()
> .... set_page_dirty() calls this.
> - fs/buffer.c : __set_page_dirty()
> .... __set_page_dirty_buffers() and mark_buffer_dirty() calls this.
>
> Why per-task dirty acconitng is done in mm/page-writeback.c::set_page_dirty() ?
>
> It seems other accounting is done in the fs/buffer.c: __set_page_dirty()
>
> The purpose of task-dirty accounting is different from others ?
>
> = fs/buffer.c
> 697 static int __set_page_dirty(struct page *page,
> 698 struct address_space *mapping, int warn)
> 699 {
> 700 if (unlikely(!mapping))
> 701 return !TestSetPageDirty(page);
> 702
> 703 if (TestSetPageDirty(page))
> 704 return 0;
> 705
> 706 write_lock_irq(&mapping->tree_lock);
> 707 if (page->mapping) { /* Race with truncate? */
> 708 WARN_ON_ONCE(warn && !PageUptodate(page));
> 709
> 710 if (mapping_cap_account_dirty(mapping)) {
> 711 __inc_zone_page_state(page, NR_FILE_DIRTY);
> 712 __inc_bdi_stat(mapping->backing_dev_info,
> 713 BDI_RECLAIMABLE);
> 714 task_io_account_write(PAGE_CACHE_SIZE);
> 715 }
> 716 radix_tree_tag_set(&mapping->page_tree,
> 717 page_index(page), PAGECACHE_TAG_DIRTY);
> 718 }
> 719 write_unlock_irq(&mapping->tree_lock);
> 720 __mark_inode_dirty(mapping->host, I_DIRTY_PAGES);
> 721
> 722 return 1;
> ==
>
> And task-dirty-limit don't have to take care of following 2 case ?
> - __set_page_dirty_nobuffers(struct page *page) (increment BDI_RECRAIMABLE)
> - test_set_page_writeback() (increment BDI_RECLAIMABLE)


Gah - what a mess...

It's in set_page_dirty() so it wouldn't have to be in all the
a_ops->set_page_dirty() functions...

But now it turns out people don't use set_page_dirty() to dirty
pages :-(

For the purpose of task_dirty_inc() I guess we might as well pair it
with task_io_account_write() for each PAGE_CACHE_SIZE (and ignore the
DIO bit, since that doesn't care about the dirty limit anyway).

Might be my ignorance, but _why_ do we have __set_page_dirty_nobuffers()
reimplemented in fs/buffers.c:__set_page_dirty() ? - those two functions
look suspiciously similar.

Also, why was the EXPORT added anyway - fs/buffers.o never ends up in
modules?

Please beat me to cleaning up this stuff - otherwise I'll have to look
at it when I get back from holidays.

Peter



\
 
 \ /
  Last update: 2008-07-05 11:47    [W:0.078 / U:2.920 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site