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

John Stoffel
2025-03-17T21:04:08Z

>>>>> "Kent" == Kent Overstreet <kent.overstreet@linux.dev> writes:

> 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.

So will open() just return an error? How will this look from
userspace?

> - 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.

Why does it need a new checksum if there are read errors? What
happens if there are more read errors? If I have a file on a
filesystem which is based in spinning rust and I get a single bit
flip, I'm super happy you catch it.

But now you re-checksum the file, with the read error, and return it?
I'm stupid and just a user/IT guy. I want notifications, but I don't
want my application to block so I can't kill it, or unmount the
filesystem. Or continue to use it if I like.

> 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.

Sorry for being dense, but does a file have one or more extents? Or
is this at a level below that?

> 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.

When doing these retries, is the filesystem locked up or will the
process doing the read() be blocked from being killed?

> - 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).

So if just a single bit flips, the extent gets moved onto better
storage, and the file gets re-checksummed. But what about if more
bits go bad afterwards?

> 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.

What does this mean exactly in terms of end user and SysAdmin point of
view? Locking up the system (no new writes to the now problematic
storage) that I can't kill would be annoyingly painful.

Don't get me wrong, I'm happy to see data integrity stuff get added,
I'm just trying to understand how it interacts with userspace.

John