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:01:13 -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>

Somehow I'm finding this hard to review, but...

> 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;
> +             }
> +             if (error)
> +                     last_error = error;
> +             /*
> +              * bail out if the filesystem is corrupted.
> +              */
> +             if (error == EFSCORRUPTED)
> +                     break;

Ok so here we are looking for EFSCORRUPTED from the "execute" function.
 This might be xfs_sync_inode_data, xfs_sync_inode_attr, or
xfs_reclaim_inode_now.  But ...

> +
> +     } while (1);

...

> @@ -85,12 +201,17 @@ xfs_sync_inode_valid(
>  STATIC int
>  xfs_sync_inode_data(
>       struct xfs_inode        *ip,
> +     struct xfs_perag        *pag,
>       int                     flags)
>  {
>       struct inode    *inode = VFS_I(ip);
>       struct address_space *mapping = inode->i_mapping;
>       int             error = 0;
>  
> +     error = xfs_sync_inode_valid(ip, pag);
> +     if (error)
> +             return 0;xfs_sync_inode_attr(
> +

xfs_sync_inode_valid can return 0, ENOENT, or EFSCORRUPTED.

Aren't we losing the error here...

>       if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
>               if (!xfs_ilock_nowait(ip, XFS_IOLOCK_SHARED)) {
>                       if (flags & SYNC_TRYLOCK)
> @@ -106,16 +227,22 @@ xfs_sync_inode_data(
>   out_wait:
>       if (flags & SYNC_IOWAIT)
>               xfs_ioend_wait(ip);
> +     IRELE(ip);
>       return error;
>  }
>  
>  STATIC int
>  xfs_sync_inode_attr(
>       struct xfs_inode        *ip,
> +     struct xfs_perag        *pag,
>       int                     flags)
>  {
>       int                     error = 0;
>  
> +     error = xfs_sync_inode_valid(ip, pag);
> +     if (error)
> +             return 0;

and here?

so xfs_sync_inode_data / xfs_sync_inode_attr are the "execute" in
xfs_inode_ag_walk():

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

above, and I think they're ignoring the return from
xfs_sync_inode_valid(), therefore xfs_inode_ag_walk won't see
EFSCORRUPTED from it either ... right?

-Eric

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