Re: [PATCH 2/6] VM/NFS: The VM must tell the filesystem when to free reclaimable pages

[Wu Fengguang <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Thu, Jan 07, 2010 at 12:49:23PM +0800, Trond Myklebust wrote:
> On Thu, 2010-01-07 at 10:29 +0800, Wu Fengguang wrote: 
> > On Thu, Jan 07, 2010 at 04:51:10AM +0800, Trond Myklebust wrote:
> > > balance_dirty_pages() should really tell the filesystem whether or not it
> > > has an excess of actual dirty pages, or whether it would be more useful to
> > > start freeing up the unstable writes.
> > > 
> > > Assume that if the number of unstable writes is more than 1/2 the number of
> > > reclaimable pages, then we should force NFS to free up the former.
> > > 
> > > Signed-off-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
> > > Acked-by: Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>
> > > ---
> > > 
> > >  fs/nfs/write.c            |    2 +-
> > >  include/linux/writeback.h |    5 +++++
> > >  mm/page-writeback.c       |    9 ++++++++-
> > >  3 files changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> > > index 910be28..ee3daf4 100644
> > > --- a/fs/nfs/write.c
> > > +++ b/fs/nfs/write.c
> > > @@ -1417,7 +1417,7 @@ int nfs_commit_unstable_pages(struct address_space *mapping,
> > >  	/* Don't commit yet if this is a non-blocking flush and there are
> > >  	 * outstanding writes for this mapping.
> > >  	 */
> > > -	if (wbc->sync_mode != WB_SYNC_ALL &&
> > > +	if (!wbc->force_commit && wbc->sync_mode != WB_SYNC_ALL &&
> > >  	    radix_tree_tagged(&NFS_I(inode)->nfs_page_tree,
> > >  		    NFS_PAGE_TAG_LOCKED)) {
> > >  		mark_inode_unstable_pages(inode);
> > > diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> > > index 76e8903..3fd5c3e 100644
> > > --- a/include/linux/writeback.h
> > > +++ b/include/linux/writeback.h
> > > @@ -62,6 +62,11 @@ struct writeback_control {
> > >  	 * so we use a single control to update them
> > >  	 */
> > >  	unsigned no_nrwrite_index_update:1;
> > > +	/*
> > > +	 * The following is used by balance_dirty_pages() to
> > > +	 * force NFS to commit unstable pages.
> > > +	 */
> > 
> > In fact it may be too late to force commit at balance_dirty_pages()
> > time: commit takes time and the application has already been blocked.
> > 
> > If not convenient for now, I can make the change -- I'll remove the
> > writeback_inodes_wbc() call altogether from balance_dirty_pages().
> 
> You could always set the 'for_background' flag instead. 

Please this is misusing ->for_background.. Anyway it's not a big
problem. I'll set the force_nfs_commit flag in background writeback.

> > > +	unsigned force_commit:1;
> > >  };
> > 
> > nfs_commit may be a more newbie friendly name?
> 
> We could possibly rename it to something like 'force_nfs_commit', but
> the comment above the declaration should really be sufficient.

"commit" could also be misread as "commit a transaction"?
Anyway I think adding an "nfs" limits the scope to NFS thus makes code
reading somehow easier. Just a personal feeling.

Thanks,
Fengguang
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html