Re: [PATCH v2 06/11] writeback: add counters for metadata usage

[Johannes Weiner]

On Wed, Dec 06, 2017 at 03:18:35PM -0500, Josef Bacik wrote:
> On Mon, Dec 04, 2017 at 02:06:30PM +0100, Jan Kara wrote:
> > On Wed 22-11-17 16:16:01, Josef Bacik wrote:
> > > diff --git a/mm/util.c b/mm/util.c
> > > index 34e57fae959d..681d62631ee0 100644
> > > --- a/mm/util.c
> > > +++ b/mm/util.c
> > > @@ -616,6 +616,7 @@ int __vm_enough_memory(struct mm_struct *mm, long 
> > > pages, int cap_sys_admin)
> > >   if (sysctl_overcommit_memory == OVERCOMMIT_GUESS) {
> > >   free = global_zone_page_state(NR_FREE_PAGES);
> > >   free += global_node_page_state(NR_FILE_PAGES);
> > > + free += global_node_page_state(NR_METADATA_BYTES) >> PAGE_SHIFT;
> > 
> > 
> > I'm not really sure this is OK. It depends on whether mm is really able to
> > reclaim these pages easily enough... Summon mm people for help :)
> > 
> 
> Well we count NR_SLAB_RECLAIMABLE here, so it's no different than that.  The
> point is that it's theoretically reclaimable, and we should at least try.

I agree with including metadata in the equation. The (somewhat dusty)
overcommit code is mostly for containing swap storms, which is why it
adds up all the reclaimable pools that aren't backed by swap. The
metadata pool belongs in that category.

> > > @@ -3812,6 +3813,7 @@ static inline unsigned long 
> > > node_unmapped_file_pages(struct pglist_data *pgdat)
> > >  static unsigned long node_pagecache_reclaimable(struct pglist_data 
> > > *pgdat)
> > >  {
> > >   unsigned long nr_pagecache_reclaimable;
> > > + unsigned long nr_metadata_reclaimable;
> > >   unsigned long delta = 0;
> > >  
> > >   /*
> > > @@ -3833,7 +3835,20 @@ static unsigned long 
> > > node_pagecache_reclaimable(struct pglist_data *pgdat)
> > >   if (unlikely(delta > nr_pagecache_reclaimable))
> > >   delta = nr_pagecache_reclaimable;
> > >  
> > > - return nr_pagecache_reclaimable - delta;
> > > + nr_metadata_reclaimable =
> > > + node_page_state(pgdat, NR_METADATA_BYTES) >> PAGE_SHIFT;
> > > + /*
> > > +  * We don't do writeout through the shrinkers so subtract any
> > > +  * dirty/writeback metadata bytes from the reclaimable count.
> > > +  */
> > > + if (nr_metadata_reclaimable) {
> > > + unsigned long unreclaimable =
> > > + node_page_state(pgdat, NR_METADATA_DIRTY_BYTES) +
> > > + node_page_state(pgdat, NR_METADATA_WRITEBACK_BYTES);
> > > + unreclaimable >>= PAGE_SHIFT;
> > > + nr_metadata_reclaimable -= unreclaimable;
> > > + }
> > > + return nr_metadata_reclaimable + nr_pagecache_reclaimable - delta;
> > >  }
> > 
> > Ditto as with __vm_enough_memory(). In particular I'm unsure whether the
> > watermarks like min_unmapped_pages or min_slab_pages would still work as
> > designed.
> > 
> 
> Yeah agreed I'd like an MM person's thoughts on this as well.  We don't count
> SLAB_RECLAIMABLE here, but that's because it's just not related to pagecache. 
>  I
> guess it only matters for node reclaim and we have our node reclaim stuff 
> turned
> off, which means it doesn't help us anyway, so I'm happy to just drop it and 
> let
> somebody who cares about node reclaim think about it later ;).  Thanks,

Few people care about node reclaim at this point, see 4f9b16a64753
("mm: disable zone_reclaim_mode by default"), and it's honestly a bit
baffling why we made min_slab_ratio a tunable in the first place. Who
knows how/if anybody relies on that behavior. I'd just leave it alone.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/11] writeback: add counters for metadata usage

[Josef Bacik]

On Mon, Dec 04, 2017 at 02:06:30PM +0100, Jan Kara wrote:
> On Wed 22-11-17 16:16:01, Josef Bacik wrote:
> > From: Josef Bacik 
> > 
> > Btrfs has no bounds except memory on the amount of dirty memory that we 
> > have in
> > use for metadata.  Historically we have used a special inode so we could 
> > take
> > advantage of the balance_dirty_pages throttling that comes with using 
> > pagecache.
> > However as we'd like to support different blocksizes it would be nice to not
> > have to rely on pagecache, but still get the balance_dirty_pages throttling
> > without having to do it ourselves.
> > 
> > So introduce *METADATA_DIRTY_BYTES and *METADATA_WRITEBACK_BYTES.  These are
> > zone and bdi_writeback counters to keep track of how many bytes we have in
> > flight for METADATA.  We need to count in bytes as blocksizes could be
> > percentages of pagesize.  We simply convert the bytes to number of pages 
> > where
> > it is needed for the throttling.
> > 
> > Also introduce NR_METADATA_BYTES so we can keep track of the total amount of
> > pages used for metadata on the system.  This is also needed so things like 
> > dirty
> > throttling know that this is dirtyable memory as well and easily reclaimed.
> 
> I'll defer to mm guys for final decision but the fact is the memory for
> metadata is likely to be allocated from some slab cache and that actually
> goes against the 'easily reclaimed' statement. Granted these are going to
> be relatively large objects (1k at least I assume) so fragmentation issues
> are not as bad but still getting actual free pages out of slab cache isn't
> that easy... More on this below.
> 
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 356a814e7c8e..fd516a0f0bfe 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -179,6 +179,9 @@ enum node_stat_item {
> > NR_VMSCAN_IMMEDIATE,/* Prioritise for reclaim when writeback ends */
> > NR_DIRTIED, /* page dirtyings since bootup */
> > NR_WRITTEN, /* page writings since bootup */
> > +   NR_METADATA_DIRTY_BYTES,/* Metadata dirty bytes */
> > +   NR_METADATA_WRITEBACK_BYTES,/* Metadata writeback bytes */
> > +   NR_METADATA_BYTES,  /* total metadata bytes in use. */
> > NR_VM_NODE_STAT_ITEMS
> >  };
> 
> I think you didn't address my comment from last version of the series.
> 
> 1) Per-cpu node-stat batching will be basically useless for these counters
> as the batch size is <128. Maybe we don't care but it would deserve a
> comment.
> 
> 2) These counters are tracked in atomic_long_t type. That means max 2GB of
> metadata on 32-bit machines. I *guess* that should be OK since you would
> not be able to address that much of slab cache on such machine anyway but 
> still worth a comment I think.
> 

You're right I missed this, sorry about that.  I've resolved the batching
problem, and I'll add a comment about the 32bit machines problem.

> > diff --git a/mm/util.c b/mm/util.c
> > index 34e57fae959d..681d62631ee0 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -616,6 +616,7 @@ int __vm_enough_memory(struct mm_struct *mm, long 
> > pages, int cap_sys_admin)
> > if (sysctl_overcommit_memory == OVERCOMMIT_GUESS) {
> > free = global_zone_page_state(NR_FREE_PAGES);
> > free += global_node_page_state(NR_FILE_PAGES);
> > +   free += global_node_page_state(NR_METADATA_BYTES) >> PAGE_SHIFT;
> 
> 
> I'm not really sure this is OK. It depends on whether mm is really able to
> reclaim these pages easily enough... Summon mm people for help :)
> 

Well we count NR_SLAB_RECLAIMABLE here, so it's no different than that.  The
point is that it's theoretically reclaimable, and we should at least try.

> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 13d711dd8776..415b003e475c 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -225,7 +225,8 @@ unsigned long pgdat_reclaimable_pages(struct 
> > pglist_data *pgdat)
> >  
> > nr = node_page_state_snapshot(pgdat, NR_ACTIVE_FILE) +
> >  node_page_state_snapshot(pgdat, NR_INACTIVE_FILE) +
> > -node_page_state_snapshot(pgdat, NR_ISOLATED_FILE);
> > +node_page_state_snapshot(pgdat, NR_ISOLATED_FILE) +
> > +(node_page_state_snapshot(pgdat, NR_METADATA_BYTES) >> PAGE_SHIFT);
> >  
> > if (get_nr_swap_pages() > 0)
> > nr += node_page_state_snapshot(pgdat, NR_ACTIVE_ANON) +
> 
> Just drop this hunk. The function is going away (and is currently unused).
> 

Will do.

> > @@ -3812,6 +3813,7 @@ static inline unsigned long 
> > node_unmapped_file_pages(struct pglist_data *pgdat)
> >  static unsigned long node_pagecache_reclaimable(struct pglist_data *pgdat)
> >  {
> > unsigned long nr_pagecache_reclaimable;
> > +   unsigned long nr_metadata_reclaimable;
> > unsigned long delta = 0;
> >  
> > /*
> > @@ -3833,7 +3835,20 @@ static unsigned long 
> > node_pagecache_reclaimable(struct 

Re: [PATCH v2 06/11] writeback: add counters for metadata usage

[Jan Kara]

On Wed 22-11-17 16:16:01, Josef Bacik wrote:
> From: Josef Bacik 
> 
> Btrfs has no bounds except memory on the amount of dirty memory that we have 
> in
> use for metadata.  Historically we have used a special inode so we could take
> advantage of the balance_dirty_pages throttling that comes with using 
> pagecache.
> However as we'd like to support different blocksizes it would be nice to not
> have to rely on pagecache, but still get the balance_dirty_pages throttling
> without having to do it ourselves.
> 
> So introduce *METADATA_DIRTY_BYTES and *METADATA_WRITEBACK_BYTES.  These are
> zone and bdi_writeback counters to keep track of how many bytes we have in
> flight for METADATA.  We need to count in bytes as blocksizes could be
> percentages of pagesize.  We simply convert the bytes to number of pages where
> it is needed for the throttling.
> 
> Also introduce NR_METADATA_BYTES so we can keep track of the total amount of
> pages used for metadata on the system.  This is also needed so things like 
> dirty
> throttling know that this is dirtyable memory as well and easily reclaimed.

I'll defer to mm guys for final decision but the fact is the memory for
metadata is likely to be allocated from some slab cache and that actually
goes against the 'easily reclaimed' statement. Granted these are going to
be relatively large objects (1k at least I assume) so fragmentation issues
are not as bad but still getting actual free pages out of slab cache isn't
that easy... More on this below.

> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 356a814e7c8e..fd516a0f0bfe 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -179,6 +179,9 @@ enum node_stat_item {
>   NR_VMSCAN_IMMEDIATE,/* Prioritise for reclaim when writeback ends */
>   NR_DIRTIED, /* page dirtyings since bootup */
>   NR_WRITTEN, /* page writings since bootup */
> + NR_METADATA_DIRTY_BYTES,/* Metadata dirty bytes */
> + NR_METADATA_WRITEBACK_BYTES,/* Metadata writeback bytes */
> + NR_METADATA_BYTES,  /* total metadata bytes in use. */
>   NR_VM_NODE_STAT_ITEMS
>  };

I think you didn't address my comment from last version of the series.

1) Per-cpu node-stat batching will be basically useless for these counters
as the batch size is <128. Maybe we don't care but it would deserve a
comment.

2) These counters are tracked in atomic_long_t type. That means max 2GB of
metadata on 32-bit machines. I *guess* that should be OK since you would
not be able to address that much of slab cache on such machine anyway but 
still worth a comment I think.

