xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH 5/7] xfs: introduce a per-ag inode iterator
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 4 Jun 2009 13:17:26 -0400
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <4A26F6B1.20509@xxxxxxxxxxx>
References: <20090514171233.942489000@xxxxxxxxxxxxxxxxxxxxxx> <20090514171558.869514000@xxxxxxxxxxxxxxxxxxxxxx> <4A26F6B1.20509@xxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Wed, Jun 03, 2009 at 05:18:25PM -0500, Eric Sandeen wrote:
> Ok, it's looking for EAGAIN here, I'm assuming this is for when we are
> calling xfs_reclaim_inode_now, because...
> 
> ...
> ... 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:

Yeah.  Updated patch below that besides addressing the other comments
makes xfs_reclaim_inode return -EAGAIN if it has to skip an inode.

Subject: xfs: introduce a per-ag inode iterator
From: Dave Chinner <david@xxxxxxxxxxxxx>

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.

Also change xfs_reclaim_inode to return -EGAIN instead of 1 for an inode
already under reclaim.  This simplifies the AG iterator and doesn't
matter for the only other caller.

[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>

Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c        2009-06-04 12:50:25.380940755 
+0200
+++ xfs/fs/xfs/linux-2.6/xfs_sync.c     2009-06-04 13:09:06.199942249 +0200
@@ -49,6 +49,123 @@
 #include <linux/freezer.h>
 
 
+STATIC xfs_inode_t *
+xfs_inode_ag_lookup(
+       struct xfs_mount        *mp,
+       struct xfs_perag        *pag,
+       uint32_t                *first_index,
+       int                     tag)
+{
+       int                     nr_found;
+       struct xfs_inode        *ip;
+
+       /*
+        * use a gang lookup to find the next inode in the tree
+        * as the tree is sparse and a gang lookup walks to find
+        * the number of objects requested.
+        */
+       read_lock(&pag->pag_ici_lock);
+       if (tag == -1) {
+               nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
+                               (void **)&ip, *first_index, 1);
+       } else {
+               nr_found = radix_tree_gang_lookup_tag(&pag->pag_ici_root,
+                               (void **)&ip, *first_index, 1, tag);
+       }
+       if (!nr_found)
+               goto unlock;
+
+       /*
+        * 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))
+               goto unlock;
+
+       return ip;
+
+unlock:
+       read_unlock(&pag->pag_ici_lock);
+       return NULL;
+}
+
+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;
+
+       } while (1);
+
+       if (skipped) {
+               delay(1);
+               goto restart;
+       }
+
+       xfs_put_perag(mp, pag);
+       return last_error;
+}
+
+STATIC int
+xfs_inode_ag_iterator(
+       struct xfs_mount        *mp,
+       int                     (*execute)(struct xfs_inode *ip,
+                                          struct xfs_perag *pag, int flags),
+       int                     flags,
+       int                     tag)
+{
+       int                     error = 0;
+       int                     last_error = 0;
+       xfs_agnumber_t          ag;
+
+       for (ag = 0; ag < mp->m_sb.sb_agcount; ag++) {
+               if (!mp->m_perag[ag].pag_ici_init)
+                       continue;
+               error = xfs_inode_ag_walk(mp, ag, execute, flags, tag);
+               if (error) {
+                       last_error = error;
+                       if (error == EFSCORRUPTED)
+                               break;
+               }
+       }
+       return XFS_ERROR(last_error);
+}
+
 /* must be called with pag_ici_lock held and releases it */
 STATIC int
 xfs_sync_inode_valid(
@@ -85,12 +202,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 error;
+
        if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
                goto out_wait;
 
@@ -107,16 +229,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 error;
+
        xfs_ilock(ip, XFS_ILOCK_SHARED);
        if (xfs_inode_clean(ip))
                goto out_unlock;
@@ -136,117 +264,33 @@ xfs_sync_inode_attr(
 
  out_unlock:
        xfs_iunlock(ip, XFS_ILOCK_SHARED);
+       IRELE(ip);
        return error;
 }
 
-/*
- * Sync all the inodes in the given AG according to the
- * direction given by the flags.
- */
-STATIC int
-xfs_sync_inodes_ag(
-       xfs_mount_t     *mp,
-       int             ag,
-       int             flags)
-{
-       xfs_perag_t     *pag = &mp->m_perag[ag];
-       int             nr_found;
-       uint32_t        first_index = 0;
-       int             error = 0;
-       int             last_error = 0;
-
-       do {
-               xfs_inode_t     *ip = NULL;
-
-               /*
-                * use a gang lookup to find the next inode in the tree
-                * as the tree is sparse and a gang lookup walks to find
-                * the number of objects requested.
-                */
-               read_lock(&pag->pag_ici_lock);
-               nr_found = radix_tree_gang_lookup(&pag->pag_ici_root,
-                               (void**)&ip, first_index, 1);
-
-               if (!nr_found) {
-                       read_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)) {
-                       read_unlock(&pag->pag_ici_lock);
-                       break;
-               }
-
-               error = xfs_sync_inode_valid(ip, pag);
-               if (error) {
-                       if (error == EFSCORRUPTED)
-                               return 0;
-                       continue;
-               }
-
-               /*
-                * If we have to flush data or wait for I/O completion
-                * we need to hold the iolock.
-                */
-               if (flags & SYNC_DELWRI)
-                       error = xfs_sync_inode_data(ip, flags);
-
-               if (flags & SYNC_ATTR)
-                       error = xfs_sync_inode_attr(ip, flags);
-
-               IRELE(ip);
-
-               if (error)
-                       last_error = error;
-               /*
-                * bail out if the filesystem is corrupted.
-                */
-               if (error == EFSCORRUPTED)
-                       return XFS_ERROR(error);
-
-       } while (nr_found);
-
-       return last_error;
-}
-
 int
 xfs_sync_inodes(
        xfs_mount_t     *mp,
        int             flags)
 {
-       int             error;
-       int             last_error;
-       int             i;
+       int             error = 0;
        int             lflags = XFS_LOG_FORCE;
 
        if (mp->m_flags & XFS_MOUNT_RDONLY)
                return 0;
-       error = 0;
-       last_error = 0;
 
        if (flags & SYNC_WAIT)
                lflags |= XFS_LOG_SYNC;
 
-       for (i = 0; i < mp->m_sb.sb_agcount; i++) {
-               if (!mp->m_perag[i].pag_ici_init)
-                       continue;
-               error = xfs_sync_inodes_ag(mp, i, flags);
-               if (error)
-                       last_error = error;
-               if (error == EFSCORRUPTED)
-                       break;
-       }
        if (flags & SYNC_DELWRI)
-               xfs_log_force(mp, 0, lflags);
+               error = xfs_inode_ag_iterator(mp, xfs_sync_inode_data, flags, 
-1);
 
-       return XFS_ERROR(last_error);
+       if (flags & SYNC_ATTR)
+               error = xfs_inode_ag_iterator(mp, xfs_sync_inode_attr, flags, 
-1);
+
+       if (!error && (flags & SYNC_DELWRI))
+               xfs_log_force(mp, 0, lflags);
+       return XFS_ERROR(error);
 }
 
 STATIC int
@@ -613,7 +657,7 @@ xfs_reclaim_inode(
                        xfs_ifunlock(ip);
                        xfs_iunlock(ip, XFS_ILOCK_EXCL);
                }
-               return 1;
+               return -EAGAIN;
        }
        __xfs_iflags_set(ip, XFS_IRECLAIM);
        spin_unlock(&ip->i_flags_lock);
