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>
(cherry picked from commit 66a9fbe2c7ba59b7cd034c17865adce3432cd2cb)

diff --git a/src/common/buffer.cc b/src/common/buffer.cc
index a32990c57d4488a652810991e586bb5644d27c2d..dc7cbdc7bde58fd6ccda8875e99598f6cb30c638 100644 (file)
--- a/src/common/buffer.cc
+++ b/src/common/buffer.cc
@@ -761,6 +761,11 @@ bool buffer_track_alloc = get_env_bool("CEPH_BUFFER_TRACK");
       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();
@@ -804,7 +809,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 8c4dfb56e174d4cf8cdeeac07390ea300dea4fb7..33e86465ba902cd117ade7bb03c70aac959b8540 100644 (file)
--- a/src/include/buffer.h
+++ b/src/include/buffer.h
@@ -368,6 +368,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 04374d91cad1bd7bc080f060538abb0a1915605a..740d6d01abb01791ebebf9353621b75d1b236143 100644 (file)
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -967,17 +967,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);
@@ -1124,7 +1124,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);
@@ -1149,7 +1149,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);
@@ -1164,30 +1164,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());
@@ -1851,6 +1863,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:
+ */
+