Skip to content

Commit 54554fa

Browse files
LchangliangLchangliang
authored
(cloud-merge) Fix the coredump because of change_cache_type to origin_type (#38518)
When call method `change_cache_type_by_mgr`, if the cache_type is same as change_type, it will return true directly and execute the next codes and break the old assumptions.
1 parent 47f2d6c commit 54554fa

File tree

3 files changed

+306
-30
lines changed

3 files changed

+306
-30
lines changed

‎be/src/io/cache/block_file_cache.cpp‎

Lines changed: 42 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,12 @@ FileBlocks BlockFileCache::get_impl(const UInt128Wrapper& hash, const CacheConte
280280
DCHECK(!file_blocks.empty());
281281
// change to ttl if the blocks aren't ttl
282282
if (context.cache_type == FileCacheType::TTL && _key_to_time.find(hash) == _key_to_time.end()) {
283+
for (auto& [_, cell] : file_blocks) {
284+
Status st = cell.file_block->update_expiration_time(context.expiration_time);
285+
if (!st.ok()) {
286+
LOG_WARNING("Failed to change key meta").error(st);
287+
}
288+
}
283289
for (auto& [_, cell] : file_blocks) {
284290
FileCacheType origin_type = cell.file_block->cache_type();
285291
if (origin_type == FileCacheType::TTL) continue;
@@ -295,9 +301,7 @@ FileBlocks BlockFileCache::get_impl(const UInt128Wrapper& hash, const CacheConte
295301
} else {
296302
cell.queue_iterator.reset();
297303
}
298-
st = cell.file_block->update_expiration_time(context.expiration_time);
299-
}
300-
if (!st.ok()) {
304+
} else {
301305
LOG_WARNING("Failed to change key meta").error(st);
302306
}
303307
}
@@ -324,7 +328,10 @@ FileBlocks BlockFileCache::get_impl(const UInt128Wrapper& hash, const CacheConte
324328
}
325329
if (context.expiration_time == 0) {
326330
for (auto& [_, cell] : file_blocks) {
327-
if (cell.file_block->change_cache_type_by_mgr(FileCacheType::NORMAL)) {
331+
auto cache_type = cell.file_block->cache_type();
332+
if (cache_type != FileCacheType::TTL) continue;
333+
auto st = cell.file_block->change_cache_type_by_mgr(FileCacheType::NORMAL);
334+
if (st.ok()) {
328335
if (config::enable_ttl_cache_evict_using_lru) {
329336
auto& ttl_queue = get_queue(FileCacheType::TTL);
330337
ttl_queue.remove(cell.queue_iterator.value(), cache_lock);
@@ -333,6 +340,8 @@ FileBlocks BlockFileCache::get_impl(const UInt128Wrapper& hash, const CacheConte
333340
cell.queue_iterator =
334341
queue.add(cell.file_block->get_hash_value(), cell.file_block->offset(),
335342
cell.file_block->range().size(), cache_lock);
343+
} else {
344+
LOG_WARNING("Failed to change key meta").error(st);
336345
}
337346
}
338347
_key_to_time.erase(iter);
@@ -681,22 +690,30 @@ BlockFileCache::FileBlockCell* BlockFileCache::add_cell(const UInt128Wrapper& ha
681690
<< ".\nCurrent cache structure: " << dump_structure_unlocked(hash, cache_lock);
682691

683692
auto& offsets = _files[hash];
684-
DCHECK((context.expiration_time == 0 && context.cache_type != FileCacheType::TTL) ||
685-
(context.cache_type == FileCacheType::TTL && context.expiration_time != 0))
686-
<< fmt::format("expiration time {}, cache type {}", context.expiration_time,
687-
context.cache_type);
688693

689694
FileCacheKey key;
690695
key.hash = hash;
691696
key.offset = offset;
692697
key.meta.type = context.cache_type;
693698
key.meta.expiration_time = context.expiration_time;
694699
FileBlockCell cell(std::make_shared<FileBlock>(key, size, this, state), cache_lock);
695-
if (context.cache_type != FileCacheType::TTL || config::enable_ttl_cache_evict_using_lru) {
696-
auto& queue = get_queue(context.cache_type);
700+
Status st;
701+
if (context.expiration_time == 0 && context.cache_type == FileCacheType::TTL) {
702+
st = cell.file_block->change_cache_type_by_mgr(FileCacheType::NORMAL);
703+
} else if (context.cache_type != FileCacheType::TTL && context.expiration_time != 0) {
704+
st = cell.file_block->change_cache_type_by_mgr(FileCacheType::TTL);
705+
}
706+
if (!st.ok()) {
707+
LOG(WARNING) << "Cannot change cache type. expiration_time=" << context.expiration_time
708+
<< " cache_type=" << cache_type_to_string(context.cache_type)
709+
<< " error=" << st.msg();
710+
}
711+
if (cell.file_block->cache_type() != FileCacheType::TTL ||
712+
config::enable_ttl_cache_evict_using_lru) {
713+
auto& queue = get_queue(cell.file_block->cache_type());
697714
cell.queue_iterator = queue.add(hash, offset, size, cache_lock);
698715
}
699-
if (context.cache_type == FileCacheType::TTL) {
716+
if (cell.file_block->cache_type() == FileCacheType::TTL) {
700717
if (_key_to_time.find(hash) == _key_to_time.end()) {
701718
_key_to_time[hash] = context.expiration_time;
702719
_time_to_key.insert(std::make_pair(context.expiration_time, hash));
@@ -1005,19 +1022,18 @@ bool BlockFileCache::remove_if_ttl_file_unlock(const UInt128Wrapper& file_key, b
10051022
}
10061023
}
10071024
for (auto& [_, cell] : _files[file_key]) {
1008-
if (cell.file_block->cache_type() == FileCacheType::TTL) {
1009-
auto st = cell.file_block->change_cache_type_by_mgr(FileCacheType::NORMAL);
1010-
if (st.ok()) {
1011-
if (config::enable_ttl_cache_evict_using_lru) {
1012-
ttl_queue.remove(cell.queue_iterator.value(), cache_lock);
1013-
}
1014-
auto& queue = get_queue(FileCacheType::NORMAL);
1015-
cell.queue_iterator = queue.add(
1016-
cell.file_block->get_hash_value(), cell.file_block->offset(),
1017-
cell.file_block->range().size(), cache_lock);
1018-
} else {
1019-
LOG_WARNING("Failed to change cache type to normal").error(st);
1025+
if (cell.file_block->cache_type() == FileCacheType::NORMAL) continue;
1026+
auto st = cell.file_block->change_cache_type_by_mgr(FileCacheType::NORMAL);
1027+
if (st.ok()) {
1028+
if (config::enable_ttl_cache_evict_using_lru) {
1029+
ttl_queue.remove(cell.queue_iterator.value(), cache_lock);
10201030
}
1031+
auto& queue = get_queue(FileCacheType::NORMAL);
1032+
cell.queue_iterator =
1033+
queue.add(cell.file_block->get_hash_value(), cell.file_block->offset(),
1034+
cell.file_block->range().size(), cache_lock);
1035+
} else {
1036+
LOG_WARNING("Failed to change cache type to normal").error(st);
10211037
}
10221038
}
10231039
} else {
@@ -1579,6 +1595,7 @@ void BlockFileCache::modify_expiration_time(const UInt128Wrapper& hash,
15791595
for (auto& [_, cell] : _files[hash]) {
15801596
Status st = cell.file_block->update_expiration_time(new_expiration_time);
15811597
if (!st.ok()) {
1598+
LOG_WARNING("Failed to modify expiration time").error(st);
15821599
}
15831600
}
15841601

@@ -1588,12 +1605,13 @@ void BlockFileCache::modify_expiration_time(const UInt128Wrapper& hash,
15881605
if (auto iter = _files.find(hash); iter != _files.end()) {
15891606
for (auto& [_, cell] : iter->second) {
15901607
Status st = cell.file_block->update_expiration_time(new_expiration_time);
1591-
if (!st.ok() && !st.is<ErrorCode::NOT_FOUND>()) {
1608+
if (!st.ok()) {
15921609
LOG_WARNING("").error(st);
15931610
}
15941611
}
15951612
for (auto& [_, cell] : iter->second) {
15961613
FileCacheType origin_type = cell.file_block->cache_type();
1614+
if (origin_type == FileCacheType::TTL) continue;
15971615
auto st = cell.file_block->change_cache_type_by_mgr(FileCacheType::TTL);
15981616
if (st.ok()) {
15991617
auto& queue = get_queue(origin_type);

‎be/src/io/cache/file_block.cpp‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include <thread>
2626

2727
#include "common/status.h"
28+
#include "cpp/sync_point.h"
2829
#include "io/cache/block_file_cache.h"
2930

3031
namespace doris {
@@ -162,14 +163,14 @@ Status FileBlock::read(Slice buffer, size_t read_offset) {
162163

163164
Status FileBlock::change_cache_type_by_mgr(FileCacheType new_type) {
164165
std::lock_guard block_lock(_mutex);
165-
if (new_type == _key.meta.type) {
166-
return Status::OK();
167-
}
166+
DCHECK(new_type != _key.meta.type);
168167
if (_download_state == State::DOWNLOADED) {
169168
KeyMeta new_meta;
170169
new_meta.expiration_time = _key.meta.expiration_time;
171170
new_meta.type = new_type;
172-
RETURN_IF_ERROR(_mgr->_storage->change_key_meta(_key, new_meta));
171+
auto st = _mgr->_storage->change_key_meta(_key, new_meta);
172+
TEST_SYNC_POINT_CALLBACK("FileBlock::change_cache_type", &st);
173+
if (!st.ok()) return st;
173174
}
174175
_key.meta.type = new_type;
175176
return Status::OK();
@@ -198,7 +199,10 @@ Status FileBlock::update_expiration_time(uint64_t expiration_time) {
198199
KeyMeta new_meta;
199200
new_meta.expiration_time = expiration_time;
200201
new_meta.type = _key.meta.type;
201-
RETURN_IF_ERROR(_mgr->_storage->change_key_meta(_key, new_meta));
202+
auto st = _mgr->_storage->change_key_meta(_key, new_meta);
203+
if (!st.ok() && !st.is<ErrorCode::NOT_FOUND>()) {
204+
return st;
205+
}
202206
}
203207
_key.meta.expiration_time = expiration_time;
204208
return Status::OK();

0 commit comments

Comments
 (0)