On Wed, Mar 04, 2020 at 07:44:21AM -0800, Christoph Hellwig wrote:
> > /*
> > - * Calculate the potential space needed by the log vector. Each region gets
> > - * its own xlog_op_header_t and may need to be double word aligned.
> > + * Calculate the potential space needed by the log vector. We may need a
> > + * start record, and each region gets its own xlog_op_header_t and may need to
> > + * be double word aligned.
>
> s/xlog_op_header_t/struct xlog_op_header/ while you're at it.
>
> > @@ -2404,25 +2391,29 @@ xlog_write(
> > int record_cnt = 0;
> > int data_cnt = 0;
> > int error = 0;
> > + int start_rec_size = sizeof(struct xlog_op_header);
> >
> > *start_lsn = 0;
> >
> > - len = xlog_write_calc_vec_length(ticket, log_vector);
> >
> > /*
> > * Region headers and bytes are already accounted for.
> > * We only need to take into account start records and
> > * split regions in this function.
> > */
> > - if (ticket->t_flags & XLOG_TIC_INITED)
> > + if (ticket->t_flags & XLOG_TIC_INITED) {
> > ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > + ticket->t_flags &= ~XLOG_TIC_INITED;
> > + }
> >
> > /*
> > * Commit record headers need to be accounted for. These
> > * come in as separate writes so are easy to detect.
> > */
> > - if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> > + if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> > ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > + start_rec_size = 0;
> > + }
> >
> > if (ticket->t_curr_res < 0) {
> > xfs_alert_tag(log->l_mp, XFS_PTAG_LOGRES,
> > @@ -2431,6 +2422,8 @@ xlog_write(
> > xfs_force_shutdown(log->l_mp, SHUTDOWN_LOG_IO_ERROR);
> > }
> >
> > + len = xlog_write_calc_vec_length(ticket, log_vector, start_rec_size);
>
> The last arg is used as a boolean in xlog_write_calc_vec_length. I
> think it would make sense to have a need_start_rec boolean in this
> function as well, and just hardcode the sizeof in the two places that
> actually need the size.
I originally had that and while the code looked kinda weird
opencoding an ophdr everywhere we wanted the size of a start record,
that wasn't an issue. The biggest problem was that using a boolean
resulted in _several_ logic bugs that I only tracked down once I
realised I'd forgotten to replace existing the start record size
variable that now wasn't initialised inside the inner loop.
So, yes, it gets converted to a boolean inside that function call,
but I think the code in this set of nested loops is more reliable if
it carries the size of the structure rather than open coding it
everywhere. Making it a boolean doesn't improve the readability of
the code at all.
> > + copy_len += sizeof(xlog_op_header_t);
>
> s/xlog_op_header_t/struct xlog_op_header/
Ok.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
