From 2dcb23afcb10bb52f637a1fb3f5962d8f27c3b99 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Fri, 21 Oct 2016 12:07:05 -0400 Subject: [PATCH] os: fix offsets in move_ranges This simplifies the interface, and avoids problems in bluestore with alignment. Signed-off-by: Sage Weil --- src/os/ObjectStore.h | 42 +++++++++--------- src/os/bluestore/BlueStore.cc | 29 ++++++------ src/os/bluestore/BlueStore.h | 11 ++--- src/os/filestore/FileStore.cc | 28 ++++++------ src/os/filestore/FileStore.h | 8 ++-- src/os/kstore/KStore.cc | 32 ++++++-------- src/os/kstore/KStore.h | 13 +++--- src/os/memstore/MemStore.cc | 35 +++++++-------- src/os/memstore/MemStore.h | 4 +- .../objectstore/DeterministicOpSequence.cc | 19 ++++---- .../objectstore/DeterministicOpSequence.h | 7 +-- src/test/objectstore/store_test.cc | 44 +++++++++---------- src/test/objectstore/test_transaction.cc | 6 ++- 13 files changed, 142 insertions(+), 136 deletions(-) diff --git a/src/os/ObjectStore.h b/src/os/ObjectStore.h index 10b513755df..bedf5f7dd76 100644 --- a/src/os/ObjectStore.h +++ b/src/os/ObjectStore.h @@ -977,7 +977,7 @@ public: void decode_attrset(map& aset) { ::decode(aset, data_bl_p); } - void decode_move_info(vector>& move_info) { + void decode_move_info(vector>& move_info) { ::decode(move_info, data_bl_p); } void decode_attrset_bl(bufferlist *pbl) { @@ -1347,26 +1347,26 @@ public: data.ops++; } - /* - * Move source object to base object. - * Data portion is only copied from source object to base object. - * The copy is done according to the move_info vector of tuple, which - * has information of src_offset, dest_offset and length. - * Finally, the source object is deleted. - */ - void move_ranges_destroy_src( - const coll_t& cid, - const ghobject_t& src_oid, - ghobject_t oid, - const vector>& move_info) { - Op* _op = _get_next_op(); - _op->op = OP_MERGE_DELETE; - _op->cid = _get_coll_id(cid); - _op->oid = _get_object_id(src_oid); - _op->dest_oid = _get_object_id(oid); - ::encode(move_info, data_bl); - data.ops++; - } + /* + * Move source object to base object. + * Data portion is only copied from source object to base object. + * The copy is done according to the move_info vector of tuple, which + * has information of offset and length. + * Finally, the source object is deleted. + */ + void move_ranges_destroy_src( + const coll_t& cid, + const ghobject_t& src_oid, + ghobject_t oid, + const vector>& move_info) { + Op* _op = _get_next_op(); + _op->op = OP_MERGE_DELETE; + _op->cid = _get_coll_id(cid); + _op->oid = _get_object_id(src_oid); + _op->dest_oid = _get_object_id(oid); + ::encode(move_info, data_bl); + data.ops++; + } /// Create the collection void create_collection(const coll_t& cid, int bits) { diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index e1fd1f8b321..e2bb50fb230 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -7067,7 +7067,7 @@ void BlueStore::_txc_add_transaction(TransContext *txc, Transaction *t) if (!no) { no = c->get_onode(noid, true); } - vector> move_info; + vector> move_info; i.decode_move_info(move_info); r = _move_ranges_destroy_src(txc, c, o, no, move_info); } @@ -8618,11 +8618,12 @@ int BlueStore::_clone_range(TransContext *txc, /* Move contents of src object according to move_info to base object. * Once the move_info is traversed completely, delete the src object. */ -int BlueStore::_move_ranges_destroy_src(TransContext *txc, +int BlueStore::_move_ranges_destroy_src( + TransContext *txc, CollectionRef& c, OnodeRef& srco, OnodeRef& baseo, - const vector> move_info) + const vector> move_info) { dout(15) << __func__ << " " << c->cid << " " << srco->oid << " -> " << baseo->oid @@ -8633,18 +8634,16 @@ int BlueStore::_move_ranges_destroy_src(TransContext *txc, // Traverse move_info completely, move contents from src object // to base object. for (unsigned i = 0; i < move_info.size(); ++i) { - uint64_t srcoff = move_info[i].get<0>(); - uint64_t dstoff = move_info[i].get<1>(); - uint64_t len = move_info[i].get<2>(); - - dout(15) << __func__ << " " << c->cid << " " << srco->oid << " -> " - << baseo->oid << " from 0x" << std::hex << srcoff << "~" << len - << " to offset 0x" << dstoff << std::dec - << dendl; - - r = _clone_range(txc, c, srco, baseo, srcoff, len, dstoff); - if (r < 0) - goto out; + uint64_t off = move_info[i].first; + uint64_t len = move_info[i].second; + + dout(15) << __func__ << " " << c->cid << " " << srco->oid << " -> " + << baseo->oid << " 0x" << std::hex << off << "~" << len + << dendl; + + r = _clone_range(txc, c, srco, baseo, off, len, off); + if (r < 0) + goto out; } // delete the src object diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h index 39ddef7a712..3f50ac1e3e5 100644 --- a/src/os/bluestore/BlueStore.h +++ b/src/os/bluestore/BlueStore.h @@ -2002,11 +2002,12 @@ private: OnodeRef& oldo, OnodeRef& newo, uint64_t srcoff, uint64_t length, uint64_t dstoff); - int _move_ranges_destroy_src(TransContext *txc, - CollectionRef& c, - OnodeRef& oldo, - OnodeRef& newo, - const vector> move_info); + int _move_ranges_destroy_src( + TransContext *txc, + CollectionRef& c, + OnodeRef& oldo, + OnodeRef& newo, + const vector> move_info); int _rename(TransContext *txc, CollectionRef& c, OnodeRef& oldo, diff --git a/src/os/filestore/FileStore.cc b/src/os/filestore/FileStore.cc index 96a45da0676..c69ec014b98 100644 --- a/src/os/filestore/FileStore.cc +++ b/src/os/filestore/FileStore.cc @@ -2707,7 +2707,7 @@ void FileStore::_do_transaction( coll_t src_cid = i.get_cid(op->cid); _kludge_temp_object_collection(cid, oid); _kludge_temp_object_collection(src_cid, src_oid); - vector> move_info; + vector> move_info; i.decode_move_info(move_info); tracepoint(objectstore, move_ranges_destroy_src_enter, osr_name); r = _move_ranges_destroy_src(src_cid, src_oid, cid, oid, move_info, spos); @@ -3768,13 +3768,16 @@ int FileStore::_clone_range(const coll_t& oldcid, const ghobject_t& oldoid, cons /* * Move contents of src object according to move_info to base object. Once the move_info is traversed completely, delete the src object. */ -int FileStore::_move_ranges_destroy_src(const coll_t& src_cid, const ghobject_t& src_oid, const coll_t& cid, const ghobject_t& oid, - const vector> move_info, - const SequencerPosition& spos) +int FileStore::_move_ranges_destroy_src( + const coll_t& src_cid, const ghobject_t& src_oid, + const coll_t& cid, const ghobject_t& oid, + const vector> move_info, + const SequencerPosition& spos) { int r = 0; - dout(10) << __func__ << src_cid << "/" << src_oid << " -> " << cid << "/" << oid << dendl; + dout(10) << __func__ << src_cid << "/" << src_oid << " -> " + << cid << "/" << oid << dendl; // check replay guard for base object. If not possible to replay, return. int dstcmp = _check_replay_guard(cid, oid, spos); @@ -3795,7 +3798,8 @@ int FileStore::_move_ranges_destroy_src(const coll_t& src_cid, const ghobject_t& FDRef t; r = lfn_open(src_cid, src_oid, false, &t); - //If we are replaying, it is possible that we do not find src obj as it is deleted before crashing. + //If we are replaying, it is possible that we do not find src obj as + //it is deleted before crashing. if (r < 0) { lfn_close(b); dout(10) << __func__ << " replaying -->" << replaying << dendl; @@ -3808,13 +3812,11 @@ int FileStore::_move_ranges_destroy_src(const coll_t& src_cid, const ghobject_t& } for (unsigned i = 0; i < move_info.size(); ++i) { - uint64_t srcoff = move_info[i].get<0>(); - uint64_t dstoff = move_info[i].get<1>(); - uint64_t len = move_info[i].get<2>(); - - r = _do_clone_range(**t, **b, srcoff, len, dstoff); - if (r < 0) - break; + uint64_t off = move_info[i].first; + uint64_t len = move_info[i].second; + r = _do_clone_range(**t, **b, off, len, off); + if (r < 0) + break; } dout(10) << __func__ << cid << "/" << oid << " " << " = " << r << dendl; diff --git a/src/os/filestore/FileStore.h b/src/os/filestore/FileStore.h index d0c75463b19..e306b7fc3f1 100644 --- a/src/os/filestore/FileStore.h +++ b/src/os/filestore/FileStore.h @@ -585,9 +585,11 @@ public: int _clone_range(const coll_t& oldcid, const ghobject_t& oldoid, const coll_t& newcid, const ghobject_t& newoid, uint64_t srcoff, uint64_t len, uint64_t dstoff, const SequencerPosition& spos); - int _move_ranges_destroy_src(const coll_t& temp_cid, const ghobject_t& temp_oid, const coll_t& cid, const ghobject_t& oid, - const vector > move_info, - const SequencerPosition& spos); + int _move_ranges_destroy_src( + const coll_t& temp_cid, const ghobject_t& temp_oid, + const coll_t& cid, const ghobject_t& oid, + const vector > move_info, + const SequencerPosition& spos); int _do_clone_range(int from, int to, uint64_t srcoff, uint64_t len, uint64_t dstoff); int _do_sparse_copy_range(int from, int to, uint64_t srcoff, uint64_t len, uint64_t dstoff); int _do_copy_range(int from, int to, uint64_t srcoff, uint64_t len, uint64_t dstoff, bool skip_sloppycrc=false); diff --git a/src/os/kstore/KStore.cc b/src/os/kstore/KStore.cc index 9b5a77f733e..f6189f22779 100755 --- a/src/os/kstore/KStore.cc +++ b/src/os/kstore/KStore.cc @@ -2393,7 +2393,7 @@ void KStore::_txc_add_transaction(TransContext *txc, Transaction *t) { const ghobject_t& boid = i.get_oid(op->dest_oid); OnodeRef bo = c->get_onode(boid, true); - vector> move_info; + vector> move_info; i.decode_move_info(move_info); r = _move_ranges_destroy_src(txc, c, o, cvec[op->dest_cid], bo, move_info); } @@ -3211,38 +3211,34 @@ int KStore::_clone_range(TransContext *txc, /* Move contents of src object according to move_info to base object. * Once the move_info is traversed completely, delete the src object. */ -int KStore::_move_ranges_destroy_src(TransContext *txc, - CollectionRef& c, - OnodeRef& srco, - CollectionRef& basec, - OnodeRef& baseo, - vector> move_info) +int KStore::_move_ranges_destroy_src( + TransContext *txc, + CollectionRef& c, + OnodeRef& srco, + CollectionRef& basec, + OnodeRef& baseo, + vector> move_info) { int r = 0; bufferlist bl; baseo->exists = true; _assign_nid(txc, baseo); -// Traverse move_info completely, move contents from src to base object. + // Traverse move_info completely, move contents from src to base object. for (unsigned i = 0; i < move_info.size(); ++i) { - uint64_t srcoff; - uint64_t dstoff; - uint64_t len; - - srcoff = move_info[i].get<0>(); - dstoff = move_info[i].get<1>(); - len = move_info[i].get<2>(); + uint64_t off = move_info[i].first; + uint64_t len = move_info[i].second; - r = _do_read(srco, srcoff, len, bl, 0); + r = _do_read(srco, off, len, bl, 0); if (r < 0) goto out; - r = _do_write(txc, baseo, dstoff, bl.length(), bl, 0); + r = _do_write(txc, baseo, off, bl.length(), bl, 0); txc->write_onode(baseo); r = 0; } -// After for loop ends, remove src obj + // After for loop ends, remove src obj r = _do_remove(txc, srco); out: diff --git a/src/os/kstore/KStore.h b/src/os/kstore/KStore.h index 5f5aea6e5f1..961b6babe8e 100644 --- a/src/os/kstore/KStore.h +++ b/src/os/kstore/KStore.h @@ -634,12 +634,13 @@ private: OnodeRef& oldo, OnodeRef& newo, uint64_t srcoff, uint64_t length, uint64_t dstoff); - int _move_ranges_destroy_src(TransContext *txc, - CollectionRef& c, - OnodeRef& oldo, - CollectionRef& bc, - OnodeRef& newo, - vector> move_info); + int _move_ranges_destroy_src( + TransContext *txc, + CollectionRef& c, + OnodeRef& oldo, + CollectionRef& bc, + OnodeRef& newo, + vector> move_info); int _rename(TransContext *txc, CollectionRef& c, OnodeRef& oldo, diff --git a/src/os/memstore/MemStore.cc b/src/os/memstore/MemStore.cc index 2feaa43b8d1..62ed9096a56 100644 --- a/src/os/memstore/MemStore.cc +++ b/src/os/memstore/MemStore.cc @@ -845,7 +845,7 @@ void MemStore::_do_transaction(Transaction& t) coll_t cid = i.get_cid(op->cid); ghobject_t oid = i.get_oid(op->oid); ghobject_t noid = i.get_oid(op->dest_oid); - vector> move_info; + vector> move_info; i.decode_move_info(move_info); r = _move_ranges_destroy_src(cid, oid, noid, move_info); } @@ -1251,11 +1251,13 @@ int MemStore::_clone_range(const coll_t& cid, const ghobject_t& oldoid, /* Move contents of src object according to move_info to base object. * Once the move_info is traversed completely, delete the src object. */ -int MemStore::_move_ranges_destroy_src(const coll_t& cid, const ghobject_t& srcoid, - const ghobject_t& baseoid, - const vector > move_info) +int MemStore::_move_ranges_destroy_src( + const coll_t& cid, const ghobject_t& srcoid, + const ghobject_t& baseoid, + const vector > move_info) { - dout(10) << __func__ << " " << cid << " " << srcoid << " -> " << baseoid << dendl; + dout(10) << __func__ << " " << cid << " " << srcoid << " -> " + << baseoid << dendl; CollectionRef c = get_collection(cid); if (!c) return -ENOENT; @@ -1266,21 +1268,18 @@ int MemStore::_move_ranges_destroy_src(const coll_t& cid, const ghobject_t& srco ObjectRef no = c->get_or_create_object(baseoid); for (unsigned i = 0; i < move_info.size(); ++i) { - uint64_t srcoff = move_info[i].get<0>(); - uint64_t dstoff = move_info[i].get<1>(); - uint64_t len = move_info[i].get<2>(); - - if (srcoff >= oo->get_size()) - return 0; - if (srcoff + len >= oo->get_size()) - len = oo->get_size() - srcoff; - - const ssize_t old_size = no->get_size(); - no->clone(oo.get(), srcoff, len, dstoff); - used_bytes += (no->get_size() - old_size); + uint64_t off = move_info[i].first; + uint64_t len = move_info[i].second; + if (off >= oo->get_size()) + return 0; + if (off + len >= oo->get_size()) + len = oo->get_size() - off; + const ssize_t old_size = no->get_size(); + no->clone(oo.get(), off, len, off); + used_bytes += (no->get_size() - old_size); } -// delete the src object + // delete the src object _remove(cid, srcoid); return 0; } diff --git a/src/os/memstore/MemStore.h b/src/os/memstore/MemStore.h index 35015fe2b70..1e59cae1807 100644 --- a/src/os/memstore/MemStore.h +++ b/src/os/memstore/MemStore.h @@ -217,8 +217,8 @@ private: const ghobject_t& newoid, uint64_t srcoff, uint64_t len, uint64_t dstoff); int _move_ranges_destroy_src(const coll_t& cid, const ghobject_t& oldoid, - const ghobject_t& newoid, - const vector > move_info); + const ghobject_t& newoid, + const vector > move_info); int _omap_clear(const coll_t& cid, const ghobject_t &oid); int _omap_setkeys(const coll_t& cid, const ghobject_t &oid, bufferlist& aset_bl); int _omap_rmkeys(const coll_t& cid, const ghobject_t &oid, bufferlist& keys_bl); diff --git a/src/test/objectstore/DeterministicOpSequence.cc b/src/test/objectstore/DeterministicOpSequence.cc index 11fcb515539..c205cfa57dc 100644 --- a/src/test/objectstore/DeterministicOpSequence.cc +++ b/src/test/objectstore/DeterministicOpSequence.cc @@ -376,7 +376,9 @@ bool DeterministicOpSequence::do_move_ranges_delete_srcobj(rngen_t& gen) boost::uniform_int<> move_len(1, bl.length()); size = (size_t) move_len(gen); - vector> move_info = { boost::make_tuple(0, 0, size)}; + vector> move_info = { + make_pair(0, size) + }; dout(0) << "do_move_ranges_delete_srcobj " << coll.to_str() << "/" << orig_obj.oid.name << " (0~" << size << ")" @@ -561,18 +563,19 @@ void DeterministicOpSequence::_do_write_and_clone_range(coll_t coll, m_store->apply_transaction(&m_osr, std::move(t)); } -void DeterministicOpSequence::_do_write_and_merge_delete(coll_t coll, - hobject_t& orig_obj, - hobject_t& new_obj, - vector> move_info, - bufferlist& bl) +void DeterministicOpSequence::_do_write_and_merge_delete( + coll_t coll, + hobject_t& orig_obj, + hobject_t& new_obj, + vector> move_info, + bufferlist& bl) { ObjectStore::Transaction t; note_txn(&t); for (unsigned i = 0; i < move_info.size(); ++i) { - uint64_t srcoff = move_info[i].get<0>(); - t.write(coll, ghobject_t(orig_obj), srcoff, bl.length(), bl); + uint64_t off = move_info[i].first; + t.write(coll, ghobject_t(orig_obj), off, bl.length(), bl); } t.move_ranges_destroy_src(coll, ghobject_t(orig_obj), ghobject_t(new_obj), move_info); m_store->apply_transaction(&m_osr, std::move(t)); diff --git a/src/test/objectstore/DeterministicOpSequence.h b/src/test/objectstore/DeterministicOpSequence.h index f4bca82e8ad..d13caa0d39d 100644 --- a/src/test/objectstore/DeterministicOpSequence.h +++ b/src/test/objectstore/DeterministicOpSequence.h @@ -82,9 +82,10 @@ class DeterministicOpSequence : public TestObjectStoreState { virtual void _do_write_and_clone_range(coll_t coll, hobject_t& orig_obj, hobject_t& new_obj, uint64_t srcoff, uint64_t srclen, uint64_t dstoff, bufferlist& bl); - virtual void _do_write_and_merge_delete(coll_t coll, hobject_t& orig_obj, - hobject_t& new_obj, vector> move_info, - bufferlist& bl); + virtual void _do_write_and_merge_delete( + coll_t coll, hobject_t& orig_obj, + hobject_t& new_obj, vector> move_info, + bufferlist& bl); virtual void _do_coll_move(coll_t orig_coll, coll_t new_coll, hobject_t& obj); virtual void _do_coll_create(coll_t cid, uint32_t pg_num, uint64_t num_objs); diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc index 2c17eb6ee4b..8c66f914a55 100644 --- a/src/test/objectstore/store_test.cc +++ b/src/test/objectstore/store_test.cc @@ -2813,7 +2813,10 @@ TEST_P(StoreTest, SimpleMoveRangeDelSrcTest) { } { - vector> move_info = { boost::make_tuple(0, 0, 5), boost::make_tuple(10, 10, 5) }; + vector> move_info = { + make_pair(0, 5), + make_pair(10, 5) + }; ObjectStore::Transaction t; t.move_ranges_destroy_src(cid, hoid2, hoid, move_info); @@ -3730,40 +3733,37 @@ public: boost::uniform_int<> u1(0, max_object_len - max_write_len); boost::uniform_int<> u2(0, max_write_len); - uint64_t srcoff = u1(*rng); - uint64_t dstoff = u1(*rng); + uint64_t off = u1(*rng); uint64_t len = u2(*rng); if (write_alignment) { - srcoff = ROUND_UP_TO(srcoff, write_alignment); - dstoff = ROUND_UP_TO(dstoff, write_alignment); + off = ROUND_UP_TO(off, write_alignment); len = ROUND_UP_TO(len, write_alignment); } - if (srcoff > srcdata.length() - 1) { - srcoff = srcdata.length() - 1; + if (off > srcdata.length() - 1) { + off = srcdata.length() - 1; } - if (srcoff + len > srcdata.length()) { - len = srcdata.length() - srcoff; + if (off + len > srcdata.length()) { + len = srcdata.length() - off; } if (0) - cout << __func__ << " from " << srcoff << "~" << len - << " (size " << srcdata.length() << ") to " - << dstoff << "~" << len << std::endl; + cout << __func__ << " " << off << "~" << len + << " (size " << srcdata.length() << ")" << std::endl; ObjectStore::Transaction t; - vector> extents; + vector> extents; extents.emplace_back( - boost::tuple(srcoff, dstoff, len)); + std::pair(off, len)); t.move_ranges_destroy_src(cid, old_obj, new_obj, extents); ++in_flight; in_flight_objects.insert(old_obj); bufferlist bl; - if (srcoff < srcdata.length()) { - if (srcoff + len > srcdata.length()) { - bl.substr_of(srcdata, srcoff, srcdata.length() - srcoff); + if (off < srcdata.length()) { + if (off + len > srcdata.length()) { + bl.substr_of(srcdata, off, srcdata.length() - off); } else { - bl.substr_of(srcdata, srcoff, len); + bl.substr_of(srcdata, off, len); } } @@ -3775,15 +3775,15 @@ public: } bufferlist& dstdata = contents[new_obj].data; - if (dstdata.length() <= dstoff) { + if (dstdata.length() <= off) { if (bl.length() > 0) { - dstdata.append_zero(dstoff - dstdata.length()); + dstdata.append_zero(off - dstdata.length()); dstdata.append(bl); } } else { bufferlist value; - assert(dstdata.length() > dstoff); - dstdata.copy(0, dstoff, value); + assert(dstdata.length() > off); + dstdata.copy(0, off, value); value.append(bl); if (value.length() < dstdata.length()) dstdata.copy(value.length(), diff --git a/src/test/objectstore/test_transaction.cc b/src/test/objectstore/test_transaction.cc index 0f8d8530dfe..0f66c75c69b 100644 --- a/src/test/objectstore/test_transaction.cc +++ b/src/test/objectstore/test_transaction.cc @@ -123,7 +123,10 @@ TEST(Transaction, MoveRangesDelSrcObj) ghobject_t o1(hobject_t("obj", "", 123, 456, -1, "")); ghobject_t o2(hobject_t("obj2", "", 123, 456, -1, "")); - vector> move_info = { boost::make_tuple(1, 1, 5), boost::make_tuple(10, 10, 5) }; + vector> move_info = { + make_pair(1, 5), + make_pair(10, 5) + }; t.touch(c, o1); bufferlist bl; @@ -136,7 +139,6 @@ TEST(Transaction, MoveRangesDelSrcObj) t.write(c, o2, 1, bl.length(), bl); t.move_ranges_destroy_src(c, o1, o2, move_info); - } TEST(Transaction, GetNumBytes) -- 2.39.5