From f3a9d2e0cd9fd0086a470752b6be8b9ce30e8e1a Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Mon, 6 Feb 2023 20:33:21 -0800 Subject: [PATCH 1/7] technically possible for child_type == CHILD_TYPE_AOF without active child --- src/aof.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/aof.cpp b/src/aof.cpp index 94229169b..0044c02ed 100644 --- a/src/aof.cpp +++ b/src/aof.cpp @@ -752,7 +752,7 @@ void feedAppendOnlyFile(struct redisCommand *cmd, int dictid, robj **argv, int a * accumulate the differences between the child DB and the current one * in a buffer, so that when the child process will do its work we * can append the differences to the new append only file. */ - if (g_pserver->child_type == CHILD_TYPE_AOF) + if (hasActiveChildProcess() && g_pserver->child_type == CHILD_TYPE_AOF) aofRewriteBufferAppend((unsigned char*)buf,sdslen(buf)); sdsfree(buf); From 3492615c6a32cc8bfbf9bf81c0a4d34588174cbf Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Thu, 9 Feb 2023 09:39:25 -0800 Subject: [PATCH 2/7] don't release lock on child as it can hang --- src/server.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/server.cpp b/src/server.cpp index b2b91cb55..ac1f17d3a 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -6888,7 +6888,6 @@ int redisFork(int purpose) { latencyAddSampleIfNeeded("fork-lock",(ustime()-startWriteLock)/1000); if ((childpid = fork()) == 0) { /* Child */ - aeReleaseForkLock(); g_pserver->in_fork_child = purpose; setOOMScoreAdj(CONFIG_OOM_BGCHILD); setupChildSignalHandlers(); From b5ab1d64e2b03276fc9d9e455e23ac4bae3928da Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Thu, 9 Feb 2023 10:28:22 -0800 Subject: [PATCH 3/7] need child specific release that doesn't trigger cv --- src/ae.cpp | 5 +++++ src/ae.h | 1 + src/readwritelock.h | 14 ++++++++++++++ src/server.cpp | 1 + 4 files changed, 21 insertions(+) diff --git a/src/ae.cpp b/src/ae.cpp index 9d8c945d5..af4bd1369 100644 --- a/src/ae.cpp +++ b/src/ae.cpp @@ -859,6 +859,11 @@ void aeReleaseForkLock() g_forkLock.downgradeWrite(); } +void aeReleaseForkLockChild() +{ + g_forkLock.downgradeWriteChild(); +} + int aeThreadOwnsLock() { return fOwnLockOverride || g_lock.fOwnLock(); diff --git a/src/ae.h b/src/ae.h index cd513f652..384aaaa2b 100644 --- a/src/ae.h +++ b/src/ae.h @@ -171,6 +171,7 @@ int aeTryAcquireLock(int fWeak); void aeThreadOffline(); void aeReleaseLock(); void aeReleaseForkLock(); +void aeReleaseForkLockChild(); int aeThreadOwnsLock(); void aeSetThreadOwnsLockOverride(int fOverride); int aeLockContested(int threshold); diff --git a/src/readwritelock.h b/src/readwritelock.h index a7318a29f..4cc839991 100644 --- a/src/readwritelock.h +++ b/src/readwritelock.h @@ -77,11 +77,25 @@ public: m_cv.notify_all(); } + void releaseWriteChild(bool exclusive = true) { + std::unique_lock rm(m_readLock); + serverAssert(m_writeCount > 0); + if (exclusive) + m_writeLock.unlock(); + m_writeCount--; + m_writeWaiting = false; + } + void downgradeWrite(bool exclusive = true) { releaseWrite(exclusive); acquireRead(); } + void downgradeWriteChild(bool exclusive = true) { + releaseWriteChild(exclusive); + acquireRead(); + } + bool hasReader() { return m_readCount > 0; } diff --git a/src/server.cpp b/src/server.cpp index ac1f17d3a..d3e641d18 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -6888,6 +6888,7 @@ int redisFork(int purpose) { latencyAddSampleIfNeeded("fork-lock",(ustime()-startWriteLock)/1000); if ((childpid = fork()) == 0) { /* Child */ + aeReleaseForkLockChild(); g_pserver->in_fork_child = purpose; setOOMScoreAdj(CONFIG_OOM_BGCHILD); setupChildSignalHandlers(); From 0cba0ed30be1c0f70abd5c6ba958d1b869280164 Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Thu, 9 Feb 2023 12:06:27 -0800 Subject: [PATCH 4/7] refactor aeReleaseForkLockChild to capture releaseRead case --- src/ae.cpp | 4 ++-- src/ae.h | 2 +- src/readwritelock.h | 21 +++++++-------------- src/server.cpp | 3 ++- 4 files changed, 12 insertions(+), 18 deletions(-) diff --git a/src/ae.cpp b/src/ae.cpp index af4bd1369..99093daa8 100644 --- a/src/ae.cpp +++ b/src/ae.cpp @@ -859,9 +859,9 @@ void aeReleaseForkLock() g_forkLock.downgradeWrite(); } -void aeReleaseForkLockChild() +void aeForkLockInChild() { - g_forkLock.downgradeWriteChild(); + g_forkLock.setNotify(false); } int aeThreadOwnsLock() diff --git a/src/ae.h b/src/ae.h index 384aaaa2b..3868db4a0 100644 --- a/src/ae.h +++ b/src/ae.h @@ -171,7 +171,7 @@ int aeTryAcquireLock(int fWeak); void aeThreadOffline(); void aeReleaseLock(); void aeReleaseForkLock(); -void aeReleaseForkLockChild(); +void aeForkLockInChild(); int aeThreadOwnsLock(); void aeSetThreadOwnsLockOverride(int fOverride); int aeLockContested(int threshold); diff --git a/src/readwritelock.h b/src/readwritelock.h index 4cc839991..79f0ac710 100644 --- a/src/readwritelock.h +++ b/src/readwritelock.h @@ -8,6 +8,7 @@ class readWriteLock { int m_readCount = 0; int m_writeCount = 0; bool m_writeWaiting = false; + bool m_notify = true; public: readWriteLock(const char *name) : m_readLock(name), m_writeLock(name) {} @@ -65,7 +66,8 @@ public: void releaseRead() { std::unique_lock rm(m_readLock); m_readCount--; - m_cv.notify_all(); + if (m_notify) + m_cv.notify_all(); } void releaseWrite(bool exclusive = true) { @@ -74,16 +76,8 @@ public: if (exclusive) m_writeLock.unlock(); m_writeCount--; - m_cv.notify_all(); - } - - void releaseWriteChild(bool exclusive = true) { - std::unique_lock rm(m_readLock); - serverAssert(m_writeCount > 0); - if (exclusive) - m_writeLock.unlock(); - m_writeCount--; - m_writeWaiting = false; + if (m_notify) + m_cv.notify_all(); } void downgradeWrite(bool exclusive = true) { @@ -91,9 +85,8 @@ public: acquireRead(); } - void downgradeWriteChild(bool exclusive = true) { - releaseWriteChild(exclusive); - acquireRead(); + void setNotify(bool notify) { + m_notify = notify; } bool hasReader() { diff --git a/src/server.cpp b/src/server.cpp index d3e641d18..9b983585e 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -6888,7 +6888,8 @@ int redisFork(int purpose) { latencyAddSampleIfNeeded("fork-lock",(ustime()-startWriteLock)/1000); if ((childpid = fork()) == 0) { /* Child */ - aeReleaseForkLockChild(); + aeForkLockInChild(); + aeReleaseForkLock(); g_pserver->in_fork_child = purpose; setOOMScoreAdj(CONFIG_OOM_BGCHILD); setupChildSignalHandlers(); From 932fc207eadb6cc068263f4ccfac29377dc06c10 Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Thu, 9 Feb 2023 12:22:29 -0800 Subject: [PATCH 5/7] make bgsave fork by default --- src/config.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.cpp b/src/config.cpp index bb2a1f413..a6d5562f0 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -2779,7 +2779,7 @@ standardConfig configs[] = { createBoolConfig("appendonly", NULL, MODIFIABLE_CONFIG, g_pserver->aof_enabled, 0, NULL, updateAppendonly), createBoolConfig("cluster-allow-reads-when-down", NULL, MODIFIABLE_CONFIG, g_pserver->cluster_allow_reads_when_down, 0, NULL, NULL), createBoolConfig("delete-on-evict", NULL, MODIFIABLE_CONFIG, cserver.delete_on_evict, 0, NULL, NULL), - createBoolConfig("use-fork", NULL, IMMUTABLE_CONFIG, cserver.fForkBgSave, 0, NULL, NULL), + createBoolConfig("use-fork", NULL, IMMUTABLE_CONFIG, cserver.fForkBgSave, 1, NULL, NULL), createBoolConfig("io-threads-do-reads", NULL, IMMUTABLE_CONFIG, fDummy, 0, NULL, NULL), createBoolConfig("time-thread-priority", NULL, IMMUTABLE_CONFIG, cserver.time_thread_priority, 0, NULL, NULL), createBoolConfig("prefetch-enabled", NULL, MODIFIABLE_CONFIG, g_pserver->prefetch_enabled, 1, NULL, NULL), From 6c26f91bcdb38170573746d093c78cca606b53a9 Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Fri, 10 Feb 2023 01:13:12 -0800 Subject: [PATCH 6/7] rdb_child_pid isn't the correct value --- src/rdb.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rdb.cpp b/src/rdb.cpp index f3ab5898b..4d699e6fb 100644 --- a/src/rdb.cpp +++ b/src/rdb.cpp @@ -3694,7 +3694,7 @@ void killRDBChild(bool fSynchronous) { serverAssert(GlobalLocksAcquired()); if (cserver.fForkBgSave) { - kill(g_pserver->rdb_child_pid,SIGUSR1); + kill(g_pserver->child_pid,SIGUSR1); } else { g_pserver->rdbThreadVars.fRdbThreadCancel = true; if (g_pserver->rdb_child_type == RDB_CHILD_TYPE_SOCKET) { From 9f4c6e6f153b962c0c5615d7cca8786b93f1e87e Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Fri, 10 Feb 2023 01:23:52 -0800 Subject: [PATCH 7/7] using forked bg save causes more harm than good --- src/config.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.cpp b/src/config.cpp index a6d5562f0..bb2a1f413 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -2779,7 +2779,7 @@ standardConfig configs[] = { createBoolConfig("appendonly", NULL, MODIFIABLE_CONFIG, g_pserver->aof_enabled, 0, NULL, updateAppendonly), createBoolConfig("cluster-allow-reads-when-down", NULL, MODIFIABLE_CONFIG, g_pserver->cluster_allow_reads_when_down, 0, NULL, NULL), createBoolConfig("delete-on-evict", NULL, MODIFIABLE_CONFIG, cserver.delete_on_evict, 0, NULL, NULL), - createBoolConfig("use-fork", NULL, IMMUTABLE_CONFIG, cserver.fForkBgSave, 1, NULL, NULL), + createBoolConfig("use-fork", NULL, IMMUTABLE_CONFIG, cserver.fForkBgSave, 0, NULL, NULL), createBoolConfig("io-threads-do-reads", NULL, IMMUTABLE_CONFIG, fDummy, 0, NULL, NULL), createBoolConfig("time-thread-priority", NULL, IMMUTABLE_CONFIG, cserver.time_thread_priority, 0, NULL, NULL), createBoolConfig("prefetch-enabled", NULL, MODIFIABLE_CONFIG, g_pserver->prefetch_enabled, 1, NULL, NULL),