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@xxxxxxxxxxxxxxx
> > > > 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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
