Sarah Jelinek has posted comments on this change.

Change subject: Persistent cache support for NVM

Patch Set 15:


Fixed all comments. Questions answered.
File src/kudu/cfile/

Line 310:         break;
File src/kudu/util/CMakeLists.txt:

Line 194:     pmem
File src/kudu/util/

PS15, Line 77: '
> extra '
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 and I could use the flags to 
check this when we are allocating the memory from however, I 
didn't want to introduce the DECLARE string in

Line 40: enum AllocType {
> is this used as part of the interface? I dont think so, should probably be 
I will move to .cc
File src/kudu/util/

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 
> 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.
File src/kudu/util/

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.
File src/kudu/util/

Line 48: // block cache in persistent mode.
> rather than being documented in a comment which the user can't see, it shou

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

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 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.

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 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

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

Line 288:   void FreeEntry(LRUHandle* e, bool deep);
> doc what 'deep' is

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)

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

PS15, Line 303: Wwr
> typo

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

PS15, Line 313: NULL
> nullptr

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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07
Gerrit-PatchSet: 15
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

Reply via email to