rgw: address crash and race in RGWIndexCompletionManager

An atomic int was used in a modulo operator to distribute contention
among a set of locks and to track completions. Because it was an int,
enough increments would cause it to go negative (due to
twos-complement encoding and overflow) thereby causing a
crash. Additionally, even though it was atomic, the read and increment
were separate operations, leading to a race.

This commit addresses both of these issues.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>

diff --git a/src/rgw/rgw_rados.cc b/src/rgw/rgw_rados.cc
index b9375d50ce1a5c925fbce8d8c3cdfdedd86402e4..4b7c236018e499a2c0f4373fa12a479c3b95754d 100644 (file)
--- a/src/rgw/rgw_rados.cc
+++ b/src/rgw/rgw_rados.cc
@@ -774,7 +774,7 @@ struct complete_op_data {
 
 class RGWIndexCompletionManager {
   RGWRados* const store;
-  const int num_shards;
+  const uint32_t num_shards;
   ceph::containers::tiny_vector<ceph::mutex> locks;
   std::vector<set<complete_op_data*>> completions;
   std::vector<complete_op_data*> retry_completions;
@@ -784,7 +784,10 @@ class RGWIndexCompletionManager {
   bool _stop{false};
   std::thread retry_thread;
 
-  std::atomic<int> cur_shard {0};
+  // used to distribute the completions and the locks they use across
+  // their respective vectors; it will get incremented and can wrap
+  // around back to 0 without issue
+  std::atomic<uint32_t> cur_shard {0};
 
   void process();
   
@@ -797,7 +800,7 @@ class RGWIndexCompletionManager {
       retry_thread.join();
     }
 
-    for (int i = 0; i < num_shards; ++i) {
+    for (uint32_t i = 0; i < num_shards; ++i) {
       std::lock_guard l{locks[i]};
       for (auto c : completions[i]) {
         c->stop();
@@ -806,10 +809,8 @@ class RGWIndexCompletionManager {
     completions.clear();
   }
   
-  int next_shard() {
-    int result = cur_shard % num_shards;
-    cur_shard++;
-    return result;
+  uint32_t next_shard() {
+    return cur_shard++ % num_shards;
   }
 
 public: