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

[Brian Foster]

On 09/18/2013 07:06 PM, Dave Chinner wrote:
> 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 at 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 ;)
> 

Hmm, well I follow what you mean as far as changing the code I think.
But what changed that makes this safe? Or are you suggesting to shutdown
on a bmap_finish/trans_commit failure instead of just "being noisy?"

(Regardless, a separate patchset sounds good..)

>> -				__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...
> 

Ok, looks like a trivial patch to append to the series. In fact, there
appears to be no reason to return a value from xfs_inactive() at all
(along with the out label being unnecessary)... Thanks.

Brian

> Otherwise it looks good.
> 
> Cheers,
> 
> Dave.
>