From 5205d4dacf7ebe2e42d2294bc30cb27f226c8d22 Mon Sep 17 00:00:00 2001 From: Loic Dachary Date: Tue, 2 Dec 2014 02:04:14 +0100 Subject: [PATCH 1/3] common: allow size alignment that is not a power of two Do not assume the alignment is a power of two in the is_n_align_sized() predicate. When used in the context of erasure code it is common for chunks to not be powers of two. Signed-off-by: Loic Dachary (cherry picked from commit 73ad2d63d479b09037d50246106bbd4075fbce80) --- src/include/buffer.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/include/buffer.h b/src/include/buffer.h index 3cd0a7a19a072..0ce133a296416 100644 --- a/src/include/buffer.h +++ b/src/include/buffer.h @@ -184,7 +184,7 @@ class buffer { bool is_page_aligned() const { return is_aligned(CEPH_PAGE_SIZE); } bool is_n_align_sized(unsigned align) const { - return (length() & (align-1)) == 0; + return (length() % align) == 0; } bool is_n_page_sized() const { return is_n_align_sized(CEPH_PAGE_SIZE); } From cc469b238f42ce989d0efa49154b95612e3d4111 Mon Sep 17 00:00:00 2001 From: Loic Dachary Date: Tue, 2 Dec 2014 01:07:34 +0100 Subject: [PATCH 2/3] erasure-code: enforce chunk size alignment Let say the ErasureCode::encode function is given a 4096 bytes bufferlist made of a 1249 bytes bufferptr followed by a 2847 bytes bufferptr, both properly starting on SIMD_ALIGN address. As a result the second 2048 had to be reallocated when bufferlist::substr_of gets the second 2048 buffer, the address starts at 799 bytes after the beginning of the 2847 buffer ptr and is not SIMD_ALIGN'ed. The ErasureCode::encode must enforce a size alignment based on the chunk size in addition to the memory alignment required by SIMD operations, using the bufferlist::rebuild_aligned_size_and_memory function instead of bufferlist::rebuild_aligned. http://tracker.ceph.com/issues/10211 Fixes: #10211 Signed-off-by: Loic Dachary (cherry picked from commit 4e955f41297283798236c505c3d21bdcabb5caa0) --- src/erasure-code/ErasureCode.cc | 11 ++------ src/test/erasure-code/TestErasureCode.cc | 36 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/erasure-code/ErasureCode.cc b/src/erasure-code/ErasureCode.cc index 96a6b39c7d383..30dc186b594a7 100644 --- a/src/erasure-code/ErasureCode.cc +++ b/src/erasure-code/ErasureCode.cc @@ -66,21 +66,14 @@ int ErasureCode::encode_prepare(const bufferlist &raw, unsigned int k = get_data_chunk_count(); unsigned int m = get_chunk_count() - k; unsigned blocksize = get_chunk_size(raw.length()); - unsigned pad_len = blocksize * k - raw.length(); unsigned padded_chunks = k - raw.length() / blocksize; bufferlist prepared = raw; - if (!prepared.is_aligned(SIMD_ALIGN)) { - // splice padded chunks off to make the rebuild faster - if (padded_chunks) - prepared.splice((k - padded_chunks) * blocksize, - padded_chunks * blocksize - pad_len); - prepared.rebuild_aligned(SIMD_ALIGN); - } - for (unsigned int i = 0; i < k - padded_chunks; i++) { bufferlist &chunk = encoded[chunk_index(i)]; chunk.substr_of(prepared, i * blocksize, blocksize); + chunk.rebuild_aligned_size_and_memory(blocksize, SIMD_ALIGN); + assert(chunk.is_contiguous()); } if (padded_chunks) { unsigned remainder = raw.length() - (k - padded_chunks) * blocksize; diff --git a/src/test/erasure-code/TestErasureCode.cc b/src/test/erasure-code/TestErasureCode.cc index c572cdc59eabd..2c972610fc7a8 100644 --- a/src/test/erasure-code/TestErasureCode.cc +++ b/src/test/erasure-code/TestErasureCode.cc @@ -111,6 +111,42 @@ TEST(ErasureCodeTest, encode_memory_align) ASSERT_NE(encoded[1][chunk_size / 2], 'X'); } +TEST(ErasureCodeTest, encode_misaligned_non_contiguous) +{ + int k = 3; + int m = 1; + unsigned chunk_size = ErasureCode::SIMD_ALIGN * 7; + ErasureCodeTest erasure_code(k, m, chunk_size); + + set want_to_encode; + for (unsigned int i = 0; i < erasure_code.get_chunk_count(); i++) + want_to_encode.insert(i); + string data(chunk_size, 'X'); + // create a non contiguous bufferlist where the frist and the second + // bufferptr are not size aligned although they are memory aligned + bufferlist in; + { + bufferptr ptr(buffer::create_aligned(data.length() - 1, ErasureCode::SIMD_ALIGN)); + in.append(ptr); + } + { + bufferptr ptr(buffer::create_aligned(data.length() + 1, ErasureCode::SIMD_ALIGN)); + in.append(ptr); + } + map encoded; + + ASSERT_FALSE(in.is_contiguous()); + ASSERT_TRUE(in.buffers().front().is_aligned(ErasureCode::SIMD_ALIGN)); + ASSERT_FALSE(in.buffers().front().is_n_align_sized(chunk_size)); + ASSERT_TRUE(in.buffers().back().is_aligned(ErasureCode::SIMD_ALIGN)); + ASSERT_FALSE(in.buffers().back().is_n_align_sized(chunk_size)); + ASSERT_EQ(0, erasure_code.encode(want_to_encode, in, &encoded)); + for (unsigned int i = 0; i < erasure_code.get_chunk_count(); i++) { + ASSERT_TRUE(encoded[i].is_aligned(ErasureCode::SIMD_ALIGN)); + ASSERT_TRUE(encoded[i].is_n_align_sized(chunk_size)); + } +} + int main(int argc, char **argv) { vector args; From ad04a677cefc1f0a02fbff0c68409fda6874fdc7 Mon Sep 17 00:00:00 2001 From: Loic Dachary Date: Tue, 2 Dec 2014 00:59:08 +0100 Subject: [PATCH 3/3] common: add bufferlist::rebuild_aligned_size_and_memory The function bufferlist::rebuild_aligned checks memory and size alignment with the same variable. It is however useful to separate memory alignment constraints from size alignment constraints. For instance rebuild_aligned could be called to allocate an erasure coded buffer where each 2048 bytes chunk needs to start on a memory address aligned on 32 bytes. Signed-off-by: Loic Dachary (cherry picked from commit 9ade88e8dacc9b96c042bb668f4452447139a544) --- src/common/buffer.cc | 18 +++++++++++------ src/include/buffer.h | 2 ++ src/test/bufferlist.cc | 46 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/src/common/buffer.cc b/src/common/buffer.cc index 9ee2bfedf3411..653bd9b748e36 100644 --- a/src/common/buffer.cc +++ b/src/common/buffer.cc @@ -1116,11 +1116,17 @@ static simple_spinlock_t buffer_debug_lock = SIMPLE_SPINLOCK_INITIALIZER; } void buffer::list::rebuild_aligned(unsigned align) +{ + rebuild_aligned_size_and_memory(align, align); +} + +void buffer::list::rebuild_aligned_size_and_memory(unsigned align_size, + unsigned align_memory) { std::list::iterator p = _buffers.begin(); while (p != _buffers.end()) { // keep anything that's already align and sized aligned - if (p->is_aligned(align) && p->is_n_align_sized(align)) { + if (p->is_aligned(align_memory) && p->is_n_align_sized(align_size)) { /*cout << " segment " << (void*)p->c_str() << " offset " << ((unsigned long)p->c_str() & (align - 1)) << " length " << p->length() @@ -1144,11 +1150,11 @@ void buffer::list::rebuild_aligned(unsigned align) unaligned.push_back(*p); _buffers.erase(p++); } while (p != _buffers.end() && - (!p->is_aligned(align) || - !p->is_n_align_sized(align) || - (offset & (align-1)))); - if (!(unaligned.is_contiguous() && unaligned._buffers.front().is_aligned(align))) { - ptr nb(buffer::create_aligned(unaligned._len, align)); + (!p->is_aligned(align_memory) || + !p->is_n_align_sized(align_size) || + (offset % align_size))); + if (!(unaligned.is_contiguous() && unaligned._buffers.front().is_aligned(align_memory))) { + ptr nb(buffer::create_aligned(unaligned._len, align_memory)); unaligned.rebuild(nb); } _buffers.insert(p, unaligned._buffers.front()); diff --git a/src/include/buffer.h b/src/include/buffer.h index 0ce133a296416..da19cf95a5194 100644 --- a/src/include/buffer.h +++ b/src/include/buffer.h @@ -393,6 +393,8 @@ class buffer { void rebuild(); void rebuild(ptr& nb); void rebuild_aligned(unsigned align); + void rebuild_aligned_size_and_memory(unsigned align_size, + unsigned align_memory); void rebuild_page_aligned(); // sort-of-like-assignment-op diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc index da4d33ff2b4d9..170bf7b465a02 100644 --- a/src/test/bufferlist.cc +++ b/src/test/bufferlist.cc @@ -1212,6 +1212,52 @@ TEST(BufferList, is_n_page_sized) { } } +TEST(BufferList, rebuild_aligned_size_and_memory) { + const unsigned SIMD_ALIGN = 32; + const unsigned BUFFER_SIZE = 67; + + bufferlist bl; + // These two must be concatenated into one memory + size aligned + // bufferptr + { + bufferptr ptr(buffer::create_aligned(2, SIMD_ALIGN)); + ptr.set_offset(1); + ptr.set_length(1); + bl.append(ptr); + } + { + bufferptr ptr(buffer::create_aligned(BUFFER_SIZE - 1, SIMD_ALIGN)); + bl.append(ptr); + } + // This one must be left alone + { + bufferptr ptr(buffer::create_aligned(BUFFER_SIZE, SIMD_ALIGN)); + bl.append(ptr); + } + // These two must be concatenated into one memory + size aligned + // bufferptr + { + bufferptr ptr(buffer::create_aligned(2, SIMD_ALIGN)); + ptr.set_offset(1); + ptr.set_length(1); + bl.append(ptr); + } + { + bufferptr ptr(buffer::create_aligned(BUFFER_SIZE - 1, SIMD_ALIGN)); + bl.append(ptr); + } + EXPECT_FALSE(bl.is_aligned(SIMD_ALIGN)); + EXPECT_FALSE(bl.is_n_align_sized(BUFFER_SIZE)); + EXPECT_EQ(BUFFER_SIZE * 3, bl.length()); + EXPECT_FALSE(bl.buffers().front().is_aligned(SIMD_ALIGN)); + EXPECT_FALSE(bl.buffers().front().is_n_align_sized(BUFFER_SIZE)); + EXPECT_EQ(5U, bl.buffers().size()); + bl.rebuild_aligned_size_and_memory(BUFFER_SIZE, SIMD_ALIGN); + EXPECT_TRUE(bl.is_aligned(SIMD_ALIGN)); + EXPECT_TRUE(bl.is_n_align_sized(BUFFER_SIZE)); + EXPECT_EQ(3U, bl.buffers().size()); +} + TEST(BufferList, is_zero) { { bufferlist bl;