Merge pull request #66 from HDFGroup/chogan/lock_ids_ptr

Improving locking logic in the MDM
HDFGroup · Jan 20, 2021 · b25af17 · b25af17
2 parents 71ccd1b + 068903b
commit b25af17
Show file tree

Hide file tree

Showing 10 changed files with 271 additions and 141 deletions.
diff --git a/src/api/bucket.h b/src/api/bucket.h
@@ -152,6 +152,8 @@ Status Bucket::PlaceBlobs(std::vector<PlacementSchema> &schemas,
     hermes::Blob blob = {};
     blob.data = (u8 *)blobs[i].data();
     blob.size = blobs[i].size() * sizeof(T);
+    LOG(INFO) << "Attaching blob '" << names[i] << "' to Bucket '" << name_
+              << "'" << std::endl;
     // TODO(chogan): @errorhandling What about partial failure?
     result = PlaceBlob(&hermes_->context_, &hermes_->rpc_, schema, blob,
                        names[i].c_str(), id_);

diff --git a/src/buffer_pool.cc b/src/buffer_pool.cc
@@ -1602,12 +1602,6 @@ Status PlaceBlob(SharedMemoryContext *context, RpcContext *rpc,
   HERMES_END_TIMED_BLOCK();
 
   if (buffer_ids.size()) {
-    MetadataManager *mdm = GetMetadataManagerFromContext(context);
-    char *bucket_name = ReverseGetFromStorage(mdm, bucket_id.as_int,
-                                              kMapType_Bucket);
-    LOG(INFO) << "Attaching blob " << std::string(name) << " to Bucket "
-              << bucket_name << std::endl;
-
     HERMES_BEGIN_TIMED_BLOCK("WriteBlobToBuffers");
     WriteBlobToBuffers(context, rpc, blob, buffer_ids);
     HERMES_END_TIMED_BLOCK();

diff --git a/src/communication_mpi.cc b/src/communication_mpi.cc
@@ -241,7 +241,7 @@ size_t InitCommunication(CommunicationContext *comm, Arena *arena,
         break;
       }
       default: {
-        assert(!"Invalid code path\n");
+        HERMES_INVALID_CODE_PATH;
         break;
       }
     }

diff --git a/src/config_parser.cc b/src/config_parser.cc
@@ -806,7 +806,7 @@ void ParseTokens(TokenList *tokens, Config *config) {
         break;
       }
       default: {
-        assert(!"Invalid code path\n");
+        HERMES_INVALID_CODE_PATH;
         break;
       }
     }

diff --git a/src/hermes_types.h b/src/hermes_types.h
@@ -50,6 +50,9 @@ constexpr char kPlaceInHierarchy[] = "PlaceInHierarchy";
 #define HERMES_NOT_IMPLEMENTED_YET \
   LOG(FATAL) << __func__ << " not implemented yet\n"
 
+#define HERMES_INVALID_CODE_PATH \
+  LOG(FATAL) << "Invalid code path." << std::endl
+
 /** A TargetID uniquely identifies a buffering target within the system. */
 union TargetID {
   struct {

diff --git a/src/metadata_management.cc b/src/metadata_management.cc
@@ -64,29 +64,6 @@ bool IsNullTargetId(TargetID id) {
   return result;
 }
 
-TicketMutex *GetMapMutex(MetadataManager *mdm, MapType map_type) {
-  TicketMutex *mutex = 0;
-  switch (map_type) {
-    case kMapType_Bucket: {
-      mutex = &mdm->bucket_map_mutex;
-      break;
-    }
-    case kMapType_VBucket: {
-      mutex = &mdm->vbucket_map_mutex;
-      break;
-    }
-    case kMapType_Blob: {
-      mutex = &mdm->blob_map_mutex;
-      break;
-    }
-    default: {
-      assert(!"Invalid code path\n");
-    }
-  }
-
-  return mutex;
-}
-
 void LocalPut(MetadataManager *mdm, const char *key, u64 val,
               MapType map_type) {
   PutToStorage(mdm, key, val, map_type);
@@ -529,10 +506,11 @@ void LocalDestroyBlobById(SharedMemoryContext *context, RpcContext *rpc,
   FreeBufferIdList(context, rpc, blob_id);
 
   MetadataManager *mdm = GetMetadataManagerFromContext(context);
-  char *blob_name = ReverseGetFromStorage(mdm, blob_id.as_int, kMapType_Blob);
+  std::string blob_name = ReverseGetFromStorage(mdm, blob_id.as_int,
+                                                kMapType_Blob);
 
-  if (blob_name) {
-    DeleteId(mdm, rpc, blob_name, kMapType_Blob);
+  if (blob_name.size() > 0) {
+    DeleteId(mdm, rpc, blob_name.c_str(), kMapType_Blob);
   } else {
     // TODO(chogan): @errorhandling
     DLOG(INFO) << "Expected to find blob_id " << blob_id.as_int

diff --git a/src/metadata_management.h b/src/metadata_management.h
@@ -88,12 +88,23 @@ struct MetadataManager {
   ptrdiff_t swap_filename_prefix_offset;
   ptrdiff_t swap_filename_suffix_offset;
 
+  // TODO(chogan): @optimization Should the TicketMutexes here be reader/writer
+  // locks?
+
+  /** Lock for accessing `BucketInfo` structures located at
+   * `bucket_info_offset` */
   TicketMutex bucket_mutex;
+  /** Lock for accessing `VBucketInfo` structures located at
+   * `vbucket_info_offset` */
   TicketMutex vbucket_mutex;
 
+  /** Lock for accessing the `IdMap` located at `bucket_map_offset` */
   TicketMutex bucket_map_mutex;
+  /** Lock for accessing the `IdMap` located at `vbucket_map_offset` */
   TicketMutex vbucket_map_mutex;
+  /** Lock for accessing the `IdMap` located at `blob_map_offset` */
   TicketMutex blob_map_mutex;
+  /** Lock for accessing `IdList`s and `ChunkedIdList`s */
   TicketMutex id_mutex;
 
   size_t map_seed;

diff --git a/src/metadata_storage.h b/src/metadata_storage.h
@@ -17,7 +17,9 @@ u64 GetFromStorage(MetadataManager *mdm, const char *key, MapType map_type);
 /**
  *
  */
-char *ReverseGetFromStorage(MetadataManager *mdm, u64 id, MapType map_type);
+std::string ReverseGetFromStorage(MetadataManager *mdm, u64 id,
+                                  MapType map_type);
+
 /**
  *
  */