radosstriper : Fixed memory leaks found in valgrind testing of (improved) striper...

Signed-off-by: Sebastien Ponce <sebastien.ponce@cern.ch>

diff --git a/src/libradosstriper/MultiAioCompletionImpl.h b/src/libradosstriper/MultiAioCompletionImpl.h
index f9c6153381c38c30dc4e699ab0b9b26cdc125734..a7d8ed4eb6b04169dd2385190e6129602e766fdd 100644 (file)
--- 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 ca0a1bb8e26299880e2ae19a669f9d15749f0e75..02c77a15dff091013c20be1ce9dcdbb85a6461f2 100644 (file)
--- 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<TimeType> *cdata =
     new StatCompletionData<TimeType>(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<libradosstriper::RadosStriperImpl::RadosRemoveCompletionData*>(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 74a3062ce862883a0a012917c17afcf815da13f3..db261db31a0cab8bf5e58cc3b797da79f4b59781 100644 (file)
--- 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 e1b5801826129ed1d761d313d0e647d894aa90b4..496ff22d898c20cfb0477cda79ce19fe08ef926c 100644 (file)
--- 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[] = {