Re: [PATCH 3/5] writeback: add counters for metadata usage

[Tejun Heo <htejun@fb.com>]

Hello, Josef.

On Wed, Oct 26, 2016 at 11:20:16AM -0400, Josef Bacik wrote:
> > > @@ -3701,7 +3703,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.
> > > +	 */
> > 
> > Hmm... up until this point, the dirty metadata was handled the same
> > way as regular dirty data but it deviates here.  Is this right?  The
> > calculations in writeback code also assumes that the dirty pages are
> > reclaimable.  If this is inherently different, it'd be nice to explain
> > more explicitly why this is different from others.
> 
> So there is logic above this that subtracts out the NR_FILE_DIRTY from the
> file reclaimable if we can't do write's during reclaim.  Since we can always
> wait on writeback during reclaim it doesn't subtract out writeback.  I took
> this to mean that the general expectation of this function is to only count
> thing things that the shrinker can specifically reclaim itself, so I
> discounted anything under io since the slab shrinkers have no idea if its ok
> to write or not and so in the case of btrfs simply skip anything that is
> dirty or under writeback.  Does that make sense?  I'll fix up the other
> issues you pointed out.  Thanks,

Yeap, that makes sense to me.

Thanks for the explanation.

-- 
tejun