> diff --git a/mm/util.c b/mm/util.c
> index 34e57fae959d..681d62631ee0 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -616,6 +616,7 @@ int __vm_enough_memory(struct mm_struct *mm, long pages, 
> int cap_sys_admin)
>   if (sysctl_overcommit_memory == OVERCOMMIT_GUESS) {
>   free = global_zone_page_state(NR_FREE_PAGES);
>   free += global_node_page_state(NR_FILE_PAGES);
> + free += global_node_page_state(NR_METADATA_BYTES) >> PAGE_SHIFT;


I'm not really sure this is OK. It depends on whether mm is really able to
reclaim these pages easily enough... Summon mm people for help :)

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 13d711dd8776..415b003e475c 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -225,7 +225,8 @@ unsigned long pgdat_reclaimable_pages(struct pglist_data 
> *pgdat)
>  
>   nr = node_page_state_snapshot(pgdat, NR_ACTIVE_FILE) +
>node_page_state_snapshot(pgdat, NR_INACTIVE_FILE) +
> -  node_page_state_snapshot(pgdat, NR_ISOLATED_FILE);
> +  node_page_state_snapshot(pgdat, NR_ISOLATED_FILE) +
> +  (node_page_state_snapshot(pgdat, NR_METADATA_BYTES) >> PAGE_SHIFT);
>  
>   if (get_nr_swap_pages() > 0)
>   nr += node_page_state_snapshot(pgdat, NR_ACTIVE_ANON) +

Just drop this hunk. The function is going away (and is currently unused).

