[PATCH] mm: set_page_dirty_balance() vs ->page_mkwrite() - Kernel

This is a discussion on [PATCH] mm: set_page_dirty_balance() vs ->page_mkwrite() - Kernel ; It seems that with the recent usage of ->page_mkwrite() a little detail
was overlooked.
..22-rc1 merged OCFS2 usage of this hook
..23-rc1 merged XFS usage
..24-rc1 will most likely merge NFS usage
Please consider this for .23 final and maybe ...

Re: [PATCH] mm: set_page_dirty_balance() vs ->page_mkwrite()

On Tuesday 09 October 2007 02:54, Peter Zijlstra wrote:
> It seems that with the recent usage of ->page_mkwrite() a little detail
> was overlooked.
>
> .22-rc1 merged OCFS2 usage of this hook
> .23-rc1 merged XFS usage
> .24-rc1 will most likely merge NFS usage
>
> Please consider this for .23 final and maybe even .22.x
>
> ---
> Subject: mm: set_page_dirty_balance() vs ->page_mkwrite()
>
> All the current page_mkwrite() implementations also set the page dirty.
> Which results in the set_page_dirty_balance() call to _not_ call balance,
> because the page is already found dirty.
>
> This allows us to dirty a _lot_ of pages without ever hitting
> balance_dirty_pages(). Not good (tm).
>
> Force a balance call if ->page_mkwrite() was successful.

Re: [PATCH] mm: set_page_dirty_balance() vs ->page_mkwrite()

On Mon, Oct 08, 2007 at 04:37:00PM +1000, Nick Piggin wrote:
> On Tuesday 09 October 2007 02:54, Peter Zijlstra wrote:
> > It seems that with the recent usage of ->page_mkwrite() a little detail
> > was overlooked.
> >
> > .22-rc1 merged OCFS2 usage of this hook
> > .23-rc1 merged XFS usage
> > .24-rc1 will most likely merge NFS usage
> >
> > Please consider this for .23 final and maybe even .22.x
> >
> > ---
> > Subject: mm: set_page_dirty_balance() vs ->page_mkwrite()
> >
> > All the current page_mkwrite() implementations also set the page dirty.
> > Which results in the set_page_dirty_balance() call to _not_ call balance,
> > because the page is already found dirty.
> >
> > This allows us to dirty a _lot_ of pages without ever hitting
> > balance_dirty_pages(). Not good (tm).
> >
> > Force a balance call if ->page_mkwrite() was successful.
>
> Would it be better to just have the callers set_page_dirty_balance()?

block_page_mkwrite() is just using generic interfaces to do this,
same as pretty much any write() system call. The idea was to make it
as similar to the write() call path as possible...

However, unlike generic_file_buffered_write(), we are not calling
balance_dirty_pages_ratelimited(mapping) between
->prepare/commit_write call pairs. Perhaps this should be added to
block_page_mkwrite() after the page is unlocked....

Re: [PATCH] mm: set_page_dirty_balance() vs ->page_mkwrite()

On Tuesday 09 October 2007 09:36, David Chinner wrote:
> On Mon, Oct 08, 2007 at 04:37:00PM +1000, Nick Piggin wrote:
> > On Tuesday 09 October 2007 02:54, Peter Zijlstra wrote:
> > > Force a balance call if ->page_mkwrite() was successful.
> >
> > Would it be better to just have the callers set_page_dirty_balance()?
>
> block_page_mkwrite() is just using generic interfaces to do this,
> same as pretty much any write() system call. The idea was to make it
> as similar to the write() call path as possible...
>
> However, unlike generic_file_buffered_write(), we are not calling
> balance_dirty_pages_ratelimited(mapping) between
> ->prepare/commit_write call pairs. Perhaps this should be added to
> block_page_mkwrite() after the page is unlocked....

Re: [PATCH] mm: set_page_dirty_balance() vs ->page_mkwrite()

On Mon, Oct 08, 2007 at 05:47:52PM +1000, Nick Piggin wrote:
> > block_page_mkwrite() is just using generic interfaces to do this,
> > same as pretty much any write() system call. The idea was to make it
> > as similar to the write() call path as possible...
> >
> > However, unlike generic_file_buffered_write(), we are not calling
> > balance_dirty_pages_ratelimited(mapping) between
> > ->prepare/commit_write call pairs. Perhaps this should be added to
> > block_page_mkwrite() after the page is unlocked....
>
> That sounds pretty sane, in terms of matching with
> generic_file_buffered_write.

I agree. We could also insert a call to balance_dirty_pages_ratelimited() in
__ocfs2_page_mkwrite.
--Mark

Re: [PATCH] mm: set_page_dirty_balance() vs ->page_mkwrite()

On Tuesday 09 October 2007 12:12, Mark Fasheh wrote:
> On Mon, Oct 08, 2007 at 05:47:52PM +1000, Nick Piggin wrote:
> > > block_page_mkwrite() is just using generic interfaces to do this,
> > > same as pretty much any write() system call. The idea was to make it
> > > as similar to the write() call path as possible...
> > >
> > > However, unlike generic_file_buffered_write(), we are not calling
> > > balance_dirty_pages_ratelimited(mapping) between
> > > ->prepare/commit_write call pairs. Perhaps this should be added to
> > > block_page_mkwrite() after the page is unlocked....
> >
> > That sounds pretty sane, in terms of matching with
> > generic_file_buffered_write.
>
> I agree. We could also insert a call to balance_dirty_pages_ratelimited()
> in __ocfs2_page_mkwrite.