| Message ID | 20180504211958.GN26569@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, May 04, 2018 at 02:19:58PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > 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> > --- > v2: return buffer with consistent hold state > --- Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/xfs_dquot.c | 48 ++++++++++++++++++++++++++++-------------------- > 1 file changed, 28 insertions(+), 20 deletions(-) > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index a7daef9e16bf..7c71a14d0d49 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; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index a7daef9e16bf..7c71a14d0d49 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;