From: Jan Kara <jack@suse.cz>
To: NeilBrown <neilb@suse.de>
Cc: Trond Myklebust <trondmy@hammerspace.com>,
"Anna.Schumaker@Netapp.com" <Anna.Schumaker@netapp.com>,
Andrew Morton <akpm@linux-foundation.org>,
Jan Kara <jack@suse.cz>,
linux-mm@kvack.org, linux-nfs@vger.kernel.org,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] Deprecate NR_UNSTABLE_NFS, use NR_WRITEBACK
Date: Thu, 2 Apr 2020 21:55:51 +0200 [thread overview]
Message-ID: <20200402195551.GD9751@quack2.suse.cz> (raw)
In-Reply-To: <87pncqyd7k.fsf@notabene.neil.brown.name>
On Thu 02-04-20 10:54:07, NeilBrown wrote:
>
> After an NFS page has been written it is considered "unstable" until a
> COMMIT request succeeds. If the COMMIT fails, the page will be
> re-written.
>
> These "unstable" pages are currently accounted as "reclaimable",
> either in WB_RECLAIMABLE, or in NR_UNSTABLE_NFS which is included in a
> 'reclaimable' count. This might have made sense when sending the COMMIT
> required a separate action by the VFS/MM (e.g. releasepage() used to
> send a COMMIT). However now that all writes generated by ->writepages()
> will automatically be followed by a COMMIT, it makes more sense to
> treat them as writeback pages.
>
> So this page deprecates NR_UNSTABLE_NFS and accounts unstable pages in
> NR_WRITEBACK and WB_WRITEBACK.
>
> A particular effect of this change is that when
> wb_check_background_flush() calls wb_over_bg_threshold(), the latter
> will report 'true' a lot less often as the 'unstable' pages are no
> longer considered 'dirty' (and there is nothing that writeback can do
> about them anyway).
>
> Currently wb_check_background_flush() will trigger writeback to NFS even
> when there are relatively few dirty pages (if there are lots of unstable
> pages), this can result in small writes going to the server (10s of
> Kilobytes rather than a Megabyte) which hurts throughput.
> With this that, there are fewer writes which are each larger on average.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
The patch looks good to me. I agree with Christoph that it would be best to
remove NR_UNSTABLE_NFS completely. We just have to be careful, not change
format of any entries in /proc/ where it currently gets reported...
Honza
> ---
> fs/fs-writeback.c | 1 -
> fs/nfs/internal.h | 7 +++++--
> fs/nfs/write.c | 4 ++--
> include/linux/mmzone.h | 2 +-
> mm/memcontrol.c | 1 -
> mm/page-writeback.c | 7 ++-----
> 6 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 76ac9c7d32ec..c5bdf46e3b4b 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1070,7 +1070,6 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
> static unsigned long get_nr_dirty_pages(void)
> {
> return global_node_page_state(NR_FILE_DIRTY) +
> - global_node_page_state(NR_UNSTABLE_NFS) +
> get_nr_dirty_inodes();
> }
>
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index f80c47d5ff27..ba1ff5adeccd 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -660,8 +660,11 @@ void nfs_mark_page_unstable(struct page *page, struct nfs_commit_info *cinfo)
> if (!cinfo->dreq) {
> struct inode *inode = page_file_mapping(page)->host;
>
> - inc_node_page_state(page, NR_UNSTABLE_NFS);
> - inc_wb_stat(&inode_to_bdi(inode)->wb, WB_RECLAIMABLE);
> + /* This page is really still in write-back - just that the
> + * writeback is happening on the server now.
> + */
> + inc_node_page_state(page, NR_WRITEBACK);
> + inc_wb_stat(&inode_to_bdi(inode)->wb, WB_WRITEBACK);
> __mark_inode_dirty(inode, I_DIRTY_DATASYNC);
> }
> }
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index c478b772cc49..2e15a56620b3 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -958,9 +958,9 @@ nfs_mark_request_commit(struct nfs_page *req, struct pnfs_layout_segment *lseg,
> static void
> nfs_clear_page_commit(struct page *page)
> {
> - dec_node_page_state(page, NR_UNSTABLE_NFS);
> + dec_node_page_state(page, NR_WRITEBACK);
> dec_wb_stat(&inode_to_bdi(page_file_mapping(page)->host)->wb,
> - WB_RECLAIMABLE);
> + WB_WRITEBACK);
> }
>
> /* Called holding the request lock on @req */
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 462f6873905a..227fcb8cd0e6 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -237,7 +237,7 @@ enum node_stat_item {
> NR_FILE_THPS,
> NR_FILE_PMDMAPPED,
> NR_ANON_THPS,
> - NR_UNSTABLE_NFS, /* NFS unstable pages */
> + NR_UNSTABLE_NFS, /* NFS unstable pages - DEPRECATED DO NOT USE */
> NR_VMSCAN_WRITE,
> NR_VMSCAN_IMMEDIATE, /* Prioritise for reclaim when writeback ends */
> NR_DIRTIED, /* page dirtyings since bootup */
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 7ddf91c4295f..fad8e8a23235 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4317,7 +4317,6 @@ void mem_cgroup_wb_stats(struct bdi_writeback *wb, unsigned long *pfilepages,
>
> *pdirty = memcg_exact_page_state(memcg, NR_FILE_DIRTY);
>
> - /* this should eventually include NR_UNSTABLE_NFS */
> *pwriteback = memcg_exact_page_state(memcg, NR_WRITEBACK);
> *pfilepages = memcg_exact_page_state(memcg, NR_INACTIVE_FILE) +
> memcg_exact_page_state(memcg, NR_ACTIVE_FILE);
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 2afb09fa2fe0..d1f03c799d11 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -504,7 +504,6 @@ bool node_dirty_ok(struct pglist_data *pgdat)
> unsigned long nr_pages = 0;
>
> nr_pages += node_page_state(pgdat, NR_FILE_DIRTY);
> - nr_pages += node_page_state(pgdat, NR_UNSTABLE_NFS);
> nr_pages += node_page_state(pgdat, NR_WRITEBACK);
>
> return nr_pages <= limit;
> @@ -1595,8 +1594,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb,
> * written to the server's write cache, but has not yet
> * been flushed to permanent storage.
> */
> - nr_reclaimable = global_node_page_state(NR_FILE_DIRTY) +
> - global_node_page_state(NR_UNSTABLE_NFS);
> + nr_reclaimable = global_node_page_state(NR_FILE_DIRTY);
> gdtc->avail = global_dirtyable_memory();
> gdtc->dirty = nr_reclaimable + global_node_page_state(NR_WRITEBACK);
>
> @@ -1940,8 +1938,7 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
> * as we're trying to decide whether to put more under writeback.
> */
> gdtc->avail = global_dirtyable_memory();
> - gdtc->dirty = global_node_page_state(NR_FILE_DIRTY) +
> - global_node_page_state(NR_UNSTABLE_NFS);
> + gdtc->dirty = global_node_page_state(NR_FILE_DIRTY);
> domain_dirty_limits(gdtc);
>
> if (gdtc->dirty > gdtc->bg_thresh)
> --
> 2.26.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2020-04-02 19:56 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-26 3:25 [PATCH/RFC] MM: fix writeback for NFS NeilBrown
2020-04-01 23:52 ` Writeback fixes " NeilBrown
2020-04-01 23:53 ` [PATCH 1/2] MM: replace PF_LESS_THROTTLE with PF_LOCAL_THROTTLE NeilBrown
2020-04-01 23:54 ` [PATCH 2/2] Deprecate NR_UNSTABLE_NFS, use NR_WRITEBACK NeilBrown
2020-04-02 15:10 ` Christoph Hellwig
2020-04-02 22:35 ` [PATCH 2/2 - v2] MM: Discard NR_UNSTABLE_NFS, use NR_WRITEBACK instead NeilBrown
2020-04-03 9:42 ` Jan Kara
2020-04-03 11:03 ` Michal Hocko
2020-04-06 0:14 ` NeilBrown
2020-04-06 7:41 ` Michal Hocko
2020-04-06 23:28 ` NeilBrown
2020-04-07 7:33 ` Michal Hocko
2020-04-02 19:55 ` Jan Kara [this message]
2020-04-02 16:35 ` [PATCH 1/2] MM: replace PF_LESS_THROTTLE with PF_LOCAL_THROTTLE Jan Kara
2020-04-03 15:15 ` Michal Hocko
2020-04-03 21:40 ` NeilBrown
2020-04-06 7:44 ` Michal Hocko
2020-04-06 9:36 ` Jan Kara
2020-04-06 10:57 ` Michal Hocko
2020-04-06 11:58 ` NeilBrown
[not found] ` <20200402042644.17028-1-hdanton@sina.com>
2020-04-02 4:57 ` NeilBrown
2020-04-06 23:42 ` Writeback fixes for NFS - V2 NeilBrown
2020-04-06 23:43 ` [PATCH 1/2] MM: replace PF_LESS_THROTTLE with PF_LOCAL_THROTTLE NeilBrown
2020-04-07 16:10 ` Chuck Lever
2020-04-16 0:29 ` Writeback fixes for NFS - V3 NeilBrown
2020-04-16 0:30 ` [PATCH 1/2 V3] MM: replace PF_LESS_THROTTLE with PF_LOCAL_THROTTLE NeilBrown
2020-04-16 6:54 ` Christoph Hellwig
2020-04-16 15:19 ` Jan Kara
2020-04-21 2:22 ` NeilBrown
2020-04-22 12:46 ` Jan Kara
2020-05-13 7:16 ` NeilBrown
2020-05-13 7:17 ` [PATCH 1/2 V4] " NeilBrown
2020-05-15 11:10 ` Jan Kara
2020-06-01 0:46 ` Writeback fixes for NFS NeilBrown
2020-06-01 0:48 ` [PATCH 1/2] MM: replace PF_LESS_THROTTLE with PF_LOCAL_THROTTLE NeilBrown
2020-06-01 0:49 ` [PATCH 2/2] MM: Discard NR_UNSTABLE_NFS, use NR_WRITEBACK instead NeilBrown
2020-05-13 7:18 ` [PATCH 2/2 V4] " NeilBrown
2020-05-15 9:59 ` Jan Kara
2020-04-16 0:31 ` [PATCH 2/2 V3] " NeilBrown
2020-04-16 6:56 ` Christoph Hellwig
2020-04-16 15:24 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200402195551.GD9751@quack2.suse.cz \
--to=jack@suse.cz \
--cc=Anna.Schumaker@netapp.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nfs@vger.kernel.org \
--cc=neilb@suse.de \
--cc=trondmy@hammerspace.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).