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