Merge bitcoin/bitcoin#30517: refactor: Add FlatFileSeq member variables in BlockManager

7aa8994 refactor: Add FlatFileSeq member variables in BlockManager (TheCharlatan)

Pull request description:

  Instead of constructing a new class every time a file operation is done, construct them once for each of the undo and block file when a new BlockManager is created.

  In future, this might make it easier to introduce an abstract block store.

  Historically, this was not easily possible prior to #27125.

ACKs for top commit:
  danielabrozzoni:
    ACK 7aa8994
  tdb3:
    ACK 7aa8994
  stickies-v:
    ACK 7aa8994
  brunoerg:
    utACK 7aa8994

Tree-SHA512: 7c181968c270956c90fa0f3687562239912a973b6a35ddbf49fc58733247ea9d986303cbf6f8fc16e8c2d9bf4505e866aed37f030a8c9be72e95bf3752902aa6

Author: fanquake

src/flatfile.cpp

@@ -30,7 +30,7 @@ fs::path FlatFileSeq::FileName(const FlatFilePos& pos) const
     return m_dir / fs::u8path(strprintf("%s%05u.dat", m_prefix, pos.nFile));
 }
 
-FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)
+FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only) const
 {
     if (pos.IsNull()) {
         return nullptr;
@@ -52,7 +52,7 @@ FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)
     return file;
 }
 
-size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_of_space)
+size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_of_space) const
 {
     out_of_space = false;
 
@@ -78,7 +78,7 @@ size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_
     return 0;
 }
 
-bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize)
+bool FlatFileSeq::Flush(const FlatFilePos& pos, bool finalize) const
 {
     FILE* file = Open(FlatFilePos(pos.nFile, 0)); // Avoid fseek to nPos
     if (!file) {

src/flatfile.h

@@ -63,7 +63,7 @@ class FlatFileSeq
     fs::path FileName(const FlatFilePos& pos) const;
 
     /** Open a handle to the file at the given position. */
-    FILE* Open(const FlatFilePos& pos, bool read_only = false);
+    FILE* Open(const FlatFilePos& pos, bool read_only = false) const;
 
     /**
      * Allocate additional space in a file after the given starting position. The amount allocated
@@ -74,7 +74,7 @@ class FlatFileSeq
      * @param[out] out_of_space Whether the allocation failed due to insufficient disk space.
      * @return The number of bytes successfully allocated.
      */
-    size_t Allocate(const FlatFilePos& pos, size_t add_size, bool& out_of_space);
+    size_t Allocate(const FlatFilePos& pos, size_t add_size, bool& out_of_space) const;
 
     /**
      * Commit a file to disk, and optionally truncate off extra pre-allocated bytes if final.
@@ -83,7 +83,7 @@ class FlatFileSeq
      * @param[in] finalize True if no more data will be written to this file.
      * @return true on success, false on failure.
      */
-    bool Flush(const FlatFilePos& pos, bool finalize = false);
+    bool Flush(const FlatFilePos& pos, bool finalize = false) const;
 };
 
 #endif // BITCOIN_FLATFILE_H

src/node/blockstorage.cpp

@@ -734,7 +734,7 @@ bool BlockManager::UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& in
 bool BlockManager::FlushUndoFile(int block_file, bool finalize)
 {
     FlatFilePos undo_pos_old(block_file, m_blockfile_info[block_file].nUndoSize);
-    if (!UndoFileSeq().Flush(undo_pos_old, finalize)) {
+    if (!m_undo_file_seq.Flush(undo_pos_old, finalize)) {
         m_opts.notifications.flushError(_("Flushing undo file to disk failed. This is likely the result of an I/O error."));
         return false;
     }
@@ -756,7 +756,7 @@ bool BlockManager::FlushBlockFile(int blockfile_num, bool fFinalize, bool finali
     assert(static_cast<int>(m_blockfile_info.size()) > blockfile_num);
 
     FlatFilePos block_pos_old(blockfile_num, m_blockfile_info[blockfile_num].nSize);
-    if (!BlockFileSeq().Flush(block_pos_old, fFinalize)) {
+    if (!m_block_file_seq.Flush(block_pos_old, fFinalize)) {
         m_opts.notifications.flushError(_("Flushing block file to disk failed. This is likely the result of an I/O error."));
         success = false;
     }
@@ -808,38 +808,28 @@ void BlockManager::UnlinkPrunedFiles(const std::set<int>& setFilesToPrune) const
     std::error_code ec;
     for (std::set<int>::iterator it = setFilesToPrune.begin(); it != setFilesToPrune.end(); ++it) {
         FlatFilePos pos(*it, 0);
-        const bool removed_blockfile{fs::remove(BlockFileSeq().FileName(pos), ec)};
-        const bool removed_undofile{fs::remove(UndoFileSeq().FileName(pos), ec)};
+        const bool removed_blockfile{fs::remove(m_block_file_seq.FileName(pos), ec)};
+        const bool removed_undofile{fs::remove(m_undo_file_seq.FileName(pos), ec)};
         if (removed_blockfile || removed_undofile) {
             LogPrint(BCLog::BLOCKSTORAGE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
         }
     }
 }
 
-FlatFileSeq BlockManager::BlockFileSeq() const
-{
-    return FlatFileSeq(m_opts.blocks_dir, "blk", m_opts.fast_prune ? 0x4000 /* 16kb */ : BLOCKFILE_CHUNK_SIZE);
-}
-
-FlatFileSeq BlockManager::UndoFileSeq() const
-{
-    return FlatFileSeq(m_opts.blocks_dir, "rev", UNDOFILE_CHUNK_SIZE);
-}
-
 AutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const
 {
-    return AutoFile{BlockFileSeq().Open(pos, fReadOnly)};
+    return AutoFile{m_block_file_seq.Open(pos, fReadOnly)};
 }
 
 /** Open an undo file (rev?????.dat) */
 AutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const
 {
-    return AutoFile{UndoFileSeq().Open(pos, fReadOnly)};
+    return AutoFile{m_undo_file_seq.Open(pos, fReadOnly)};
 }
 
 fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const
 {
-    return BlockFileSeq().FileName(pos);
+    return m_block_file_seq.FileName(pos);
 }
 
 FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int nHeight, uint64_t nTime)
@@ -919,7 +909,7 @@ FlatFilePos BlockManager::FindNextBlockPos(unsigned int nAddSize, unsigned int n
     m_blockfile_info[nFile].nSize += nAddSize;
 
     bool out_of_space;
-    size_t bytes_allocated = BlockFileSeq().Allocate(pos, nAddSize, out_of_space);
+    size_t bytes_allocated = m_block_file_seq.Allocate(pos, nAddSize, out_of_space);
     if (out_of_space) {
         m_opts.notifications.fatalError(_("Disk space is too low!"));
         return {};
@@ -965,7 +955,7 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP
     m_dirty_fileinfo.insert(nFile);
 
     bool out_of_space;
-    size_t bytes_allocated = UndoFileSeq().Allocate(pos, nAddSize, out_of_space);
+    size_t bytes_allocated = m_undo_file_seq.Allocate(pos, nAddSize, out_of_space);
     if (out_of_space) {
         return FatalError(m_opts.notifications, state, _("Disk space is too low!"));
     }

src/node/blockstorage.h

@@ -166,9 +166,6 @@ class BlockManager
     [[nodiscard]] bool FlushChainstateBlockFile(int tip_height);
     bool FindUndoPos(BlockValidationState& state, int nFile, FlatFilePos& pos, unsigned int nAddSize);
 
-    FlatFileSeq BlockFileSeq() const;
-    FlatFileSeq UndoFileSeq() const;
-
     AutoFile OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const;
 
     /**
@@ -261,12 +258,17 @@ class BlockManager
 
     const kernel::BlockManagerOpts m_opts;
 
+    const FlatFileSeq m_block_file_seq;
+    const FlatFileSeq m_undo_file_seq;
+
 public:
     using Options = kernel::BlockManagerOpts;
 
     explicit BlockManager(const util::SignalInterrupt& interrupt, Options opts)
         : m_prune_mode{opts.prune_target > 0},
           m_opts{std::move(opts)},
+          m_block_file_seq{FlatFileSeq{m_opts.blocks_dir, "blk", m_opts.fast_prune ? 0x4000 /* 16kB */ : BLOCKFILE_CHUNK_SIZE}},
+          m_undo_file_seq{FlatFileSeq{m_opts.blocks_dir, "rev", UNDOFILE_CHUNK_SIZE}},
           m_interrupt{interrupt} {}
 
     const util::SignalInterrupt& m_interrupt;