Re: [PATCH V7 13/17] xfs: xfs_growfs_rt_alloc: Unlock inode explicitly rather than through iop_committing()

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 01, 2022 at 04:09:34PM +0530, Chandan Babu R wrote:
> In order to be able to upgrade inodes to XFS_DIFLAG2_NREXT64, a future commit
> will perform such an upgrade in a transaction context. This requires the
> transaction to be rolled once. Hence inodes which have been added to the
> tranasction (via xfs_trans_ijoin()) with non-zero value for lock_flags
> argument would cause the inode to be unlocked when the transaction is rolled.
> 
> To prevent this from happening in the case of realtime bitmap/summary inodes,
> this commit now unlocks the inode explictly rather than through
> iop_committing() call back.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  fs/xfs/xfs_rtalloc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index b8c79ee791af..a70140b35e8b 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -780,6 +780,7 @@ xfs_growfs_rt_alloc(
>  	int			resblks;	/* space reservation */
>  	enum xfs_blft		buf_type;
>  	struct xfs_trans	*tp;
> +	bool			unlock_inode;
>  
>  	if (ip == mp->m_rsumip)
>  		buf_type = XFS_BLFT_RTSUMMARY_BUF;
> @@ -802,7 +803,8 @@ xfs_growfs_rt_alloc(
>  		 * Lock the inode.
>  		 */
>  		xfs_ilock(ip, XFS_ILOCK_EXCL);
> -		xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +		xfs_trans_ijoin(tp, ip, 0);
> +		unlock_inode = true;
>  
>  		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
>  				XFS_IEXT_ADD_NOSPLIT_CNT);
> @@ -823,8 +825,11 @@ xfs_growfs_rt_alloc(
>  		 * Free any blocks freed up in the transaction, then commit.
>  		 */
>  		error = xfs_trans_commit(tp);
> -		if (error)
> +                unlock_inode = false;
> +                xfs_iunlock(ip, XFS_ILOCK_EXCL);
> +                if (error)
>  			return error;
> +

whitespace damage.

>  		/*
>  		 * Now we need to clear the allocated blocks.
>  		 * Do this one block per transaction, to keep it simple.
> @@ -874,6 +879,8 @@ xfs_growfs_rt_alloc(
>  
>  out_trans_cancel:
>  	xfs_trans_cancel(tp);
> +	if (unlock_inode)
> +		xfs_iunlock(ip, XFS_ILOCK_EXCL);
>  	return error;

That's kinda messy, IMO. If you create a new error stack like:

out_trans_cancel:
	xfs_trans_cancel(tp);
	return error;

out_cancel_unlock:
	xfs_trans_cancel(tp);
	xfs_iunlock(ip, XFS_ILOCK_EXCL);
	return error;

Then you can get rid of the unlock_inode variable and just change
the if (error) goto ... jumps in the appropriate places where
unlock on cancel is needed. That seems much cleaner and easier to
verify.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com