From: Loic Dachary Date: Sat, 26 Oct 2013 00:11:43 +0000 (+0200) Subject: common: rebuild_page_aligned sometimes rebuilds unaligned X-Git-Tag: v0.73~63^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=66a9fbe2c7ba59b7cd034c17865adce3432cd2cb;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 --- diff --git a/src/common/buffer.cc b/src/common/buffer.cc index 493070557159..acdfee0885b5 100644 --- 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::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 0b497a7cf38d..657e66e2cc34 100644 --- 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 8b6ca269234f..167f4cb4a239 100644 --- 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: + */ +