From: Loic Dachary Date: Sat, 26 Oct 2013 00:11:43 +0000 (+0200) Subject: common: rebuild_page_aligned sometimes rebuilds unaligned X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=1d7a228d85117bf877f99f37bcfce502c4886b7b;p=ceph.git 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 (cherry picked from commit 66a9fbe2c7ba59b7cd034c17865adce3432cd2cb) --- diff --git a/src/common/buffer.cc b/src/common/buffer.cc index a32990c57d4..dc7cbdc7bde 100644 --- 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::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 8c4dfb56e17..33e86465ba9 100644 --- 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 04374d91cad..740d6d01abb 100644 --- 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: + */ +