[PATCH] ext4: fix dirtyclusters double decrement on fs shutdown

[Brian Foster <bfoster@redhat.com>]

fstests test generic/388 occasionally reproduces a warning in
ext4_put_super() associated with the dirty clusters count:

  WARNING: CPU: 7 PID: 76064 at fs/ext4/super.c:1324 ext4_put_super+0x48c/0x590 [ext4]

Tracing the failure shows that the warning fires due to an
s_dirtyclusters_counter value of -1. IOW, this appears to be a
spurious decrement as opposed to some sort of leak. Further tracing
of the dirty cluster count deltas and an LLM scan of the resulting
output identified the cause as a double decrement in the error path
between ext4_mb_mark_diskspace_used() and the caller
ext4_mb_new_blocks().

First, note that generic/388 is a shutdown vs. fsstress test and so
produces a random set of operations and shutdown injections. In the
problematic case, the shutdown triggers an error return from the
ext4_handle_dirty_metadata() call(s) made from
ext4_mb_mark_context(). The changed value is non-zero at this point,
so ext4_mb_mark_diskspace_used() does not exit after the error
bubbles up from ext4_mb_mark_context(). Instead, the former
decrements both cluster counters and returns the error up to
ext4_mb_new_blocks(). The latter falls into the !ar->len out path
which decrements the dirty clusters counter a second time, creating
the inconsistency.

AFAICT the solution here is to exit immediately from
ext4_mb_mark_diskspace_used() on error, regardless of the changed
value. This leaves the caller responsible for clearing the block
reservation at the same level it is acquired. This also skips the
free clusters update, but the caller also calls into
ext4_discard_allocated_blocks() to free the blocks back into the
group. This survives an overnight loop test of generic/388 on an
otherwise reproducing system and survives a local regression run.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---

Hi all,

I've thrown some testing at this and poked around the area enough that I
_think_ it is reasonably sane, but the error paths are hairy and I could
certainly be missing some details. I'm happy to try a different approach
if there are any thoughts around that.. thanks.

Brian

 fs/ext4/mballoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 56d50fd3310b..224abfd6a42b 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4234,7 +4234,7 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 				   ac->ac_b_ex.fe_start, ac->ac_b_ex.fe_len,
 				   flags, &changed);
 
-	if (err && changed == 0)
+	if (err)
 		return err;
 
 #ifdef AGGRESSIVE_CHECK
-- 
2.51.1