common: rebuild_page_aligned sometimes rebuilds unaligned

rebuild_page_aligned relies on rebuild to create memory that is aligned
according to list::is_page_aligned(). However, when the bufferlist only
contains a single ptr and that its size is not list::is_n_page_size(),
rebuild will not create the expected alligned bufferlist.

The allocation of the ptr is moved out of rebuild which is now given the
ptr as an argument. The rebuild_page_aligned function always require an
aligned ptr with buffer::create_page_aligned(_len) for consistency.

The test

    bufferlist bl;
    bufferptr ptr(buffer::create_page_aligned(2));
    ptr.set_offset(1);
    ptr.set_length(1);
    bl.append(ptr);
    EXPECT_FALSE(bl.is_page_aligned());
    bl.rebuild_page_aligned();
    EXPECT_FALSE(bl.is_page_aligned());

demonstrated the problem. It was assumed to be a feature but should have
been identified as a bug. The last ligne is replaced with

    EXPECT_TRUE(bl.is_page_aligned());

Most tests related to is_page_aligned() wrongfully assumed that

    bufferptr ptr(2);

is never page aligned. Most of the time it is not but sometime it is
when the pointer address is by chance on a CEPH_PAGE_SIZE boundary,
which triggered #6614. Non aligned ptr are created as follows instead:

    bufferptr ptr(buffer::create_page_aligned(2));
    ptr.set_offset(1);
    ptr.set_length(1);

http://tracker.ceph.com/issues/6614 fixes: #6614

Signed-off-by: Loic Dachary <loic@dachary.org>

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index 493070557159e1dbc9d37d2abd16c407911f20f9..acdfee0885b5d56bc4a5b8d3e20ff2c2291a85ca 100644 (file)
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -806,6 +806,11 @@ static uint32_t simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZE
       nb = buffer::create_page_aligned(_len);
     else
       nb = buffer::create(_len);
+    rebuild(nb);
+  }
+
+  void buffer::list::rebuild(ptr& nb)
+  {
     unsigned pos = 0;
     for (std::list<ptr>::iterator it = _buffers.begin();
         it != _buffers.end();
@@ -849,7 +854,8 @@ void buffer::list::rebuild_page_aligned()
             (!p->is_page_aligned() ||
              !p->is_n_page_sized() ||
              (offset & ~CEPH_PAGE_MASK)));
-    unaligned.rebuild();
+    ptr nb(buffer::create_page_aligned(_len));
+    unaligned.rebuild(nb);
     _buffers.insert(p, unaligned._buffers.front());
   }
 }

diff --git a/src/include/buffer.h b/src/include/buffer.h
index 0b497a7cf38d6344d9b1ce290ce4aed4abb21db6..657e66e2cc3414117378a7b5497a1a9640f8d7f5 100644 (file)
--- a/src/include/buffer.h
+++ b/src/include/buffer.h
@@ -379,6 +379,7 @@ public:
 
     bool is_contiguous();
     void rebuild();
+    void rebuild(ptr& nb);
     void rebuild_page_aligned();
 
     // sort-of-like-assignment-op

diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index 8b6ca269234ff59e7c2292b615aa5a88ee65ea3f..167f4cb4a23985691edbfc25543f444f8c2c7faa 100644 (file)
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -969,17 +969,17 @@ TEST(BufferList, is_page_aligned) {
   }
   {
     bufferlist bl;
-    bufferptr ptr(2);
+    bufferptr ptr(buffer::create_page_aligned(2));
     ptr.set_offset(1);
     ptr.set_length(1);
     bl.append(ptr);
     EXPECT_FALSE(bl.is_page_aligned());
     bl.rebuild_page_aligned();
-    EXPECT_FALSE(bl.is_page_aligned());
+    EXPECT_TRUE(bl.is_page_aligned());
   }
   {
     bufferlist bl;
-    bufferptr ptr(CEPH_PAGE_SIZE + 1);
+    bufferptr ptr(buffer::create_page_aligned(CEPH_PAGE_SIZE + 1));
     ptr.set_offset(1);
     ptr.set_length(CEPH_PAGE_SIZE);
     bl.append(ptr);
@@ -1126,7 +1126,7 @@ TEST(BufferList, is_contiguous) {
 TEST(BufferList, rebuild) {
   {
     bufferlist bl;
-    bufferptr ptr(2);
+    bufferptr ptr(buffer::create_page_aligned(2));
     ptr.set_offset(1);
     ptr.set_length(1);
     bl.append(ptr);
@@ -1151,7 +1151,7 @@ TEST(BufferList, rebuild_page_aligned) {
   {
     bufferlist bl;
     {
-      bufferptr ptr(CEPH_PAGE_SIZE + 1);
+      bufferptr ptr(buffer::create_page_aligned(CEPH_PAGE_SIZE + 1));
       ptr.set_offset(1);
       ptr.set_length(CEPH_PAGE_SIZE);
       bl.append(ptr);
@@ -1166,30 +1166,42 @@ TEST(BufferList, rebuild_page_aligned) {
     bufferlist bl;
     {
       bufferptr ptr(buffer::create_page_aligned(CEPH_PAGE_SIZE));
+      EXPECT_TRUE(ptr.is_page_aligned());
+      EXPECT_TRUE(ptr.is_n_page_sized());
       bl.append(ptr);
     }
     {
-      bufferptr ptr(CEPH_PAGE_SIZE + 1);
+      bufferptr ptr(buffer::create_page_aligned(CEPH_PAGE_SIZE + 1));
+      EXPECT_TRUE(ptr.is_page_aligned());
+      EXPECT_FALSE(ptr.is_n_page_sized());
       bl.append(ptr);
     }
     {
-      bufferptr ptr(2);
+      bufferptr ptr(buffer::create_page_aligned(2));
       ptr.set_offset(1);
       ptr.set_length(1);
+      EXPECT_FALSE(ptr.is_page_aligned());
+      EXPECT_FALSE(ptr.is_n_page_sized());
       bl.append(ptr);
     }
     {
-      bufferptr ptr(CEPH_PAGE_SIZE - 2);
+      bufferptr ptr(buffer::create_page_aligned(CEPH_PAGE_SIZE - 2));
+      EXPECT_TRUE(ptr.is_page_aligned());
+      EXPECT_FALSE(ptr.is_n_page_sized());
       bl.append(ptr);
     }
     {
       bufferptr ptr(buffer::create_page_aligned(CEPH_PAGE_SIZE));
+      EXPECT_TRUE(ptr.is_page_aligned());
+      EXPECT_TRUE(ptr.is_n_page_sized());
       bl.append(ptr);
     }
     {
-      bufferptr ptr(CEPH_PAGE_SIZE + 1);
+      bufferptr ptr(buffer::create_page_aligned(CEPH_PAGE_SIZE + 1));
       ptr.set_offset(1);
       ptr.set_length(CEPH_PAGE_SIZE);
+      EXPECT_FALSE(ptr.is_page_aligned());
+      EXPECT_TRUE(ptr.is_n_page_sized());
       bl.append(ptr);
     }
     EXPECT_EQ((unsigned)6, bl.buffers().size());
@@ -2012,6 +2024,12 @@ TEST(BufferHash, all) {
   }
 }
 
-// Local Variables:
-// compile-command: "cd .. ; make unittest_bufferlist ; ulimit -s unlimited ; CEPH_BUFFER_TRACK=true valgrind --max-stackframe=20000000 --tool=memcheck ./unittest_bufferlist # --gtest_filter=BufferList.constructors"
-// End:
+/*
+ * Local Variables:
+ * compile-command: "cd .. ; make unittest_bufferlist && 
+ *    ulimit -s unlimited ; CEPH_BUFFER_TRACK=true valgrind \
+ *    --max-stackframe=20000000 --tool=memcheck \
+ *    ./unittest_bufferlist # --gtest_filter=BufferList.constructors"
+ * End:
+ */
+