storage: assert out on EIO in read #7633
Conversation
| if (ec.code().value() == EIO) { | ||
| vassert(false, "I/O error during read! Disk failure?"); |
There was a problem hiding this comment.
+1
I'm curious if we'll hit a crash loop in automated deployments, but this still seems much better than the alternative. I wonder if longer term we'd want to peel back this limitation, e.g. assuming the entire disk isn't fully borked, stopping/moving just the replicas that hit a bad reads, while gracefully decommissioning the node.
There was a problem hiding this comment.
Longer term, I suspect that for things like decode errors we'll want to treat them as non-crashing errors and report a "damaged" partition, but for EIOs we might always keep the termination behavior, as it's such a critical failure indicator.
|
/backport v22.3.x |
|
/backport v22.2.x |
|
/backport v22.1.x |
|
Failed to run cherry-pick command. I executed the below command: |
|
Manual backport to v22.1.x at #7703 |
| return ss::make_exception_future<result<records_t>>( | ||
| std::current_exception()); |
There was a problem hiding this comment.
I think that we should pass ec here instead of std::current_exception. While future::handle_exception_type does invoke its continuation inside a catch block, that doesn't seem to be a guarantee that the API needs to maintain.
We already assert out on EIO in writes. However on reads we only surfaced the exceptional future, and some code paths (like state_machine.cc) have general exception handlers that retry forever, resulting in nodes with bad disks staying up but not progressing.
Fixes #7637
Backports Required
UX Changes
See release notes.
Release Notes
Bug Fixes