rbd: snap limit should't be set smaller than the number of existing snaps #16597

PCzhangPC · 2017-07-26T11:35:18Z

Snap limit shouldn't be set smaller than the number of existing snaps.

Signed-off-by: PCzhangPC pengcheng.zhang@easystack.cn

joscollin · 2017-07-27T02:37:03Z

src/tools/rbd/action/Snap.cc

-  return image.snap_set_limit(limit);
+  int r = image.snap_set_limit(limit);
+  if (r == -EINVAL)
+      std::cerr << "rbd: the image now has more snaps than the limit you set " << std::endl;


Looks Good.

Nit:
What about changing the error text to rbd: the image has more snaps than the new limit set to avoid you ?

sounds good , and the change has been made

joscollin

LGTM

yangdongsheng · 2017-07-27T06:21:30Z

src/tools/rbd/action/Snap.cc

-  return image.snap_set_limit(limit);
+  int r = image.snap_set_limit(limit);
+  if (r == -EINVAL)
+      std::cerr << "rbd: the image has more snaps than the new limit set " << std::endl;


We can't print this message for all -EINVAL. because we would get EINVAL in other cases, such as image.old_format == 1.

what about -EBADF, bad file number?

No, EINVAL is a good choice. but the error message should not printed here. move it into snap_set_limit.

change has been made

dillaman

This should probably be solved in cls_rbd::snapshot_set_limit otherwise it's just a race condition [1]

[1] https://github.com/ceph/ceph/blob/master/src/cls/rbd/cls_rbd.cc#L2791

PCzhangPC · 2017-07-28T09:24:51Z

thx...and for some reason, I'll come back to solve this one week later

dillaman

Please move logic to cls_rbd

PCzhangPC · 2017-08-10T10:36:45Z

@dillaman sorry for late, please help to review this again

dillaman · 2017-08-10T13:20:00Z

src/cls/rbd/cls_rbd.cc

