Attempt to disable NvmCacheState when NVM cache is not enabled.

[victoria-mcgrath]

It seems that NvmCacheState object assumes that when cacheDir is specified then NVM cache is enabled. In case of cachebench used with multiple tiers and no NVM cache, this assumption is causing an error message: "E1122 21:27:43.287637 1212 NvmCacheState.cpp:135] unable to deserialize nvm metadata file: no content in file : ...".

---

This change is 

[igchor]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @igchor and @victoria-mcgrath)

---

cachelib/allocator/CacheAllocator-inl.h, line 53 at r1 (raw file):

                        std::make_shared<MurmurHash2>()),
      cacheCreationTime_{util::getCurrentTimeSec()},
      nvmCacheState_{config_.cacheDir, config_.isNvmCacheEncryptionEnabled(),

Hm, maybe instead of adding a new isNvmCacheEnabled parameter to nvmCacheState_ we could just convert nvmCacheState_ into a pointer (or std::optional) and move this initialization to initNvmCache? I think it would be quite natural to keep everything related to nvm in that method.

[igchor]

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @victoria-mcgrath)

[victoria-mcgrath]

---

cachelib/allocator/CacheAllocator-inl.h, line 53 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Hm, maybe instead of adding a new isNvmCacheEnabled parameter to nvmCacheState_ we could just convert nvmCacheState_ into a pointer (or std::optional) and move this initialization to initNvmCache? I think it would be quite natural to keep everything related to nvm in that method.

I tried std::optional - please take a look.

[igchor]

Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @igchor and @victoria-mcgrath)

---

cachelib/allocator/CacheAllocator.h, line 972 at r2 (raw file):

  time_t getCacheCreationTime() const noexcept { return cacheCreationTime_; }
  time_t getNVMCacheCreationTime() const {
    return nvmCacheState_.value().getCreationTime();

I think you should handle the case where nvmCacheState_ is not specified - to have legacy behavior - you can just return cacheCreationTime.

---

cachelib/allocator/CacheAllocator-inl.h, line 3254 at r2 (raw file):

  ret.nvmCacheEnabled = nvmCache_ ? nvmCache_->isEnabled() : false;
  if (nvmCacheState_.has_value()) {
    ret.nvmUpTime = currTime - nvmCacheState_.value().getCreationTime();

I think you can just use getNVMCacheCreationTime one you fix it.

[igchor]

LGTM

Reviewable status: 1 of 5 files reviewed, 1 unresolved discussion (waiting on @igchor and @victoria-mcgrath)

---

cachelib/allocator/CacheAllocator.h, line 979 at r3 (raw file):

    auto result = getCacheCreationTime();
    if (nvmCacheState_.has_value()) {
      result = nvmCacheState_.value().getCreationTime();

nit: you can use value_or instead of this if

[igchor]

Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @igchor)

[victoria-mcgrath]

Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @igchor)

---

cachelib/allocator/CacheAllocator.h, line 972 at r2 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

I think you should handle the case where nvmCacheState_ is not specified - to have legacy behavior - you can just return cacheCreationTime.

Done.