Merge pull request #216 from basho/bugfix/file-delete-blocking

Prevent one frozen dir from blocking other deletes

Reviewed-by: mrallen1

Author: borshop

src/bitcask.erl

@@ -2835,6 +2835,73 @@ delete_keydir_test2() ->
     bitcask:close(KDB),
     ok.
 
+retry_until_true(_, _, N) when N =< 0 ->
+   false;
+retry_until_true(F, Delay, Retries) ->
+    case F() of
+        true ->
+            true;
+        _ ->
+            timer:sleep(Delay),
+            retry_until_true(F, Delay, Retries - 1)
+    end.
+
+no_pending_delete_bottleneck_test_() ->
+    {timeout, 30, fun no_pending_delete_bottleneck_test2/0}.
+
+no_pending_delete_bottleneck_test2() ->
+    % Populate B1 and B2. Then put till frozen both after reopening.
+    % Delete all keys in both. Merge in both. Unfreeze B2. With the bug
+    % B2 files will not be deleted.
+
+    Dir1 = "/tmp/bc.test.del.block.1",
+    Dir2 = "/tmp/bc.test.del.block.2",
+
+    Data = default_dataset(),
+    bitcask:close(init_dataset(Dir1, [{max_file_size, 1}], Data)),
+    bitcask:close(init_dataset(Dir2, [{max_file_size, 1}], Data)),
+
+    B1 = bitcask:open(Dir1, [read_write]),
+    B2 = bitcask:open(Dir2, [read_write]),
+
+    Files2 = readable_files(Dir2),
+
+    FileDeleted = fun(Fname) ->
+                          not filelib:is_regular(Fname)
+                  end,
+
+    FilesDeleted = fun() ->
+                           lists:all(FileDeleted, Files2)
+                   end,
+
+    try
+        bitcask:iterator(B1, -1, -1),
+        put_till_frozen(B1),
+        [begin
+             ?assertEqual(ok, bitcask:delete(B1, K))
+         end || {K, _} <- Data],
+        bitcask:merge(Dir1),
+
+        bitcask:iterator(B2, -1, -1),
+        put_till_frozen(B2),
+        [begin
+             ?assertEqual(ok, bitcask:delete(B2, K))
+         end || {K, _} <- Data],
+        bitcask:merge(Dir2),
+        bitcask:iterator_release(B2),
+
+        %% Timing is everything. This test will timeout in 60 seconds.
+        %% The merge delete worker works on a 1 second tick. So it should
+        %% have at least a chance to delete the files in 10 seconds.
+        %% Famous last words.
+        ?assert(retry_until_true(FilesDeleted, 1000, 10))
+    after
+        catch bitcask:close(B1),
+        catch bitcask:close(B2)
+    end.
+
+
+
 frag_status_test_() ->
     {timeout, 60, fun frag_status_test2/0}.
 

src/bitcask_merge_delete.erl

@@ -82,7 +82,7 @@ handle_call({defer_delete, Dirname, IterGeneration, Files}, _From, State) ->
 handle_call({queue_length}, _From, State) ->
     {reply, queue:len(State#state.q), State, ?TIMEOUT};
 handle_call({testonly__delete_trigger}, _From, State) ->
-    {reply, ok, check_status(State), ?TIMEOUT};
+    {reply, ok, delete_ready_files(State), ?TIMEOUT};
 handle_call(_Request, _From, State) ->
     Reply = unknown_request,
     {reply, Reply, State, ?TIMEOUT}.
@@ -91,7 +91,7 @@ handle_cast(_Msg, State) ->
     {noreply, State, ?TIMEOUT}.
 
 handle_info(timeout, State) ->
-    {noreply, check_status(State), ?TIMEOUT};
+    {noreply, delete_ready_files(State), ?TIMEOUT};
 handle_info(_Info, State) ->
     {noreply, State, ?TIMEOUT}.
 
@@ -105,29 +105,37 @@ code_change(_OldVsn, State, _Extra) ->
 %%% Internal functions
 %%%===================================================================
 
-check_status(S) ->
+delete_ready_files(S) ->
+    delete_ready_files(S, queue:new()).
+
+delete_ready_files(S, PendingQ) ->
     case queue:out(S#state.q) of
         {empty, _} ->
-            S;
-        {{value, {Dirname, IterGeneration, Files}}, NewQ} ->
+            S#state{q = PendingQ};
+        {{value, {Dirname, IterGeneration, Files} = Entry}, NewQ} ->
             {_, KeyDir} = bitcask_nifs:keydir_new(Dirname),
             try
 
                 {_,_,_,IterStatus,_} = bitcask_nifs:keydir_info(KeyDir),
-                CleanAndGo = fun() ->
-                                     delete_files(Files),
-                                     bitcask_nifs:keydir_release(KeyDir),
-                                     check_status(S#state{q = NewQ})
-                             end,
-                case IterStatus of
-                    {_, _, false, _} ->
-                        CleanAndGo();
-                    {CurGen, _, true, _} when CurGen > IterGeneration ->
-                        CleanAndGo();
-                    _ ->
-                        %% Do nothing, ignore NewQ
-                        S
+                ReadyToDelete =
+                    case IterStatus of
+                        {_, _, false, _} ->
+                            true;
+                        {CurGen, _, true, _} when CurGen > IterGeneration ->
+                            true;
+                        _ ->
+                            false
+                    end,
+                S2 = S#state{q = NewQ},
+                case ReadyToDelete of
+                    true ->
+                        delete_files(Files),
+                        bitcask_nifs:keydir_release(KeyDir),
+                        delete_ready_files(S2, PendingQ);
+                    false ->
+                        delete_ready_files(S2, queue:in(Entry, PendingQ))
                 end
+
             catch _X:_Y ->
                     %% Not sure what problem was: keydir is no longer
                     %% valid, or a problem deleting files, but in any