Re: [PATCH 3/3] xfs: push down inactive transaction mgmt for ifree

[Dave Chinner <david@fromorbit.com>]

On Wed, Sep 18, 2013 at 12:16:00PM -0400, Brian Foster wrote:
> Push the inode free work performed during xfs_inactive() down into
> a new xfs_inactive_ifree() helper. This clears xfs_inactive() from
> all inode locking and transaction management more directly
> associated with freeing the inode xattrs, extents and the inode
> itself.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 121 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 71 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 9416462..a6ed69d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1710,6 +1710,74 @@ error0:
>  }
>  
>  /*
> + * xfs_inactive_ifree()
> + *
> + * Perform the inode free when an inode is unlinked.
> + */
> +STATIC int
> +xfs_inactive_ifree(
> +	struct xfs_inode *ip)
> +{
> +	xfs_bmap_free_t		free_list;
> +	xfs_fsblock_t		first_block;
> +	int			committed;
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
> +	if (error) {
> +		ASSERT(XFS_FORCED_SHUTDOWN(mp));
> +		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
> +		return error;
> +	}
> +
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
> +
> +	xfs_bmap_init(&free_list, &first_block);
> +	error = xfs_ifree(tp, ip, &free_list);
> +	if (error) {
> +		/*
> +		 * If we fail to free the inode, shut down.  The cancel
> +		 * might do that, we need to make sure.  Otherwise the
> +		 * inode might be lost for a long time or forever.
> +		 */
> +		if (!XFS_FORCED_SHUTDOWN(mp)) {
> +			xfs_notice(mp, "%s: xfs_ifree returned error %d",
> +				__func__, error);
> +			xfs_force_shutdown(mp, SHUTDOWN_META_IO_ERROR);
> +		}
> +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
> +		return error;
> +	}
> +
> +	/*
> +	 * Credit the quota account(s). The inode is gone.
> +	 */
> +	xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_ICOUNT, -1);
> +
> +	/*
> +	 * Just ignore errors at this point.  There is nothing we can
> +	 * do except to try to keep going. Make sure it's not a silent
> +	 * error.
> +	 */
> +	error = xfs_bmap_finish(&tp,  &free_list, &committed);
> +	if (error)
> +		xfs_notice(mp, "%s: xfs_bmap_finish returned error %d",
> +			__func__, error);
> +	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> +	if (error)
> +		xfs_notice(mp, "%s: xfs_trans_commit returned error %d",
> +			__func__, error);
> +
> +	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +	return 0;
> +}

I suspect we can clean up the error handling here now, and make it
look like the symlink remove inactive handle where we cancel bmaps
and abort transactions and trigger shutdowns appropriately. I'd
leave that to a separate patchset, though ;)

> -				__func__, error);
> -	}
> +	error = xfs_inactive_ifree(ip);
> +	if (error)
> +		goto out;
>  
>  	/*
>  	 * Release the dquots held by inode, if any.
>  	 */
>  	xfs_qm_dqdetach(ip);
> -	xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  out:
>  	return VN_INACTIVE_CACHE;
>  }

I think it's time to kill VN_INACTIVE_CACHE, as well. It's a
hold-over from Irix, and it serves no purpose what-so-ever on
Linux...

Otherwise it looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs