Re: [PATCH 8/8] xfs: order CIL checkpoint start records

On Thu, Jun 17, 2021 at 02:31:43PM -0700, Darrick J. Wong wrote:

On Thu, Jun 17, 2021 at 06:26:17PM +1000, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

Because log recovery depends on strictly ordered start records as
well as strictly ordered commit records.

This is a zero day bug in the way XFS writes pipelined transactions
to the journal which is exposed by commit facd77e4e38b ("xfs: CIL
work is serialised, not pipelined") which re-introduces explicit
concurrent commits back into the on-disk journal.

The XFS journal commit code has never ordered start records and we
have relied on strict commit record ordering for correct recovery
ordering of concurrently written transactions. Unfortunately, root
cause analysis uncovered the fact that log recovery uses the LSN of
the start record for transaction commit processing. Hence the
commits are processed in strict orderi by recovery, but the LSNs

s/orderi/order/ ?

quoted

associated with the commits can be out of order and so recovery may
stamp incorrect LSNs into objects and/or misorder intents in the AIL
for later processing. This can result in log recovery failures
and/or on disk corruption, sometimes silent.

Because this is a long standing log recovery issue, we can't just
fix log recovery and call it good.

Could there be production filesystems out there that have this
mismatched ordering of start lsn and commit lsn?  This still leaves the
mystery of crashed customer filesystems containing btree blocks where
128 bytes in the middle clearly contain contents that are don't match or
duplicate the rest of the block, as though someone forgot to replay a
buffer vector or something.

Modulo bugs in delayed logging, I doubt there's any delayed logging
filesystems out there that have the problem. Older, non-delayed
logging filesystems are almost certain to see it, but they have much
smaller transactions and only EFIs to deal with so the corruption
risk is much, much, much lower.

What would a fix to log recovery entail?  Not skipping recovered items
if the start/commit sequencing is not the same?  Or am I not
understanding the problem correctly?

I've been going back and forth on this trying to come up with a sane
solution, but I haven't come up with anything practical.

We could use the commit record LSN for recovery, but we write start
record LSNs into on-disk metadata when we flush it to disk and that
forces checkpoints that need recovery to use the same LSN in the
metadata it recovers and writes back as we use for runtime
writeback. Hence we then get problems with recovered filesystems not
having the same on-disk state as they would if the metadata was
written back from in-memory. i.e. two pieces of metadata in the same
atomic transaction could have different LSNs stamped in them
depending on whether they were written back at runtime or recovered
by log recovery at mount time...

And then my head explodes trying to work out what happens when we
have overlapping checkpoints and partial metadata writeback and
different LSN values for recovery vs writeback and recovery retries
after a failed recovery and <BOOM>

However, given that there are runtime integrity issues with out of
order start LSNs (log head can overwrite the log tail - I can give
more detail if you want), the only way out of this I can see is to
ensure that the start records are properly ordered at runtime to
avoid all the potential runtime issues that exist.  This also has
the nice "side effect" of avoiding the log recovery LSN ordering
problem.

IOWs, I'm not looking at this as log recovery bug that needs fixing.
Yes, there is a log recovery issue there (and has been forever), but
the more I think on this, the more I'm concerned about the potential
runtime impacts on data integrity correctness and potential
head-tail journal overwrite corruption. 

quoted

+	ctx->commit_lsn = lsn;
+	wake_up_all(&cil->xc_commit_wait);
+	spin_unlock(&cil->xc_push_lock);
 }
 
 /*

@@ -834,10 +849,16 @@ xlog_cil_set_ctx_write_state(
  * relies on the context LSN being zero until the log write has guaranteed the
  * LSN that the log write will start at via xlog_state_get_iclog_space().
  */
+enum {
+	_START_RECORD,
+	_COMMIT_RECORD,
+};

Stupid nit: If this enum had a name you could skip the default clause
below because the compiler would typecheck the usage for you.

OK.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com