On Thu, Feb 25, 2021 at 07:54:01PM +0100, Christoph Hellwig wrote:
> 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?
Oh, it was debug code I added while tracking down loop iteration
bugs. I forgot to remove it - it didn't actually catch any bugs...
> > + 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;
> }
> }
Sure.
> > 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.
Yeah, another bad debug cleanup.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
