On Wed, Feb 24, 2021 at 05:34:57PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Handle writing of a logvec chain into an iclog that doesn't have
> enough space to fit it all. The iclog has already been changed to
> WANT_SYNC by xlog_get_iclog_space(), so the entire remaining space
> in the iclog is exclusively owned by this logvec chain.
>
> The difference between the single and partial cases is that
> we end up with partial iovec writes in the iclog and have to split
> a log vec regions across two iclogs. The state handling for this is
> currently awful and so we're building up the pieces needed to
> handle this more cleanly one at a time.
I did not fully grasp the refactoring yet, so just some superficial
ramblings for now:
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 456ab3294621..74a1dddf1c15 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2060,6 +2060,7 @@ xlog_state_finish_copy(
>
> be32_add_cpu(&iclog->ic_header.h_num_logops, record_cnt);
> iclog->ic_offset += copy_bytes;
> + ASSERT(iclog->ic_offset <= iclog->ic_size);
How is this related to the rest of the patch? Maybe just add it
in a prep patch?
> + error = xlog_state_get_iclog_space(log, len, &iclog, ticket,
> + &contwr, &log_offset);
> + if (error)
> + return error;
>
> + /* start_lsn is the LSN of the first iclog written to. */
> + if (start_lsn)
> + *start_lsn = be64_to_cpu(iclog->ic_header.h_lsn);
>
> + /*
> + * iclogs containing commit records or unmount records need
> + * to issue ordering cache flushes and commit immediately
> + * to stable storage to guarantee journal vs metadata ordering
> + * is correctly maintained in the storage media. This will always
> + * fit in the iclog we have been already been passed.
> + */
> + if (optype & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> + iclog->ic_flags |= (XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA);
> + ASSERT(!contwr);
> + }
>
> + while (lv) {
> + lv = xlog_write_single(lv, ticket, iclog, &log_offset,
> + &len, &record_cnt, &data_cnt);
> + if (!lv)
> break;
>
> + ASSERT(!(optype & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)));
> + lv = xlog_write_partial(log, lv, ticket, &iclog, &log_offset,
> + &len, &record_cnt, &data_cnt, &contwr);
> + if (IS_ERR(lv)) {
> + error = PTR_ERR(lv);
> + break;
> }
> }
Maybe user IS_ERR_OR_NULL and PTR_ERR_OR_ZERO here to catch the NULL
case as well? e.g.
for (;;) {
....
lv = xlog_write_partial();
if (IS_ERR_OR_NULL(lv)) {
error = PTR_ERR_OR_ZERO(lv);
break;
}
}
> diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> index acf20c2e5018..c978c52e7ba8 100644
> --- a/fs/xfs/xfs_log_cil.c
> +++ b/fs/xfs/xfs_log_cil.c
> @@ -896,7 +896,6 @@ xlog_cil_push_work(
> num_iovecs += lvhdr.lv_niovecs;
> num_bytes += lvhdr.lv_bytes;
>
> -
> /*
This seems misplaced.
