Re: [PATCH 09/10] repair: prefetch runs too far ahead

From: Brian Foster <bfoster@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 09/10] repair: prefetch runs too far ahead
Date: Thu, 27 Feb 2014 15:45:40 -0500	[thread overview]
Message-ID: <20140227204539.GA27267@bfoster.bfoster> (raw)
In-Reply-To: <20140227202405.GE30131@dastard>

On Fri, Feb 28, 2014 at 07:24:05AM +1100, Dave Chinner wrote:
> On Fri, Feb 28, 2014 at 07:01:50AM +1100, Dave Chinner wrote:
> > On Thu, Feb 27, 2014 at 09:08:46AM -0500, Brian Foster wrote:
> > > On Thu, Feb 27, 2014 at 08:51:14PM +1100, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > 
> > > Hmm, I replied to this one in the previous thread, but now I notice that
> > > it apparently never made it to the list. Dave, did you happen to see
> > > that in your inbox? Anyways, I had a couple minor comments/questions
> > > that I'll duplicate here (which probably don't require another
> > > repost)...
> > 
> > No, I didn't.
> > 
> > [snip typos that need fixing]
> > 
> > > > diff --git a/repair/prefetch.c b/repair/prefetch.c
> > > > index aee6342..7d3efde 100644
> > > > --- a/repair/prefetch.c
> > > > +++ b/repair/prefetch.c
> > > > @@ -866,6 +866,48 @@ start_inode_prefetch(
> > > >  	return args;
> > > >  }
> > > >  
> > > 
> > > A brief comment before the prefetch_ag_range bits that explain the
> > > implicit design constraints (e.g., throttle prefetch based on
> > > processing) would be nice. :)
> > 
> > Can do.
> 
> Added this:
> 
> /*
>  * prefetch_ag_range runs a prefetch-and-process loop across a range of AGs. It
>  * begins with @start+ag, and finishes with @end_ag - 1 (i.e. does not prefetch
>  * or process @end_ag). The function starts prefetch on the first AG, then loops
>  * starting prefetch on the next AG and then blocks processing the current AG as
>  * the prefetch queue brings inodes into the processing queue.
>  *
>  * There is only one prefetch taking place at a time, so the prefetch on the
>  * next AG only starts once the current AG has been completely prefetched. Hence
>  * the prefetch of the next AG will start some time before the processing of the
>  * current AG finishes, ensuring that when we iterate an start processing the
							and
>  * next AG there is already a significant queue of inodes to process.
>  *
>  * Prefetch is done this way to prevent it from running too far ahead of the
>  * processing. Allowing it to do so can cause cache thrashing, where new
>  * prefetch causes previously prefetched buffers to be reclaimed before the
>  * processing thread uses them. This results in reading all the inodes and
>  * metadata twice per phase and it greatly slows down the processing. Hence we
>  * have to carefully control how far ahead we prefetch...
>  */
> 

Looks good, thanks!

Reviewed-by: Brian Foster <bfoster@redhat.com>

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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