Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: add negative cache to the system object #35777

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

ofriedma
Copy link
Contributor

add negative cache to the system object

Signed-off-by: Or Friedmann ofriedma@redhat.com

Fixes: https://tracker.ceph.com/issues/45816

Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks potentially good--can you do a teuthology run?

@@ -53,6 +53,7 @@ struct ObjectCacheInfo {
int status = 0;
uint32_t flags = 0;
uint64_t epoch = 0;
bool noentry = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this just duplicate the int status field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -72,6 +72,10 @@ int ObjectCache::get(const string& name, ObjectCacheInfo& info, uint32_t mask, r
<< std::hex << mask << ", cached=0x" << src.flags
<< std::dec << ")" << dendl;
if(perfcounter) perfcounter->inc(l_rgw_cache_miss);
if(src.noentry) {
ldout(cct, 20) << "negative cache entry" << dendl;
return -ENODATA;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be counted or logged as a cache miss - it's a hit on a negative entry. i think we should move this check above of this 'flags' block, ie:

   ObjectCacheInfo& src = iter->second.info;
+  if (src.status == -ENOENT) {
+    ldout(cct, 10) << "cache get: name=" << name << " : hit (negative entry)" << dendl;
+    if (perfcounter) perfcounter->inc(l_rgw_cache_hit);
+    return -ENODATA;
+  }
   if ((src.flags & mask) != mask) {

we shouldn't treat any status other than ENOENT as a negative entry - if we got any other error trying to read it, that shouldn't prevent us from trying again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

add negative cache to the system object

Signed-off-by: Or Friedmann <ofriedma@redhat.com>

Fixes: https://tracker.ceph.com/issues/45816
@ofriedma
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants