xfs
[Top] [All Lists]

Re: [PATCH 16/18] xfs: serialise inode reclaim within an AG

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 16/18] xfs: serialise inode reclaim within an AG
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 27 Sep 2010 10:56:13 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100925234929.GB23975@xxxxxxxxxxxxx>
References: <1285331476-23015-1-git-send-email-david@xxxxxxxxxxxxx> <1285331476-23015-17-git-send-email-david@xxxxxxxxxxxxx> <20100925234929.GB23975@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Sat, Sep 25, 2010 at 07:49:29PM -0400, Christoph Hellwig wrote:
> I really don't like the way the "trylock" variable is overloaded here.
> Just add a new skipped variable for restarting the scan and otherwise
> use (flags & SYNC_TRYLOCK) directly.
> 
> > +   int                     trylock = !!(flags & SYNC_TRYLOCK);
> >  
> > +restart:
> >     ag = 0;
> >     while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_RECLAIM_TAG))) {
> >             unsigned long   first_index = 0;
> > @@ -837,6 +839,17 @@ xfs_reclaim_inodes_ag(
> >  
> >             ag = pag->pag_agno + 1;
> >  
> > +           if (!mutex_trylock(&pag->pag_ici_reclaim_lock)) {
> > +                   if (trylock) {
> > +                           trylock++;
> > +                           continue;
> > +                   }
> > +                   mutex_lock(&pag->pag_ici_reclaim_lock);
> > +           }
> > +
> > +           if (trylock)
> > +                   first_index = pag->pag_ici_reclaim_cursor;
> 
> Also this could be made more clear by:
> 
>       if (flags & SYNC_TRYLOCK) {
>               if (!mutex_trylock(&pag->pag_ici_reclaim_lock)) {
>                       skipped++;
>                       continue;
>               }
> 
>               first_index = pag->pag_ici_reclaim_cursor;
>       } else {
>               mutex_lock(&pag->pag_ici_reclaim_lock);
>       }

I'll leave the trylock variable, but add a skipped variable. The
trylock two-pass algorithm (where the second pass goes into full
blocking reclaim mode) is intentional to ensure that we block
shrinker calls when there are more shrinkers than AGs rather than
spinning just trying to get per-ag reclaim locks. Regardless, I'll
clean up the code like you suggest because it is neater.

> 
> > +
> >             do {
> >                     struct xfs_inode *batch[XFS_LOOKUP_BATCH];
> >                     int     i;
> > @@ -889,8 +902,19 @@ xfs_reclaim_inodes_ag(
> >  
> >             } while (nr_found && !done && *nr_to_scan > 0);
> >  
> > +           pag->pag_ici_reclaim_cursor = (done || !trylock) ? 0 : 
> > first_index;
> 
>               if ((flags & SYNC_TRYLOCK) && !done)
>                       pag->pag_ici_reclaim_cursor = first_index;
>               else
>                       pag->pag_ici_reclaim_cursor = 0;
> 
> > +   /*
> > +    * if we skipped any AG, and we still have scan count remaining, do
> > +    * another pass this time waiting on the reclaim locks.
> > +    */
> > +   if (trylock > 1 && *nr_to_scan) {
> > +           trylock = 0;
> > +           goto restart;
> > +   }
> 
> In addition to waiting on the lock this also ignores the reclaim cursor.

Fixed the comment to indicate this is intentіonal.

Cheers,

Dave.

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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