On Thu, Mar 05, 2020 at 01:06:44PM -0500, Brian Foster wrote:
> Ok, but it looks like the original xfs_log_done() includes a bit of
> logic to end a regrant in the event of commit_record() error (shutdown).
> Do we need to lift that logic into callers (that might regrant) as well?
Yes.
>
> > +/*
> > + * Release or regrant the ticket reservation now the transaction is done with
> > + * it depending on caller context. Rolling transactions need the ticket
> > + * regranted, otherwise we release it completely.
> > + */
> > +void
> > +xlog_ticket_done(
> > + struct xlog *log,
> > + struct xlog_ticket *ticket,
> > + bool regrant)
> > +{
> > if (!regrant) {
> > trace_xfs_log_done_nonperm(log, ticket);
> > -
> > - /*
> > - * Release ticket if not permanent reservation or a specific
> > - * request has been made to release a permanent reservation.
> > - */
> > xlog_ungrant_log_space(log, ticket);
> > } else {
> > trace_xfs_log_done_perm(log, ticket);
> > -
> > xlog_regrant_reserve_log_space(log, ticket);
> > - /* If this ticket was a permanent reservation and we aren't
> > - * trying to release it, reset the inited flags; so next time
> > - * we write, a start record will be written out.
> > - */
> > - ticket->t_flags |= XLOG_TIC_INITED;
> > }
> > - /* xfs_log_done always frees the ticket on error. */
> > - commit_lsn = xfs_log_done(log->l_mp, tic, &commit_iclog, false);
> > - if (commit_lsn == -1)
> > - goto out_abort;
> > + error = xlog_write_done(log, tic, &commit_iclog, &commit_lsn);
> > + if (error)
> > + goto out_abort_free_ticket;
> > +
> > + xlog_ticket_done(log, tic, false);
>
> Seems it would be more simple to call xlog_ticket_done() before the
> error check and use the out_abort label (killing off the free ticket
> one). Otherwise looks Ok.
There are two other jumps to that label, so it can't be removed.
