[kudu-CR] Persistent cache support for NVM

["Sarah Jelinek (Code Review)" <ge...@cloudera.org>]

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, Line 298: The current AllocType values are
              :   // DRAM_ALLOC, VMEM_ALLOC and PMEM_ALLOC.
> this bit is superfluous (redundant with the enum definition itself)
Done


PS15, Line 300: unsigned int type_num
> should specify what type_num is
Done


PS15, Line 303: Wwr
> typo
Done


Line 312:   bool IsPersistentMode() {
> can be 'const'
Done


PS15, Line 313: NULL
> nullptr
Done


Line 362:       }
> above 5 lines would probably read beter like:
Done


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 
I don't think it should be freed. My understanding of this function is simply to release the LRUHandle resource. In the test cases I have seen its used to release a handle after a lookup and when after an insert the progam no longer wants to hold onto the handle. This isn't a delete. It's still a valid entry.


Line 846:       // simply get the existing one.
> how do we handle upgrade? Imagine we want to change the KeyValue format on 
We don't at this point. We need to talk about the version and upgrade strategy. I need to know what kind of changes you think you will be allowing. If it is invasive(let's say we add more data structures that are persisted and you want to allow them to change) then that's a lot of change in the code to allow the upgrade. Or we simply don't handle the upgrade and it has to be done manually.


-- 
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 <sa...@intel.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sarah Jelinek <sa...@intel.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes