common: avoid atomical nref bumping on freshly created buffer::raw.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index 66b3f5ec85d885bcb3a9f99787fb1a64b4bbf45f..f6a33e43b6806b8fc479c53d92aee08201741be5 100644 (file)
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -361,16 +361,16 @@ static ceph::spinlock debug_lock;
   }
 #endif /* HAVE_XIO */
 
-  buffer::raw* buffer::copy(const char *c, unsigned len) {
-    raw* r = buffer::create_aligned(len, sizeof(size_t));
+  ceph::unique_leakable_ptr<buffer::raw> buffer::copy(const char *c, unsigned len) {
+    auto r = buffer::create_aligned(len, sizeof(size_t));
     memcpy(r->data, c, len);
     return r;
   }
 
-  buffer::raw* buffer::create(unsigned len) {
+  ceph::unique_leakable_ptr<buffer::raw> buffer::create(unsigned len) {
     return buffer::create_aligned(len, sizeof(size_t));
   }
-  buffer::raw* buffer::create_in_mempool(unsigned len, int mempool) {
+  ceph::unique_leakable_ptr<buffer::raw> buffer::create_in_mempool(unsigned len, int mempool) {
     return buffer::create_aligned_in_mempool(len, sizeof(size_t), mempool);
   }
   buffer::raw* buffer::claim_char(unsigned len, char *buf) {
@@ -389,7 +389,7 @@ static ceph::spinlock debug_lock;
     return new raw_claim_buffer(buf, len, std::move(del));
   }
 
-  buffer::raw* buffer::create_aligned_in_mempool(
+  ceph::unique_leakable_ptr<buffer::raw> buffer::create_aligned_in_mempool(
     unsigned len, unsigned align, int mempool) {
     // If alignment is a page multiple, use a separate buffer::raw to
     // avoid fragmenting the heap.
@@ -403,23 +403,24 @@ static ceph::spinlock debug_lock;
     if ((align & ~CEPH_PAGE_MASK) == 0 ||
        len >= CEPH_PAGE_SIZE * 2) {
 #ifndef __CYGWIN__
-      return new raw_posix_aligned(len, align);
+      return ceph::unique_leakable_ptr<buffer::raw>(new raw_posix_aligned(len, align));
 #else
-      return new raw_hack_aligned(len, align);
+      return ceph::unique_leakable_ptr<buffer::raw>(new raw_hack_aligned(len, align));
 #endif
     }
-    return raw_combined::create(len, align, mempool);
+    return ceph::unique_leakable_ptr<buffer::raw>(
+      raw_combined::create(len, align, mempool));
   }
-  buffer::raw* buffer::create_aligned(
+  ceph::unique_leakable_ptr<buffer::raw> buffer::create_aligned(
     unsigned len, unsigned align) {
     return create_aligned_in_mempool(len, align,
                                     mempool::mempool_buffer_anon);
   }
 
-  buffer::raw* buffer::create_page_aligned(unsigned len) {
+  ceph::unique_leakable_ptr<buffer::raw> buffer::create_page_aligned(unsigned len) {
     return create_aligned(len, CEPH_PAGE_SIZE);
   }
-  buffer::raw* buffer::create_small_page_aligned(unsigned len) {
+  ceph::unique_leakable_ptr<buffer::raw> buffer::create_small_page_aligned(unsigned len) {
     if (len < CEPH_PAGE_SIZE) {
       return create_aligned(len, CEPH_BUFFER_ALLOC_UNIT);
     } else
@@ -435,16 +436,24 @@ static ceph::spinlock debug_lock;
     r->nref++;
     bdout << "ptr " << this << " get " << _raw << bendl;
   }
+  buffer::ptr::ptr(ceph::unique_leakable_ptr<raw> r)
+    : _raw(r.release()),
+      _off(0),
+      _len(_raw->len)
+  {
+    _raw->nref.store(1, std::memory_order_release);
+    bdout << "ptr " << this << " get " << _raw << bendl;
+  }
   buffer::ptr::ptr(unsigned l) : _off(0), _len(l)
   {
-    _raw = create(l);
-    _raw->nref++;
+    _raw = buffer::create(l).release();
+    _raw->nref.store(1, std::memory_order_release);
     bdout << "ptr " << this << " get " << _raw << bendl;
   }
   buffer::ptr::ptr(const char *d, unsigned l) : _off(0), _len(l)    // ditto.
   {
-    _raw = copy(d, l);
-    _raw->nref++;
+    _raw = buffer::copy(d, l).release();
+    _raw->nref.store(1, std::memory_order_release);
     bdout << "ptr " << this << " get " << _raw << bendl;
   }
   buffer::ptr::ptr(const ptr& p) : _raw(p._raw), _off(p._off), _len(p._len)
@@ -2159,6 +2168,13 @@ bool buffer::ptr_node::dispose_if_hypercombined(
   return is_hypercombined;
 }
 
+std::unique_ptr<buffer::ptr_node, buffer::ptr_node::disposer>
+buffer::ptr_node::create_hypercombined(ceph::unique_leakable_ptr<buffer::raw> r)
+{
+  return std::unique_ptr<buffer::ptr_node, buffer::ptr_node::disposer>(
+    new (&r->bptr_storage) ptr_node(std::move(r)));
+}
+
 std::unique_ptr<buffer::ptr_node, buffer::ptr_node::disposer>
 buffer::ptr_node::create_hypercombined(buffer::raw* const r)
 {

diff --git a/src/common/buffer_seastar.cc b/src/common/buffer_seastar.cc
index 4ef511a6760c13a756cef07139ccfa1115b757dd..80af898b3dfbe518bf05ddc944e309834a6dd0e6 100644 (file)
--- a/src/common/buffer_seastar.cc
+++ b/src/common/buffer_seastar.cc
@@ -27,7 +27,7 @@ class raw_seastar_foreign_ptr : public raw {
   raw_seastar_foreign_ptr(temporary_buffer&& buf)
     : raw(buf.get_write(), buf.size()), ptr(std::move(buf)) {}
   raw* clone_empty() override {
-    return create(len);
+    return create(len).release();
   }
 };
 
@@ -41,7 +41,7 @@ class raw_seastar_local_ptr : public raw {
   raw_seastar_local_ptr(temporary_buffer&& buf)
     : raw(buf.get_write(), buf.size()), buf(std::move(buf)) {}
   raw* clone_empty() override {
-    return create(len);
+    return create(len).release();
   }
 };
 
@@ -88,7 +88,7 @@ public:
   raw_seastar_local_shared_ptr(temporary_buffer& buf)
     : raw(buf.get_write(), buf.size()), buf(buf.share()) {}
   raw* clone_empty() override {
-    return ceph::buffer::create(len);
+    return ceph::buffer::create(len).release();
   }
 };
 }
@@ -103,6 +103,5 @@ buffer::ptr seastar_buffer_iterator::get_ptr(size_t len)
 
 buffer::ptr const_seastar_buffer_iterator::get_ptr(size_t len)
 {
-  buffer::raw* r = buffer::copy(get_pos_add(len), len);
-  return buffer::ptr{r};
+  return buffer::ptr{ buffer::copy(get_pos_add(len), len) };
 }

diff --git a/src/include/buffer.h b/src/include/buffer.h
index 97bc27a10001ca0d27199c5b43a41f93eef14482..b9967cce6a92cf740c30e8f19053fb0e69d6061c 100644 (file)
--- a/src/include/buffer.h
+++ b/src/include/buffer.h
@@ -165,17 +165,17 @@ namespace buffer CEPH_BUFFER_API {
   /*
    * named constructors
    */
-  raw* copy(const char *c, unsigned len);
-  raw* create(unsigned len);
-  raw* create_in_mempool(unsigned len, int mempool);
+  ceph::unique_leakable_ptr<raw> copy(const char *c, unsigned len);
+  ceph::unique_leakable_ptr<raw> create(unsigned len);
+  ceph::unique_leakable_ptr<raw> create_in_mempool(unsigned len, int mempool);
   raw* claim_char(unsigned len, char *buf);
   raw* create_malloc(unsigned len);
   raw* claim_malloc(unsigned len, char *buf);
   raw* create_static(unsigned len, char *buf);
-  raw* create_aligned(unsigned len, unsigned align);
-  raw* create_aligned_in_mempool(unsigned len, unsigned align, int mempool);
-  raw* create_page_aligned(unsigned len);
-  raw* create_small_page_aligned(unsigned len);
+  ceph::unique_leakable_ptr<raw> create_aligned(unsigned len, unsigned align);
+  ceph::unique_leakable_ptr<raw> create_aligned_in_mempool(unsigned len, unsigned align, int mempool);
+  ceph::unique_leakable_ptr<raw> create_page_aligned(unsigned len);
+  ceph::unique_leakable_ptr<raw> create_small_page_aligned(unsigned len);
   raw* create_unshareable(unsigned len);
   raw* create_static(unsigned len, char *buf);
   raw* claim_buffer(unsigned len, char *buf, deleter del);
@@ -269,6 +269,7 @@ namespace buffer CEPH_BUFFER_API {
     ptr() : _raw(nullptr), _off(0), _len(0) {}
     // cppcheck-suppress noExplicitConstructor
     ptr(raw* r);
+    ptr(ceph::unique_leakable_ptr<raw> r);
     // cppcheck-suppress noExplicitConstructor
     ptr(unsigned l);
     ptr(const char *d, unsigned l);
@@ -409,6 +410,10 @@ namespace buffer CEPH_BUFFER_API {
 
     ~ptr_node() = default;
 
+    static std::unique_ptr<ptr_node, disposer>
+    create(ceph::unique_leakable_ptr<raw> r) {
+      return create_hypercombined(std::move(r));
+    }
     static std::unique_ptr<ptr_node, disposer> create(raw* const r) {
       return create_hypercombined(r);
     }
@@ -437,7 +442,10 @@ namespace buffer CEPH_BUFFER_API {
     void swap(ptr_node& other) noexcept = delete;
 
     static bool dispose_if_hypercombined(ptr_node* delete_this);
-    static std::unique_ptr<ptr_node, disposer> create_hypercombined(raw* r);
+    static std::unique_ptr<ptr_node, disposer> create_hypercombined(
+      buffer::raw* r);
+    static std::unique_ptr<ptr_node, disposer> create_hypercombined(
+      ceph::unique_leakable_ptr<raw> r);
   };
   /*
    * list - the useful bit!

diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc
index 35439cae8520a0c76e4f216acd77865bff3bfe1a..56fff96e09ac303a434652388849d8ecacc18ef4 100644 (file)
--- a/src/librbd/librbd.cc
+++ b/src/librbd/librbd.cc
@@ -69,8 +69,8 @@ namespace {
 
 TracepointProvider::Traits tracepoint_traits("librbd_tp.so", "rbd_tracing");
 
-buffer::raw* create_write_raw(librbd::ImageCtx *ictx, const char *buf,
-                              size_t len) {
+static auto create_write_raw(librbd::ImageCtx *ictx, const char *buf,
+                             size_t len) {
   // TODO: until librados can guarantee memory won't be referenced after
   // it ACKs a request, always make a copy of the user-provided memory
   return buffer::copy(buf, len);

diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index b33018dd044966b1090c96a221aa42591e76f291..8a5bc65d31dadb46db1a6b1f3afd1a0492264480 100644 (file)
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -24,6 +24,7 @@
 #include <sys/uio.h>
 
 #include "include/buffer.h"
+#include "include/buffer_raw.h"
 #include "include/utime.h"
 #include "include/coredumpctl.h"
 #include "include/encoding.h"