Received: by 2002:a25:6193:0:0:0:0:0 with SMTP id v141csp286702ybb; Fri, 3 Apr 2020 02:42:57 -0700 (PDT) X-Google-Smtp-Source: APiQypLg9MdPSbmwj8nQ732lCYKtsaETQtkUOvGh4MwvYqWJrNuwiTeuAXbpNpDC+4FrooCdoUiN X-Received: by 2002:aca:2806:: with SMTP id 6mr2342920oix.135.1585906977097; Fri, 03 Apr 2020 02:42:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1585906977; cv=none; d=google.com; s=arc-20160816; b=I/Rr+KiZ+P+1k8BZBuNreVpX8AV571pDxbDAgg7xWpxolTtVrfDwK2DPHlIgNJlr2+ mcOVOaAe5DlM6FfiUr64M/H40sxDRBbJ1vESmaTMtPc2lS1L1r3PE6QDZOyNDEswOHdm p8G5kgVAMMyP4VVPR9ieBJCdV4UwrjkmIFRFtHd30g+tPoFLeEna6AQ7sjtRStw6vUuj JafsWzs+BSPrmtUcPtTipr5CdZirMlvPYn9xhouJAQvYroHeZm5MdAhwUN0FCdmoqLji XlRKWGFbvM3N661/yd4Wf1X8LG1SfvivpXY5kadWc8AMDuVY8j5pjKnPFaE/bT9scXtv knBw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=mqFKZBlPbfFD55NlfTAQ8bfAEEvLWNKK7/V8zDyodzQ=; b=JMZYY3531qDNdH9636sV0YakcJHBK/ES0wCb0oMyKJjoiNx8fsEKgGi6iLkxmr5upw d4i6ivnJM1oeZb4u1UiDtHaTeGlTDb0njHVq2qtziHN7jCJUxc9OoWDQ2N757FbI1xJH 3WEgPP8YkN1grsl2ea7Mso/iVXUyYLXgZPDpJoKdoBChADY5F9c+RwZW1ENncFhiTwFY euCrJAPJw9dGr/8K8JhPSlw6Ktu6lL3qmjuWtqz4puo8aIFbgpwh/YmeL3PLqBwPaQdS pas4fTIQsP/8tVOefxUkx44t76E16p51sTgwhmBE360Jn/xyrLJT+hwZ9tjnodc0kJZP zr4w== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-nfs-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id r7si3402137oij.4.2020.04.03.02.42.33; Fri, 03 Apr 2020 02:42:57 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-nfs-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2390689AbgDCJmc (ORCPT + 99 others); Fri, 3 Apr 2020 05:42:32 -0400 Received: from mx2.suse.de ([195.135.220.15]:60698 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727912AbgDCJmb (ORCPT ); Fri, 3 Apr 2020 05:42:31 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 2491BAF19; Fri, 3 Apr 2020 09:42:27 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id 448091E1235; Fri, 3 Apr 2020 11:42:20 +0200 (CEST) Date: Fri, 3 Apr 2020 11:42:20 +0200 From: Jan Kara To: NeilBrown Cc: Christoph Hellwig , Trond Myklebust , "Anna.Schumaker@Netapp.com" , Andrew Morton , Jan Kara , linux-mm@kvack.org, linux-nfs@vger.kernel.org, LKML Subject: Re: [PATCH 2/2 - v2] MM: Discard NR_UNSTABLE_NFS, use NR_WRITEBACK instead. Message-ID: <20200403094220.GA29920@quack2.suse.cz> References: <87tv2b7q72.fsf@notabene.neil.brown.name> <87v9miydai.fsf@notabene.neil.brown.name> <87sghmyd8v.fsf@notabene.neil.brown.name> <87pncqyd7k.fsf@notabene.neil.brown.name> <20200402151009.GA14130@infradead.org> <87h7y1y0ra.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87h7y1y0ra.fsf@notabene.neil.brown.name> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org 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 > --- > > 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 SUSE Labs, CR