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

test/fio: fix global CephContext life cycle #12245

Merged
merged 1 commit into from Dec 2, 2016

Conversation

ifed01
Copy link
Contributor

@ifed01 ifed01 commented Dec 1, 2016

It looks like that's a bad idea to omit add_ref when returning intrusive_ptr from global_init. Assigning temporary resulting pointer to another var doesn't increment ref counter. From the boost manual - this is equivalent to swap call instead. Hence we might have either lack of CephContext release or its' premature release. The latter case I observed in fio plugin.

Consider the following code snippet:

#include <iostream>
#include <boost/intrusive_ptr.hpp>
using namespace std;
struct  A
{
  int nref = 0;
  A() { cout<<"A"<<this<<std::endl;}
  ~A() { cout<<"~A"<<this<<std::endl;}

  void put() { if (--nref == 0 ) delete this;}
  void get() { nref++;}
};

void intrusive_ptr_add_ref(A* a)
{
  a->get();
  cout<<"add "<<a<<" "<<a->nref<<std::endl;
}
void intrusive_ptr_release(A* a)
{
  cout<<"release "<<a<<" "<<a->nref-1<<std::endl;
  a->put();
}

typedef boost::intrusive_ptr<A> APtr;
 APtr createAPtr() {
  A* res = new A;
  return APtr(res, false);
}

int main()
{
  auto aptr = createAPtr();
  APtr aptr2;
  aptr2 = createAPtr();
  APtr aptr3;
  aptr3 = createAPtr();
  aptr3->get(); //OK
  APtr aptr4(createAPtr());
  APtr aptr5;
  aptr5 = createAPtr();
  aptr5->get(); //OK
  APtr aptr6;
  aptr6 = aptr5; //OK
  createAPtr();
  cout<<"-----------------------"<<std::endl;
}

Corresponding output:

A0x9d8c20
A0x9d9050
A0x9d9070
A0x9d9090
A0x9d90b0
add 0x9d90b0 2
A0x19cd0d0
release 0x19cd0d0 -1
-----------------------
release 0x9d90b0 1
release 0x9d90b0 0
~A0x9d90b0
release 0x9d9090 -1
release 0x9d9070 0
~A0x9d9070
release 0x9d9050 -1
release 0x9d8c20 -1

@cbodley
Copy link
Contributor

cbodley commented Dec 1, 2016

the difference between CephContext and struct A in your example is that the CephContext ctor initializes nref(1)

your change to the fio engine is correct though

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

agreed with @cbodley , f6ea4d5 looks good to me. but we might want to drop c139e9b from this PR.

@ifed01 ifed01 changed the title global/global_init, test/fio: fix global CephContext refcounting test/fio: fix global CephContext life cycle Dec 1, 2016
@ifed01 ifed01 force-pushed the wip-fix-global-init-refcount branch from f6ea4d5 to c139e9b Compare December 1, 2016 17:36
@ifed01
Copy link
Contributor Author

ifed01 commented Dec 1, 2016

@cbodley @tchaikov yeah, missed that "minor" thing... Fixed.

@ifed01
Copy link
Contributor Author

ifed01 commented Dec 1, 2016

oops, dropped another commit..

Signed-off-by: Igor Fedotov <ifedotov@mirantis.com>
@ifed01 ifed01 force-pushed the wip-fix-global-init-refcount branch from c139e9b to 212294a Compare December 1, 2016 17:45
@ifed01
Copy link
Contributor Author

ifed01 commented Dec 1, 2016

fixed.

@ifed01
Copy link
Contributor Author

ifed01 commented Dec 2, 2016

@tchaikov would you approve?

@cbodley cbodley dismissed tchaikov’s stale review December 2, 2016 17:14

review comments addressed

@cbodley cbodley merged commit 0a60d35 into ceph:master Dec 2, 2016
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

lgtm =)

@ifed01 ifed01 deleted the wip-fix-global-init-refcount branch December 16, 2016 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants