Re: [PATCH 2/2 - v2] MM: Discard NR_UNSTABLE_NFS, use NR_WRITEBACK instead.

[Jan Kara]

On Fri 03-04-20 09:35:21, 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 (since commit 919e3bd9a875
> ("NFS: Ensure we commit after writeback is complete")) it makes more
> sense to treat them as writeback pages.
> 
> So this patch removes 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 patch, there are fewer writes which are each larger on average.
> 
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> 
> NR_UNSTABLE_NFS completely removed as recommended by Christoph, removal
> of an unnecessary comment, and improvements to commit message.
> Thanks.
> 
>  Documentation/filesystems/proc.txt |  3 ---
>  drivers/base/node.c                |  2 --
>  fs/fs-writeback.c                  |  1 -
>  fs/nfs/internal.h                  | 10 +++++++---
>  fs/nfs/write.c                     |  4 ++--
>  fs/proc/meminfo.c                  |  2 --
>  include/linux/mmzone.h             |  1 -
>  include/trace/events/writeback.h   |  5 +----
>  mm/memcontrol.c                    |  1 -
>  mm/page-writeback.c                | 17 ++++-------------
>  mm/page_alloc.c                    |  5 +----
>  mm/vmstat.c                        |  1 -
>  12 files changed, 15 insertions(+), 37 deletions(-)
> 
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index 99ca040e3f90..690c712f5f79 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -904,7 +904,6 @@ Slab:           284364 kB
>  SReclaimable:   159856 kB
>  SUnreclaim:     124508 kB
>  PageTables:      24448 kB
> -NFS_Unstable:        0 kB
>  Bounce:              0 kB
>  WritebackTmp:        0 kB
>  CommitLimit:   7669796 kB
> @@ -975,8 +974,6 @@ SReclaimable: Part of Slab, that might be reclaimed, such as caches
>    SUnreclaim: Part of Slab, that cannot be reclaimed on memory pressure
>    PageTables: amount of memory dedicated to the lowest level of page
>                tables.
> -NFS_Unstable: NFS pages sent to the server, but not yet committed to stable
> -	      storage
>        Bounce: Memory used for block device "bounce buffers"
>  WritebackTmp: Memory used by FUSE for temporary writeback buffers
>   CommitLimit: Based on the overcommit ratio ('vm.overcommit_ratio'),
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 98a31bafc8a2..7059021ce2af 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -416,7 +416,6 @@ static ssize_t node_read_meminfo(struct device *dev,
>  		       "Node %d Shmem:          %8lu kB\n"
>  		       "Node %d KernelStack:    %8lu kB\n"
>  		       "Node %d PageTables:     %8lu kB\n"
> -		       "Node %d NFS_Unstable:   %8lu kB\n"
>  		       "Node %d Bounce:         %8lu kB\n"
>  		       "Node %d WritebackTmp:   %8lu kB\n"
>  		       "Node %d KReclaimable:   %8lu kB\n"
> @@ -439,7 +438,6 @@ static ssize_t node_read_meminfo(struct device *dev,
>  		       nid, K(i.sharedram),
>  		       nid, sum_zone_node_page_state(nid, NR_KERNEL_STACK_KB),
>  		       nid, K(sum_zone_node_page_state(nid, NR_PAGETABLE)),
> -		       nid, K(node_page_state(pgdat, NR_UNSTABLE_NFS)),
>  		       nid, K(sum_zone_node_page_state(nid, NR_BOUNCE)),
>  		       nid, K(node_page_state(pgdat, NR_WRITEBACK_TEMP)),
>  		       nid, K(sreclaimable +

So I don't think we can just remove lines from procfs files like this. That
has a high potential of breaking some userspace app that is not careful
enough when parsing the file. So I think that we need to leave there the
format string and just replace K(node_page_state(pgdat, NR_UNSTABLE_NFS))
with 0.

> diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
> index 8c1f1bb1a5ce..1378a132ff7e 100644
> --- a/fs/proc/meminfo.c
> +++ b/fs/proc/meminfo.c
> @@ -106,8 +106,6 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
>  	show_val_kb(m, "PageTables:     ",
>  		    global_zone_page_state(NR_PAGETABLE));
>  
> -	show_val_kb(m, "NFS_Unstable:   ",
> -		    global_node_page_state(NR_UNSTABLE_NFS));
>  	show_val_kb(m, "Bounce:         ",
>  		    global_zone_page_state(NR_BOUNCE));
>  	show_val_kb(m, "WritebackTmp:   ",

Similarly here.

> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 78d53378db99..d1291537bbb9 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -1162,7 +1162,6 @@ const char * const vmstat_text[] = {
>  	"nr_file_hugepages",
>  	"nr_file_pmdmapped",
>  	"nr_anon_transparent_hugepages",
> -	"nr_unstable",
>  	"nr_vmscan_write",
>  	"nr_vmscan_immediate_reclaim",
>  	"nr_dirtied",

This is probably the most tricky to deal with given how /proc/vmstat is
formatted. OTOH for this file there's good chance we'd get away with just
deleting nr_unstable line because there are entries added to it in the
middle (e.g. in 60fbf0ab5da1 last September) and nobody complained yet.

What do mm people think? How were changes to vmstat counters handled in the
past?

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR