[PATCH 00/14] better handling of checksum errors/bitrot

Kent Overstreet
2025-03-11T20:15:39Z

Roland Vet spotted a good one: currently, rebalance/copygc get stuck if
we've got an extent that can't be read, due to checksum error/bitrot.

This took some doing to fix properly, because

- We don't want to just delete such data (replace it with
KEY_TYPE_error); we never want to delete anything except when
explicitly told to by the user, and while we don't yet have an API for
"read this file even though there's errors, just give me what you
have" we definitely will in the future.

- Not being able to move data is a non-option: that would block copygc,
device removal, etc.

- And, moving said extent requires giving it a new checksum - strictly
required if the move has to fragment it, teaching the write/move path
about extents with bad checksums is unpalateable, and anyways we'd
like to be able to guard against more bitrot, if we can.

So that means:

- Extents need a poison bit: "reads return errors, even though it now
has a good checksum" - this was added in a separate patch queued up
for 6.15.

It's an incompat feature because it's a new extent field, and old
versions can't parse extents with unknown field types, since they
won't know their sizes - meaning users will have to explicitly do an
incompat upgrade to make use of this stuff.

- The read path needs to do additional retries after checksum errors
before giving up and marking it poisoned, so that we don't
accidentally convert a transient error to permanent corruption.

- The read path gets a whole bunch of work to plumb precise modern error
codes around, so that e.g. the retry path, the data move path, and the
"mark extent poisoned" path all know exactly what's going on.

- Read path is responsible for marking extents poisoned after sufficient
retry attempts (controlled by a new filesystem option)

- Data move path is allowed to move extents after a read error, if it's
a checksum error (giving it a new checksum) if it's been poisoned
(i.e. the extent flags feature is enabled).

Code should be more or less finalized - still have more tests for corner
cases to write, but "write corrupt data and then tell rebalance to move
it to another device" works as expected.

TODO:

- NVME has a "read recovery level" attribute that controlls how hard the
erasure coding algorithms work - we want that plumbed.

Before we give up and move data that we know is bad, we need to try
_as hard as possible_ to get a successful read.

Code currently lives in
https://evilpiepirate.org/git/bcachefs.git/log/?h=bcachefs-testing

Kent Overstreet (14):
bcachefs: Convert read path to standard error codes
bcachefs: Fix BCH_ERR_data_read_csum_err_maybe_userspace in retry path
bcachefs: Read error message now indicates if it was for an internal
move
bcachefs: BCH_ERR_data_read_buffer_too_small
bcachefs: Return errors to top level bch2_rbio_retry()
bcachefs: Print message on successful read retry
bcachefs: Don't create bch_io_failures unless it's needed
bcachefs: Checksum errors get additional retries
bcachefs: __bch2_read() now takes a btree_trans
bcachefs: Poison extents that can't be read due to checksum errors
bcachefs: Data move can read from poisoned extents
bcachefs: Debug params for data corruption injection
block: Allow REQ_FUA|REQ_READ
bcachefs: Read retries are after checksum errors now REQ_FUA

block/blk-core.c | 19 +-
fs/bcachefs/bcachefs_format.h | 2 +
fs/bcachefs/btree_io.c | 2 +-
fs/bcachefs/errcode.h | 19 +-
fs/bcachefs/extents.c | 157 +++++++++-------
fs/bcachefs/extents.h | 7 +-
fs/bcachefs/extents_types.h | 11 +-
fs/bcachefs/io_read.c | 325 +++++++++++++++++++++++++---------
fs/bcachefs/io_read.h | 21 +--
fs/bcachefs/io_write.c | 24 +++
fs/bcachefs/move.c | 26 ++-
fs/bcachefs/opts.h | 5 +
fs/bcachefs/super-io.c | 4 +
fs/bcachefs/util.c | 21 +++
fs/bcachefs/util.h | 12 ++
15 files changed, 473 insertions(+), 182 deletions(-)

--
2.47.2