> @@ -3812,6 +3813,7 @@ static inline unsigned long 
> node_unmapped_file_pages(struct pglist_data *pgdat)
>  static unsigned long node_pagecache_reclaimable(struct pglist_data *pgdat)
>  {
>   unsigned long nr_pagecache_reclaimable;
> + unsigned long nr_metadata_reclaimable;
>   unsigned long delta = 0;
>  
>   /*
> @@ -3833,7 +3835,20 @@ static unsigned long node_pagecache_reclaimable(struct 
> pglist_data *pgdat)
>   if (unlikely(delta > nr_pagecache_reclaimable))
>   delta = nr_pagecache_reclaimable;
>  
> - return nr_pagecache_reclaimable - delta;
> + nr_metadata_reclaimable =
> + node_page_state(pgdat, NR_METADATA_BYTES) >> PAGE_SHIFT;
> + /*
> +  * We don't do writeout through the shrinkers so subtract any
> +  * dirty/writeback metadata bytes from the reclaimable count.
> +  */
> + if (nr_metadata_reclaimable) {
> + unsigned long unreclaimable =
> 

[PATCH v2 06/11] writeback: add counters for metadata usage

[Josef Bacik]

From: Josef Bacik 

Btrfs has no bounds except memory on the amount of dirty memory that we have in
use for metadata.  Historically we have used a special inode so we could take
advantage of the balance_dirty_pages throttling that comes with using pagecache.
However as we'd like to support different blocksizes it would be nice to not
have to rely on pagecache, but still get the balance_dirty_pages throttling
without having to do it ourselves.

So introduce *METADATA_DIRTY_BYTES and *METADATA_WRITEBACK_BYTES.  These are
zone and bdi_writeback counters to keep track of how many bytes we have in
flight for METADATA.  We need to count in bytes as blocksizes could be
percentages of pagesize.  We simply convert the bytes to number of pages where
it is needed for the throttling.

Also introduce NR_METADATA_BYTES so we can keep track of the total amount of
pages used for metadata on the system.  This is also needed so things like dirty
throttling know that this is dirtyable memory as well and easily reclaimed.

Signed-off-by: Josef Bacik 
---
 drivers/base/node.c  |   8 +++
 fs/fs-writeback.c|   2 +
 fs/proc/meminfo.c|   8 +++
 include/linux/backing-dev-defs.h |   2 +
 include/linux/mm.h   |   9 +++
 include/linux/mmzone.h   |   3 +
 include/trace/events/writeback.h |  13 +++-
 mm/backing-dev.c |   4 ++
 mm/page-writeback.c  | 142 +++
 mm/page_alloc.c  |  20 --
 mm/util.c|   1 +
 mm/vmscan.c  |  19 +-
 mm/vmstat.c  |   3 +
 13 files changed, 212 insertions(+), 22 deletions(-)

diff --git a/drivers/base/node.c b/drivers/base/node.c
index 3855902f2c5b..a39cecc8957a 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -51,6 +51,8 @@ static DEVICE_ATTR(cpumap,  S_IRUGO, node_read_cpumask, NULL);
 static DEVICE_ATTR(cpulist, S_IRUGO, node_read_cpulist, NULL);
 
 #define K(x) ((x) << (PAGE_SHIFT - 10))
+#define BtoK(x) ((x) >> 10)
+
 static ssize_t node_read_meminfo(struct device *dev,
struct device_attribute *attr, char *buf)
 {
@@ -99,7 +101,10 @@ static ssize_t node_read_meminfo(struct device *dev,
 #endif
n += sprintf(buf + n,
   "Node %d Dirty:  %8lu kB\n"
+  "Node %d MetadataDirty:  %8lu kB\n"
   "Node %d Writeback:  %8lu kB\n"
+  "Node %d MetaWriteback:  %8lu kB\n"
+  "Node %d Metadata:   %8lu kB\n"
   "Node %d FilePages:  %8lu kB\n"
   "Node %d Mapped: %8lu kB\n"
   "Node %d AnonPages:  %8lu kB\n"
@@ -119,8 +124,11 @@ static ssize_t node_read_meminfo(struct device *dev,
 #endif
,
   nid, K(node_page_state(pgdat, NR_FILE_DIRTY)),
+  nid, BtoK(node_page_state(pgdat, 
NR_METADATA_DIRTY_BYTES)),
   nid, K(node_page_state(pgdat, NR_WRITEBACK)),
+  nid, BtoK(node_page_state(pgdat, 
NR_METADATA_WRITEBACK_BYTES)),
   nid, K(node_page_state(pgdat, NR_FILE_PAGES)),
+  nid, BtoK(node_page_state(pgdat, NR_METADATA_BYTES)),
   nid, K(node_page_state(pgdat, NR_FILE_MAPPED)),
   nid, K(node_page_state(pgdat, NR_ANON_MAPPED)),
   nid, K(i.sharedram),
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 245c430a2e41..987448ed7698 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1814,6 +1814,7 @@ static struct wb_writeback_work 
*get_next_work_item(struct bdi_writeback *wb)
return work;
 }
 
+#define BtoP(x) ((x) >> PAGE_SHIFT)
 /*
  * Add in the number of potentially dirty inodes, because each inode
  * write can dirty pagecache in the underlying blockdev.
@@ -1822,6 +1823,7 @@ static unsigned long get_nr_dirty_pages(void)
 {
return global_node_page_state(NR_FILE_DIRTY) +
global_node_page_state(NR_UNSTABLE_NFS) +
+   BtoP(global_node_page_state(NR_METADATA_DIRTY_BYTES)) +
get_nr_dirty_inodes();
 }
 
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index cdd979724c74..fa1fd24a4d99 100644
--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -42,6 +42,8 @@ static void show_val_kb(struct seq_file *m, const char *s, 
unsigned long num)
seq_write(m, " kB\n", 4);
 }
 
+#define BtoP(x) ((x) >> PAGE_SHIFT)
+
 static int meminfo_proc_show(struct seq_file *m, void *v)
 {
struct sysinfo i;
@@ -71,6 +73,8 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
show_val_kb(m, "Buffers:", i.bufferram);
show_val_kb(m, "Cached: ", cached);
show_val_kb(m, "SwapCached: ", total_swapcache_pages());
+   show_val_kb(m, "Metadata:   ",