Re: [PATCH 9/9] libxfs: keep unflushable buffers off the cache MRUs

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 05, 2016 at 01:34:17PM -0500, Brian Foster wrote:
> On Tue, Dec 22, 2015 at 08:37:09AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > There's no point trying to free buffers that are dirty and return
> > errors on flush as we have to keep them around until the corruption
> > is fixed. Hence if we fail to flush an inode during a cache shake,
> > move the buffer to a special dirty MRU list that the cache does not
> > shake. This prevents memory pressure from seeing these buffers, but
> > allows subsequent cache lookups to still find them through the hash.
> > This ensures we don't waste huge amounts of CPU trying to flush and
> > reclaim buffers that canot be flushed or reclaimed.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  include/cache.h |  3 ++-
> >  libxfs/cache.c  | 34 +++++++++++++++++++++++++++++-----
> >  2 files changed, 31 insertions(+), 6 deletions(-)
> > 
> ...
> > diff --git a/libxfs/cache.c b/libxfs/cache.c
> > index a48ebd9..d5ea461 100644
> > --- a/libxfs/cache.c
> > +++ b/libxfs/cache.c
> ...
> > @@ -202,10 +223,11 @@ cache_shake(
> >  	struct cache_node *	node;
> >  	unsigned int		count;
> >  
> > -	ASSERT(priority <= CACHE_MAX_PRIORITY);
> > -	if (priority > CACHE_MAX_PRIORITY)
> > +	ASSERT(priority <= CACHE_DIRTY_PRIORITY);
> > +	if (priority > CACHE_MAX_PRIORITY && !all)
> >  		priority = 0;
> >  
> > +
> 
> Extra newline. FWIW, it also looks like the only cache_shake() caller
> where all == 0 already prevents calling with priority >
> CACHE_MAX_PRIORITY. I think a brief comment in one or both places with
> regard to why >max priority is skipped unless 'all == 1' would be good,
> though.

OK, can add.

> Also, it looks like the loop in cache_report() could be updated to dump
> the dirty priority mru entry count.

OK.

> 
> Finally, what happens once a buffer on the dirty mru is fully repaired,
> rewritten and released? Is it placed right back on the "unshakeable"
> dirty mru or is cn_priority updated somewhere? On further digging, it
> looks like a subsequent buffer lookup (__cache_lookup()) drops the
> priority, though it appears to be designed to deal with prefetched
> buffers and the associated B_INODE..B_DIR_BMAP mappings.

Somehow I missed sending the last patch in the series which
addresses this exact issue (libxfs: reset dirty buffer priority on
lookup) by saving the priority when the buffer is moved to the dirty
MRU and restoring it when the buffer is removed from the dirty MRU
on the next successful lookup. I'll make sure that's in the updated
patch series ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs