mon, client: Filter statfs for file systems with a single data pool #16378

fullerdj · 2017-07-17T21:29:23Z

No description provided.

jcsp

This looks great. Let's add an entry to PendingReleaseNotes to publicise it.

fullerdj · 2017-07-26T16:37:29Z

@batrick @jcsp Can we merge this?

batrick · 2017-07-26T16:41:55Z

@fullerdj sorry I msised this because of no cephfs label. I'll take a look now.

fullerdj · 2017-07-26T16:43:30Z

looks like it needs a rebase while I'm at it.

batrick

Looks good overall to me but should have a trivial test confirming it works. An extra pool using a few megabytes which is shown not to be included in the statfs on the data pool should suffice?

batrick · 2017-07-26T16:44:56Z

src/messages/MStatfs.h

  }

  void encode_payload(uint64_t features) override {
    paxos_encode();
    ::encode(fsid, payload);
+    ::encode(data_pool, payload);


Is this always encoded as an int?

It's encoded as a boost::optional. There's a defined encoding for that:

https://github.com/ceph/ceph/blob/master/src/include/encoding.h#L289

batrick · 2017-07-26T16:46:25Z

src/mon/MgrStatMonitor.cc

@@ -290,11 +291,11 @@ bool MgrStatMonitor::preprocess_getpoolstats(MonOpRequestRef op)
  return true;
 }

