From 7dd07080c78cc7f725424d45a612efa66f6adb83 Mon Sep 17 00:00:00 2001 From: Martin Sumner Date: Tue, 26 Feb 2019 10:33:20 +0000 Subject: [PATCH] Double-check safety of rolling memory Make sure there is no change pending regardless of why maybe_roll_memory has been called. Also, check that the manifest SQN has been incremented before accepting the change. Conflict here would lead to data loss in the penciller, so extra safety is important. --- src/leveled_log.erl | 2 +- src/leveled_penciller.erl | 89 +++++++++++++++++++++------------------ 2 files changed, 48 insertions(+), 43 deletions(-) diff --git a/src/leveled_log.erl b/src/leveled_log.erl index 4c1f061..c88bfc5 100644 --- a/src/leveled_log.erl +++ b/src/leveled_log.erl @@ -98,7 +98,7 @@ {"P0008", {info, "Penciller closing for reason ~w"}}, {"P0010", - {info, "No level zero action on close of Penciller ~w"}}, + {info, "No level zero action on close of Penciller discarded=~w"}}, {"P0011", {info, "Shutdown complete for Penciller for reason ~w"}}, {"P0012", diff --git a/src/leveled_penciller.erl b/src/leveled_penciller.erl index e60a7b6..05744df 100644 --- a/src/leveled_penciller.erl +++ b/src/leveled_penciller.erl @@ -683,15 +683,12 @@ handle_call({push_mem, {LedgerTable, PushedIdx, MinSQN, MaxSQN}}, {reply, returned, State}; {false, true} -> leveled_log:log("P0042", [State#state.levelzero_size]), - % The cache is full (there are 127 items already in it), so - % can't accept any more. However, we need to try and roll - % memory otherwise cache may be permanently full. + % The cache is full (the maximum line items have been reached), so + % can't accept any more. However, we need to try and roll memory + % otherwise cache may be permanently full. gen_server:reply(From, returned), - {L0Pend, L0Constructor, none} = - maybe_roll_memory(State, false), - {noreply, - State#state{levelzero_pending=L0Pend, - levelzero_constructor=L0Constructor}}; + {UpdState, none} = maybe_roll_memory(State, true, false), + {noreply, UpdState}; {false, false} -> % leveled_log:log("P0018", [ok, false, false]), PushedTree = @@ -909,18 +906,19 @@ handle_call(close, _From, State) -> % on the clerk. ok = leveled_pclerk:clerk_close(State#state.clerk), leveled_log:log("P0008", [close]), - L0_Left = State#state.levelzero_size > 0, - case {State#state.levelzero_pending, L0_Left} of - {false, true} -> - {_L0Pend, L0Pid, _L0Bloom} = maybe_roll_memory(State, true), + case State#state.levelzero_pending of + true -> + leveled_log:log("P0010", [State#state.levelzero_size]); + false -> + L0_Left = State#state.levelzero_size > 0, + {UpdState, _L0Bloom} = maybe_roll_memory(State, L0_Left, true), + L0Pid = UpdState#state.levelzero_constructor, case is_pid(L0Pid) of true -> ok = leveled_sst:sst_close(L0Pid); false -> ok - end; - StatusTuple -> - leveled_log:log("P0010", [StatusTuple]) + end end, shutdown_manifest(State#state.manifest, State#state.levelzero_constructor), @@ -958,14 +956,21 @@ handle_call(check_for_work, _From, State) -> handle_call(persisted_sqn, _From, State) -> {reply, State#state.persisted_sqn, State}. -handle_cast({manifest_change, NewManifest}, State) -> - NewManSQN = leveled_pmanifest:get_manifest_sqn(NewManifest), +handle_cast({manifest_change, Manifest}, State) -> + NewManSQN = leveled_pmanifest:get_manifest_sqn(Manifest), OldManSQN = leveled_pmanifest:get_manifest_sqn(State#state.manifest), leveled_log:log("P0041", [OldManSQN, NewManSQN]), - ok = leveled_pclerk:clerk_promptdeletions(State#state.clerk, NewManSQN), - UpdManifest = leveled_pmanifest:merge_snapshot(State#state.manifest, - NewManifest), - {noreply, State#state{manifest = UpdManifest, work_ongoing=false}}; + % Only safe to update the manifest if the SQN increments + if NewManSQN > OldManSQN -> + ok = + leveled_pclerk:clerk_promptdeletions(State#state.clerk, NewManSQN), + % This is accepted as the new manifest, files may be deleted + UpdManifest = + leveled_pmanifest:merge_snapshot(State#state.manifest, Manifest), + % Need to preserve the penciller's view of snapshots stored in + % the manifest + {noreply, State#state{manifest=UpdManifest, work_ongoing=false}} + end; handle_cast({release_snapshot, Snapshot}, State) -> Manifest0 = leveled_pmanifest:release_snapshot(State#state.manifest, Snapshot), @@ -1293,37 +1298,37 @@ update_levelzero(L0Size, {PushedTree, PushedIdx, MinSQN, MaxSQN}, false -> true end, - NoPendingManifestChange = not State#state.work_ongoing, JitterCheck = RandomFactor or CacheMuchTooBig, Due = CacheTooBig and JitterCheck, - case {Due, NoPendingManifestChange} of - {true, true} -> - {L0Pend, L0Constructor, none} = - maybe_roll_memory(UpdState, false), - LogSubs = [NewL0Size, true, true], - leveled_log:log_timer("P0031", LogSubs, SW), - UpdState#state{levelzero_pending=L0Pend, - levelzero_constructor=L0Constructor}; - _ -> - LogSubs = [NewL0Size, Due, NoPendingManifestChange], - leveled_log:log_timer("P0031", LogSubs, SW), - UpdState - end + {UpdState0, _L0Bloom} = maybe_roll_memory(UpdState, Due, false), + LogSubs = [NewL0Size, Due, State#state.work_ongoing], + leveled_log:log_timer("P0031", LogSubs, SW), + UpdState0 end. --spec maybe_roll_memory(pcl_state(), boolean()) - -> {boolean(), pid()|undefined, leveled_ebloom:bloom()|none}. +-spec maybe_roll_memory(pcl_state(), boolean(), boolean()) + -> {pcl_state(), leveled_ebloom:bloom()|none}. %% @doc -%% Check that no L0 file is present before rolling memory -maybe_roll_memory(State, SyncRoll) -> +%% Check that no L0 file is present before rolling memory. Returns a boolean +%% to indicate if memory has been rolled, the Pid of the L0 constructor and +%% The bloom of the L0 file (or none) +maybe_roll_memory(State, false, _SyncRoll) -> + {State, none}; +maybe_roll_memory(State, true, SyncRoll) -> BlockedByL0 = leveled_pmanifest:levelzero_present(State#state.manifest), - case BlockedByL0 of + PendingManifestChange = State#state.work_ongoing, + % It is critical that memory is not rolled if the manifest is due to be + % updated by a change by the clerk. When that manifest change is made it + % will override the addition of L0 and data will be lost. + case (BlockedByL0 or PendingManifestChange) of true -> - {false, undefined, none}; + {State, none}; false -> {L0Constructor, Bloom} = roll_memory(State, SyncRoll), - {true, L0Constructor, Bloom} + {State#state{levelzero_pending=true, + levelzero_constructor=L0Constructor}, + Bloom} end. -spec roll_memory(pcl_state(), boolean())