[kudu-CR] Persistent cache support for NVM

[Kudu Jenkins (Code Review)]

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

[Sarah Jelinek (Code Review)]

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 (Code Review)]

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

[Sarah Jelinek (Code Review)]

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 (Code Review)]

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,