-bool MgrStatMonitor::preprocess_statfs(MonOpRequestRef op)
-{
+bool MgrStatMonitor::preprocess_statfs(MonOpRequestRef op) {


Unnecessary code churn (and conflicts with style of the file).

More likely unnecessary press of the shift key whilst scrolling.

batrick · 2017-07-26T16:49:07Z

src/mon/PGMap.h

+   */
+  int64_t get_pool_free_space(const OSDMap &osd_map, int poolid) const;
+
+  float pool_raw_used_rate(const OSDMap &osd_map, int poolid) const


Why not put this as a static function in PGMap.cc?

Most of the other stats gathering functions are exported. Some of these stats gathering functions are rather new; we seem to be interested in exposing more statistics that rely on telling the user how many more bytes they're allowed to store.

These are certainly only used in PGMap.cc for now, though, so I can move it there if you prefer.

Let's just keep it in PGMap.cc for now.

fullerdj · 2017-07-26T17:40:23Z

For a test, we can just see if the report accounts for replication/ec overhead. I'll come up with something.

liewegas

a few comments, but otherwise this is awesome!

I wonder if it is possible to do this based on the current directory? Linux maps that back to a superblock so no, but the glibc call takes a path name, which means in theory we could look at that directory's layout (vs the whole fs).

liewegas · 2017-07-26T18:38:59Z

src/messages/MStatfs.h

-  MStatfs() : PaxosServiceMessage(CEPH_MSG_STATFS, 0) {}
-  MStatfs(const uuid_d& f, ceph_tid_t t, version_t v) :
-    PaxosServiceMessage(CEPH_MSG_STATFS, v), fsid(f) {
+  MStatfs() : PaxosServiceMessage(CEPH_MSG_STATFS, 0, 2) {}


should use the const definitions at the top of the class like other messages... see e.g.
https://github.com/ceph/ceph/blob/master/src/messages/MOSDOp.h#L36

liewegas · 2017-07-26T18:39:27Z

src/messages/MStatfs.h

@@ -22,10 +22,12 @@
 class MStatfs : public PaxosServiceMessage {
 public:
  uuid_d fsid;
+  boost::optional<int> data_pool;


this should be int64_t instead of int

liewegas · 2017-07-26T18:39:50Z

src/mon/MgrStatMonitor.cc

-  ceph_statfs get_statfs() const override {
-    return digest.get_statfs();
+  ceph_statfs get_statfs(OSDMap& osdmap,
+			 boost::optional<int> data_pool) const override {


s/int/int64_t/

liewegas · 2017-07-26T18:40:07Z

src/mon/PGMap.cc

@@ -614,6 +614,21 @@ void PGMapDigest::pool_cache_io_rate_summary(Formatter *f, ostream *out,
  cache_io_rate_summary(f, out, p->second.first, ts->second);
 }

+int64_t PGMapDigest::get_pool_free_space(const OSDMap &osd_map,
+					 int poolid) const


liewegas · 2017-07-26T18:42:01Z

src/mon/PGMap.h

-    statfs.num_objects = pg_sum.stats.sum.num_objects;
+
+    if (data_pool) {
+      const pool_stat_t &stat = pg_pool_sum.at(*data_pool);


this should probably fall back if the pool doesn't exist (e.g., if a client statfs request races with a pool deletion)

batrick · 2017-07-26T20:09:02Z

Tracker ticket: http://tracker.ceph.com/issues/19109

batrick · 2017-07-28T17:16:10Z

@fullerdj your rebase got screwed up somehow.

fullerdj · 2017-07-28T17:34:55Z

It seems so. I'm writing up a test, so at least this will make sure this PR doesn't get merged beforehand.

fullerdj · 2017-07-31T13:27:19Z

Ok, I've added a test. This should be ready to go.

tchaikov · 2017-08-01T08:00:28Z

qa/tasks/cephfs/test_misc.py

+        fs_avail = float(fs_avail) * 1024
+
+        ratio = (raw_avail / pool_size) / fs_avail
+        assert ratio > 0.9 and ratio < 1.1


nit, could just put

assert 0.9 < ratio < 1.1

fullerdj · 2017-08-03T13:39:38Z

I'm checking if there's an easy fix for that but maybe you will find it first @fullerdj.

The test is racy. Even though it does conv=fdatasync, that is a nop on fuse. This probably used to work because of the previous dd; although it looks for data_bin_mb + other_bin_mb bytes, but df would have reported data_bin_mb * pool_size bytes used. I'll fix the test.

Bump encoding version accordingly. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

MStatfs now includes an optional data pool element for filesystems with a single data pool, support this element and return data for a single data pool if requested. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

Extend the Objecter's StatfsOp to support MStatfs::data_pool. This requests statistics for a single data pool in the case of a filesystem that has exactly one. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

When statfs() is called for a filesystem with a single data pool, add that as an argument to receive statistics for that pool only. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

Signed-off-by: Douglas Fuller <dfuller@redhat.com>

Add a test for filtered df for file systems with single data pools. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

Signed-off-by: Douglas Fuller <dfuller@redhat.com>

liewegas · 2017-08-05T18:37:10Z

rados suite looks good: http://pulpito.ceph.com/sage-2017-08-04_23:16:58-rados-wip-sage-testing-20170804b-distro-basic-smithi/

Presumably this was caused by a bad rebase. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

* refs/remotes/upstream/pull/16378/head: doc: remove accidental additions to release notes qa/cephfs: Fix race in test_volume_client qa/cephfs: Test filtered df PendingReleaseNotes: add note about df filtering client: Support new, filtered MStatfs objecter: Support new, filtered MStatfs mon/PGMap stats: Support new, filtered MStatfs messages: Add optional data pool to MStatfs Reviewed-by: John Spray <john.spray@redhat.com> Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> Reviewed-by: Sage Weil <sage@redhat.com>

fullerdj requested a review from jcsp July 17, 2017 21:29

fullerdj added feature mon needs-review needs-test labels Jul 17, 2017

jcsp approved these changes Jul 18, 2017

View reviewed changes

fullerdj force-pushed the wip-djf-19109 branch from 3548324 to d5ccfc0 Compare July 18, 2017 14:30

batrick added the cephfs Ceph File System label Jul 26, 2017

fullerdj force-pushed the wip-djf-19109 branch from 899c1bb to ab7dd91 Compare July 26, 2017 16:46

batrick requested changes Jul 26, 2017

View reviewed changes

fullerdj force-pushed the wip-djf-19109 branch 2 times, most recently from 087ad1b to da8f820 Compare July 26, 2017 17:35

liewegas requested changes Jul 26, 2017

View reviewed changes

fullerdj force-pushed the wip-djf-19109 branch 2 times, most recently from 8a21c5c to 90edb88 Compare July 26, 2017 19:45

fullerdj force-pushed the wip-djf-19109 branch from 90edb88 to 607c6d4 Compare July 26, 2017 21:29

fullerdj force-pushed the wip-djf-19109 branch from cd33539 to 478c7bb Compare July 31, 2017 13:26

batrick approved these changes Aug 1, 2017

View reviewed changes

tchaikov reviewed Aug 1, 2017

View reviewed changes

fullerdj force-pushed the wip-djf-19109 branch from 478c7bb to 0928710 Compare August 1, 2017 14:54

batrick approved these changes Aug 2, 2017

View reviewed changes

batrick added the wip-pdonnell-testing label Aug 2, 2017

messages: Add optional data pool to MStatfs

575d613

Bump encoding version accordingly. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

fullerdj force-pushed the wip-djf-19109 branch 2 times, most recently from 35a640d to ac1c0a3 Compare August 3, 2017 14:21

Douglas Fuller added 5 commits August 3, 2017 14:11

mon/PGMap stats: Support new, filtered MStatfs

4aef4ea

MStatfs now includes an optional data pool element for filesystems with a single data pool, support this element and return data for a single data pool if requested. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

objecter: Support new, filtered MStatfs

76fc99a

Extend the Objecter's StatfsOp to support MStatfs::data_pool. This requests statistics for a single data pool in the case of a filesystem that has exactly one. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

client: Support new, filtered MStatfs

aeeeb43

When statfs() is called for a filesystem with a single data pool, add that as an argument to receive statistics for that pool only. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

PendingReleaseNotes: add note about df filtering

8f33e3b

Signed-off-by: Douglas Fuller <dfuller@redhat.com>

qa/cephfs: Test filtered df

b9d11af

Add a test for filtered df for file systems with single data pools. Signed-off-by: Douglas Fuller <dfuller@redhat.com>

fullerdj force-pushed the wip-djf-19109 branch from ac1c0a3 to 3923a10 Compare August 3, 2017 18:12

batrick added wip-pdonnell-testing and removed wip-pdonnell-testing labels Aug 3, 2017

fullerdj force-pushed the wip-djf-19109 branch 2 times, most recently from 10c9ece to 3b70b33 Compare August 4, 2017 17:21

qa/cephfs: Fix race in test_volume_client

552225f

Signed-off-by: Douglas Fuller <dfuller@redhat.com>

fullerdj force-pushed the wip-djf-19109 branch from 3b70b33 to 552225f Compare August 4, 2017 18:39

liewegas added the wip-sage-testing label Aug 4, 2017

liewegas approved these changes Aug 5, 2017

View reviewed changes

liewegas added wip-sage-testing and removed wip-sage-testing labels Aug 5, 2017

batrick added the wip-pdonnell-testing label Aug 7, 2017

liewegas removed this from the luminous milestone Aug 7, 2017

liewegas removed the wip-sage-testing label Aug 8, 2017

doc: remove accidental additions to release notes

41849fd

Presumably this was caused by a bad rebase. Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

batrick merged commit 41849fd into ceph:master Aug 8, 2017

batrick added the wip-pdonnell-testing label Aug 8, 2017

fullerdj deleted the wip-djf-19109 branch August 8, 2017 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mon, client: Filter statfs for file systems with a single data pool #16378

mon, client: Filter statfs for file systems with a single data pool #16378

fullerdj commented Jul 17, 2017

jcsp left a comment

fullerdj commented Jul 26, 2017

batrick commented Jul 26, 2017

fullerdj commented Jul 26, 2017

batrick left a comment

batrick Jul 26, 2017

fullerdj Jul 26, 2017

batrick Jul 26, 2017

batrick Jul 26, 2017

fullerdj Jul 26, 2017

batrick Jul 26, 2017

fullerdj Jul 26, 2017 •

edited

Loading

batrick Jul 26, 2017

fullerdj commented Jul 26, 2017

liewegas left a comment

liewegas Jul 26, 2017

liewegas Jul 26, 2017

liewegas Jul 26, 2017

liewegas Jul 26, 2017

liewegas Jul 26, 2017

batrick commented Jul 26, 2017

batrick commented Jul 28, 2017

fullerdj commented Jul 28, 2017

fullerdj commented Jul 31, 2017

tchaikov Aug 1, 2017

fullerdj commented Aug 3, 2017

liewegas commented Aug 5, 2017

mon, client: Filter statfs for file systems with a single data pool #16378

mon, client: Filter statfs for file systems with a single data pool #16378

Conversation

fullerdj commented Jul 17, 2017

jcsp left a comment

Choose a reason for hiding this comment

fullerdj commented Jul 26, 2017

batrick commented Jul 26, 2017

fullerdj commented Jul 26, 2017

batrick left a 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

fullerdj Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullerdj commented Jul 26, 2017

liewegas left a 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 commented Jul 26, 2017

batrick commented Jul 28, 2017

fullerdj commented Jul 28, 2017

fullerdj commented Jul 31, 2017

Choose a reason for hiding this comment

fullerdj commented Aug 3, 2017

liewegas commented Aug 5, 2017

fullerdj Jul 26, 2017 •

edited

Loading