Skip to content

Commit

Permalink
Merge pull request ceph#37606 from ifed01/wip-ifed-fix-mempool-and-ot…
Browse files Browse the repository at this point in the history 
…hers

os/bluestore: a bunch of minor fixes

Reviewed-by: Neha Ojha <nojha@redhat.com>
Reviewed-by: Kefu Chai <kchai@redhat.com>
  • Loading branch information
@tchaikov
tchaikov committed Nov 18, 2020
2 parents ca77787 + 7801dc2 commit 50f580c
Show file tree
Hide file tree
Showing 3 changed files with 203 additions and 16 deletions.
29 changes: 17 additions & 12 deletions src/os/bluestore/BlueStore.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1602,12 +1602,13 @@ int BlueStore::BufferSpace::_discard(BufferCacheShard* cache, uint32_t offset, u
if (b->data.length()) {
bufferlist bl;
bl.substr_of(b->data, b->length - tail, tail);
Buffer *nb = new Buffer(this, b->state, b->seq, end, bl);
Buffer *nb = new Buffer(this, b->state, b->seq, end, bl, b->flags);
nb->maybe_rebuild();
_add_buffer(cache, nb, 0, b);
} else {
_add_buffer(cache, new Buffer(this, b->state, b->seq, end, tail),
0, b);
_add_buffer(cache, new Buffer(this, b->state, b->seq, end, tail,
b->flags),
0, b);
}
if (!b->is_writing()) {
cache->_adjust_size(b, front - (int64_t)b->length);
Expand Down Expand Up @@ -1637,11 +1638,13 @@ int BlueStore::BufferSpace::_discard(BufferCacheShard* cache, uint32_t offset, u
if (b->data.length()) {
bufferlist bl;
bl.substr_of(b->data, b->length - keep, keep);
Buffer *nb = new Buffer(this, b->state, b->seq, end, bl);
Buffer *nb = new Buffer(this, b->state, b->seq, end, bl, b->flags);
nb->maybe_rebuild();
_add_buffer(cache, nb, 0, b);
} else {
_add_buffer(cache, new Buffer(this, b->state, b->seq, end, keep), 0, b);
_add_buffer(cache, new Buffer(this, b->state, b->seq, end, keep,
b->flags),
0, b);
}
_rm_buffer(cache, i);
cache->_audit("discard end 2");
Expand Down Expand Up @@ -1686,7 +1689,7 @@ void BlueStore::BufferSpace::read(
length -= l;
if (!b->is_writing()) {
cache->_touch(b);
}
}
continue;
}
if (b->offset > offset) {
Expand Down Expand Up @@ -1773,10 +1776,12 @@ void BlueStore::BufferSpace::split(BufferCacheShard* cache, size_t pos, BlueStor
if (p->second->data.length()) {
bufferlist bl;
bl.substr_of(p->second->data, left, right);
r._add_buffer(cache, new Buffer(&r, p->second->state, p->second->seq, 0, bl),
r._add_buffer(cache, new Buffer(&r, p->second->state, p->second->seq,
0, bl, p->second->flags),
0, p->second.get());
} else {
r._add_buffer(cache, new Buffer(&r, p->second->state, p->second->seq, 0, right),
r._add_buffer(cache, new Buffer(&r, p->second->state, p->second->seq,
0, right, p->second->flags),
0, p->second.get());
}
cache->_adjust_size(p->second.get(), -right);
Expand All @@ -1788,11 +1793,11 @@ void BlueStore::BufferSpace::split(BufferCacheShard* cache, size_t pos, BlueStor
ldout(cache->cct, 30) << __func__ << " move " << *p->second << dendl;
if (p->second->data.length()) {
r._add_buffer(cache, new Buffer(&r, p->second->state, p->second->seq,
p->second->offset - pos, p->second->data),
p->second->offset - pos, p->second->data, p->second->flags),
0, p->second.get());
} else {
r._add_buffer(cache, new Buffer(&r, p->second->state, p->second->seq,
p->second->offset - pos, p->second->length),
p->second->offset - pos, p->second->length, p->second->flags),
0, p->second.get());
}
if (p == buffer_map.begin()) {
Expand Down Expand Up @@ -13821,7 +13826,7 @@ void BlueStore::_wctx_finish(
auto& r = lo.r;
txc->statfs_delta.stored() -= lo.e.length;
if (!r.empty()) {
dout(20) << __func__ << " blob release " << r << dendl;
dout(20) << __func__ << " blob " << *b << " release " << r << dendl;
if (blob.is_shared()) {
PExtentVector final;
c->load_shared_blob(b->shared_blob);
Expand Down Expand Up @@ -14354,7 +14359,7 @@ int BlueStore::_do_remove(
<< maybe_unshared_blobs << dendl;
ghobject_t nogen = o->oid;
nogen.generation = ghobject_t::NO_GEN;
OnodeRef h = c->onode_map.lookup(nogen);
OnodeRef h = c->get_onode(nogen, false);

if (!h || !h->exists) {
return 0;
Expand Down
13 changes: 9 additions & 4 deletions src/os/bluestore/BlueStore.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,11 +297,13 @@ class BlueStore : public ObjectStore,
ceph_assert(writing.empty());
}

void _add_buffer(BufferCacheShard* cache, Buffer *b, int level, Buffer *near) {
void _add_buffer(BufferCacheShard* cache, Buffer* b, int level, Buffer* near) {
cache->_audit("_add_buffer start");
buffer_map[b->offset].reset(b);
if (b->is_writing()) {
b->data.reassign_to_mempool(mempool::mempool_bluestore_writing);
// we might get already cached data for which resetting mempool is inppropriate
// hence calling try_assign_to_mempool
b->data.try_assign_to_mempool(mempool::mempool_bluestore_writing);
if (writing.empty() || writing.rbegin()->seq <= b->seq) {
writing.push_back(*b);
} else {
Expand All @@ -316,8 +318,8 @@ class BlueStore : public ObjectStore,
writing.insert(it, *b);
}
} else {
b->data.reassign_to_mempool(mempool::mempool_bluestore_cache_data);
cache->_add(b, level, near);
b->data.reassign_to_mempool(mempool::mempool_bluestore_cache_data);
cache->_add(b, level, near);
}
cache->_audit("_add_buffer end");
}
Expand Down Expand Up @@ -2947,6 +2949,9 @@ class BlueStore : public ObjectStore,
const PerfCounters* get_bluefs_perf_counters() const {
return bluefs->get_perf_counters();
}
KeyValueDB* get_kv() {
return db;
}

int queue_transactions(
CollectionHandle& ch,
Expand Down
177 changes: 177 additions & 0 deletions src/test/objectstore/store_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3521,13 +3521,190 @@ TEST_P(StoreTest, SimpleCloneRangeTest) {
ObjectStore::Transaction t;
t.remove(cid, hoid);
t.remove(cid, hoid2);
r = queue_transaction(store, ch, std::move(t));
ASSERT_EQ(r, 0);
}
}

#if defined(WITH_BLUESTORE)
TEST_P(StoreTest, BlueStoreUnshareBlobTest) {
if (string(GetParam()) != "bluestore")
return;
int r;
coll_t cid;
auto ch = store->create_new_collection(cid);
{
ObjectStore::Transaction t;
t.create_collection(cid, 0);
cerr << "Creating collection " << cid << std::endl;
r = queue_transaction(store, ch, std::move(t));
ASSERT_EQ(r, 0);
}
ghobject_t hoid(hobject_t(sobject_t("Object 1", CEPH_NOSNAP)));
hoid.hobj.pool = -1;
ghobject_t hoid2(hobject_t(sobject_t("Object 1", CEPH_NOSNAP)));
hoid2.hobj.pool = -1;
hoid2.generation = 2;
{
// check if blob is unshared properly
bufferlist data, newdata;
data.append(string(8192, 'a'));

ObjectStore::Transaction t;
t.write(cid, hoid, 0, data.length(), data);
cerr << "Creating object and write 8K " << hoid << std::endl;
r = queue_transaction(store, ch, std::move(t));
ASSERT_EQ(r, 0);

ObjectStore::Transaction t2;
t2.clone_range(cid, hoid, hoid2, 0, 4096, 0);
cerr << "Clone range object" << std::endl;
r = queue_transaction(store, ch, std::move(t2));
ASSERT_EQ(r, 0);

data.clear();
data.append(string(4096, 'b'));

ObjectStore::Transaction t3;
t3.write(cid, hoid, 0, data.length(), data);
cerr << "Writing 4k to source object " << hoid << std::endl;
r = queue_transaction(store, ch, std::move(t3));
ASSERT_EQ(r, 0);

{
// this trims hoid one out of onode cache
EXPECT_EQ(store->umount(), 0);
EXPECT_EQ(store->mount(), 0);
ch = store->open_collection(cid);
}

ObjectStore::Transaction t4;
t4.remove(cid, hoid2);
cerr << "Deleting dest object" << hoid2 << std::endl;
r = queue_transaction(store, ch, std::move(t4));
ASSERT_EQ(r, 0);

bufferlist resdata;
r = store->read(ch, hoid, 0, 0x2000, resdata);
ASSERT_EQ(r, 0x2000);

{
BlueStore* bstore = dynamic_cast<BlueStore*> (store.get());
auto* kv = bstore->get_kv();

// to be inline with BlueStore.cc
const string PREFIX_SHARED_BLOB = "X";

size_t cnt = 0;
auto it = kv->get_iterator(PREFIX_SHARED_BLOB);
ceph_assert(it);
for (it->lower_bound(string()); it->valid(); it->next()) {
++cnt;
}
ASSERT_EQ(cnt, 0);
}
}
{
ObjectStore::Transaction t;
t.remove(cid, hoid);
t.remove_collection(cid);
cerr << "Cleaning" << std::endl;
r = queue_transaction(store, ch, std::move(t));
ASSERT_EQ(r, 0);
}
}

TEST_P(StoreTest, BlueStoreUnshareBlobBugTest) {
if (string(GetParam()) != "bluestore")
return;
int r;
coll_t cid;
auto ch = store->create_new_collection(cid);
{
ObjectStore::Transaction t;
t.create_collection(cid, 0);
cerr << "Creating collection " << cid << std::endl;
r = queue_transaction(store, ch, std::move(t));
ASSERT_EQ(r, 0);
}
ghobject_t hoid(hobject_t(sobject_t("Object 1", CEPH_NOSNAP)));
hoid.hobj.pool = -1;
ghobject_t hoid2(hobject_t(sobject_t("Object 1", CEPH_NOSNAP)));
hoid2.hobj.pool = -1;
hoid2.generation = 2;
{
// check if blob is unshared properly
bufferlist data, newdata;
data.append(string(8192, 'a'));

ObjectStore::Transaction t;
t.write(cid, hoid, 0, data.length(), data);
cerr << "Creating object and write 8K " << hoid << std::endl;
r = queue_transaction(store, ch, std::move(t));
ASSERT_EQ(r, 0);

ObjectStore::Transaction t2;
t2.clone_range(cid, hoid, hoid2, 0, 4096, 0);
cerr << "Clone range object" << std::endl;
r = queue_transaction(store, ch, std::move(t2));
ASSERT_EQ(r, 0);

data.clear();
data.append(string(4096, 'b'));

ObjectStore::Transaction t3;
t3.write(cid, hoid, 0, data.length(), data);
cerr << "Writing 4k to source object " << hoid << std::endl;
r = queue_transaction(store, ch, std::move(t3));
ASSERT_EQ(r, 0);

{
// this trims hoid one out of onode cache
EXPECT_EQ(store->umount(), 0);
EXPECT_EQ(store->mount(), 0);
ch = store->open_collection(cid);
}

ObjectStore::Transaction t4;
t4.write(cid, hoid2, 0, data.length(), data);
cerr << "Writing 4k to second object " << hoid2 << std::endl;
r = queue_transaction(store, ch, std::move(t4));
ASSERT_EQ(r, 0);

bufferlist resdata;
r = store->read(ch, hoid, 0, 0x2000, resdata);
ASSERT_EQ(r, 0x2000);

{
BlueStore* bstore = dynamic_cast<BlueStore*> (store.get());
auto* kv = bstore->get_kv();

// to be inline with BlueStore.cc
const string PREFIX_SHARED_BLOB = "X";

size_t cnt = 0;
auto it = kv->get_iterator(PREFIX_SHARED_BLOB);
ceph_assert(it);
for (it->lower_bound(string()); it->valid(); it->next()) {
++cnt;
}
// This shows a bug in unsharing a blob,
// after writing to 0x0~1000 to hoid2 share blob at hoid should be
//unshared but it doesn't in the current implementation
ASSERT_EQ(cnt, 1);
}
}
{
ObjectStore::Transaction t;
t.remove(cid, hoid);
t.remove(cid, hoid2);
t.remove_collection(cid);
cerr << "Cleaning" << std::endl;
r = queue_transaction(store, ch, std::move(t));
ASSERT_EQ(r, 0);
}
}
#endif

TEST_P(StoreTest, SimpleObjectLongnameTest) {
int r;
Expand Down

0 comments on commit 50f580c

Please sign in to comment.