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

[Chandan Babu R <chandan.babu@oracle.com>]

On 04 Mar 2022 at 12:55, Dave Chinner wrote:
> 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.

The above suggestion is correct. I will include this change in the next
version.

-- 
chandan