Re: [PATCH 03/11] xfs: refactor and split xfs_log_done()

[Christoph Hellwig <hch@infradead.org>]

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.