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

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

On 25 Feb 2022 at 10:36, Darrick J. Wong wrote:
> On Thu, Feb 24, 2022 at 06:32:07PM +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 | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
>> index b8c79ee791af..379ef99722c5 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;
>
> Hmm.  Eventually you could replace all this with a single call to
> xfs_trans_alloc_inode, which would fix the quota leak from the rt
> metadata file expansion:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/commit/?h=realtime-quotas&id=c82a57844b60bb434103eacf757e47417f94a631
>
> However, as rt+quota are not a supported feature, I'll let that lie for
> now.
>
>>  
>>  		error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
>>  				XFS_IEXT_ADD_NOSPLIT_CNT);
>> @@ -824,7 +826,11 @@ xfs_growfs_rt_alloc(
>>  		 */
>>  		error = xfs_trans_commit(tp);
>>  		if (error)
>> -			return error;
>> +			goto out_trans_cancel;
>
> xfs_trans_commit frees tp even if the commit fails, which means that it
> is not correct to call xfs_trans_cancel on tp here.  I think you could
> do:
>
> 		error = xfs_trans_commit(tp);
> 		unlock_inode = false;
> 		xfs_iunlock(ip, XFS_ILOCK_EXCL);
> 		if (error)
> 			return error;
>
> Right?
>

Thanks for pointing out the bug. Yes, I agree with the above suggestion. I
will fix it up in the next version of the patchset.

-- 
chandan