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

Wu Fengguang
2010-01-07T02:29:03Z

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@netapp.com>
> Acked-by: Jan Kara <jack@suse.cz>
> ---
>
> 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().

> + unsigned force_commit:1;
> };

nfs_commit may be a more newbie friendly name?

Thanks,
Fengguang

> /*
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 0b19943..ede5356 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -485,6 +485,7 @@ static void balance_dirty_pages(struct address_space *mapping,
> {
> long nr_reclaimable, bdi_nr_reclaimable;
> long nr_writeback, bdi_nr_writeback;
> + long nr_unstable_nfs;
> unsigned long background_thresh;
> unsigned long dirty_thresh;
> unsigned long bdi_thresh;
> @@ -505,8 +506,9 @@ static void balance_dirty_pages(struct address_space *mapping,
> get_dirty_limits(&background_thresh, &dirty_thresh,
> &bdi_thresh, bdi);
>
> + nr_unstable_nfs = global_page_state(NR_UNSTABLE_NFS);
> nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
> - global_page_state(NR_UNSTABLE_NFS);
> + nr_unstable_nfs;
> nr_writeback = global_page_state(NR_WRITEBACK);
>
> bdi_nr_reclaimable = bdi_stat(bdi, BDI_RECLAIMABLE);
> @@ -537,6 +539,11 @@ static void balance_dirty_pages(struct address_space *mapping,
> * up.
> */
> if (bdi_nr_reclaimable > bdi_thresh) {
> + wbc.force_commit = 0;
> + /* Force NFS to also free up unstable writes. */
> + if (nr_unstable_nfs > nr_reclaimable / 2)
> + wbc.force_commit = 1;
> +
> writeback_inodes_wbc(&wbc);
> pages_written += write_chunk - wbc.nr_to_write;
> get_dirty_limits(&background_thresh, &dirty_thresh,
>