* re: xfs: cancel dfops on xfs_defer_finish() error
@ 2018-08-07 14:14 Colin Ian King
2018-08-07 14:37 ` Brian Foster
0 siblings, 1 reply; 6+ messages in thread
From: Colin Ian King @ 2018-08-07 14:14 UTC (permalink / raw)
To: Brian Foster, darrick.wong, linux-xfs,
linux-kernel@vger.kernel.org
Hi,
Recent commit 82ff27bc52a88cb5cc400bfa64e210d3ec8dfebd ("xfs: automatic
dfops buffer relogging") removed the assignment of variable error:
- error = xfs_defer_bjoin(tp->t_dfops, bp);
if (error) {
xfs_trans_bhold_release(tp, bp);
xfs_trans_brelse(tp, bp);
.. the removal of the assignment leads to dead code on the following if
statement as error is always zero at this point. Not sure if that was
intended, but it tripped a static analysis warning from CoverityScan.
364 xfs_trans_bhold(tp, bp);
const: At condition error, the value of error must be equal to 0.
dead_error_condition: The condition error cannot be true.
365 if (error) {
CID 1472288 (#1 of 1): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement:
xfs_trans_bhold_release(tp,....
366 xfs_trans_bhold_release(tp, bp);
367 xfs_trans_brelse(tp, bp);
368 goto error1;
369 }
Colin
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: xfs: cancel dfops on xfs_defer_finish() error 2018-08-07 14:14 xfs: cancel dfops on xfs_defer_finish() error Colin Ian King @ 2018-08-07 14:37 ` Brian Foster 2018-08-07 15:10 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Brian Foster @ 2018-08-07 14:37 UTC (permalink / raw) To: Colin Ian King; +Cc: darrick.wong, linux-xfs, linux-kernel@vger.kernel.org On Tue, Aug 07, 2018 at 03:14:07PM +0100, Colin Ian King wrote: > Hi, > > Recent commit 82ff27bc52a88cb5cc400bfa64e210d3ec8dfebd ("xfs: automatic > dfops buffer relogging") removed the assignment of variable error: > > - error = xfs_defer_bjoin(tp->t_dfops, bp); > if (error) { > xfs_trans_bhold_release(tp, bp); > xfs_trans_brelse(tp, bp); > Hmm, I _think_ we can just drop these error checks now that this pre-finish error state is non-existent, something like the appended diff (with additional cleanups). E.g., if the buffer is held in the transaction then the bjoin is implicit. If the finish fails, then the state is essentially unchanged by the relogging patch. That said, the error handling is a bit tricky here. Darrick, I think you reworked this recently.. thoughts? Brian --- 8< --- diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index 70a76ac41f01..2106c4142ecd 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -311,7 +311,7 @@ xfs_dquot_disk_alloc( XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA, XFS_QM_DQALLOC_SPACE_RES(mp), &map, &nmaps); if (error) - goto error0; + return error; ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB); ASSERT(nmaps == 1); ASSERT((map.br_startblock != DELAYSTARTBLOCK) && @@ -326,8 +326,8 @@ xfs_dquot_disk_alloc( bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, dqp->q_blkno, mp->m_quotainfo->qi_dqchunklen, 0); if (!bp) { - error = -ENOMEM; - goto error1; + xfs_defer_cancel(tp); + return -ENOMEM; } bp->b_ops = &xfs_dquot_buf_ops; @@ -349,10 +349,8 @@ xfs_dquot_disk_alloc( * the buffer locked across the _defer_finish call. We can now do * this correctly with xfs_defer_bjoin. * - * 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 + * Above, we allocated a disk block for the dquot information and used + * get_buf to initialize the dquot. 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. * @@ -362,24 +360,14 @@ xfs_dquot_disk_alloc( * manually or by committing the transaction. */ xfs_trans_bhold(tp, bp); - if (error) { - xfs_trans_bhold_release(tp, bp); - xfs_trans_brelse(tp, bp); - goto error1; - } error = xfs_defer_finish(tpp); tp = *tpp; if (error) { xfs_buf_relse(bp); - goto error0; + return error; } *bpp = bp; return 0; - -error1: - xfs_defer_cancel(tp); -error0: - return error; } /* ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: xfs: cancel dfops on xfs_defer_finish() error 2018-08-07 14:37 ` Brian Foster @ 2018-08-07 15:10 ` Darrick J. Wong 2018-08-07 15:18 ` Brian Foster 0 siblings, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2018-08-07 15:10 UTC (permalink / raw) To: Brian Foster; +Cc: Colin Ian King, linux-xfs, linux-kernel@vger.kernel.org On Tue, Aug 07, 2018 at 10:37:21AM -0400, Brian Foster wrote: > On Tue, Aug 07, 2018 at 03:14:07PM +0100, Colin Ian King wrote: > > Hi, > > > > Recent commit 82ff27bc52a88cb5cc400bfa64e210d3ec8dfebd ("xfs: automatic > > dfops buffer relogging") removed the assignment of variable error: > > > > - error = xfs_defer_bjoin(tp->t_dfops, bp); > > if (error) { > > xfs_trans_bhold_release(tp, bp); > > xfs_trans_brelse(tp, bp); > > > > Hmm, I _think_ we can just drop these error checks now that this > pre-finish error state is non-existent, something like the appended diff > (with additional cleanups). E.g., if the buffer is held in the > transaction then the bjoin is implicit. If the finish fails, then the > state is essentially unchanged by the relogging patch. > > That said, the error handling is a bit tricky here. Darrick, I think you > reworked this recently.. thoughts? > > Brian > > --- 8< --- > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > index 70a76ac41f01..2106c4142ecd 100644 > --- a/fs/xfs/xfs_dquot.c > +++ b/fs/xfs/xfs_dquot.c > @@ -311,7 +311,7 @@ xfs_dquot_disk_alloc( > XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA, > XFS_QM_DQALLOC_SPACE_RES(mp), &map, &nmaps); > if (error) > - goto error0; > + return error; > ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB); > ASSERT(nmaps == 1); > ASSERT((map.br_startblock != DELAYSTARTBLOCK) && > @@ -326,8 +326,8 @@ xfs_dquot_disk_alloc( > bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, dqp->q_blkno, > mp->m_quotainfo->qi_dqchunklen, 0); > if (!bp) { > - error = -ENOMEM; > - goto error1; > + xfs_defer_cancel(tp); > + return -ENOMEM; The only caller of xfs_dquot_disk_alloc checks the return value and xfs_trans_cancels the transaction, which should take care of calling xfs_defer_cancel, right? --D > } > bp->b_ops = &xfs_dquot_buf_ops; > > @@ -349,10 +349,8 @@ xfs_dquot_disk_alloc( > * the buffer locked across the _defer_finish call. We can now do > * this correctly with xfs_defer_bjoin. > * > - * 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 > + * Above, we allocated a disk block for the dquot information and used > + * get_buf to initialize the dquot. 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. > * > @@ -362,24 +360,14 @@ xfs_dquot_disk_alloc( > * manually or by committing the transaction. > */ > xfs_trans_bhold(tp, bp); > - if (error) { > - xfs_trans_bhold_release(tp, bp); > - xfs_trans_brelse(tp, bp); > - goto error1; > - } > error = xfs_defer_finish(tpp); > tp = *tpp; > if (error) { > xfs_buf_relse(bp); > - goto error0; > + return error; > } > *bpp = bp; > return 0; > - > -error1: > - xfs_defer_cancel(tp); > -error0: > - return error; > } > > /* > -- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xfs: cancel dfops on xfs_defer_finish() error 2018-08-07 15:10 ` Darrick J. Wong @ 2018-08-07 15:18 ` Brian Foster 2018-08-07 15:26 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Brian Foster @ 2018-08-07 15:18 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Colin Ian King, linux-xfs, linux-kernel@vger.kernel.org On Tue, Aug 07, 2018 at 08:10:29AM -0700, Darrick J. Wong wrote: > On Tue, Aug 07, 2018 at 10:37:21AM -0400, Brian Foster wrote: > > On Tue, Aug 07, 2018 at 03:14:07PM +0100, Colin Ian King wrote: > > > Hi, > > > > > > Recent commit 82ff27bc52a88cb5cc400bfa64e210d3ec8dfebd ("xfs: automatic > > > dfops buffer relogging") removed the assignment of variable error: > > > > > > - error = xfs_defer_bjoin(tp->t_dfops, bp); > > > if (error) { > > > xfs_trans_bhold_release(tp, bp); > > > xfs_trans_brelse(tp, bp); > > > > > > > Hmm, I _think_ we can just drop these error checks now that this > > pre-finish error state is non-existent, something like the appended diff > > (with additional cleanups). E.g., if the buffer is held in the > > transaction then the bjoin is implicit. If the finish fails, then the > > state is essentially unchanged by the relogging patch. > > > > That said, the error handling is a bit tricky here. Darrick, I think you > > reworked this recently.. thoughts? > > > > Brian > > > > --- 8< --- > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > > index 70a76ac41f01..2106c4142ecd 100644 > > --- a/fs/xfs/xfs_dquot.c > > +++ b/fs/xfs/xfs_dquot.c > > @@ -311,7 +311,7 @@ xfs_dquot_disk_alloc( > > XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA, > > XFS_QM_DQALLOC_SPACE_RES(mp), &map, &nmaps); > > if (error) > > - goto error0; > > + return error; > > ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB); > > ASSERT(nmaps == 1); > > ASSERT((map.br_startblock != DELAYSTARTBLOCK) && > > @@ -326,8 +326,8 @@ xfs_dquot_disk_alloc( > > bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, dqp->q_blkno, > > mp->m_quotainfo->qi_dqchunklen, 0); > > if (!bp) { > > - error = -ENOMEM; > > - goto error1; > > + xfs_defer_cancel(tp); > > + return -ENOMEM; > > The only caller of xfs_dquot_disk_alloc checks the return value and > xfs_trans_cancels the transaction, which should take care of calling > xfs_defer_cancel, right? > Yeah. IIRC I originally left the defer_cancel() alone in this function just to be consistent, since it calls xfs_defer_finish() as well on the caller's transaction. Technically I don't think it matters either way, so I don't have much preference on if it stays or goes (assuming the other changes are correct)... Brian > --D > > > } > > bp->b_ops = &xfs_dquot_buf_ops; > > > > @@ -349,10 +349,8 @@ xfs_dquot_disk_alloc( > > * the buffer locked across the _defer_finish call. We can now do > > * this correctly with xfs_defer_bjoin. > > * > > - * 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 > > + * Above, we allocated a disk block for the dquot information and used > > + * get_buf to initialize the dquot. 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. > > * > > @@ -362,24 +360,14 @@ xfs_dquot_disk_alloc( > > * manually or by committing the transaction. > > */ > > xfs_trans_bhold(tp, bp); > > - if (error) { > > - xfs_trans_bhold_release(tp, bp); > > - xfs_trans_brelse(tp, bp); > > - goto error1; > > - } > > error = xfs_defer_finish(tpp); > > tp = *tpp; > > if (error) { > > xfs_buf_relse(bp); > > - goto error0; > > + return error; > > } > > *bpp = bp; > > return 0; > > - > > -error1: > > - xfs_defer_cancel(tp); > > -error0: > > - return error; > > } > > > > /* > > -- > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xfs: cancel dfops on xfs_defer_finish() error 2018-08-07 15:18 ` Brian Foster @ 2018-08-07 15:26 ` Darrick J. Wong 2018-08-07 15:34 ` Brian Foster 0 siblings, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2018-08-07 15:26 UTC (permalink / raw) To: Brian Foster; +Cc: Colin Ian King, linux-xfs, linux-kernel@vger.kernel.org On Tue, Aug 07, 2018 at 11:18:36AM -0400, Brian Foster wrote: > On Tue, Aug 07, 2018 at 08:10:29AM -0700, Darrick J. Wong wrote: > > On Tue, Aug 07, 2018 at 10:37:21AM -0400, Brian Foster wrote: > > > On Tue, Aug 07, 2018 at 03:14:07PM +0100, Colin Ian King wrote: > > > > Hi, > > > > > > > > Recent commit 82ff27bc52a88cb5cc400bfa64e210d3ec8dfebd ("xfs: automatic > > > > dfops buffer relogging") removed the assignment of variable error: > > > > > > > > - error = xfs_defer_bjoin(tp->t_dfops, bp); > > > > if (error) { > > > > xfs_trans_bhold_release(tp, bp); > > > > xfs_trans_brelse(tp, bp); > > > > > > > > > > Hmm, I _think_ we can just drop these error checks now that this > > > pre-finish error state is non-existent, something like the appended diff > > > (with additional cleanups). E.g., if the buffer is held in the > > > transaction then the bjoin is implicit. If the finish fails, then the > > > state is essentially unchanged by the relogging patch. > > > > > > That said, the error handling is a bit tricky here. Darrick, I think you > > > reworked this recently.. thoughts? > > > > > > Brian > > > > > > --- 8< --- > > > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > > > index 70a76ac41f01..2106c4142ecd 100644 > > > --- a/fs/xfs/xfs_dquot.c > > > +++ b/fs/xfs/xfs_dquot.c > > > @@ -311,7 +311,7 @@ xfs_dquot_disk_alloc( > > > XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA, > > > XFS_QM_DQALLOC_SPACE_RES(mp), &map, &nmaps); > > > if (error) > > > - goto error0; > > > + return error; > > > ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB); > > > ASSERT(nmaps == 1); > > > ASSERT((map.br_startblock != DELAYSTARTBLOCK) && > > > @@ -326,8 +326,8 @@ xfs_dquot_disk_alloc( > > > bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, dqp->q_blkno, > > > mp->m_quotainfo->qi_dqchunklen, 0); > > > if (!bp) { > > > - error = -ENOMEM; > > > - goto error1; > > > + xfs_defer_cancel(tp); > > > + return -ENOMEM; > > > > The only caller of xfs_dquot_disk_alloc checks the return value and > > xfs_trans_cancels the transaction, which should take care of calling > > xfs_defer_cancel, right? > > > > Yeah. IIRC I originally left the defer_cancel() alone in this function > just to be consistent, since it calls xfs_defer_finish() as well on the > caller's transaction. Technically I don't think it matters either way, > so I don't have much preference on if it stays or goes (assuming the > other changes are correct)... I can't think of a good reason for anything (except xfs_trans_cancel) to call xfs_defer_cancel. It's never correct to kill all the deferred ops attached to a transaction and then continue logging things to the transaction, which means that the only thing you can really do after cancelling the dfops is to cancel the transaction. I'd have to look more carefully but I think all the remaining callers can all go away. xfs_defer_finish is a different story -- it can be used (by online repair for example) to process the deferred operations so they don't sit around pinning memory and log, even if there's a lot more work to be done before _trans_commit. But I don't think we need to _defer_cancel if things go awry. --D > Brian > > > --D > > > > > } > > > bp->b_ops = &xfs_dquot_buf_ops; > > > > > > @@ -349,10 +349,8 @@ xfs_dquot_disk_alloc( > > > * the buffer locked across the _defer_finish call. We can now do > > > * this correctly with xfs_defer_bjoin. > > > * > > > - * 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 > > > + * Above, we allocated a disk block for the dquot information and used > > > + * get_buf to initialize the dquot. 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. > > > * > > > @@ -362,24 +360,14 @@ xfs_dquot_disk_alloc( > > > * manually or by committing the transaction. > > > */ > > > xfs_trans_bhold(tp, bp); > > > - if (error) { > > > - xfs_trans_bhold_release(tp, bp); > > > - xfs_trans_brelse(tp, bp); > > > - goto error1; > > > - } > > > error = xfs_defer_finish(tpp); > > > tp = *tpp; > > > if (error) { > > > xfs_buf_relse(bp); > > > - goto error0; > > > + return error; > > > } > > > *bpp = bp; > > > return 0; > > > - > > > -error1: > > > - xfs_defer_cancel(tp); > > > -error0: > > > - return error; > > > } > > > > > > /* > > > -- > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: xfs: cancel dfops on xfs_defer_finish() error 2018-08-07 15:26 ` Darrick J. Wong @ 2018-08-07 15:34 ` Brian Foster 0 siblings, 0 replies; 6+ messages in thread From: Brian Foster @ 2018-08-07 15:34 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Colin Ian King, linux-xfs, linux-kernel@vger.kernel.org On Tue, Aug 07, 2018 at 08:26:44AM -0700, Darrick J. Wong wrote: > On Tue, Aug 07, 2018 at 11:18:36AM -0400, Brian Foster wrote: > > On Tue, Aug 07, 2018 at 08:10:29AM -0700, Darrick J. Wong wrote: > > > On Tue, Aug 07, 2018 at 10:37:21AM -0400, Brian Foster wrote: > > > > On Tue, Aug 07, 2018 at 03:14:07PM +0100, Colin Ian King wrote: > > > > > Hi, > > > > > > > > > > Recent commit 82ff27bc52a88cb5cc400bfa64e210d3ec8dfebd ("xfs: automatic > > > > > dfops buffer relogging") removed the assignment of variable error: > > > > > > > > > > - error = xfs_defer_bjoin(tp->t_dfops, bp); > > > > > if (error) { > > > > > xfs_trans_bhold_release(tp, bp); > > > > > xfs_trans_brelse(tp, bp); > > > > > > > > > > > > > Hmm, I _think_ we can just drop these error checks now that this > > > > pre-finish error state is non-existent, something like the appended diff > > > > (with additional cleanups). E.g., if the buffer is held in the > > > > transaction then the bjoin is implicit. If the finish fails, then the > > > > state is essentially unchanged by the relogging patch. > > > > > > > > That said, the error handling is a bit tricky here. Darrick, I think you > > > > reworked this recently.. thoughts? > > > > > > > > Brian > > > > > > > > --- 8< --- > > > > > > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c > > > > index 70a76ac41f01..2106c4142ecd 100644 > > > > --- a/fs/xfs/xfs_dquot.c > > > > +++ b/fs/xfs/xfs_dquot.c > > > > @@ -311,7 +311,7 @@ xfs_dquot_disk_alloc( > > > > XFS_DQUOT_CLUSTER_SIZE_FSB, XFS_BMAPI_METADATA, > > > > XFS_QM_DQALLOC_SPACE_RES(mp), &map, &nmaps); > > > > if (error) > > > > - goto error0; > > > > + return error; > > > > ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB); > > > > ASSERT(nmaps == 1); > > > > ASSERT((map.br_startblock != DELAYSTARTBLOCK) && > > > > @@ -326,8 +326,8 @@ xfs_dquot_disk_alloc( > > > > bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, dqp->q_blkno, > > > > mp->m_quotainfo->qi_dqchunklen, 0); > > > > if (!bp) { > > > > - error = -ENOMEM; > > > > - goto error1; > > > > + xfs_defer_cancel(tp); > > > > + return -ENOMEM; > > > > > > The only caller of xfs_dquot_disk_alloc checks the return value and > > > xfs_trans_cancels the transaction, which should take care of calling > > > xfs_defer_cancel, right? > > > > > > > Yeah. IIRC I originally left the defer_cancel() alone in this function > > just to be consistent, since it calls xfs_defer_finish() as well on the > > caller's transaction. Technically I don't think it matters either way, > > so I don't have much preference on if it stays or goes (assuming the > > other changes are correct)... > > I can't think of a good reason for anything (except xfs_trans_cancel) to > call xfs_defer_cancel. It's never correct to kill all the deferred ops > attached to a transaction and then continue logging things to the > transaction, which means that the only thing you can really do after > cancelling the dfops is to cancel the transaction. I'd have to look > more carefully but I think all the remaining callers can all go away. > Indeed, sounds good to me. I'll post a variant of the patch with the xfs_defer_cancel() call removed. I've also made a note to audit the remaining xfs_defer_cancel() callers to see if we can kill those off. Brian > xfs_defer_finish is a different story -- it can be used (by online > repair for example) to process the deferred operations so they don't sit > around pinning memory and log, even if there's a lot more work to be > done before _trans_commit. But I don't think we need to _defer_cancel > if things go awry. > > --D > > > Brian > > > > > --D > > > > > > > } > > > > bp->b_ops = &xfs_dquot_buf_ops; > > > > > > > > @@ -349,10 +349,8 @@ xfs_dquot_disk_alloc( > > > > * the buffer locked across the _defer_finish call. We can now do > > > > * this correctly with xfs_defer_bjoin. > > > > * > > > > - * 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 > > > > + * Above, we allocated a disk block for the dquot information and used > > > > + * get_buf to initialize the dquot. 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. > > > > * > > > > @@ -362,24 +360,14 @@ xfs_dquot_disk_alloc( > > > > * manually or by committing the transaction. > > > > */ > > > > xfs_trans_bhold(tp, bp); > > > > - if (error) { > > > > - xfs_trans_bhold_release(tp, bp); > > > > - xfs_trans_brelse(tp, bp); > > > > - goto error1; > > > > - } > > > > error = xfs_defer_finish(tpp); > > > > tp = *tpp; > > > > if (error) { > > > > xfs_buf_relse(bp); > > > > - goto error0; > > > > + return error; > > > > } > > > > *bpp = bp; > > > > return 0; > > > > - > > > > -error1: > > > > - xfs_defer_cancel(tp); > > > > -error0: > > > > - return error; > > > > } > > > > > > > > /* > > > > -- > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-07 17:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-08-07 14:14 xfs: cancel dfops on xfs_defer_finish() error Colin Ian King 2018-08-07 14:37 ` Brian Foster 2018-08-07 15:10 ` Darrick J. Wong 2018-08-07 15:18 ` Brian Foster 2018-08-07 15:26 ` Darrick J. Wong 2018-08-07 15:34 ` Brian Foster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).