xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 07/10] repair: prefetch runs too far ahead
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 26 Feb 2014 16:51:09 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140226015250.GB3616@xxxxxxxxxxxxx>
References: <1393223369-4696-1-git-send-email-david@xxxxxxxxxxxxx> <1393223369-4696-8-git-send-email-david@xxxxxxxxxxxxx> <20140226015250.GB3616@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Feb 25, 2014 at 05:52:50PM -0800, Christoph Hellwig wrote:
> On Mon, Feb 24, 2014 at 05:29:26PM +1100, Dave Chinner wrote:
> > @@ -842,7 +842,7 @@ start_inode_prefetch(
> >      * and not any other associated metadata like directories
> >      */
> >  
> > -   max_queue = libxfs_bcache->c_maxcount / thread_count / 8;
> > +   max_queue = libxfs_bcache->c_maxcount / thread_count / 32;
> 
> I can't correlate this to anything mentioned in the changelog.
> Also if you're touching it anyway it might be a good idea to document
> the magic number here.

I was fiddling with the magic number to see if it affected the
readahead behaviour (it didn't) and forgot to set it back to the
original value. Will fix.

> 
> > +void
> > +prefetch_ag_range(
> > +   struct work_queue       *work,
> > +   xfs_agnumber_t          start_ag,
> > +   xfs_agnumber_t          end_ag,
> > +   bool                    dirs_only,
> > +   void                    (*func)(struct work_queue *,
> > +                                   xfs_agnumber_t, void *))
> > +{
> > +   int                     i;
> > +   struct prefetch_args    *pf_args[2];
> > +
> > +   pf_args[start_ag & 1] = start_inode_prefetch(start_ag, dirs_only, NULL);
> > +   for (i = start_ag; i < end_ag; i++) {
> > +           /* Don't prefetch end_ag */
> > +           if (i + 1 < end_ag)
> > +                   pf_args[(~i) & 1] = start_inode_prefetch(i + 1,
> > +                                           dirs_only, pf_args[i & 1]);
> > +           func(work, i, pf_args[i & 1]);
> > +   }
> > +}
> 
> This seems to largely duplicate the common code added in patch 5.
> Having _range variants of those that the non-range ones wrap with 0 and
> mp->m_sb.sb_agcount as default parameters would avoid that duplication.

Actually, that's pretty much what this patch does in this hunk:

@@ -905,12 +945,8 @@ do_inode_prefetch(
         */
        if (!stride) {
                queue.mp = mp;
-               pf_args[0] = start_inode_prefetch(0, dirs_only, NULL);
-               for (i = 0; i < mp->m_sb.sb_agcount; i++) {
-                       pf_args[(~i) & 1] = start_inode_prefetch(i + 1,
-                                       dirs_only, pf_args[i & 1]);
-                       func(&queue, i, pf_args[i & 1]);
-               }
+               prefetch_ag_range(&queue, 0, mp->m_sb.sb_agcount,
+                                 dirs_only, func);
                return;
        }


Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>