summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDarrick J. Wong <darrick.wong@oracle.com>2018-05-04 15:30:19 -0700
committerDarrick J. Wong <darrick.wong@oracle.com>2018-05-10 08:56:47 -0700
commit7b6b50f55c1064e619f2da0696ed99f9db10b960 (patch)
tree141b3a966adcb0249c9f81d7c217a6c53fdf932e
parent84ca484ecf2f8e1dc3afddc895cb9b62c531db49 (diff)
downloadlinux-7b6b50f55c1064e619f2da0696ed99f9db10b960.tar.gz
linux-7b6b50f55c1064e619f2da0696ed99f9db10b960.tar.bz2
linux-7b6b50f55c1064e619f2da0696ed99f9db10b960.zip
xfs: release new dquot buffer on defer_finish error
In commit efa092f3d4c6 "[XFS] Fixes a bug in the quota code when allocating a new dquot record", we allocate a new dquot block, grab a buffer to initialize it, and return the locked initialized dquot buffer to the caller for further in-core dquot initialization. Unfortunately, if the _bmap_finish errored out, _qm_dqalloc would also error out without bothering to free the (locked) buffer. Leaking a locked buffer caused hangs in generic/388 when quotas are enabled. Furthermore, the _bmap_finish -> _defer_finish conversion in 310a75a3c6c747 ("xfs: change xfs_bmap_{finish,cancel,init,free} -> xfs_defer_*") failed to observe that the buffer was held going into _defer_finish and therefore failed to notice that the buffer lock is /not/ maintained afterwards. Now that we can bjoin a buffer to a defer_ops, use this mechanism to ensure that the buffer stays locked across the _defer_finish. Release the holds and locks on the buffer as appropriate if we have to error out. There is a subtlety here for the caller in that the buffer emerges locked and held to the transaction, so if the _trans_commit fails we have to release the buffer explicitly. This fixes the unmount hang in generic/388 when quotas are enabled. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Reviewed-by: Brian Foster <bfoster@redhat.com>
-rw-r--r--fs/xfs/xfs_dquot.c48
1 files changed, 28 insertions, 20 deletions
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 4ca9c39879ae..32d7359b3c18 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -362,32 +362,40 @@ xfs_qm_dqalloc(
dqp->dq_flags & XFS_DQ_ALLTYPES, bp);
/*
- * xfs_defer_finish() may commit the current transaction and
- * start a second transaction if the freelist is not empty.
+ * Hold the buffer and join it to the dfops so that we'll still own
+ * the buffer when we return to the caller. The buffer disposal on
+ * error must be paid attention to very carefully, as it has been
+ * broken since commit efa092f3d4c6 "[XFS] Fixes a bug in the quota
+ * code when allocating a new dquot record" in 2005, and the later
+ * conversion to xfs_defer_ops in commit 310a75a3c6c747 failed to keep
+ * the buffer locked across the _defer_finish call. We can now do
+ * this correctly with xfs_defer_bjoin.
*
- * Since we still want to modify this buffer, we need to
- * ensure that the buffer is not released on commit of
- * the first transaction and ensure the buffer is added to the
- * second transaction.
+ * Above, we allocated a disk block for the dquot information and
+ * used get_buf to initialize the dquot. If the _defer_bjoin fails,
+ * the buffer is still locked to *tpp, so we must _bhold_release and
+ * then _trans_brelse the buffer. If the _defer_finish fails, the old
+ * transaction is gone but the new buffer is not joined or held to any
+ * transaction, so we must _buf_relse it.
*
- * If there is only one transaction then don't stop the buffer
- * from being released when it commits later on.
+ * If everything succeeds, the caller of this function is returned a
+ * buffer that is locked and joined to the transaction. The caller
+ * is responsible for unlocking any buffer passed back, either
+ * manually or by committing the transaction.
*/
-
- xfs_trans_bhold(tp, bp);
-
+ xfs_trans_bhold(*tpp, bp);
+ error = xfs_defer_bjoin(&dfops, bp);
+ if (error) {
+ xfs_trans_bhold_release(*tpp, bp);
+ xfs_trans_brelse(*tpp, bp);
+ goto error1;
+ }
error = xfs_defer_finish(tpp, &dfops);
- if (error)
+ if (error) {
+ xfs_buf_relse(bp);
goto error1;
-
- /* Transaction was committed? */
- if (*tpp != tp) {
- tp = *tpp;
- xfs_trans_bjoin(tp, bp);
- } else {
- xfs_trans_bhold_release(tp, bp);
}
-
+ xfs_trans_bhold_release(*tpp, bp);
*O_bpp = bp;
return 0;