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

objecter: pg listing can deadlock when throttling is in use #5043

Merged
1 commit merged into from Sep 4, 2015

Conversation

Projects
None yet
4 participants
@smithfarm
Copy link
Contributor

commented Jun 22, 2015

@smithfarm smithfarm changed the title Objecter: pg listing can deadlock when throttling is in use [DNM] Objecter: pg listing can deadlock when throttling is in use Jun 22, 2015

@smithfarm smithfarm added this to the firefly milestone Jun 22, 2015

@smithfarm smithfarm self-assigned this Jun 22, 2015

@smithfarm smithfarm force-pushed the SUSE:wip-12007-firefly branch from 8840f1c to f56da44 Jun 22, 2015

@smithfarm smithfarm assigned yehudasa and unassigned smithfarm Jun 22, 2015

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2015

[DNM] pending review

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2015

@yehudasa I would appreciate it if you could look at this. It is a backport of http://tracker.ceph.com/issues/9008

@yehudasa yehudasa assigned athanatos and unassigned yehudasa Jun 23, 2015

@yehudasa

This comment has been minimized.

Copy link
Member

commented Jun 23, 2015

@smithfarm overall looks fine, but I think it'll need to go through regression tests.
@athanatos assigning to you.

@athanatos athanatos assigned ghost and unassigned athanatos Jun 23, 2015

@smithfarm smithfarm changed the title [DNM] Objecter: pg listing can deadlock when throttling is in use Objecter: pg listing can deadlock when throttling is in use Jun 24, 2015

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2015

ready for integration testing

@ghost

This comment has been minimized.

Copy link

commented Jul 8, 2015

@smithfarm @xinxinsh it conflicts with #4597, could you please make it so they do not ? I will help with integration :-)

@ghost ghost changed the title Objecter: pg listing can deadlock when throttling is in use [DNM] Objecter: pg listing can deadlock when throttling is in use Jul 8, 2015

For pgls OP, get/put budget on per list session basis, instead of per…
… OP basis, which could lead to deadlock.

Signed-off-by: Guang Yang (yguang@yahoo-inc.com)
(cherry picked from commit 0f884fd)

Conflicts:
	src/librados/IoCtxImpl.cc
            In firefly, return value of objecter->pg_read() is not assigned to c->tid.
	src/osdc/Objecter.cc
	src/osdc/Objecter.h
            There is no _op_submit_with_budget() function in firefly.
            There is no Objecter::_finish_op() function in firefly.
            In firefly, _take_op_budget() is called take_op_budget().

@smithfarm smithfarm force-pushed the SUSE:wip-12007-firefly branch from f56da44 to 5559a5f Jul 22, 2015

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2015

rebased

@smithfarm smithfarm changed the title [DNM] Objecter: pg listing can deadlock when throttling is in use Objecter: pg listing can deadlock when throttling is in use Aug 10, 2015

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2015

This PR is in the integration branch that is currently undergoing integration tests - see http://tracker.ceph.com/issues/11644 for details. The current status of those tests is as follows:

  • the rgw suite has finished; all 13 tests passed.
  • the other suites are still pending
@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2015

@athanatos Hi, this is a backport of 0f884fd which you merged. It has passed an rgw suite and a rados suite as detailed by http://tracker.ceph.com/issues/11644 . Do you think it's ready to merge?

@smithfarm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2015

@dachary ?

@ghost

This comment has been minimized.

Copy link

commented Sep 4, 2015

$ commit=5559a5f ; picked_from=$(git show --no-patch --pretty=%b $commit  | perl -ne 'print if(s/.*cherry picked from commit (\w+).*/$1/)') ; diff -u --ignore-matching-lines '^[^+-]'   <(git show $picked_from) <(git show $commit)
--- /dev/fd/63  2015-09-04 17:11:24.352065547 +0200
+++ /dev/fd/62  2015-09-04 17:11:24.352065547 +0200
@@ -14,8 +24,8 @@
    ::ObjectOperation rd;
    rd.hit_set_ls(pls, NULL);
    object_locator_t oloc(poolid);
--  c->tid = objecter->pg_read(hash, oloc, rd, NULL, 0, onack, NULL);
-+  c->tid = objecter->pg_read(hash, oloc, rd, NULL, 0, onack, NULL, NULL);
+-  objecter->pg_read(hash, oloc, rd, NULL, 0, onack, NULL);
++  objecter->pg_read(hash, oloc, rd, NULL, 0, onack, NULL, NULL);
    return 0;
  }
  
@@ -23,8 +33,8 @@
    ::ObjectOperation rd;
    rd.hit_set_get(utime_t(stamp, 0), pbl, 0);
    object_locator_t oloc(poolid);
