On Thu, Feb 25, 2021 at 09:49:20AM +0100, Christoph Hellwig wrote:
> > + if (optype & XLOG_START_TRANS)
> > + headers++;
>
> This deserves a comment.
It gets killed off later, so it's a waste of time to prettify this.
> > + len = xlog_write_calc_vec_length(ticket, log_vector, optype);
> > + if (start_lsn)
> > + *start_lsn = 0;
>
> I'd slightly prefer that allowing a NULL start_lsn was a separate prep
> patch. As-is it really clutters the patch and detracts from the real
> change.
No, I've already got enough patches in this whole series to deal
with. I'm not splitting out simple, obvious changes into tiny two
line patches that require me to do more work for zero gain.
> > int copy_len;
> > int copy_off;
> > bool ordered = false;
> > + bool wrote_start_rec = false;
> >
> > /* ordered log vectors have no regions to write */
> > if (lv->lv_buf_len == XFS_LOG_VEC_ORDERED) {
> > @@ -2502,13 +2501,15 @@ xlog_write(
> > * write a start record. Only do this for the first
> > * iclog we write to.
> > */
> > - if (need_start_rec) {
> > + if (optype & XLOG_START_TRANS) {
>
> So this relies on the fact that the only callers that passes an optype of
> XLOG_START_TRANS only writes a single lv. I think we want an assert for
> that somewhere to avoid a bad surprise later.
This also gets killed off later, so again such things are largely a
waste of my time as all it does is cause rebase conflicts in
multiple patches and doesn't actually change the end result. So,
again, no.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
