osd: add 'cache drop' and 'cache status' commands #24270

mogeb · 2018-09-25T14:02:42Z

Fixes: http://tracker.ceph.com/issues/24176

Signed-off-by: Mohamad Gebai mgebai@suse.com

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

mogeb · 2018-09-25T14:06:34Z

@batrick there is still the cached buffers in Bluestore that can be cleared. I'll make sure to add those as well, but it can be a separate PR.

mogeb · 2018-09-25T14:07:43Z

src/os/ObjectStore.h

@@ -1459,6 +1459,9 @@ class ObjectStore {
  virtual void generate_db_histogram(Formatter *f) { }
  virtual void flush_cache() { }
  virtual void dump_perf_counters(Formatter *f) {}
+  virtual int get_cache_obj_count() {
+    return -1;
+  }


Not too happy about implementing it that way, but wasn't sure if making it a pure method would make things cleaner.

You know more about the nitty-gritty of the particular situation than I do, but I'd suggest a pure virtual member function in preference to something that always fails. Always failing /suggests/ that the behavior just isn't valid or available (or belongs in a parent class that may just not exist). That's just my $0.25, and again you know more about the specific situation.

Yeah especially since it's returning an integer and the sole user is just blindly adding the value...

RIght, these are good points. This function is just going to disappear, I'm working on a different implementation that takes the Formatter as an argument and prints to it what is appropriate for each object store.

neha-ojha · 2018-09-25T18:52:54Z

retest this please

batrick · 2018-09-26T17:15:14Z

@batrick there is still the cached buffers in Bluestore that can be cleared. I'll make sure to add those as well, but it can be a separate PR.

FWIW, I'd prefer the object store cache also be cleared in this PR.

Note for filestore, it's necessary to write 3 to /proc/sys/vm/drop_caches. If the OSD cannot write to this file then it should return an appropriate error to the caller. Bluestore does not have this problem because the device is opened with O_DIRECT and there are no dentries/inodes to worry about in the Linux dircache.

mogeb · 2018-09-28T16:58:58Z

Just as an update, I've added the implementation of clearing Filestore's buffer cache to this PR, and I'm implementing the same for Bluestore (clearing Bluestore's data cache).

batrick · 2018-09-28T18:40:55Z

src/os/filestore/FileStore.cc

