From 3ef67952f71225bda8d5ab09e97a24f0bf384144 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Wed, 9 Feb 2022 11:01:15 +0800 Subject: [PATCH 1/9] Fix blobstore used removed file --- dbms/src/Common/LRUCache.h | 14 ++++++++++++++ dbms/src/Storages/Page/PageUtil.h | 14 ++++++++------ dbms/src/Storages/Page/V3/BlobStore.cpp | 9 +++++---- dbms/src/Storages/Page/V3/BlobStore.h | 2 +- 4 files changed, 28 insertions(+), 11 deletions(-) diff --git a/dbms/src/Common/LRUCache.h b/dbms/src/Common/LRUCache.h index 59483cfbe25..b89a750ef91 100644 --- a/dbms/src/Common/LRUCache.h +++ b/dbms/src/Common/LRUCache.h @@ -126,6 +126,20 @@ class LRUCache return std::make_pair(token->value, true); } + void remove(const Key & key) + { + std::lock_guard lock(mutex); + cells.erase(key); + for (auto it = queue.begin(); it != queue.end(); it++) + { + if (*it == key) + { + queue.erase(it); + return; + } + } + } + void getStats(size_t & out_hits, size_t & out_misses) const { std::lock_guard lock(mutex); diff --git a/dbms/src/Storages/Page/PageUtil.h b/dbms/src/Storages/Page/PageUtil.h index 051eb81578d..d1440155ed4 100644 --- a/dbms/src/Storages/Page/PageUtil.h +++ b/dbms/src/Storages/Page/PageUtil.h @@ -105,7 +105,8 @@ int openFile(const std::string & path) return 0; } } - DB::throwFromErrno("Cannot open file " + path, errno == ENOENT ? ErrorCodes::FILE_DOESNT_EXIST : ErrorCodes::CANNOT_OPEN_FILE); + DB::throwFromErrno(fmt::format("Cannot open file {}. ", path), + errno == ENOENT ? ErrorCodes::FILE_DOESNT_EXIST : ErrorCodes::CANNOT_OPEN_FILE); } return fd; @@ -118,21 +119,21 @@ inline void touchFile(const std::string & path) if (fd > 0) ::close(fd); else - throw Exception("Touch file failed: " + path); + throw Exception(fmt::format("Touch file failed: ", path)); } template void syncFile(T & file) { if (-1 == file->fsync()) - DB::throwFromErrno("Cannot fsync file: " + file->getFileName(), ErrorCodes::CANNOT_FSYNC); + DB::throwFromErrno(fmt::format("Cannot fsync file: ", file->getFileName()), ErrorCodes::CANNOT_FSYNC); } template void ftruncateFile(T & file, off_t length) { if (-1 == file->ftruncate(length)) - DB::throwFromErrno("Cannot truncate file: " + file->getFileName(), ErrorCodes::CANNOT_FTRUNCATE); + DB::throwFromErrno(fmt::format("Cannot truncate file: ", file->getFileName()), ErrorCodes::CANNOT_FTRUNCATE); } @@ -235,7 +236,7 @@ void readFile(T & file, if (-1 == res && errno != EINTR) { ProfileEvents::increment(ProfileEvents::PSMReadFailed); - DB::throwFromErrno("Cannot read from file " + file->getFileName(), ErrorCodes::CANNOT_READ_FROM_FILE_DESCRIPTOR); + DB::throwFromErrno(fmt::format("Cannot read from file {}.", file->getFileName()), ErrorCodes::CANNOT_READ_FROM_FILE_DESCRIPTOR); } if (res > 0) @@ -245,7 +246,8 @@ void readFile(T & file, ProfileEvents::increment(ProfileEvents::PSMReadBytes, bytes_read); if (unlikely(bytes_read != expected_bytes)) - throw DB::TiFlashException("Not enough data in file " + file->getFileName(), Errors::PageStorage::FileSizeNotMatch); + throw DB::TiFlashException(fmt::format("No enough data in file {}, read bytes : {} , expected bytes : {}", file->getFileName(), bytes_read, expected_bytes), + Errors::PageStorage::FileSizeNotMatch); } /// Write and advance sizeof(T) bytes. diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index c9f6c4e001a..f7f18cbe048 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -37,7 +37,7 @@ BlobStore::BlobStore(const FileProviderPtr & file_provider_, String path_, BlobS , config(config_) , log(&Poco::Logger::get("BlobStore")) , blob_stats(log, config_) - , cached_file(config.cached_fd_size) + , cached_files(config.cached_fd_size) { } @@ -253,6 +253,7 @@ void BlobStore::removePosFromStats(BlobFileId blob_id, BlobFileOffset offset, si auto lock_stats = blob_stats.lock(); blob_stats.eraseStat(std::move(stat), lock_stats); getBlobFile(blob_id)->remove(); + cached_files.remove(blob_id); } } @@ -656,9 +657,9 @@ String BlobStore::getBlobFilePath(BlobFileId blob_id) const BlobFilePtr BlobStore::getBlobFile(BlobFileId blob_id) { - return cached_file.getOrSet(blob_id, [this, blob_id]() -> BlobFilePtr { - return std::make_shared(getBlobFilePath(blob_id), file_provider); - }) + return cached_files.getOrSet(blob_id, [this, blob_id]() -> BlobFilePtr { + return std::make_shared(getBlobFilePath(blob_id), file_provider); + }) .first; } diff --git a/dbms/src/Storages/Page/V3/BlobStore.h b/dbms/src/Storages/Page/V3/BlobStore.h index ec40d6f4678..890e4cbe18a 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.h +++ b/dbms/src/Storages/Page/V3/BlobStore.h @@ -216,7 +216,7 @@ class BlobStore : public Allocator BlobStats blob_stats; - DB::LRUCache cached_file; + DB::LRUCache cached_files; }; using BlobStorePtr = std::shared_ptr; From 1b34f18e3ab0e6f9f232f710f092cb910b878584 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Wed, 9 Feb 2022 11:09:31 +0800 Subject: [PATCH 2/9] fix tidy --- dbms/src/Storages/Page/V3/BlobStore.cpp | 2 +- dbms/src/Storages/Page/V3/BlobStore.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index f7f18cbe048..c178bc341db 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -720,7 +720,7 @@ void BlobStore::BlobStats::eraseStat(const BlobStatPtr && stat, const std::lock_ stats_map.remove(stat); } -void BlobStore::BlobStats::eraseStat(const BlobFileId blob_file_id, const std::lock_guard & lock) +void BlobStore::BlobStats::eraseStat(BlobFileId blob_file_id, const std::lock_guard & lock) { BlobStatPtr stat = nullptr; diff --git a/dbms/src/Storages/Page/V3/BlobStore.h b/dbms/src/Storages/Page/V3/BlobStore.h index 890e4cbe18a..0201f376cc5 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.h +++ b/dbms/src/Storages/Page/V3/BlobStore.h @@ -105,7 +105,7 @@ class BlobStore : public Allocator void eraseStat(const BlobStatPtr && stat, const std::lock_guard &); - void eraseStat(const BlobFileId blob_file_id, const std::lock_guard &); + void eraseStat(BlobFileId blob_file_id, const std::lock_guard &); /** * Choose a available `BlobStat` from `BlobStats`. From 6124f76466a62185e7546839acf89a06f2b17f3a Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Wed, 9 Feb 2022 12:54:12 +0800 Subject: [PATCH 3/9] fix stress test --- dbms/src/Storages/Page/stress/PSRunnable.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dbms/src/Storages/Page/stress/PSRunnable.cpp b/dbms/src/Storages/Page/stress/PSRunnable.cpp index 5a3507e9074..970a11bfcc2 100644 --- a/dbms/src/Storages/Page/stress/PSRunnable.cpp +++ b/dbms/src/Storages/Page/stress/PSRunnable.cpp @@ -293,7 +293,8 @@ DB::PageId PSWindowWriter::genRandomPageId() std::lock_guard page_id_lock(page_id_mutex); if (pageid_boundary < (window_size / 2)) { - return static_cast(pageid_boundary++); + writing_page[index] = pageid_boundary++; + return static_cast(writing_page[index]); } // Generate a random number in the window From d76624500ebea1ac96b6d3085a92217560d4a6f7 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Wed, 9 Feb 2022 17:43:14 +0800 Subject: [PATCH 4/9] fix --- dbms/src/Common/LRUCache.h | 34 +++++++++++++++---------------- dbms/src/Storages/Page/PageUtil.h | 9 ++++---- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/dbms/src/Common/LRUCache.h b/dbms/src/Common/LRUCache.h index b89a750ef91..54ce523826b 100644 --- a/dbms/src/Common/LRUCache.h +++ b/dbms/src/Common/LRUCache.h @@ -49,9 +49,9 @@ class LRUCache MappedPtr get(const Key & key) { - std::lock_guard lock(mutex); + std::lock_guard cache_lock(mutex); - auto res = getImpl(key, lock); + auto res = getImpl(key, cache_lock); if (res) ++hits; else @@ -62,9 +62,9 @@ class LRUCache void set(const Key & key, const MappedPtr & mapped) { - std::lock_guard lock(mutex); + std::lock_guard cache_lock(mutex); - setImpl(key, mapped, lock); + setImpl(key, mapped, cache_lock); } /// If the value for the key is in the cache, returns it. If it is not, calls load_func() to @@ -128,40 +128,38 @@ class LRUCache void remove(const Key & key) { - std::lock_guard lock(mutex); - cells.erase(key); - for (auto it = queue.begin(); it != queue.end(); it++) - { - if (*it == key) - { - queue.erase(it); - return; - } - } + std::lock_guard cache_lock(mutex); + auto it = cells.find(key); + if (it == cells.end()) + return; + + Cell & cell = it->second; + queue.erase(cell.queue_iterator); + cells.erase(it); } void getStats(size_t & out_hits, size_t & out_misses) const { - std::lock_guard lock(mutex); + std::lock_guard cache_lock(mutex); out_hits = hits; out_misses = misses; } size_t weight() const { - std::lock_guard lock(mutex); + std::lock_guard cache_lock(mutex); return current_size; } size_t count() const { - std::lock_guard lock(mutex); + std::lock_guard cache_lock(mutex); return cells.size(); } void reset() { - std::lock_guard lock(mutex); + std::lock_guard cache_lock(mutex); queue.clear(); cells.clear(); insert_tokens.clear(); diff --git a/dbms/src/Storages/Page/PageUtil.h b/dbms/src/Storages/Page/PageUtil.h index d1440155ed4..5372b27917a 100644 --- a/dbms/src/Storages/Page/PageUtil.h +++ b/dbms/src/Storages/Page/PageUtil.h @@ -119,21 +119,21 @@ inline void touchFile(const std::string & path) if (fd > 0) ::close(fd); else - throw Exception(fmt::format("Touch file failed: ", path)); + throw Exception(fmt::format("Touch file failed: {}. ", path)); } template void syncFile(T & file) { if (-1 == file->fsync()) - DB::throwFromErrno(fmt::format("Cannot fsync file: ", file->getFileName()), ErrorCodes::CANNOT_FSYNC); + DB::throwFromErrno(fmt::format("Cannot fsync file: {]. ", file->getFileName()), ErrorCodes::CANNOT_FSYNC); } template void ftruncateFile(T & file, off_t length) { if (-1 == file->ftruncate(length)) - DB::throwFromErrno(fmt::format("Cannot truncate file: ", file->getFileName()), ErrorCodes::CANNOT_FTRUNCATE); + DB::throwFromErrno(fmt::format("Cannot truncate file: {}. ", file->getFileName()), ErrorCodes::CANNOT_FTRUNCATE); } @@ -199,6 +199,7 @@ void writeFile( bytes_written += res; } ProfileEvents::increment(ProfileEvents::PSMWriteIOCalls, write_io_calls); + ProfileEvents::increment(ProfileEvents::PSMWriteBytes, bytes_written); } template @@ -246,7 +247,7 @@ void readFile(T & file, ProfileEvents::increment(ProfileEvents::PSMReadBytes, bytes_read); if (unlikely(bytes_read != expected_bytes)) - throw DB::TiFlashException(fmt::format("No enough data in file {}, read bytes : {} , expected bytes : {}", file->getFileName(), bytes_read, expected_bytes), + throw DB::TiFlashException(fmt::format("No enough data in file {}, read bytes: {} , expected bytes: {}", file->getFileName(), bytes_read, expected_bytes), Errors::PageStorage::FileSizeNotMatch); } From cb877419676507d38c3fdebe3ecc8c159978e759 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Wed, 9 Feb 2022 17:45:09 +0800 Subject: [PATCH 5/9] Fix BlobStore normal blob been truncate. --- dbms/src/Storages/Page/V3/BlobFile.h | 2 +- dbms/src/Storages/Page/V3/BlobStore.cpp | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/dbms/src/Storages/Page/V3/BlobFile.h b/dbms/src/Storages/Page/V3/BlobFile.h index 470464f3c05..c0b632055ee 100644 --- a/dbms/src/Storages/Page/V3/BlobFile.h +++ b/dbms/src/Storages/Page/V3/BlobFile.h @@ -16,7 +16,7 @@ class BlobFile public: BlobFile(String path_, FileProviderPtr file_provider_, - bool truncate_if_exists = true); + bool truncate_if_exists = false); ~BlobFile(); diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index c178bc341db..1463d7e9c9b 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -8,7 +9,6 @@ #include #include - namespace ProfileEvents { extern const Event PSMWritePages; @@ -475,7 +475,14 @@ std::vector BlobStore::getGCStats() auto lock = stat->lock(); auto right_margin = stat->smap->getRightMargin(); + if (right_margin == 0) + { + LOG_FMT_TRACE(log, "Current blob is empty [blob_id={}, total size(all invalid) = ].", stat->id, stat->sm_total_size); + continue; + } + stat->sm_valid_rate = stat->sm_valid_size * 1.0 / right_margin; + if (stat->sm_valid_rate > 1.0) { LOG_FMT_ERROR( @@ -493,7 +500,6 @@ std::vector BlobStore::getGCStats() if (stat->sm_valid_rate <= config.heavy_gc_valid_rate) { LOG_FMT_TRACE(log, "Current [blob_id={}] valid rate is {:.2f}, Need do compact GC", stat->id, stat->sm_valid_rate); - stat->sm_total_size = stat->sm_valid_size; blob_need_gc.emplace_back(stat->id); // Change current stat to read only @@ -507,6 +513,7 @@ std::vector BlobStore::getGCStats() if (right_margin != stat->sm_total_size) { auto blobfile = getBlobFile(stat->id); + LOG_FMT_TRACE(log, "Current [blob_id={}] do truncate, [origin size={}, truncated size={}].", stat->id, stat->sm_total_size, right_margin); blobfile->truncate(right_margin); stat->sm_total_size = right_margin; } @@ -658,7 +665,7 @@ String BlobStore::getBlobFilePath(BlobFileId blob_id) const BlobFilePtr BlobStore::getBlobFile(BlobFileId blob_id) { return cached_files.getOrSet(blob_id, [this, blob_id]() -> BlobFilePtr { - return std::make_shared(getBlobFilePath(blob_id), file_provider); + return std::make_shared(getBlobFilePath(blob_id), file_provider, false); }) .first; } From f7a9dfadec3ad49a7ffa83c941f6800781c9528a Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Wed, 9 Feb 2022 18:16:49 +0800 Subject: [PATCH 6/9] update --- dbms/src/Storages/Page/PageUtil.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/dbms/src/Storages/Page/PageUtil.h b/dbms/src/Storages/Page/PageUtil.h index 5372b27917a..00792b3a37c 100644 --- a/dbms/src/Storages/Page/PageUtil.h +++ b/dbms/src/Storages/Page/PageUtil.h @@ -146,8 +146,6 @@ void writeFile( const WriteLimiterPtr & write_limiter = nullptr, [[maybe_unused]] bool enable_failpoint = false) { - ProfileEvents::increment(ProfileEvents::PSMWriteBytes, to_write); - if (write_limiter) write_limiter->request(to_write); From 4ac6ad5b6ff1f431f3871c9f5457617d31417208 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Thu, 10 Feb 2022 10:24:37 +0800 Subject: [PATCH 7/9] update --- dbms/src/Storages/Page/V3/BlobFile.cpp | 8 ++++---- dbms/src/Storages/Page/V3/BlobFile.h | 3 +-- dbms/src/Storages/Page/V3/BlobStore.cpp | 7 ++++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/dbms/src/Storages/Page/V3/BlobFile.cpp b/dbms/src/Storages/Page/V3/BlobFile.cpp index f8983c0e0f4..9e603a26826 100644 --- a/dbms/src/Storages/Page/V3/BlobFile.cpp +++ b/dbms/src/Storages/Page/V3/BlobFile.cpp @@ -12,16 +12,16 @@ extern const char exception_before_page_file_write_sync[]; namespace PS::V3 { BlobFile::BlobFile(String path_, - FileProviderPtr file_provider_, - bool truncate_if_exists) + FileProviderPtr file_provider_) : file_provider{file_provider_} , path(path_) { + // TODO: support encryption file wrfile = file_provider->newWriteReadableFile( getPath(), getEncryptionPath(), - truncate_if_exists, - /*create_new_encryption_info_*/ truncate_if_exists); + false, + /*create_new_encryption_info_*/ false); } void BlobFile::read(char * buffer, size_t offset, size_t size, const ReadLimiterPtr & read_limiter) diff --git a/dbms/src/Storages/Page/V3/BlobFile.h b/dbms/src/Storages/Page/V3/BlobFile.h index c0b632055ee..7c9720f61ad 100644 --- a/dbms/src/Storages/Page/V3/BlobFile.h +++ b/dbms/src/Storages/Page/V3/BlobFile.h @@ -15,8 +15,7 @@ class BlobFile { public: BlobFile(String path_, - FileProviderPtr file_provider_, - bool truncate_if_exists = false); + FileProviderPtr file_provider_); ~BlobFile(); diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index 1463d7e9c9b..d12d87dd661 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -475,9 +475,10 @@ std::vector BlobStore::getGCStats() auto lock = stat->lock(); auto right_margin = stat->smap->getRightMargin(); + // Avoid divide by zero if (right_margin == 0) { - LOG_FMT_TRACE(log, "Current blob is empty [blob_id={}, total size(all invalid) = ].", stat->id, stat->sm_total_size); + LOG_FMT_TRACE(log, "Current blob is empty [blob_id={}, total size(all invalid)={}].", stat->id, stat->sm_total_size); continue; } @@ -513,7 +514,7 @@ std::vector BlobStore::getGCStats() if (right_margin != stat->sm_total_size) { auto blobfile = getBlobFile(stat->id); - LOG_FMT_TRACE(log, "Current [blob_id={}] do truncate, [origin size={}, truncated size={}].", stat->id, stat->sm_total_size, right_margin); + LOG_FMT_TRACE(log, "Truncate blob file [blob_id={}] [origin size={}] [truncated size={}]", stat->id, stat->sm_total_size, right_margin); blobfile->truncate(right_margin); stat->sm_total_size = right_margin; } @@ -665,7 +666,7 @@ String BlobStore::getBlobFilePath(BlobFileId blob_id) const BlobFilePtr BlobStore::getBlobFile(BlobFileId blob_id) { return cached_files.getOrSet(blob_id, [this, blob_id]() -> BlobFilePtr { - return std::make_shared(getBlobFilePath(blob_id), file_provider, false); + return std::make_shared(getBlobFilePath(blob_id), file_provider); }) .first; } From 28b4e58345cf0535a74efef1e528d886a132627f Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Thu, 10 Feb 2022 10:40:27 +0800 Subject: [PATCH 8/9] remove unused include --- dbms/src/Storages/Page/V3/BlobStore.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index d12d87dd661..2458146c084 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -1,7 +1,6 @@ #include #include #include -#include #include #include #include From 6a9fa3232c181a2bf6904a1081cff944c4f709e1 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Thu, 10 Feb 2022 12:40:52 +0800 Subject: [PATCH 9/9] update --- dbms/src/Storages/Page/V3/BlobStore.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index 2458146c084..8ba8082e756 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -494,6 +494,7 @@ std::vector BlobStore::getGCStats() right_margin, stat->id); assert(false); + continue; } // Check if GC is required