From: Filipe Manana fdmanana@suse.com
[ Upstream commit e6dd405b6671b9753b98d8bdf76f8f0ed36c11cd ]
In the process_one_buffer() log tree walk callback we return errors to the log tree walk caller and then the caller aborts the transaction, if we have one, or turns the fs into error state if we don't have one. While this reduces code it makes it harder to figure out where exactly an error came from. So add the transaction aborts after every failure inside the process_one_buffer() callback, so that it helps figuring out why failures happen.
Reviewed-by: Boris Burkov boris@bur.io Reviewed-by: Qu Wenruo wqu@suse.com Signed-off-by: Filipe Manana fdmanana@suse.com Reviewed-by: David Sterba dsterba@suse.com Signed-off-by: David Sterba dsterba@suse.com Signed-off-by: Sasha Levin sashal@kernel.org ---
LLM Generated explanations, may be completely bogus:
Based on my extensive investigation, I need to provide my analysis:
## Analysis Complete
**Backport Status: YES**
### Rationale:
This commit is part of a coordinated effort by Filipe Manana to improve error handling and debugging in btrfs log replay code. While primarily a **debugging/observability improvement**, it does include a functional enhancement.
### Code Changes Analysis:
**1. Adds explicit transaction aborts at error sites** (fs/btrfs/tree- log.c:347-389):
- **Before**: Errors from `btrfs_read_extent_buffer()`, `btrfs_pin_extent_for_log_replay()`, and `btrfs_exclude_logged_extents()` were returned to the caller, and the caller would abort the transaction - **After**: Transaction aborts happen immediately at the failure point with either `btrfs_abort_transaction()` or `btrfs_handle_fs_error()`
**2. Adds missing error handling** for `btrfs_exclude_logged_extents()`:
```c // BEFORE: ret assigned but transaction not aborted here if (btrfs_buffer_uptodate(eb, gen, 0) && btrfs_header_level(eb) == 0) ret = btrfs_exclude_logged_extents(eb);
// AFTER: Explicit error check and abort if (btrfs_buffer_uptodate(eb, gen, 0) && btrfs_header_level(eb) == 0) { ret = btrfs_exclude_logged_extents(eb); if (ret) btrfs_abort_transaction(trans, ret); } ```
**3. Adds ASSERT** for transaction handle validation in the wc->pin path
### Why Backport:
1. **Part of coordinated error handling improvements**: This is commit 2/4 in a series (preceded by 6ebd726b104fa on same day, followed by commits 77ea42e3faa1f, 97b027586a189, and ultimately 2753e49176240 which enhances all these aborts with detailed debugging)
2. **Slightly improves error handling**: While errors were eventually handled, this ensures transaction aborts happen closer to the failure point, potentially preventing additional operations on a doomed transaction
3. **Minimal risk**: The changes only add error handling paths without modifying core logic
4. **Already selected by AUTOSEL**: Commit exists as 5650f88050dd5 with backport tag, indicating it passed automated selection criteria
5. **Well-reviewed**: Has reviews from Boris Burkov, Qu Wenruo, and David Sterba
6. **Confined scope**: Changes limited to process_one_buffer() in tree- log.c
### Caveats:
- **No "Fixes:" tag**: Author did not tag this as fixing a specific bug - **No "Cc: stable" tag**: Author did not explicitly request stable backport - **Primary benefit is debugging**: The main value is better error diagnosis rather than fixing user-visible issues - **Part of larger series**: Functions best with the full series of log replay improvements
**Verdict**: YES - This is a quality improvement that enhances error handling with minimal risk. While borderline as it's primarily for debugging, the slight functional improvement to error handling and AUTOSEL selection support backporting to stable trees.
fs/btrfs/tree-log.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 6d92326a1a0c7..50ed84cb68a69 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -347,6 +347,7 @@ static int process_one_buffer(struct btrfs_root *log, struct extent_buffer *eb, struct walk_control *wc, u64 gen, int level) { + struct btrfs_trans_handle *trans = wc->trans; struct btrfs_fs_info *fs_info = log->fs_info; int ret = 0;
@@ -361,18 +362,29 @@ static int process_one_buffer(struct btrfs_root *log, };
ret = btrfs_read_extent_buffer(eb, &check); - if (ret) + if (ret) { + if (trans) + btrfs_abort_transaction(trans, ret); + else + btrfs_handle_fs_error(fs_info, ret, NULL); return ret; + } }
if (wc->pin) { - ret = btrfs_pin_extent_for_log_replay(wc->trans, eb); - if (ret) + ASSERT(trans != NULL); + ret = btrfs_pin_extent_for_log_replay(trans, eb); + if (ret) { + btrfs_abort_transaction(trans, ret); return ret; + }
if (btrfs_buffer_uptodate(eb, gen, 0) && - btrfs_header_level(eb) == 0) + btrfs_header_level(eb) == 0) { ret = btrfs_exclude_logged_extents(eb); + if (ret) + btrfs_abort_transaction(trans, ret); + } } return ret; }