Todd Lipcon has posted comments on this change. Change subject: Persistent cache support for NVM ......................................................................
Patch Set 15: (27 comments) http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 294: // persistent mode, but for testing purposes, we'll point it at our test dir, tabs (here and below) http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: Line 247: rt) this seems to have snuck in, but already covered below http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/cache-test.cc File src/kudu/util/cache-test.cc: Line 77: ' extra ' http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/cache.h File src/kudu/util/cache.h: Line 37: NVM_CACHE_PERSISTENT // Is equal to NVM + nvm_cache_persistent. it's odd that we have the different enum here, but we also have the flag. Does this enum actually affect anything? It doesn't seem so. Line 40: enum AllocType { is this used as part of the interface? I dont think so, should probably be in the .cc http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/nvm_cache-test.cc File src/kudu/util/nvm_cache-test.cc: Line 74: FLAGS_nvm_cache_path = "/tmp/pmem"; this should be inside the test path, not /tmp Line 76: // pool is available after the first test. Don't create the directory this relies on test ordering, which isn't necessarily stable. It also doesn't test any crash scenarios. I think a single test which forks and causes its child to crash at various interesting points is probably more effective http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 48: // block cache in persistent mode. rather than being documented in a comment which the user can't see, it should be in the text for the flag itself. Line 54: DEFINE_string(nvm_cache_directory, "/pm_cache", odd that we use two different configs, one for volatile, and one for persistent, and one of them's a directory whereas the other is a file? Can we consolidate these to make it easier for the user? Line 86: struct KeyVal { comment should also say that this is the physical entry that is persisted as the pmemobj "object" Line 87: uint8_t valid; // Really a boolean but for alignment reasons make it uint8_t should have a comment on its usage as well Line 90: uint8_t kv_data[]; // holds key and value data It strikes me that the value should probably be 8-byte or even 16-byte aligned, so we can better use SSE/AVX instructions to deserialize various things inside the cached block. The normal non-NVM cache already aligns to 8 bytes at least, should probably do the same here. We might be getting it "accidentally" but perhaps better to be explicit Line 111: // Finds the persistent memory object id for the given ptr and offset. should be clearer what 'offset' is here. Line 126: // Can return NULL if object not found. misplaced comment Line 139: // data from pmem. this comment above seems misplaced -- maybe it belongs somewhere like the initialization code? Or this comment should be more specifically discussing that this LRUHandle is never persisted, but instead recreated on initialization? Line 221: ptr = &(*ptr)->next_hash; spurious changes Line 271: // This method allocates the 'size' memory from one of the following types(type_num): the comment seems incorrect - 'type_num' is the pmemobj type number, whereas 'atype' is the allocation type described here. Looking at usage, though, it seems like 'type_num' is always TOID_TYPE_NUM(struct KeyVal) so maybe we shouldn't bother taking it as a parameter, since it just clouds the interface? Line 277: void PopulateCacheHandle(LRUHandle* e, docs Line 288: void FreeEntry(LRUHandle* e, bool deep); doc what 'deep' is Line 298: The current AllocType values are : // DRAM_ALLOC, VMEM_ALLOC and PMEM_ALLOC. this bit is superfluous (redundant with the enum definition itself) Line 300: unsigned int type_num should specify what type_num is Line 303: Wwr typo Line 312: bool IsPersistentMode() { can be 'const' Line 313: NULL nullptr Line 362: } above 5 lines would probably read beter like: bool deep = !IsPersistentMode(); FreeEntry(e, deep); Line 560: // only the LRUHandle entry not the data on the NVM device why not free the data from the NVM device? This is called in the cast that an entry has been evicted from the LRU as well as not used by the user, in which case the NVM space should be freed up. Otherwise, it will just be resuscitated next time we restart (and "leaked" until then) I think this is a rare case that is hit only when an entry has been evicted from the LRU list while a user still holds a reference to it, so maybe it's not well covered by tests? Line 846: // simply get the existing one. how do we handle upgrade? Imagine we want to change the KeyValue format on disk in a later version, how can we identify the version that originally wrote the file, so we can either clear it or "upgrade" it? -- To view, visit http://gerrit.cloudera.org:8080/2593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