@@ -698,72 +742,20 @@ xfs_inode_clear_reclaim_tag(
        xfs_put_perag(mp, pag);
 }
 
-
-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 {
-               /*
-                * use a gang lookup to find the next inode in the tree
-                * as the tree is sparse and a gang lookup walks to find
-                * the number of objects requested.
-                */
-               read_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) {
-                       read_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)) {
-                       read_unlock(&pag->pag_ici_lock);
-                       break;
-               }
-
-               /* ignore if already under reclaim */
-               if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
-                       read_unlock(&pag->pag_ici_lock);
-                       continue;
-               }
-
+       /* ignore if already under reclaim */
+       if (xfs_iflags_test(ip, XFS_IRECLAIM)) {
                read_unlock(&pag->pag_ici_lock);
-
-               /*
-                * 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);
-
-       if (skipped) {
-               delay(1);
-               goto restart;
+               return 0;
        }
-       return;
+       read_unlock(&pag->pag_ici_lock);
 
+       return xfs_reclaim_inode(ip, 0, flags);
 }
 
 int
@@ -771,14 +763,6 @@ xfs_reclaim_inodes(
        xfs_mount_t     *mp,
        int             mode)
 {
-       int             i;
-
-       for (i = 0; i < mp->m_sb.sb_agcount; i++) {
-               if (!mp->m_perag[i].pag_ici_init)
-                       continue;
-               xfs_reclaim_inodes_ag(mp, i, mode);
-       }
-       return 0;
+       return xfs_inode_ag_iterator(mp, xfs_reclaim_inode_now, mode,
+                                       XFS_ICI_RECLAIM_TAG);
 }
-
-

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