+
+  if(new_limit < snap_ids.size()) {
+      CLS_ERR("error setting snapshot limit: %s", cpp_strerror(rc).c_str());
+      return -EINVAL;


Nit: perhaps -ERANGE and then add op_event->ignore_error_codes = {-ERANGE}; to https://github.com/ceph/ceph/blob/master/src/librbd/journal/Replay.cc#L774

dillaman · 2017-08-10T13:20:33Z

src/cls/rbd/cls_rbd.cc

@@ -2807,6 +2807,33 @@ int snapshot_set_limit(cls_method_context_t hctx, bufferlist *in,
    return -EINVAL;
  }

+  int max_read = RBD_MAX_KEYS_READ;


Nit: move max_read and snap_ids into loop

dillaman · 2017-08-10T13:21:12Z

src/cls/rbd/cls_rbd.cc

+      if (rc < 0)
+          return rc;
+
+      for (set<string>::const_iterator it = keys.begin();


Nit: you can just use for (auto& key : keys) {

dillaman · 2017-08-10T13:22:17Z

src/cls/rbd/cls_rbd.cc

@@ -2807,6 +2807,33 @@ int snapshot_set_limit(cls_method_context_t hctx, bufferlist *in,
    return -EINVAL;
  }

+  int max_read = RBD_MAX_KEYS_READ;
+  vector<snapid_t> snap_ids;


Nit: use a size_t snap_count = 0; instead and increment

dillaman · 2017-08-10T13:22:47Z

src/cls/rbd/cls_rbd.cc

@@ -5305,7 +5332,7 @@ CLS_INIT(rbd)
 			  CLS_METHOD_RD,
 			  snapshot_get_limit, &h_snapshot_get_limit);
  cls_register_cxx_method(h_class, "snapshot_set_limit",
-			  CLS_METHOD_WR,
+			  CLS_METHOD_WR | CLS_METHOD_RD,


Nit: for consistency, _RD before _WR

PCzhangPC · 2017-08-11T08:12:47Z

updated, please help to review this @yangdongsheng @dillaman

yangdongsheng · 2017-08-11T08:22:59Z

src/cls/rbd/cls_rbd.cc

+  } while (more);
+
+  if (new_limit < snap_count){
+    CLS_ERR("error setting snapshot limit: %s", cpp_strerror(rc).c_str());


In this case, rc should be 0. so cpp_strerror(rc) does not make sense. what about:

if (new_limit < snap_count){ rc = -ERANGE; CLS_ERR("error setting snapshot limit: %s", cpp_strerror(rc).c_str()); return rc; }

oh,yes. And because there is a "return rc" after this if-condition, i remove the return here.

dillaman

Can you also update the cls_rbd and/or librbd unit tests to ensure this logic works as intended (and isn't broken in the future)?

dillaman · 2017-08-11T12:24:20Z

src/cls/rbd/cls_rbd.cc

+
+  if (new_limit < snap_count){
+    rc = -ERANGE;
+    CLS_ERR("error setting snapshot limit: %s", cpp_strerror(rc).c_str());


The error log should really be on line 2819 (text adjusted to "error retrieving snapshots") -- this should just be a debug log message saying "snapshot limit is less than the number of snapshots".

dillaman · 2017-08-11T12:25:20Z

src/cls/rbd/cls_rbd.cc

+  if (new_limit < snap_count){
+    rc = -ERANGE;
+    CLS_ERR("error setting snapshot limit: %s", cpp_strerror(rc).c_str());
+  } else if (new_limit == UINT64_MAX) {


Nit: no need to count the number of existing snapshots if the limit is being cleared.

PCzhangPC · 2017-08-15T05:00:43Z

ok, do it right now

dillaman · 2017-08-15T11:36:10Z

@PCzhangPC The two new unit tests failed

PCzhangPC · 2017-08-15T11:40:02Z

@dillaman i'm woring on this, thx

PCzhangPC · 2017-08-17T01:33:31Z

@dillaman now i use two different logic to handle v1 and v2 format, please help to review this

dillaman · 2017-08-17T12:30:58Z

src/cls/rbd/cls_rbd.cc

+      if (!keys.empty())
+        last_read = *(keys.rbegin());
+    } while (more);
+  } else {


Nit: I would keep the v1 logic together -- so move this block up. I would also test for a failure to read the header:

if (rc < 0 && rc != -EINVAL) { ... error } else if (rc >= 0) { .. v1 logic } else { .. v2 logic }

ok, will be updated next monday

new snap limit must be bigger than the num of existing snaps Signed-off-by: PCzhangPC <pengcheng.zhang@easystack.cn>

PCzhangPC · 2017-08-23T11:43:04Z

@dillaman update already, please help to review this if convenient for you, thx

dillaman

lgtm

joscollin added bug-fix rbd labels Jul 26, 2017

joscollin requested changes Jul 27, 2017

View reviewed changes

PCzhangPC force-pushed the fix6 branch from a8826d6 to c226b5b Compare July 27, 2017 02:52

joscollin approved these changes Jul 27, 2017

View reviewed changes

joscollin added the needs-qa label Jul 27, 2017

yangdongsheng reviewed Jul 27, 2017

View reviewed changes

PCzhangPC force-pushed the fix6 branch from c226b5b to e04b29a Compare July 27, 2017 06:58

dillaman reviewed Jul 27, 2017

View reviewed changes

dillaman removed the needs-qa label Jul 31, 2017

dillaman suggested changes Aug 7, 2017

View reviewed changes

PCzhangPC force-pushed the fix6 branch 3 times, most recently from 8d844f7 to a72ff0d Compare August 10, 2017 04:38

dillaman reviewed Aug 10, 2017

View reviewed changes

PCzhangPC force-pushed the fix6 branch 2 times, most recently from fd606e2 to 9038d30 Compare August 11, 2017 05:27

yangdongsheng reviewed Aug 11, 2017

View reviewed changes

PCzhangPC force-pushed the fix6 branch from 9038d30 to ee4190a Compare August 11, 2017 08:34

dillaman reviewed Aug 11, 2017

View reviewed changes

PCzhangPC force-pushed the fix6 branch 6 times, most recently from a213806 to f36692c Compare August 15, 2017 10:55

PCzhangPC force-pushed the fix6 branch from f36692c to 747100f Compare August 16, 2017 11:30

PCzhangPC force-pushed the fix6 branch 2 times, most recently from eccd9da to 928046b Compare August 17, 2017 02:49

dillaman reviewed Aug 17, 2017

View reviewed changes

PCzhangPC force-pushed the fix6 branch from 928046b to 582ad68 Compare August 21, 2017 02:07

rbd:snap limit should't be set smaller than the num of existing snaps

121b6ca

new snap limit must be bigger than the num of existing snaps Signed-off-by: PCzhangPC <pengcheng.zhang@easystack.cn>

PCzhangPC force-pushed the fix6 branch from 582ad68 to 121b6ca Compare August 21, 2017 11:54

dillaman approved these changes Aug 23, 2017

View reviewed changes

dillaman added the needs-qa label Aug 23, 2017

dillaman added the wip-jason-testing label Sep 5, 2017

dillaman merged commit dfb8139 into ceph:master Sep 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: snap limit should't be set smaller than the number of existing snaps #16597

rbd: snap limit should't be set smaller than the number of existing snaps #16597

PCzhangPC commented Jul 26, 2017 •

edited

joscollin Jul 27, 2017

PCzhangPC Jul 27, 2017

joscollin left a comment

yangdongsheng Jul 27, 2017

PCzhangPC Jul 27, 2017

yangdongsheng Jul 27, 2017

PCzhangPC Jul 27, 2017

dillaman left a comment

PCzhangPC commented Jul 28, 2017

dillaman left a comment

PCzhangPC commented Aug 10, 2017

dillaman Aug 10, 2017

dillaman Aug 10, 2017

dillaman Aug 10, 2017

dillaman Aug 10, 2017

dillaman Aug 10, 2017

PCzhangPC commented Aug 11, 2017

yangdongsheng Aug 11, 2017 •

edited

PCzhangPC Aug 11, 2017 •

edited

dillaman left a comment

dillaman Aug 11, 2017

dillaman Aug 11, 2017

PCzhangPC commented Aug 15, 2017

dillaman commented Aug 15, 2017

PCzhangPC commented Aug 15, 2017

PCzhangPC commented Aug 17, 2017

dillaman Aug 17, 2017 •

edited

PCzhangPC Aug 18, 2017

PCzhangPC commented Aug 23, 2017

dillaman left a comment

rbd: snap limit should't be set smaller than the number of existing snaps #16597

rbd: snap limit should't be set smaller than the number of existing snaps #16597

Conversation

PCzhangPC commented Jul 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joscollin 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

dillaman left a comment

Choose a reason for hiding this comment

PCzhangPC commented Jul 28, 2017

dillaman left a comment

Choose a reason for hiding this comment

PCzhangPC commented Aug 10, 2017

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

PCzhangPC commented Aug 11, 2017

yangdongsheng Aug 11, 2017 • edited

Choose a reason for hiding this comment

PCzhangPC Aug 11, 2017 • edited

Choose a reason for hiding this comment

dillaman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PCzhangPC commented Aug 15, 2017

dillaman commented Aug 15, 2017

PCzhangPC commented Aug 15, 2017

PCzhangPC commented Aug 17, 2017

dillaman Aug 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PCzhangPC commented Aug 23, 2017

dillaman left a comment

Choose a reason for hiding this comment

PCzhangPC commented Jul 26, 2017 •

edited

yangdongsheng Aug 11, 2017 •

edited

PCzhangPC Aug 11, 2017 •

edited

dillaman Aug 17, 2017 •

edited