Skip to content

Commit

Permalink
Fix fork done handler wrongly update fsync metrics and enhance AOF_FS…
Browse files Browse the repository at this point in the history
…YNC_ALWAYS

This PR fix two bugs:

1. About the MP-AOF one, that code assumes that we started a new AOF file just
now (when we have a new base into which we're gonna incrementally write), but the
fact is that with multipart-aof, the fork done handler doesn't really affect the
incremental file being maintained by the parent process, there's no reason to re-issue
SELECT, and i suppose there's no reason to update any of the fsync metrics.
This should have been deleted with MP-AOF (introduced in redis#9788, 7.0).
The aof_fsync_offset it updates will cause us to miss an fsync in flushAppendOnlyFile,
if we stop write commands in AOF_FSYNC_EVERYSEC

2. About the AOF_FSYNC_ALWAYS one, it is follow redis#6053 (the details is in redis#5985),
we also check AOF_FSYNC_ALWAYS to prevent users from switching from everysec to always.

We detected these problems with WAITAOF test (redis 7.2), introduced in redis#11713.
  • Loading branch information
enjoy-binbin committed Mar 26, 2023
1 parent aa2403c commit 91cb649
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 9 deletions.
18 changes: 9 additions & 9 deletions src/aof.c
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,15 @@ void flushAppendOnlyFile(int force) {
server.unixtime > server.aof_last_fsync &&
!(sync_in_progress = aofFsyncInProgress())) {
goto try_fsync;

/* Check if we need to do fsync even the aof buffer is empty,
* the reason is described in the previous AOF_FSYNC_EVERYSEC if,
* and AOF_FSYNC_ALWAYS is also checked here to prevent users from
* changing the aof_fsync from everysec to always. */
} else if (server.aof_fsync == AOF_FSYNC_ALWAYS &&
server.aof_fsync_offset != server.aof_current_size)
{
goto try_fsync;
} else {
return;
}
Expand Down Expand Up @@ -2666,15 +2675,6 @@ void backgroundRewriteDoneHandler(int exitcode, int bysignal) {
/* We can safely let `server.aof_manifest` point to 'temp_am' and free the previous one. */
aofManifestFreeAndUpdate(temp_am);

if (server.aof_fd != -1) {
/* AOF enabled. */
server.aof_selected_db = -1; /* Make sure SELECT is re-issued */
server.aof_current_size = getAppendOnlyFileSize(new_base_filename, NULL) + server.aof_last_incr_size;
server.aof_rewrite_base_size = server.aof_current_size;
server.aof_fsync_offset = server.aof_current_size;
server.aof_last_fsync = server.unixtime;
}

/* We don't care about the return value of `aofDelHistoryFiles`, because the history
* deletion failure will not cause any problems. */
aofDelHistoryFiles();
Expand Down
29 changes: 29 additions & 0 deletions tests/unit/wait.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,39 @@ tags {"wait aof network external:skip"} {
$replica config set appendfsync everysec

test {WAITAOF replica copy everysec} {
$replica config set appendfsync everysec
waitForBgrewriteaof $replica ;# Make sure there is no AOFRW

$master incr foo
assert_equal [$master waitaof 0 1 0] {1 1}
}

test {WAITAOF replica copy everysec with AOFRW} {
$replica config set appendfsync everysec

# Need an AOFRW that can be done in a second.
$replica bgrewriteaof

$master incr foo
assert_equal [$master waitaof 0 1 0] {1 1}
}

test {WAITAOF replica copy everysec->always with AOFRW} {
$replica config set appendfsync everysec

waitForBgrewriteaof $replica

# Need an AOFRW that can be done in a second.
$replica bgrewriteaof

$master incr foo

# Change appendfsync from everysec to always
$replica config set appendfsync always

assert_equal [$master waitaof 0 1 0] {1 1}
}

test {WAITAOF replica copy appendfsync always} {
$replica config set appendfsync always
$master incr foo
Expand Down

0 comments on commit 91cb649

Please sign in to comment.