xfs
[Top] [All Lists]

RE: [PATCH] xfs: simplify inode teardown

To: "Christoph Hellwig" <hch@xxxxxxxxxxxxx>
Subject: RE: [PATCH] xfs: simplify inode teardown
From: "Alex Elder" <aelder@xxxxxxx>
Date: Wed, 7 Oct 2009 17:56:57 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20090929134856.GA17864@xxxxxxxxxxxxx>
Thread-index: AcpBEtCrhjZMIKDOS3S9Y9Pusa3rlAGjorGA
Thread-topic: [PATCH] xfs: simplify inode teardown
Christoph Hellwig wrote:
> Currently the reclaim code for the case where we don't reclaim the final
> reclaim is overly complicated.  We know that the inode is clean but instead
> of just directly reclaiming the clean inode we go through the whole
> process of marking the inode reclaimable just to directly reclaim it
> from the calling context.  Besides beeing overly complicated this introduces
> a race where iget could recycle an inode between marked reclaimable and
> actually beeing reclaimed leading to panics.
> 
> This patch gets rid of the existing reclaim path, and replaces it with a
> simple call to xfs_ireclaim if the inode was clean.  While we're at it
> we also use the slightly more lax xfs_inode_clean check we'd use later
> to determine if we need to flush the inode here.
> 
> Finally get rid of xfs_reclaim function and place the remaining small
> bits of reclaim code directly into xfs_fs_destroy_inode.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reported-by: Patrick Schreurs <patrick@xxxxxxxxxxxxxxxx>
> Reported-by: Tommy van Leeuwen <tommy@xxxxxxxxxxxxxxxx>
> Tested-by: Patrick Schreurs <patrick@xxxxxxxxxxxxxxxx>


Looks good.

Reviewed-by: Alex Elder <aelder@xxxxxxx>