+int FileStore::flush_cache()
+{
+  string drop_caches_file = "/proc/sys/vm/drop_caches";
+  int drop_caches_fd = ::open(drop_caches_file.c_str(), O_WRONLY), ret = 0;


Add O_CLOEXEC too please.

batrick · 2018-09-28T18:43:26Z

src/osd/OSD.cc

+  else if (prefix == "clear_cache") {
+    dout(20) << "clearing all caches" << dendl;
+    // Clear the objectstore's cache - onode for Bluestore, pagecache for Filestore
+    r = store->flush_cache();


I recommend passing a ostream here which flush_cache can write errors too (like that it can't open /proc/sys/vm/drop_caches). This can then be returned in the MCommandReply message and displayed in the output of ceph tell osd.* clear_cache.

The derr calls do print out the messages:

> ./bin/ceph tell osd.* drop_cache Error EACCES: osd.0: Error flushing objectstore cache: (13) Permission denied Error EACCES: osd.1: Error flushing objectstore cache: (13) Permission denied

Would going the ostream way cover any case where the derr might be missed?

I think you're just seeing the messages printed to the terminal from a vstart cluster? (IOW, all of the daemons' stderr is connected to your terminal so you see all of these derr messages.) I don't think it's supposed to be automatic like that.

You're right, and it makes sense since these derr messages aren't passed around anywhere. I updated the PR.

batrick · 2018-09-28T18:44:44Z

src/osd/OSD.cc

@@ -6287,6 +6293,50 @@ int OSD::_do_command(
    probe_smart(devid, ds);
  }

+  else if (prefix == "clear_cache") {


Let's use drop cache to be consistent with the MDS #21566.

batrick · 2018-09-28T18:45:33Z

src/os/filestore/FileStore.cc

+  string drop_caches_file = "/proc/sys/vm/drop_caches";
+  int drop_caches_fd = ::open(drop_caches_file.c_str(), O_WRONLY), ret = 0;
+  char buf[2] = "3";
+  int len = strlen(buf);


size_t len

batrick

Otherwise looks okay to me.

batrick · 2018-10-02T17:11:45Z

src/osd/OSD.cc

+    // Clear the objectcontext cache (per PG)
+    vector<PGRef> pgs;
+    _get_pgs(&pgs);
+    for (auto pg: pgs) {


mogeb · 2018-10-03T16:12:58Z

I'm just changing the get_cache_object_count command to get cache stats and making it a bit better.

chardan · 2018-10-03T17:13:59Z

src/os/bluestore/BlueStore.cc

@@ -12415,12 +12415,14 @@ void BlueStore::_flush_cache()
 // We use a best-effort policy instead, e.g.,
 // we don't care if there are still some pinned onodes/data in the cache
 // after this command is completed.
-void BlueStore::flush_cache()
+int BlueStore::flush_cache(ostream *os)


Looks like the parameter "os" is just ignored. Consider just not taking it, or perhaps using std::optional<>. (Bare pointer parameters make me sad.)

The signature of the virtual method changed.

@mogeb please add override tot his method.

There already is the override keyword in the header files, this is the .cc file.

Oh whoops. Sorry for the noise.

gregsfortytwo · 2018-10-03T18:07:56Z

src/os/ObjectStore.h

@@ -1459,6 +1459,9 @@ class ObjectStore {
  virtual void generate_db_histogram(Formatter *f) { }
  virtual void flush_cache() { }
  virtual void dump_perf_counters(Formatter *f) {}
+  virtual int get_cache_obj_count() {
+    return -1;
+  }


Yeah especially since it's returning an integer and the sole user is just blindly adding the value...

gregsfortytwo · 2018-10-03T18:09:43Z

src/osd/OSD.cc

+    // Clear osd map cache
+    {
+      Mutex::Locker l(service.map_cache_lock);
+      service.map_cache.clear();


What's the purpose of dropping the map caches? This is not a data cache; it's an OSD functionality cache and while probably anybody running a "drop cache" command is going to be okay with what happens, it could cause some problems on an overloaded cluster running at the end...(the OSD map cache is never emptied!).

Yes, that's a good point. Especially that it doesn't impact IO directly. I'll remove this part.

gregsfortytwo · 2018-10-03T18:23:32Z

src/os/ObjectStore.h

@@ -1457,8 +1457,11 @@ class ObjectStore {

  virtual void get_db_statistics(Formatter *f) { }
  virtual void generate_db_histogram(Formatter *f) { }
-  virtual void flush_cache() { }
+  virtual int flush_cache(ostream *os = NULL) { return 0; }


This one should probably return -1 — eg MemStore isn't ever going to be able to flush the cache and may not implement it.

mogeb · 2018-10-03T19:10:05Z

src/osd/OSD.cc

+      f->open_object_section("caches_object_count");
+      f->dump_int("object_ctx", obj_ctx_count);
+      f->dump_int("objectstore_onode", store_cache_count);
+      f->dump_int("osd_map", osd_map_count);


@gregsfortytwo should we keep printing the stats of the osdmap cache here?

I guess as a counterpart to "drop cache" we probably shouldn't? And I think the number of cached osd maps is exposed as a perfcounter anyway

mogeb · 2018-10-03T19:37:10Z

src/os/bluestore/BlueStore.h

@@ -2334,6 +2334,24 @@ class BlueStore : public ObjectStore,
  int _fsck(bool deep, bool repair);

  void set_cache_shards(unsigned num) override;
+  void dump_cache_stats(Formatter *f) override {


@gregsfortytwo @chardan printing the cache stats is now offloaded to the object store. There is no more function get_cache_object_count that can be called from the OSD to get the store's cache stats. I think this makes more sense, since the cache stats mean different things for different object stores.

batrick · 2018-10-04T18:26:19Z

Great work! Thanks @mogeb !

batrick · 2018-10-04T21:23:25Z

@mogeb i was trying out this PR and messed up when I asked you to change the command to "drop cache" to match the MDS. We're actually using "cache drop" in the MDS (to group commands by what they manipulate). Think it's reasonable to match? :)

mogeb · 2018-10-05T11:52:38Z

@batrick no problem at all. :) This one is also on me since I missed that, even though I did go through that PR. The MDS also has a cache status command, should we match that too instead of get cache stats?

batrick · 2018-10-05T18:42:48Z

@mogeb cache status sounds great. Let's match that too :)

mogeb · 2018-10-08T15:27:14Z

@batrick done!

gregsfortytwo · 2018-10-10T15:17:39Z

src/osd/OSD.cc

@@ -6020,6 +6020,12 @@ COMMAND("compact",
 COMMAND("smart name=devid,type=CephString,req=False",
        "runs smartctl on this osd devices.  ",
        "osd", "rw", "cli,rest")
+COMMAND("cache drop",
+        "Drop all OSD caches",
+        "osd", "rw", "cli,rest")


Hmm actually can we make this require rwx? We don't want normal clients to be able to force a cache drop command since it's got performance implications on all the others as well.

Ack and done.

Fixes: http://tracker.ceph.com/issues/24176 Signed-off-by: Mohamad Gebai <mgebai@suse.com>

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

…nent Rename 'drop cache' to 'cache drop' and 'get cache stats' to 'cache status'. Signed-off-by: Mohamad Gebai <mgebai@suse.com>

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

mogeb · 2018-10-10T15:47:41Z

Rebased.

batrick · 2018-10-10T16:21:35Z

I still think this is fine as-is but the next step here is to also deliver a "cache drop" command via the OSDMap (as an epoch). We want to be able to tell RADOS clients to drop their caches too.

gregsfortytwo · 2018-10-10T19:15:33Z

Woah that is getting to a whole other level of thing @batrick. If you want that let's do it as another PR — that's going to be a bit contentious.

gregsfortytwo · 2018-10-10T19:16:24Z

What kind of testing do we want for this? Just stick it in one of the simple "make check" command-running tests, or actually try and verify that caches get dropped?

mogeb · 2018-10-10T19:22:18Z

or actually try and verify that caches get dropped?

@gregsfortytwo that's why I originally added the cache status command. Looking at rados bench throughput also reflects the caches being dropped.

batrick · 2018-10-10T20:20:42Z

Woah that is getting to a whole other level of thing @batrick. If you want that let's do it as another PR — that's going to be a bit contentious.

I wouldn't suggest putting it in this PR. I talked about this in the tracker ticket originally. We need a solution in Ceph to drop the multiple cache levels we have (client, osd, mds, kernel when possible). That's a next step.

* refs/pull/24270/head: osd: make 'cache drop' command require 'executable' permission osd: rename 'drop cache' and 'get cache stats' to group them by component doc: add documentation for 'drop cache' and 'get cache stats' osd: don't print osdmap cache stats in 'get cache stats' command osd: do not clear osdmap cache on 'drop cache' command osd: offload dumping cache stats to the object store osd: pass a stream to flush_cache commands for more verbosity osd: implement flush_cache() method for Filestore osd: add clear_cache and get_cache_object_count commands Reviewed-by: Gregory Farnum <gfarnum@redhat.com> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>

mogeb added feature bluestore and removed bluestore labels Sep 25, 2018

mogeb requested a review from batrick September 25, 2018 14:04

mogeb commented Sep 25, 2018

View reviewed changes

batrick added core needs-review labels Sep 25, 2018

batrick requested review from gregsfortytwo, liewegas, neha-ojha and jdurgin September 25, 2018 14:08

mogeb force-pushed the clear-cache branch 2 times, most recently from ce75fba to 96f3870 Compare September 27, 2018 17:49

batrick requested changes Sep 28, 2018

View reviewed changes

mogeb changed the title ~~osd: add clear_cache and get_cache_object_count commands~~ osd: add 'drop cache' and 'get_cache_object_count' commands Oct 1, 2018

batrick requested changes Oct 2, 2018

View reviewed changes

src/osd/OSD.cc Outdated

// Clear the objectcontext cache (per PG)

vector<PGRef> pgs;

_get_pgs(&pgs);

for (auto pg: pgs) {

Copy link

Member

batrick Oct 2, 2018

Choose a reason for hiding this comment

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

auto& pg

mogeb force-pushed the clear-cache branch from cd74cd7 to 52c4117 Compare October 3, 2018 15:03

chardan reviewed Oct 3, 2018

View reviewed changes

gregsfortytwo requested changes Oct 3, 2018

View reviewed changes

gregsfortytwo reviewed Oct 3, 2018

View reviewed changes

mogeb commented Oct 3, 2018

View reviewed changes

mogeb force-pushed the clear-cache branch from d05a3d2 to 1962f8d Compare October 3, 2018 19:36

mogeb commented Oct 3, 2018

View reviewed changes

mogeb changed the title ~~osd: add 'drop cache' and 'get_cache_object_count' commands~~ osd: add 'drop cache' and 'get cache stats' commands Oct 3, 2018

gregsfortytwo approved these changes Oct 3, 2018

View reviewed changes

mogeb changed the title ~~osd: add 'drop cache' and 'get cache stats' commands~~ osd: add 'cache drop' and 'cache status' commands Oct 8, 2018

batrick approved these changes Oct 8, 2018

View reviewed changes

tchaikov added the needs-qa label Oct 8, 2018

liewegas added wip-sage-testing and removed wip-sage-testing labels Oct 10, 2018

gregsfortytwo reviewed Oct 10, 2018

View reviewed changes

mogeb added 9 commits October 10, 2018 11:45

osd: add clear_cache and get_cache_object_count commands

65767fb

Fixes: http://tracker.ceph.com/issues/24176 Signed-off-by: Mohamad Gebai <mgebai@suse.com>

osd: implement flush_cache() method for Filestore

d6eed7b

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

osd: pass a stream to flush_cache commands for more verbosity

7e4ed82

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

osd: offload dumping cache stats to the object store

62ffc9a

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

osd: do not clear osdmap cache on 'drop cache' command

6d1aa95

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

osd: don't print osdmap cache stats in 'get cache stats' command

e2b06de

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

doc: add documentation for 'drop cache' and 'get cache stats'

fc761f9

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

osd: rename 'drop cache' and 'get cache stats' to group them by compo…

2b0b6a7

…nent Rename 'drop cache' to 'cache drop' and 'get cache stats' to 'cache status'. Signed-off-by: Mohamad Gebai <mgebai@suse.com>

osd: make 'cache drop' command require 'executable' permission

e8f28c2

Signed-off-by: Mohamad Gebai <mgebai@suse.com>

mogeb force-pushed the clear-cache branch from 464d1b6 to e8f28c2 Compare October 10, 2018 15:47

liewegas added the wip-sage-testing label Oct 11, 2018

liewegas merged commit e8f28c2 into ceph:master Oct 12, 2018

osd: add 'cache drop' and 'cache status' commands #24270

osd: add 'cache drop' and 'cache status' commands #24270

Conversation

mogeb commented Sep 25, 2018 • edited Loading

mogeb commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neha-ojha commented Sep 25, 2018

batrick commented Sep 26, 2018

mogeb commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batrick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogeb commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogeb Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

batrick commented Oct 4, 2018

batrick commented Oct 4, 2018

mogeb commented Oct 5, 2018

batrick commented Oct 5, 2018

mogeb commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogeb commented Oct 10, 2018

batrick commented Oct 10, 2018

gregsfortytwo commented Oct 10, 2018

gregsfortytwo commented Oct 10, 2018

mogeb commented Oct 10, 2018

batrick commented Oct 10, 2018

mogeb commented Sep 25, 2018 •

edited

Loading

mogeb Oct 3, 2018 •

edited

Loading