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>

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 4ca9c39..32d7359 100644 (file)
--- 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;