Re: [PATCH 12/16] xfs:_introduce xlog_write_partial()

[Dave Chinner <david@fromorbit.com>]

On Tue, Nov 16, 2021 at 09:21:51PM -0800, Darrick J. Wong wrote:
> > Subject: [PATCH 12/16] xfs:_introduce xlog_write_partial()
> 
> Nit: There's still an        ^ underscore in the subject.

fixed.

> > +		ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
> > +		ophdr->oh_len = cpu_to_be32(rlen - sizeof(struct xlog_op_header));
> > +		if (rlen != reg->i_len)
> > +			ophdr->oh_flags |= XLOG_CONTINUE_TRANS;
> > +
> > +		xlog_verify_dest_ptr(log, iclog->ic_datap + *log_offset);
> > +		xlog_write_iovec(iclog, log_offset, reg->i_addr,
> > +				rlen, len, record_cnt, data_cnt);
> > +
> > +		/* If we wrote the whole region, move to the next. */
> > +		if (rlen == reg->i_len)
> > +			continue;
> >  
> > -	if (*partial_copy) {
> >  		/*
> > -		 * This iclog has already been marked WANT_SYNC by
> > -		 * xlog_state_get_iclog_space.
> > +		 * We now have a partially written iovec, but it can span
> > +		 * multiple iclogs so we loop here. First we release the iclog
> > +		 * we currently have, then we get a new iclog and add a new
> > +		 * opheader. Then we continue copying from where we were until
> > +		 * we either complete the iovec or fill the iclog. If we
> > +		 * complete the iovec, then we increment the index and go right
> > +		 * back to the top of the outer loop. if we fill the iclog, we
> > +		 * run the inner loop again.
> > +		 *
> > +		 * This is complicated by the tail of a region using all the
> > +		 * space in an iclog and hence requiring us to release the iclog
> > +		 * and get a new one before returning to the outer loop. We must
> > +		 * always guarantee that we exit this inner loop with at least
> > +		 * space for log transaction opheaders left in the current
> > +		 * iclog, hence we cannot just terminate the loop at the end
> > +		 * of the of the continuation. So we loop while there is no
> > +		 * space left in the current iclog, and check for the end of the
> > +		 * continuation after getting a new iclog.
> >  		 */
> > -		spin_lock(&log->l_icloglock);
> > -		xlog_state_finish_copy(log, iclog, *record_cnt, *data_cnt);
> > -		*record_cnt = 0;
> > -		*data_cnt = 0;
> > -		goto release_iclog;
> > -	}
> > +		do {
> > +			/*
> > +			 * Ensure we include the continuation opheader in the
> > +			 * space we need in the new iclog by adding that size
> > +			 * to the length we require. This continuation opheader
> > +			 * needs to be accounted to the ticket as the space it
> > +			 * consumes hasn't been accounted to the lv we are
> > +			 * writing.
> > +			 */
> > +			error = xlog_write_get_more_iclog_space(ticket,
> > +					&iclog, log_offset,
> > +					*len + sizeof(struct xlog_op_header),
> 
> Hm.  The last time I saw this patch, it incremented *len by the sizeof
> expression, but now we merely pass (*len + sizeof(...)) into
> xlog_write_get_more_iclog_space.  Why is that?

The space required in the iclog includes an extra ophdr, but *len
tracks the amount of region data we've copied. The continuation
ophdr space is not included in the region data, so that space does
not affect how we account for copied data.

However, we do need account for the space used by the continuation
ophdr in the space we use - reservation space, *log_offset and
*data_cnt - because those are used to track offset into the iclog,
data space used in the iclog and transaction reservation consumed.
The continuation header accounting is done separately ....

> 
> The rest of this looks more or less like a slightly reorganized version
> that I looked at the from June, so that was the only question I had.
> 
> --D
> 
> > +					record_cnt, data_cnt, contwr);
> > +			if (error)
> > +				return error;
> > +
> > +			ophdr = iclog->ic_datap + *log_offset;
> > +			ophdr->oh_tid = cpu_to_be32(ticket->t_tid);
> > +			ophdr->oh_clientid = XFS_TRANSACTION;
> > +			ophdr->oh_res2 = 0;
> > +			ophdr->oh_flags = XLOG_WAS_CONT_TRANS;
> >  
> > -	*partial_copy = 0;
> > -	*partial_copy_len = 0;
> > +			ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > +			*log_offset += sizeof(struct xlog_op_header);
> > +			*data_cnt += sizeof(struct xlog_op_header);

.... right here, immediately after we write it into the iclog data
buffer.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com