xfs
[Top] [All Lists]

Re: [PATCH 11/16] xfs: split inode AG walking into separate code for rec

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 11/16] xfs: split inode AG walking into separate code for reclaim
From: Alex Elder <aelder@xxxxxxx>
Date: Thu, 23 Sep 2010 11:45:52 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1285137869-10310-12-git-send-email-david@xxxxxxxxxxxxx>
References: <1285137869-10310-1-git-send-email-david@xxxxxxxxxxxxx> <1285137869-10310-12-git-send-email-david@xxxxxxxxxxxxx>
Reply-to: aelder@xxxxxxx
On Wed, 2010-09-22 at 16:44 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The reclaim walk requires different locking and has a slightly
> different walk algorithm, so separate it out so that it can be
> optimised separately.

You assert this above xfs_reclaim_inodes_ag():
 Walk the AGs and reclaim the inodes in them. Even if the filesystem is
 corrupted, we still want to try to reclaim all the inodes.

I don't refute that statement, but if I'm not mistaken
this is new handling.  I just felt it should not slip in
without being mentioned.  I also suggest a short sentence
along with that comment to justify it might be in order.

I have one other minor comment below, but otherwise this
looks good.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  fs/xfs/linux-2.6/xfs_sync.c    |  200 
> ++++++++++++++++++----------------------
>  fs/xfs/linux-2.6/xfs_sync.h    |    2 +-
>  fs/xfs/linux-2.6/xfs_trace.h   |    2 +-
>  fs/xfs/quota/xfs_qm_syscalls.c |    3 +-
>  fs/xfs/xfs_mount.c             |   26 +++++
>  fs/xfs/xfs_mount.h             |    2 +
>  6 files changed, 120 insertions(+), 115 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
> index ddeaff9..7737a13 100644
> --- a/fs/xfs/linux-2.6/xfs_sync.c
> +++ b/fs/xfs/linux-2.6/xfs_sync.c

. . .

> @@ -859,13 +781,70 @@ reclaim:
>  
>  }
>  
> +/*
> + * Walk the AGs and reclaim the inodes in them. Even if the filesystem is
> + * corrupted, we still want to try to reclaim all the inodes.
> + */
> +int
> +xfs_reclaim_inodes_ag(
> +     struct xfs_mount        *mp,
> +     int                     flags,
> +     int                     *nr_to_scan)
> +{
> +     struct xfs_perag        *pag;
> +     int                     error = 0;
> +     int                     last_error = 0;
> +     xfs_agnumber_t          ag;
> +
> +     ag = 0;
> +     while ((pag = xfs_perag_get_tag(mp, ag, XFS_ICI_RECLAIM_TAG))) {
> +             unsigned long   first_index = 0;
> +             int             done = 0;
> +
> +             ag = pag->pag_agno + 1;
> +
> +             do {
> +                     struct xfs_inode *ip;
> +                     int     nr_found;
> +
> +                     write_lock(&pag->pag_ici_lock);
> +                     nr_found = 
> radix_tree_gang_lookup_tag(&pag->pag_ici_root,
> +                                     (void **)&ip, first_index, 1,
> +                                     XFS_ICI_RECLAIM_TAG);
> +                     if (!nr_found) {
> +                             write_unlock(&pag->pag_ici_lock);
> +                             break;
> +                     }
> +
> +                     /*
> +                      * Update the index for the next lookup. Catch overflows
> +                      * into the next AG range which can occur if we have 
> inodes
> +                      * in the last block of the AG and we are currently
> +                      * pointing to the last inode.
> +                      */
> +                     first_index = XFS_INO_TO_AGINO(mp, ip->i_ino + 1);
> +                     if (first_index < XFS_INO_TO_AGINO(mp, ip->i_ino))
> +                             done = 1;
> +
> +                     error = xfs_reclaim_inode(ip, pag, flags);
> +                     if (error && last_error != EFSCORRUPTED)
> +                             last_error = error;
> +
> +             } while (!done && (*nr_to_scan)--);

I know you guys disagreed with me before on this point.
But since there is no reason anybody outside this function
has a need to see the intermediate values of *nr_to_scan,
you should use a local variable to keep track of it instead,
and then assign its final value to *nr_to_scan before returning.
It allows registers to be used rather than updating real
memory at each iteration.

> +
> +             xfs_perag_put(pag);
> +     }
> +     return XFS_ERROR(last_error);
> +}
> +
>  int
>  xfs_reclaim_inodes(
>       xfs_mount_t     *mp,
>       int             mode)
>  {

. . .

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