From: Sebastien Ponce Date: Tue, 16 Aug 2016 16:25:36 +0000 (+0200) Subject: radosstriper : Fixed memory leaks found in valgrind testing of (improved) striper... X-Git-Tag: v11.1.0~355^2~5 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=412ee3adcd77e71df3911d2e20aea6cdc0b56f1a;p=ceph.git radosstriper : Fixed memory leaks found in valgrind testing of (improved) striper library Signed-off-by: Sebastien Ponce --- diff --git a/src/libradosstriper/MultiAioCompletionImpl.h b/src/libradosstriper/MultiAioCompletionImpl.h index f9c6153381c3..a7d8ed4eb6b0 100644 --- a/src/libradosstriper/MultiAioCompletionImpl.h +++ b/src/libradosstriper/MultiAioCompletionImpl.h @@ -148,6 +148,12 @@ struct libradosstriper::MultiAioCompletionImpl { _get(); lock.Unlock(); } + void add_safe_request() { + lock.Lock(); + pending_complete++; + _get(); + lock.Unlock(); + } void complete() { assert(lock.is_locked()); if (callback_complete) { diff --git a/src/libradosstriper/RadosStriperImpl.cc b/src/libradosstriper/RadosStriperImpl.cc index ca0a1bb8e262..02c77a15dff0 100644 --- a/src/libradosstriper/RadosStriperImpl.cc +++ b/src/libradosstriper/RadosStriperImpl.cc @@ -223,7 +223,7 @@ void rados_req_unlock_complete(rados_completion_t c, void *arg) } libradosstriper::RadosStriperImpl::RadosExclusiveLock::RadosExclusiveLock(librados::IoCtx* ioCtx, - const std::string& oid) : + const std::string oid) : m_ioCtx(ioCtx), m_oid(oid) { librados::ObjectWriteOperation op; @@ -598,6 +598,7 @@ static void striper_stat_aio_stat_complete(rados_completion_t c, void *arg) { data->m_statRC = rc; } data->m_multiCompletion->complete_request(rc); + data->put(); } static void striper_stat_aio_getxattr_complete(rados_completion_t c, void *arg) { @@ -620,6 +621,7 @@ static void striper_stat_aio_getxattr_complete(rados_completion_t c, void *arg) rc = 0; } data->m_multiCompletion->complete_request(rc); + data->put(); } static void striper_stat_aio_req_complete(rados_striper_multi_completion_t c, @@ -654,13 +656,12 @@ int libradosstriper::RadosStriperImpl::aio_generic_stat std::string firstObjOid = getObjectId(soid, 0); StatCompletionData *cdata = new StatCompletionData(this, firstObjOid, c, - multi_completion, psize, pmtime); + multi_completion, psize, pmtime, 4); multi_completion->set_complete_callback(cdata, striper_stat_aio_req_complete); // use a regular AioCompletion for the stat async call librados::AioCompletion *stat_completion = - librados::Rados::aio_create_completion(cdata, - striper_stat_aio_stat_complete, 0); - multi_completion->add_request(); + librados::Rados::aio_create_completion(cdata, striper_stat_aio_stat_complete, 0); + multi_completion->add_safe_request(); object_t obj(firstObjOid); int rc = (m_ioCtxImpl->*statFunction)(obj, stat_completion->pc, &cdata->m_objectSize, cdata->m_pmtime); @@ -674,9 +675,8 @@ int libradosstriper::RadosStriperImpl::aio_generic_stat } // use a regular AioCompletion for the getxattr async call librados::AioCompletion *getxattr_completion = - librados::Rados::aio_create_completion(cdata, - striper_stat_aio_getxattr_complete, 0); - multi_completion->add_request(); + librados::Rados::aio_create_completion(cdata, striper_stat_aio_getxattr_complete, 0); + multi_completion->add_safe_request(); // in parallel, get the pmsize from the first object asynchronously rc = m_ioCtxImpl->aio_getxattr(obj, getxattr_completion->pc, XATTR_SIZE, cdata->m_bl); @@ -688,8 +688,11 @@ int libradosstriper::RadosStriperImpl::aio_generic_stat cdata->m_getxattrRC = rc; delete getxattr_completion; multi_completion->complete_request(rc); + multi_completion->put(); return rc; } + cdata->put(); + multi_completion->put(); return 0; } @@ -734,7 +737,20 @@ static void rados_req_remove_complete(rados_completion_t c, void *arg) rc = 0; } cdata->m_multiAioCompl->complete_request(rc); - cdata -> put(); + cdata->put(); +} + +static void rados_req_remove_safe(rados_completion_t c, void *arg) +{ + libradosstriper::RadosStriperImpl::RadosRemoveCompletionData *cdata = + reinterpret_cast(arg); + int rc = rados_aio_get_return_value(c); + // in case the object did not exist, it means we had a sparse file, all is fine + if (rc == -ENOENT) { + rc = 0; + } + cdata->m_multiAioCompl->safe_request(rc); + cdata->put(); } static void striper_remove_aio_req_complete(rados_striper_multi_completion_t c, void *arg) @@ -788,7 +804,6 @@ int libradosstriper::RadosStriperImpl::aio_remove(const std::string& soid, RadosExclusiveLock *lock = new RadosExclusiveLock(&m_ioCtx, getObjectId(soid, 0)); // lock ownership is transferred to RemoveCompletionData here RemoveCompletionData *cdata = new RemoveCompletionData(this, soid, lockCookie, c, lock, flags); - cdata->get(); libradosstriper::MultiAioCompletionImpl *multi_completion = new libradosstriper::MultiAioCompletionImpl; multi_completion->set_complete_callback(cdata, striper_remove_aio_req_complete); @@ -796,7 +811,9 @@ int libradosstriper::RadosStriperImpl::aio_remove(const std::string& soid, ldout(cct(), 10) << "RadosStriperImpl : Aio_remove starting for " << soid << dendl; - return internal_aio_remove(soid, multi_completion); + int rc = internal_aio_remove(soid, multi_completion); + multi_completion->put(); + return rc; } catch (ErrorCode &e) { return e.m_code; } @@ -851,7 +868,9 @@ int libradosstriper::RadosStriperImpl::internal_aio_remove RadosRemoveCompletionData *data = new RadosRemoveCompletionData(multi_completion, cct()); librados::AioCompletion *rados_completion = - librados::Rados::aio_create_completion(data, rados_req_remove_complete, 0); + librados::Rados::aio_create_completion(data, + rados_req_remove_complete, + rados_req_remove_safe); if (flags == 0) { rcr = m_ioCtx.aio_remove(getObjectId(soid, i), rados_completion); } else { @@ -1124,6 +1143,8 @@ int libradosstriper::RadosStriperImpl::internal_get_layout_and_size( size_t ssize; rc = extract_sizet_attr(attrs, XATTR_SIZE, &ssize); *size = ssize; + // make valgrind happy by setting unused fl_pg_pool + layout->fl_pg_pool = 0; return rc; } @@ -1283,7 +1304,6 @@ int libradosstriper::RadosStriperImpl::truncate(const std::string& soid, ceph_file_layout &layout) { TruncateCompletionData *cdata = new TruncateCompletionData(this, soid, size); - cdata->get(); libradosstriper::MultiAioCompletionImpl *multi_completion = new libradosstriper::MultiAioCompletionImpl; multi_completion->set_complete_callback(cdata, striper_truncate_aio_req_complete); @@ -1335,10 +1355,12 @@ int libradosstriper::RadosStriperImpl::aio_truncate if (exists) { // remove asynchronously multi_completion->add_request(); + RadosRemoveCompletionData *data = + new RadosRemoveCompletionData(multi_completion, cct()); librados::AioCompletion *rados_completion = - librados::Rados::aio_create_completion(multi_completion, + librados::Rados::aio_create_completion(data, rados_req_remove_complete, - 0); + rados_req_remove_safe); int rc = m_ioCtx.aio_remove(getObjectId(soid, objectno), rados_completion); rados_completion->release(); // in case the object did not exist, it means we had a sparse file, all is fine @@ -1368,10 +1390,12 @@ int libradosstriper::RadosStriperImpl::aio_truncate } else { // removes are asynchronous in order to speed up truncations of big files multi_completion->add_request(); + RadosRemoveCompletionData *data = + new RadosRemoveCompletionData(multi_completion, cct()); librados::AioCompletion *rados_completion = - librados::Rados::aio_create_completion(multi_completion, + librados::Rados::aio_create_completion(data, rados_req_remove_complete, - 0); + rados_req_remove_safe); rc = m_ioCtx.aio_remove(getObjectId(soid, objectno), rados_completion); rados_completion->release(); } diff --git a/src/libradosstriper/RadosStriperImpl.h b/src/libradosstriper/RadosStriperImpl.h index 74a3062ce862..db261db31a0c 100644 --- a/src/libradosstriper/RadosStriperImpl.h +++ b/src/libradosstriper/RadosStriperImpl.h @@ -133,8 +133,9 @@ struct libradosstriper::RadosStriperImpl { const std::string& soid, librados::AioCompletionImpl *userCompletion, libradosstriper::MultiAioCompletionImpl *multiCompletion, - uint64_t *psize) : - CompletionData(striper, soid, "", userCompletion), + uint64_t *psize, + int n = 1) : + CompletionData(striper, soid, "", userCompletion, n), m_multiCompletion(multiCompletion), m_psize(psize), m_statRC(0), m_getxattrRC(0) {}; // MultiAioCompletionImpl used to handle the double aysnc @@ -169,8 +170,9 @@ struct libradosstriper::RadosStriperImpl { librados::AioCompletionImpl *userCompletion, libradosstriper::MultiAioCompletionImpl *multiCompletion, uint64_t *psize, - TimeType *pmtime) : - BasicStatCompletionData(striper, soid, userCompletion, multiCompletion, psize), + TimeType *pmtime, + int n = 1) : + BasicStatCompletionData(striper, soid, userCompletion, multiCompletion, psize, n), m_pmtime(pmtime) {}; // where to store the file time TimeType *m_pmtime; @@ -184,7 +186,7 @@ struct libradosstriper::RadosStriperImpl { /// constructor RadosRemoveCompletionData(MultiAioCompletionImpl *multiAioCompl, CephContext *context) : - RefCountedObject(context, 1), + RefCountedObject(context, 2), m_multiAioCompl(multiAioCompl) {}; /// the multi asynch io completion object to be used MultiAioCompletionImpl *m_multiAioCompl; @@ -197,11 +199,11 @@ struct libradosstriper::RadosStriperImpl { /// striper to be used to handle the locking librados::IoCtx* m_ioCtx; /// object to be locked - const std::string& m_oid; + const std::string m_oid; /// name of the lock std::string m_lockCookie; /// constructor : takes the lock - RadosExclusiveLock(librados::IoCtx* ioCtx, const std::string &oid); + RadosExclusiveLock(librados::IoCtx* ioCtx, const std::string oid); /// destructor : releases the lock ~RadosExclusiveLock(); }; diff --git a/src/test/libradosstriper/striping.cc b/src/test/libradosstriper/striping.cc index e1b580182612..496ff22d898c 100644 --- a/src/test/libradosstriper/striping.cc +++ b/src/test/libradosstriper/striping.cc @@ -46,15 +46,18 @@ protected: uint64_t stripe_unit = strtoll(s_xattr.c_str(), NULL, 10); ASSERT_LT((unsigned)0, stripe_unit); ASSERT_EQ(stripe_unit, exp_stripe_unit); + xattrbl.clear(); ASSERT_LT(0, ioctx.getxattr(firstOid, "striper.layout.stripe_count", xattrbl)); s_xattr = std::string(xattrbl.c_str(), xattrbl.length()); // adds 0 byte at the end uint64_t stripe_count = strtoll(s_xattr.c_str(), NULL, 10); ASSERT_LT(0U, stripe_count); ASSERT_EQ(stripe_count, exp_stripe_count); + xattrbl.clear(); ASSERT_LT(0, ioctx.getxattr(firstOid, "striper.layout.object_size", xattrbl)); s_xattr = std::string(xattrbl.c_str(), xattrbl.length()); // adds 0 byte at the end uint64_t object_size = strtoll(s_xattr.c_str(), NULL, 10); ASSERT_EQ(object_size, exp_object_size); + xattrbl.clear(); ASSERT_LT(0, ioctx.getxattr(firstOid, "striper.size", xattrbl)); s_xattr = std::string(xattrbl.c_str(), xattrbl.length()); // adds 0 byte at the end uint64_t xa_size = strtoll(s_xattr.c_str(), NULL, 10); @@ -162,12 +165,13 @@ TEST_P(StriperTestRT, StripedRoundtrip) { << "_" << testData.size; std::string soid = oss.str(); // writing striped data + char* buf1; bufferlist bl1; { SCOPED_TRACE("Writing initial object"); - char buf[testData.size]; - for (unsigned int i = 0; i < testData.size; i++) buf[i] = 13*((unsigned char)i); - bl1.append(buf, testData.size); + buf1 = (char*) calloc(1, testData.size); + for (unsigned int i = 0; i < testData.size; i++) buf1[i] = 13*((unsigned char)i); + bl1.append(buf1, testData.size); ASSERT_EQ(0, striper.write(soid, bl1, testData.size, 0)); // checking object state from Rados point of view ASSERT_NO_FATAL_FAILURE(checkObjectFromRados(soid, bl1, testData.stripe_unit, @@ -175,10 +179,11 @@ TEST_P(StriperTestRT, StripedRoundtrip) { testData.size)); } // adding more data to object and checking again + char* buf2; bufferlist bl2; { SCOPED_TRACE("Testing append"); - char buf2[testData.size]; + buf2 = (char*) calloc(1, testData.size); for (unsigned int i = 0; i < testData.size; i++) buf2[i] = 17*((unsigned char)i); bl2.append(buf2, testData.size); ASSERT_EQ(0, striper.append(soid, bl2, testData.size)); @@ -224,7 +229,9 @@ TEST_P(StriperTestRT, StripedRoundtrip) { for (uint64_t object_nb = 0; object_nb < testData.size*2/testData.object_size + testData.stripe_count; object_nb++) { - ASSERT_EQ(-ENOENT, ioctx.stat(getObjName(soid, object_nb), &size, &mtime)); + char* oid = getObjName(soid, object_nb); + ASSERT_EQ(-ENOENT, ioctx.stat(oid, &size, &mtime)); + free(oid); } } { @@ -234,6 +241,7 @@ TEST_P(StriperTestRT, StripedRoundtrip) { // remove the object size attribute from the striped object char* firstOid = getObjName(soid, 0); ASSERT_EQ(0, ioctx.rmxattr(firstOid, "striper.size")); + free(firstOid); // check that stat fails uint64_t size; time_t mtime; @@ -244,10 +252,13 @@ TEST_P(StriperTestRT, StripedRoundtrip) { for (uint64_t object_nb = 0; object_nb < testData.size*2/testData.object_size + testData.stripe_count; object_nb++) { - ASSERT_EQ(-ENOENT, ioctx.stat(getObjName(soid, object_nb), &size, &mtime)); + char* oid = getObjName(soid, object_nb); + ASSERT_EQ(-ENOENT, ioctx.stat(oid, &size, &mtime)); + free(oid); } - free(firstOid); } + free(buf1); + free(buf2); } const TestData simple_stripe_schemes[] = {