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@xxxxxxxxxx> >> --- >> 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
