[kudu-CR] Persistent cache support for NVM
Sarah Jelinek has posted comments on this change. Change subject: Persistent cache support for NVM .. Patch Set 17: You need to update NVML to 1.1 so I can make use of the later use of PMEM_IS_PMEM_FORCE when I am creating/opening the pmemobjpool. -- 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: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Persistent cache support for NVM
Kudu Jenkins has posted comments on this change. Change subject: Persistent cache support for NVM .. Patch Set 17: Build Started http://104.196.14.100/job/kudu-gerrit/2088/ -- 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: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Persistent cache support for NVM
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/2593 to look at the new patch set (#17). Change subject: Persistent cache support for NVM .. Persistent cache support for NVM Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07 --- M build-support/run-test.sh M src/kudu/cfile/block_cache.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/util/CMakeLists.txt M src/kudu/util/cache-test.cc M src/kudu/util/cache.cc M src/kudu/util/cache.h A src/kudu/util/nvm_cache-test.cc M src/kudu/util/nvm_cache.cc 10 files changed, 694 insertions(+), 192 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/2593/17 -- To view, visit http://gerrit.cloudera.org:8080/2593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Persistent cache support for NVM
Sarah Jelinek has posted comments on this change. Change subject: Persistent cache support for NVM .. Patch Set 16: (1 comment) Pushed all code review changes. http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 54: TAG_FLAG(nvm_cache_path, experimental); > The reason for this is that when operating in persistent mode the user has I died modify this to be nvm_cache_pool to indicate that when persistent memory is in use a pool with a specified name is created. Otherwise it's just a directory where a pool is created. -- 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: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Persistent cache support for NVM
Kudu Jenkins has posted comments on this change. Change subject: Persistent cache support for NVM .. Patch Set 16: Build Started http://104.196.14.100/job/kudu-gerrit/2078/ -- 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: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Persistent cache support for NVM
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/2593 to look at the new patch set (#16). Change subject: Persistent cache support for NVM .. Persistent cache support for NVM Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07 --- M build-support/run-test.sh M src/kudu/cfile/block_cache.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/util/CMakeLists.txt M src/kudu/util/cache-test.cc M src/kudu/util/cache.cc M src/kudu/util/cache.h A src/kudu/util/nvm_cache-test.cc M src/kudu/util/nvm_cache.cc 10 files changed, 713 insertions(+), 201 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/2593/16 -- To view, visit http://gerrit.cloudera.org:8080/2593 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07 Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Persistent cache support for NVM
Sarah Jelinek has posted comments on this change. Change subject: Persistent cache support for NVM .. Patch Set 15: (28 comments) Fixed all comments. Questions answered. http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 310: break; Fixed. http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: Line 194: pmem Fixed. http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/cache-test.cc File src/kudu/util/cache-test.cc: PS15, Line 77: ' > extra ' Fixed. 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. D This enum is used in block_cache.cc and cfile-test.cc. I could use the flags to check this when we are allocating the memory from block_cache.cc however, I didn't want to introduce the DECLARE string in cache.cc. Line 40: enum AllocType { > is this used as part of the interface? I dont think so, should probably be I will move to .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 Actually, the problem is that when a test is done the test path is no longer valid. I have to ensure that the pool I originally created is available to me when I run this test. 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 I don't think the test ordering is an issue here is it? This ordering is enforced only within this test. And, I have moved the pool to a location outside the standard test path for that reason. I agree that a test that forces a crash is the best thing. I have opened a bug for this feature in NVML. I have talked with other engineers about how to simulate a crash. http://gerrit.cloudera.org:8080/#/c/2593/11/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 463: l.unlock(); > right, but this eviction call isnt' actually freeing up any NVM space, so i Yeah, I see it now. The delayed delete means that the current request won't be fulfilled but subsequent requests should be. We could do this loop, but put the allocation of the memory outside of it, after we have really freed the entries. I will make this change and submit it and see what you think. 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 shou Moved. Line 54: DEFINE_string(nvm_cache_directory, "/pm_cache", > odd that we use two different configs, one for volatile, and one for persis The reason for this is that when operating in persistent mode the user has to give a file name, where as when in volatile mode the pool is created as a hidden file. And, the user cannot input a name. I do have a default file name specified, pm_cache for the persistent mode only. I do not check this when the user requests volatile mode. There is no real way for me to consolidate these. Both of the modes require a directory and persistent mode requires a file name. Line 86: struct KeyVal { > comment should also say that this is the physical entry that is persisted a Done 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 Done 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 alig I will look at this. Fixed. Line 111: // Finds the persistent memory object id for the given ptr and offset. > should be clearer what 'offset' is here. Done Line 126: // Can return NULL if object not found. > misplaced comment Removed. Line 139: // data from pmem. > this comment above seems misplaced -- maybe it belongs somewhere like the i I will change this to be more specific to indicate that the LRUHandle is not kept in persistent memory. Line 221: ptr = &(*ptr)->next_hash; > spurious changes Done 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, wherea I fixed the comment. I don't want to change this since we may add new TOID types to this, such as the LRUHandle or the hash table and I would prefer to have this interface accept different types. Line 277: void PopulateCacheHandle(LRUHandle* e, > docs Done Line 288: void FreeEntry(LRUHandle* e, bool deep); > doc what 'deep' is Done PS15, L