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

Chandan Babu R
2022-03-05T12:45:15Z

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