From: Igor Fedotov Date: Thu, 3 Sep 2020 18:42:55 +0000 (+0300) Subject: os/bluestore: minor improvement X-Git-Tag: v16.1.0~1128^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=24bb93991c8d3b391e2508d921d54994cfe3d285;p=ceph.git os/bluestore: minor improvement Signed-off-by: Igor Fedotov --- diff --git a/src/os/bluestore/BlueFS.cc b/src/os/bluestore/BlueFS.cc index 5752eb407cc9..6c95d845d7d0 100644 --- a/src/os/bluestore/BlueFS.cc +++ b/src/os/bluestore/BlueFS.cc @@ -170,16 +170,14 @@ private: } }; -BlueFS::BlueFS(CephContext* cct, - bluefs_shared_alloc_context_t* _shared_alloc) +BlueFS::BlueFS(CephContext* cct) : cct(cct), bdev(MAX_BDEV), ioc(MAX_BDEV), block_reserved(MAX_BDEV), alloc(MAX_BDEV), alloc_size(MAX_BDEV, 0), - pending_release(MAX_BDEV), - shared_alloc(_shared_alloc) + pending_release(MAX_BDEV) { discard_cb[BDEV_WAL] = wal_discard_cb; discard_cb[BDEV_DB] = db_discard_cb; @@ -317,7 +315,7 @@ void BlueFS::_update_logger_stats() int BlueFS::add_block_device(unsigned id, const string& path, bool trim, uint64_t reserved, - bool shared_with_bluestore) + bluefs_shared_alloc_context_t* _shared_alloc) { dout(10) << __func__ << " bdev " << id << " path " << path << " " << reserved << dendl; @@ -326,7 +324,7 @@ int BlueFS::add_block_device(unsigned id, const string& path, bool trim, BlockDevice *b = BlockDevice::create(cct, path, NULL, NULL, discard_cb[id], static_cast(this)); block_reserved[id] = reserved; - if (shared_with_bluestore) { + if (_shared_alloc) { b->set_no_exclusive_lock(); } int r = b->open(path); @@ -342,8 +340,9 @@ int BlueFS::add_block_device(unsigned id, const string& path, bool trim, << " size " << byte_u_t(b->get_size()) << dendl; bdev[id] = b; ioc[id] = new IOContext(cct, NULL); - if (shared_with_bluestore) { - ceph_assert(shared_alloc); // to be set in ctor before + if (_shared_alloc) { + ceph_assert(!shared_alloc); + shared_alloc = _shared_alloc; alloc[id] = shared_alloc->a; shared_alloc_id = id; } @@ -453,11 +452,10 @@ int BlueFS::get_block_extents(unsigned id, interval_set *extents) { std::lock_guard l(lock); dout(10) << __func__ << " bdev " << id << dendl; - if (id >= alloc.size()) - return -EINVAL; + ceph_assert(id < alloc.size()); for (auto& p : file_map) { for (auto& q : p.second->fnode.extents) { - if (q.bdev == id && alloc[q.bdev] == shared_alloc->a) { + if (q.bdev == id) { extents->insert(q.offset, q.length); } } @@ -600,7 +598,6 @@ int BlueFS::mount() { dout(1) << __func__ << dendl; - bool shared_alloc_ready = shared_alloc && shared_alloc->a; int r = _open_super(); if (r < 0) { derr << __func__ << " failed to open super: " << cpp_strerror(r) << dendl; @@ -630,25 +627,24 @@ int BlueFS::mount() for (auto& p : file_map) { dout(30) << __func__ << " noting alloc for " << p.second->fnode << dendl; for (auto& q : p.second->fnode.extents) { - if (is_shared_alloc(q.bdev)) { - // we might have still uninitialized shared_alloc at this point - // just bypass initialization then - if (shared_alloc_ready && shared_alloc->need_init) { - ceph_assert(shared_alloc->a); - alloc[q.bdev]->init_rm_free(q.offset, q.length); - shared_alloc->bluefs_used += q.length; - } - } else { + bool is_shared = is_shared_alloc(q.bdev); + ceph_assert(!is_shared || (is_shared && shared_alloc)); + if (is_shared && shared_alloc->need_init && shared_alloc->a) { + shared_alloc->bluefs_used += q.length; + alloc[q.bdev]->init_rm_free(q.offset, q.length); + } else if (!is_shared) { alloc[q.bdev]->init_rm_free(q.offset, q.length); } } } - if (shared_alloc_ready) { + if (shared_alloc) { shared_alloc->need_init = false; + dout(1) << __func__ << " shared_bdev_used = " + << shared_alloc->bluefs_used << dendl; + } else { + dout(1) << __func__ << " shared bdev not used" + << dendl; } - dout(1) << __func__ << " shared_bdev_used = " - << (shared_alloc_ready ? (int64_t)shared_alloc->bluefs_used : -1) - << dendl; // set up the log for future writes log_writer = _create_writer(_get_file(1)); diff --git a/src/os/bluestore/BlueFS.h b/src/os/bluestore/BlueFS.h index b14b94b911d7..d7ac37488deb 100644 --- a/src/os/bluestore/BlueFS.h +++ b/src/os/bluestore/BlueFS.h @@ -324,6 +324,7 @@ private: BlockDevice::aio_callback_t discard_cb[3]; //discard callbacks for each dev std::unique_ptr vselector; + bluefs_shared_alloc_context_t* shared_alloc = nullptr; unsigned shared_alloc_id = unsigned(-1); inline bool is_shared_alloc(unsigned id) const { @@ -438,7 +439,7 @@ private: } public: - BlueFS(CephContext* cct, bluefs_shared_alloc_context_t* _shared_alloc); + BlueFS(CephContext* cct); ~BlueFS(); // the super is always stored on bdev 0 @@ -533,7 +534,7 @@ public: int add_block_device(unsigned bdev, const std::string& path, bool trim, uint64_t reserved, - bool shared_with_bluestore = false); + bluefs_shared_alloc_context_t* _shared_alloc = nullptr); bool bdev_support_label(unsigned id); uint64_t get_block_device_size(unsigned bdev) const; diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index 61854c183e36..4a3d2c54ee6c 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -5055,7 +5055,7 @@ int BlueStore::_write_out_fm_meta(uint64_t target_size) return r; } -int BlueStore::_open_alloc() +int BlueStore::_create_alloc() { ceph_assert(shared_alloc.a == NULL); ceph_assert(bdev->get_size()); @@ -5069,15 +5069,25 @@ int BlueStore::_open_alloc() } shared_alloc.set(Allocator::create(cct, cct->_conf->bluestore_allocator, - bdev->get_size(), - alloc_size, "block")); + bdev->get_size(), + alloc_size, "block")); if (!shared_alloc.a) { - lderr(cct) << __func__ << " Allocator::unknown alloc type " - << cct->_conf->bluestore_allocator - << dendl; + lderr(cct) << __func__ << "Failed to create allocator:: " + << cct->_conf->bluestore_allocator + << dendl; return -EINVAL; } + return 0; +} + +int BlueStore::_init_alloc() +{ + int r = _create_alloc(); + if (r < 0) { + return r; + } + ceph_assert(shared_alloc.a != NULL); if (bdev->is_smr()) { shared_alloc.a->set_zone_states(fm->get_zone_states(db)); @@ -5274,7 +5284,7 @@ bool BlueStore::test_mount_in_use() int BlueStore::_minimal_open_bluefs(bool create) { int r; - bluefs = new BlueFS(cct, &shared_alloc); + bluefs = new BlueFS(cct); string bfn; struct stat st; @@ -5322,7 +5332,7 @@ int BlueStore::_minimal_open_bluefs(bool create) // never trim here r = bluefs->add_block_device(bluefs_layout.shared_bdev, bfn, false, 0, // no need to provide valid 'reserved' for shared dev - true); + &shared_alloc); if (r < 0) { derr << __func__ << " add block device(" << bfn << ") returned: " << cpp_strerror(r) << dendl; @@ -5497,7 +5507,6 @@ int BlueStore::_open_db_and_around(bool read_only, bool to_repair) // open in read-only first to read FM list and init allocator // as they might be needed for some BlueFS procedures - r = _open_db(false, false, true); if (r < 0) goto out_bdev; @@ -5511,7 +5520,7 @@ int BlueStore::_open_db_and_around(bool read_only, bool to_repair) if (r < 0) goto out_db; - r = _open_alloc(); + r = _init_alloc(); if (r < 0) goto out_fm; @@ -5525,11 +5534,13 @@ int BlueStore::_open_db_and_around(bool read_only, bool to_repair) r = _open_db(false, to_repair, read_only); if (r < 0) { - goto out_fm; + goto out_alloc; } return 0; - out_fm: +out_alloc: + _close_alloc(); +out_fm: _close_fm(); out_db: _close_db(read_only); @@ -5545,8 +5556,8 @@ int BlueStore::_open_db_and_around(bool read_only, bool to_repair) void BlueStore::_close_db_and_around(bool read_only) { _close_db(read_only); - _close_alloc(); _close_fm(); + _close_alloc(); _close_bdev(); _close_fsid(); _close_path(); @@ -6119,28 +6130,18 @@ int BlueStore::mkfs() goto out_close_bdev; } - uint64_t alloc_size; - alloc_size = min_alloc_size; - if (bdev->is_smr()) { - int r = _zoned_check_config_settings(); - if (r < 0) - return r; - alloc_size = _zoned_piggyback_device_parameters_onto(alloc_size); - } - shared_alloc.set(Allocator::create(cct, cct->_conf->bluestore_allocator, - bdev->get_size(), - alloc_size, "block")); - if (!shared_alloc.a) { - r = -EINVAL; + r = _create_alloc(); + if (r < 0) { goto out_close_bdev; } + reserved = _get_ondisk_reserved(); shared_alloc.a->init_add_free(reserved, p2align(bdev->get_size(), min_alloc_size) - reserved); r = _open_db(true); if (r < 0) - goto out_close_bdev; + goto out_close_alloc; { KeyValueDB::Transaction t = db->get_transaction(); @@ -6189,9 +6190,9 @@ int BlueStore::mkfs() _close_fm(); out_close_db: _close_db(false); + out_close_alloc: + _close_alloc(); out_close_bdev: - delete shared_alloc.a; - shared_alloc.reset(); _close_bdev(); out_close_fsid: _close_fsid(); diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h index 27ab942316b3..9bdf6f976b2b 100644 --- a/src/os/bluestore/BlueStore.h +++ b/src/os/bluestore/BlueStore.h @@ -2380,7 +2380,8 @@ private: int _open_fm(KeyValueDB::Transaction t, bool read_only); void _close_fm(); int _write_out_fm_meta(uint64_t target_size); - int _open_alloc(); + int _create_alloc(); + int _init_alloc(); void _close_alloc(); int _open_collections(); void _fsck_collections(int64_t* errors); diff --git a/src/os/bluestore/bluestore_tool.cc b/src/os/bluestore/bluestore_tool.cc index 899d17a6bfe2..9497232dc1af 100644 --- a/src/os/bluestore/bluestore_tool.cc +++ b/src/os/bluestore/bluestore_tool.cc @@ -161,6 +161,9 @@ void add_devices( cout << " -> " << target_path; } cout << std::endl; + + // We provide no shared allocator which prevents bluefs to operate in R/W mode. + // Read-only mode isn't strictly enforced though int r = fs->add_block_device(e.second, e.first, false, 0); // 'reserved' is fake if (r < 0) { cerr << "unable to open " << e.first << ": " << cpp_strerror(r) << std::endl; @@ -175,9 +178,7 @@ BlueFS *open_bluefs_readonly( const vector& devs) { validate_path(cct, path, true); - // We provide no shared allocator which prevents bluefs to operate in R/W mode. - // Read-only mode isn't strictly enforced though - BlueFS *fs = new BlueFS(cct, nullptr); + BlueFS *fs = new BlueFS(cct); add_devices(fs, cct, devs); @@ -196,9 +197,7 @@ void log_dump( const vector& devs) { validate_path(cct, path, true); - // We provide no shared allocator which prevents bluefs to operate in R/W mode. - // Read-only mode isn't strictly enforced though - BlueFS *fs = new BlueFS(cct, nullptr); + BlueFS *fs = new BlueFS(cct); add_devices(fs, cct, devs); int r = fs->log_dump(); diff --git a/src/test/objectstore/test_bluefs.cc b/src/test/objectstore/test_bluefs.cc index add3f86c62c1..4bbd2e6dd04f 100644 --- a/src/test/objectstore/test_bluefs.cc +++ b/src/test/objectstore/test_bluefs.cc @@ -89,7 +89,7 @@ TEST(BlueFS, mkfs) { uint64_t size = 1048576 * 128; TempBdev bdev{size}; uuid_d fsid; - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); } @@ -97,7 +97,7 @@ TEST(BlueFS, mkfs) { TEST(BlueFS, mkfs_mount) { uint64_t size = 1048576 * 128; TempBdev bdev{size}; - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -111,7 +111,7 @@ TEST(BlueFS, mkfs_mount) { TEST(BlueFS, write_read) { uint64_t size = 1048576 * 128; TempBdev bdev{size}; - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -141,7 +141,7 @@ TEST(BlueFS, write_read) { TEST(BlueFS, small_appends) { uint64_t size = 1048576 * 128; TempBdev bdev{size}; - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -173,7 +173,7 @@ TEST(BlueFS, very_large_write) { // we'll write a ~5G file, so allocate more than that for the whole fs uint64_t size = 1048576 * 1024 * 8ull; TempBdev bdev{size}; - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); bool old = g_ceph_context->_conf.get_val("bluefs_buffered_io"); g_ceph_context->_conf.set_val("bluefs_buffered_io", "false"); @@ -363,7 +363,7 @@ TEST(BlueFS, test_flush_1) { "65536"); g_ceph_context->_conf.apply_changes(nullptr); - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -397,7 +397,7 @@ TEST(BlueFS, test_flush_2) { "65536"); g_ceph_context->_conf.apply_changes(nullptr); - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -424,7 +424,7 @@ TEST(BlueFS, test_flush_3) { "65536"); g_ceph_context->_conf.apply_changes(nullptr); - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -457,7 +457,7 @@ TEST(BlueFS, test_simple_compaction_sync) { uint64_t size = 1048576 * 128; TempBdev bdev{size}; - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -509,7 +509,7 @@ TEST(BlueFS, test_simple_compaction_async) { uint64_t size = 1048576 * 128; TempBdev bdev{size}; - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -564,7 +564,7 @@ TEST(BlueFS, test_compaction_sync) { "bluefs_compact_log_sync", "true"); - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -601,7 +601,7 @@ TEST(BlueFS, test_compaction_async) { "bluefs_compact_log_sync", "false"); - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -638,7 +638,7 @@ TEST(BlueFS, test_replay) { "bluefs_compact_log_sync", "false"); - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false })); @@ -683,7 +683,7 @@ TEST(BlueFS, test_replay_growth) { conf.SetVal("bluefs_sync_write", "true"); conf.ApplyChanges(); - BlueFS fs(g_ceph_context, nullptr); + BlueFS fs(g_ceph_context); ASSERT_EQ(0, fs.add_block_device(BlueFS::BDEV_DB, bdev.path, false, 1048576)); uuid_d fsid; ASSERT_EQ(0, fs.mkfs(fsid, { BlueFS::BDEV_DB, false, false }));