> Index: xfs/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_vnodeops.c    2009-09-18 09:06:33.595274598 -0300
> +++ xfs/fs/xfs/xfs_vnodeops.c 2009-09-18 09:07:30.437255826 -0300
> @@ -2456,46 +2456,6 @@ xfs_set_dmattrs(
>       return error;
>  }
> 
> -int
> -xfs_reclaim(
> -     xfs_inode_t     *ip)
> -{
> -
> -     xfs_itrace_entry(ip);
> -
> -     ASSERT(!VN_MAPPED(VFS_I(ip)));
> -
> -     /* bad inode, get out here ASAP */
> -     if (is_bad_inode(VFS_I(ip))) {
> -             xfs_ireclaim(ip);
> -             return 0;
> -     }
> -
> -     xfs_ioend_wait(ip);
> -
> -     ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
> -
> -     /*
> -      * If we have nothing to flush with this inode then complete the
> -      * teardown now, otherwise break the link between the xfs inode and the
> -      * linux inode and clean up the xfs inode later. This avoids flushing
> -      * the inode to disk during the delete operation itself.
> -      *
> -      * When breaking the link, we need to set the XFS_IRECLAIMABLE flag
> -      * first to ensure that xfs_iunpin() will never see an xfs inode
> -      * that has a linux inode being reclaimed. Synchronisation is provided
> -      * by the i_flags_lock.
> -      */
> -     if (!ip->i_update_core && (ip->i_itemp == NULL)) {
> -             xfs_ilock(ip, XFS_ILOCK_EXCL);
> -             xfs_iflock(ip);
> -             xfs_iflags_set(ip, XFS_IRECLAIMABLE);
> -             return xfs_reclaim_inode(ip, 1, XFS_IFLUSH_DELWRI_ELSE_SYNC);
> -     }
> -     xfs_inode_set_reclaim_tag(ip);
> -     return 0;
> -}
> -
>  /*
>   * xfs_alloc_file_space()
>   *      This routine allocates disk space for the given file.
> Index: xfs/fs/xfs/linux-2.6/xfs_super.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_super.c     2009-09-18 09:07:15.869281043 
> -0300
> +++ xfs/fs/xfs/linux-2.6/xfs_super.c  2009-09-18 09:12:39.301006661 -0300
> @@ -930,13 +930,39 @@ xfs_fs_alloc_inode(
>   */
>  STATIC void
>  xfs_fs_destroy_inode(
> -     struct inode    *inode)
> +     struct inode            *inode)
>  {
> -     xfs_inode_t             *ip = XFS_I(inode);
> +     struct xfs_inode        *ip = XFS_I(inode);
> +
> +     xfs_itrace_entry(ip);
> 
>       XFS_STATS_INC(vn_reclaim);
> -     if (xfs_reclaim(ip))
> -             panic("%s: cannot reclaim 0x%p\n", __func__, inode);
> +
> +     /* bad inode, get out here ASAP */
> +     if (is_bad_inode(inode))
> +             goto out_reclaim;
> +
> +     xfs_ioend_wait(ip);
> +
> +     ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
> +
> +     /*
> +      * We should never get here with one of the reclaim flags already set.
> +      */
> +     ASSERT_ALWAYS(!xfs_iflags_test(ip, XFS_IRECLAIMABLE));
> +     ASSERT_ALWAYS(!xfs_iflags_test(ip, XFS_IRECLAIM));
> +
> +     /*
> +      * If we have nothing to flush with this inode then complete the
> +      * teardown now, otherwise delay the flush operation.
> +      */
> +     if (!xfs_inode_clean(ip)) {
> +             xfs_inode_set_reclaim_tag(ip);
> +             return;
> +     }
> +
> +out_reclaim:
> +     xfs_ireclaim(ip);
>  }
> 
>  /*
> Index: xfs/fs/xfs/xfs_vnodeops.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_vnodeops.h    2009-09-18 09:10:07.930003796 -0300
> +++ xfs/fs/xfs/xfs_vnodeops.h 2009-09-18 09:10:11.789032549 -0300
> @@ -38,7 +38,6 @@ int xfs_symlink(struct xfs_inode *dp, st
>               const char *target_path, mode_t mode, struct xfs_inode **ipp,
>               cred_t *credp);
>  int xfs_set_dmattrs(struct xfs_inode *ip, u_int evmask, u_int16_t state);
> -int xfs_reclaim(struct xfs_inode *ip);
>  int xfs_change_file_space(struct xfs_inode *ip, int cmd,
>               xfs_flock64_t *bf, xfs_off_t offset, int attr_flags);
>  int xfs_rename(struct xfs_inode *src_dp, struct xfs_name *src_name,
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.c      2009-09-18 09:13:28.627003540 
> -0300
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.c   2009-09-18 09:22:33.861006462 -0300
> @@ -663,10 +663,9 @@ xfs_syncd_stop(
>       kthread_stop(mp->m_sync_task);
>  }
> 
> -int
> +STATIC int
>  xfs_reclaim_inode(
>       xfs_inode_t     *ip,
> -     int             locked,
>       int             sync_mode)
>  {
>       xfs_perag_t     *pag = xfs_get_perag(ip->i_mount, ip->i_ino);
> @@ -682,10 +681,6 @@ xfs_reclaim_inode(
>           !__xfs_iflags_test(ip, XFS_IRECLAIMABLE)) {
>               spin_unlock(&ip->i_flags_lock);
>               write_unlock(&pag->pag_ici_lock);
> -             if (locked) {
> -                     xfs_ifunlock(ip);
> -                     xfs_iunlock(ip, XFS_ILOCK_EXCL);
> -             }
>               return -EAGAIN;
>       }
>       __xfs_iflags_set(ip, XFS_IRECLAIM);
> @@ -704,10 +699,8 @@ xfs_reclaim_inode(
>        * We get the flush lock regardless, though, just to make sure
>        * we don't free it while it is being flushed.
>        */
> -     if (!locked) {
> -             xfs_ilock(ip, XFS_ILOCK_EXCL);
> -             xfs_iflock(ip);
> -     }
> +     xfs_ilock(ip, XFS_ILOCK_EXCL);
> +     xfs_iflock(ip);
> 
>       /*
>        * In the case of a forced shutdown we rely on xfs_iflush() to
> @@ -778,7 +771,7 @@ xfs_reclaim_inode_now(
>       }
>       read_unlock(&pag->pag_ici_lock);
> 
> -     return xfs_reclaim_inode(ip, 0, flags);
> +     return xfs_reclaim_inode(ip, flags);
>  }
> 
>  int
> Index: xfs/fs/xfs/linux-2.6/xfs_sync.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_sync.h      2009-09-18 09:13:28.639004162 
> -0300
> +++ xfs/fs/xfs/linux-2.6/xfs_sync.h   2009-09-18 09:13:38.354031011 -0300
> @@ -44,7 +44,6 @@ void xfs_quiesce_attr(struct xfs_mount *
> 
>  void xfs_flush_inodes(struct xfs_inode *ip);
> 
> -int xfs_reclaim_inode(struct xfs_inode *ip, int locked, int sync_mode);
>  int xfs_reclaim_inodes(struct xfs_mount *mp, int mode);
> 
>  void xfs_inode_set_reclaim_tag(struct xfs_inode *ip);
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>
  • RE: [PATCH] xfs: simplify inode teardown, Alex Elder <=