--  c->tid = objecter->pg_read(hash, oloc, rd, NULL, 0, onack, NULL);
-+  c->tid = objecter->pg_read(hash, oloc, rd, NULL, 0, onack, NULL, NULL);
+-  objecter->pg_read(hash, oloc, rd, NULL, 0, onack, NULL);
++  objecter->pg_read(hash, oloc, rd, NULL, 0, onack, NULL, NULL);
    return 0;
  }
  
@@ -39,24 +49,15 @@
 -ceph_tid_t Objecter::op_submit(Op *op)
 +ceph_tid_t Objecter::op_submit(Op *op, int *ctx_budget)
  {
-   RWLock::RLocker rl(rwlock);
-   RWLock::Context lc(rwlock, RWLock::Context::TakenForRead);
--  return _op_submit_with_budget(op, lc);
-+  return _op_submit_with_budget(op, lc, ctx_budget);
- }
- 
--ceph_tid_t Objecter::_op_submit_with_budget(Op *op, RWLock::Context& lc)
-+ceph_tid_t Objecter::_op_submit_with_budget(Op *op, RWLock::Context& lc, int *ctx_budget)
- {
-   assert(initialized.read());
- 
-@@ -1691,7 +1691,14 @@ ceph_tid_t Objecter::_op_submit_with_budget(Op *op, RWLock::Context& lc)
+   assert(client_lock.is_locked());
+   assert(initialized);
+@@ -1236,7 +1236,14 @@ ceph_tid_t Objecter::op_submit(Op *op)
  
    // throttle.  before we look at any state, because
    // take_op_budget() may drop our lock while it blocks.
--  _take_op_budget(op);
+-  take_op_budget(op);
 +  if (!op->ctx_budgeted || (ctx_budget && (*ctx_budget == -1))) {
-+    int op_budget = _take_op_budget(op);
++    int op_budget = take_op_budget(op);
 +    // take and pass out the budget for the first OP
 +    // in the context session
 +    if (ctx_budget && (*ctx_budget == -1)) {
@@ -134,13 +135,13 @@
 +
      Op(const object_t& o, const object_locator_t& ol, vector& op,
         int f, Context *ac, Context *co, version_t *ov) :
-       session(NULL), incarnation(0),
-@@ -1150,7 +1155,8 @@ public:
-       objver(ov), reply_epoch(NULL),
+       session(NULL), session_item(this), incarnation(0),
+@@ -1156,7 +1161,8 @@ public:
        map_dne_bound(0),
        budgeted(false),
--      should_resend(true) {
-+      should_resend(true),
+       should_resend(true),
+-      last_force_resend(0) {
++      last_force_resend(0),
 +      ctx_budgeted(false) {
        ops.swap(op);
        
@@ -171,16 +172,16 @@
  
      bool at_end() const {
        return at_end_of_pool;
-@@ -1567,7 +1586,7 @@ public:
+@@ -1529,7 +1548,7 @@ public:
     */
    int calc_op_budget(Op *op);
-   void _throttle_op(Op *op, int op_size=0);
--  void _take_op_budget(Op *op) {
-+  int _take_op_budget(Op *op) {
-     assert(rwlock.is_locked());
+   void throttle_op(Op *op, int op_size=0);
+-  void take_op_budget(Op *op) {
++  int take_op_budget(Op *op) {
      int op_budget = calc_op_budget(op);
      if (keep_balanced_budget) {
-@@ -1577,13 +1596,19 @@ public:
+       throttle_op(op, op_budget);
+@@ -1538,13 +1557,19 @@ public:
        op_throttle_ops.take(1);
      }
      op->budgeted = true;
@@ -202,13 +203,7 @@
    Throttle op_throttle_bytes, op_throttle_ops;
  
   public:
-@@ -1679,12 +1704,12 @@ private:
- 
-   // low-level
-   ceph_tid_t _op_submit(Op *op, RWLock::Context& lc);
--  ceph_tid_t _op_submit_with_budget(Op *op, RWLock::Context& lc);
-+  ceph_tid_t _op_submit_with_budget(Op *op, RWLock::Context& lc, int *ctx_budget = NULL);
-   inline void unregister_op(Op *op);
+@@ -1615,7 +1640,7 @@ private:
  
    // public interface
  public:
@ghost

This comment has been minimized.

Copy link

commented Sep 4, 2015

The conflict resolution looks good.

ghost pushed a commit that referenced this pull request Sep 4, 2015

Loic Dachary
Merge pull request #5043 from SUSE/wip-12007-firefly
Objecter: pg listing can deadlock when throttling is in use

Reviewed-by: Yehuda Sadeh <ysadehwe@redhat.com>
Reviewed-by: Loic Dachary <ldachary@redhat.com>

@ghost ghost merged commit 6053bff into ceph:firefly Sep 4, 2015

@smithfarm smithfarm deleted the SUSE:wip-12007-firefly branch Sep 5, 2015

@ghost ghost changed the title Objecter: pg listing can deadlock when throttling is in use objecter: pg listing can deadlock when throttling is in use Oct 24, 2015

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.