xfs
[Top] [All Lists]

Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Wed, 03 Jun 2009 17:18:25 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20090514171558.869514000@xxxxxxxxxxxxxxxxxxxxxx>
References: <20090514171233.942489000@xxxxxxxxxxxxxxxxxxxxxx> <20090514171558.869514000@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.21 (X11/20090320)
Christoph Hellwig wrote:

> From: Dave Chinner <david@xxxxxxxxxxxxx>
> 
> Given that we walk across the per-ag inode lists so often, it makes sense to
> introduce an iterator for this.
> 
> Convert the sync and reclaim code to use this new iterator, quota code will
> follow in the next patch.
> 
> [hch: merged the lookup and execute callbacks back into one to get the
>  pag_ici_lock locking correct and simplify the code flow]
> 
> Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

And a similar error handling question...

> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c      2009-05-14 16:20:37.012658983 
> +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c   2009-05-14 16:22:26.321659103 +0200

...

> +STATIC int
> +xfs_inode_ag_walk(
> +     struct xfs_mount        *mp,
> +     xfs_agnumber_t          ag,
> +     int                     (*execute)(struct xfs_inode *ip,
> +                                        struct xfs_perag *pag, int flags),
> +     int                     flags,
> +     int                     tag)
> +{
> +     struct xfs_perag        *pag = &mp->m_perag[ag];
> +     uint32_t                first_index;
> +     int                     last_error = 0;
> +     int                     skipped;
> +
> +restart:
> +     skipped = 0;
> +     first_index = 0;
> +     do {
> +             int             error = 0;
> +             xfs_inode_t     *ip;
> +
> +             ip = xfs_inode_ag_lookup(mp, pag, &first_index, tag);
> +             if (!ip)
> +                     break;
> +
> +             error = execute(ip, pag, flags);
> +             if (error == EAGAIN) {
> +                     skipped++;
> +                     continue;
> +             }

Ok, it's looking for EAGAIN here, I'm assuming this is for when we are
calling xfs_reclaim_inode_now, because...

...

> -STATIC void
> -xfs_reclaim_inodes_ag(
> -     xfs_mount_t     *mp,
> -     int             ag,
> -     int             mode)
> +STATIC int
> +xfs_reclaim_inode_now(
> +     struct xfs_inode        *ip,
> +     struct xfs_perag        *pag,
> +     int                     flags)
>  {
> -     xfs_inode_t     *ip = NULL;
> -     xfs_perag_t     *pag = &mp->m_perag[ag];
> -     int             nr_found;
> -     uint32_t        first_index;
> -     int             skipped;
> -
> -restart:
> -     first_index = 0;
> -     skipped = 0;
> -     do {

...

> -
> -             /*
> -              * hmmm - this is an inode already in reclaim. Do
> -              * we even bother catching it here?
> -              */
> -             if (xfs_reclaim_inode(ip, 0, mode))
> -                     skipped++;
> -     } while (nr_found);

... because before, that's what we did above, after testing for a non-0
return from xfs_reclaim_inode.

But xfs_reclaim_inode_now() returns 0 or the result of
xfs_reclaim_inode, which is 0/1, so above:

> +             error = execute(ip, pag, flags);
> +             if (error == EAGAIN) {
> +                     skipped++;
> +                     continue;
> +             }

isn't going to see EAGAIN from xfs_reclaim_inode_now... am I following
this right?

-Eric

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