[PATCH] xfs: simplify inode teardown
Alex Elder
aelder at sgi.com
Wed Oct 7 17:56:57 CDT 2009
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 at lst.de>
> Reported-by: Patrick Schreurs <patrick at news-service.com>
> Reported-by: Tommy van Leeuwen <tommy at news-service.com>
> Tested-by: Patrick Schreurs <patrick at news-service.com>
Looks good.
Reviewed-by: Alex Elder <aelder at sgi.com>
> 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 at oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
More information about the xfs
mailing list