Avoid opening idxfile to early for writing. Especially as it might get truncated due to error handling later on.

Instead just open the file when you need to write it. It does not happen that often to warrant keeping it open all the time.

4 files changed, 50 insertions, 40 deletions

diff --git a/fastos/src/vespa/fastos/file.h b/fastos/src/vespa/fastos/file.h
index dcc2b401846..8a9de224259 100644
--- a/fastos/src/vespa/fastos/file.h
+++ b/fastos/src/vespa/fastos/file.h
@@ -514,6 +514,8 @@ public:
 
     virtual void EnableSyncWrites();
 
+    bool useSyncWrites() const { return _syncWritesEnabled; }
+
     /**
      * Set the write chunk size used in WriteBuf.
      */
diff --git a/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp b/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp
index 407bf3fad98..d41adb2cdef 100644
--- a/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp
+++ b/searchlib/src/tests/docstore/logdatastore/logdatastore_test.cpp
@@ -296,17 +296,21 @@ TEST("testTruncatedIdxFile"){
                                fileHeaderContext, tlSyncer, NULL);
         EXPECT_EQUAL(354ul, datastore.lastSyncToken());
     }
+    const char * magic = "mumbo jumbo";
     {
         LogDataStore datastore(executor, "bug-7257706-truncated", config,
                                GrowStrategy(), TuneFileSummary(),
                                fileHeaderContext, tlSyncer, NULL);
         EXPECT_EQUAL(331ul, datastore.lastSyncToken());
+        datastore.write(332, 7, magic, strlen(magic));
+        datastore.write(333, 8, magic, strlen(magic));
+        datastore.flush(datastore.initFlush(334));
     }
     {
         LogDataStore datastore(executor, "bug-7257706-truncated", config,
                                GrowStrategy(), TuneFileSummary(),
                                fileHeaderContext, tlSyncer, NULL);
-        EXPECT_EQUAL(331ul, datastore.lastSyncToken());
+        EXPECT_EQUAL(334ul, datastore.lastSyncToken());
     }
 }
 
diff --git a/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.cpp b/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.cpp
index edd31ebf12d..c6b93a59ae8 100644
--- a/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.cpp
+++ b/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.cpp
@@ -94,11 +94,11 @@ WriteableFileChunk(vespalib::ThreadExecutor &executor,
       _writeLock(),
       _flushLock(),
       _dataFile(_dataFileName.c_str()),
-      _idxFile(_idxFileName.c_str()),
       _chunkMap(),
       _pendingChunks(),
       _pendingIdx(0),
       _pendingDat(0),
+      _idxFileSize(0),
       _currentDiskFootprint(0),
       _nextChunkId(1),
       _active(new Chunk(0, Chunk::Config(config.getMaxChunkBytes()))),
@@ -116,7 +116,6 @@ WriteableFileChunk(vespalib::ThreadExecutor &executor,
     }
     if (tune._write.getWantSyncWrites()) {
         _dataFile.EnableSyncWrites();
-        _idxFile.EnableSyncWrites();
     }
     if (_dataFile.OpenReadWrite()) {
         readDataHeader();
@@ -130,16 +129,13 @@ WriteableFileChunk(vespalib::ThreadExecutor &executor,
                            _dataFile.GetFileName(), _dataFile.getLastErrorString().c_str());
             }
         }
-        if (_idxFile.OpenReadWrite()) {
-            readIdxHeader();
-            if (_idxHeaderLen == 0) {
-                _idxHeaderLen = writeIdxHeader(fileHeaderContext, _docIdLimit, _idxFile);
-            }
-            _idxFile.SetPosition(_idxFile.GetSize());
-        } else {
-            _dataFile.Close();
-            throw SummaryException("Failed opening idx file", _idxFile, VESPA_STRLOC);
+        auto idxFile = openIdx();
+        readIdxHeader(*idxFile);
+        if (_idxHeaderLen == 0) {
+            _idxHeaderLen = writeIdxHeader(fileHeaderContext, _docIdLimit, *idxFile);
         }
+        _idxFileSize = idxFile->GetSize();
+        idxFile->Sync();
     } else {
         throw SummaryException("Failed opening data file", _dataFile, VESPA_STRLOC);
     }
@@ -147,6 +143,17 @@ WriteableFileChunk(vespalib::ThreadExecutor &executor,
     updateCurrentDiskFootprint();
 }
 
+std::unique_ptr<FastOS_FileInterface>
+WriteableFileChunk::openIdx() {
+    auto file = std::make_unique<FastOS_File>(_idxFileName.c_str());
+    if (_dataFile.useSyncWrites()) {
+        file->EnableSyncWrites();
+    }
+    if ( ! file->OpenReadWrite() ) {
+        throw SummaryException("Failed opening idx file", *file, VESPA_STRLOC);
+    }
+    return file;
+}
 WriteableFileChunk::~WriteableFileChunk()
 {
     if (!frozen()) {
@@ -162,11 +169,6 @@ WriteableFileChunk::~WriteableFileChunk()
             assert(false);
         }
     }
-    if (_idxFile.IsOpened()) {
-        if (! _idxFile.Sync()) {
-            assert(false);
-        }
-    }
 }
 
 size_t
@@ -461,7 +463,7 @@ WriteableFileChunk::writeData(const ProcessedChunkQ & chunks, size_t sz)
     if (wlen != static_cast<ssize_t>(buf.getDataLen())) {
         throw SummaryException(make_string("Failed writing %ld bytes to dat file. Only %ld written",
                                            buf.getDataLen(), wlen),
-                               _idxFile, VESPA_STRLOC);
+                               _dataFile, VESPA_STRLOC);
     }
     updateCurrentDiskFootprint();
 }
@@ -559,7 +561,6 @@ WriteableFileChunk::freeze()
             _frozen = true;
         }
         _dataFile.Close();
-        _idxFile.Close();
         _bucketMap = BucketDensityComputer(_bucketizer);
     }
 }
@@ -750,33 +751,32 @@ WriteableFileChunk::readDataHeader()
 
 
 void
-WriteableFileChunk::readIdxHeader()
+WriteableFileChunk::readIdxHeader(FastOS_FileInterface & idxFile)
 {
-    int64_t fSize(_idxFile.GetSize());
+    int64_t fSize(idxFile.GetSize());
     try {
         FileHeader h;
-        _idxHeaderLen = h.readFile(_idxFile);
-        _idxFile.SetPosition(_idxHeaderLen);
+        _idxHeaderLen = h.readFile(idxFile);
+        idxFile.SetPosition(_idxHeaderLen);
         _docIdLimit = readDocIdLimit(h);
     } catch (IllegalHeaderException &e) {
-        _idxFile.SetPosition(0);
+        idxFile.SetPosition(0);
         try {
-            FileHeader::FileReader fr(_idxFile);
+            FileHeader::FileReader fr(idxFile);
             uint32_t header2Len = FileHeader::readSize(fr);
-            if (header2Len <= fSize)
+            if (header2Len <= fSize) {
                 e.throwSelf(); // header not truncated
+            }
         } catch (IllegalHeaderException &e2) {
         }
         if (fSize > 0) {
             // Truncate file (dropping header) if cannot even read
             // header length, or if header has been truncated.
-            _idxFile.SetPosition(0);
-            _idxFile.SetSize(0);
-            assert(_idxFile.GetSize() == 0);
-            assert(_idxFile.GetPosition() == 0);
-            LOG(warning,
-                "Truncated file chunk index %s due to truncated file header",
-                _idxFile.GetFileName());
+            idxFile.SetPosition(0);
+            idxFile.SetSize(0);
+            assert(idxFile.GetSize() == 0);
+            assert(idxFile.GetPosition() == 0);
+            LOG(warning, "Truncated file chunk index %s due to truncated file header", idxFile.GetFileName());
         }
     }
 }
@@ -838,7 +838,7 @@ WriteableFileChunk::needFlushPendingChunks(const MonitorGuard & guard, uint64_t
 
 void
 WriteableFileChunk::updateCurrentDiskFootprint() {
-    _currentDiskFootprint = _idxFile.getSize() + _dataFile.getSize();
+    _currentDiskFootprint = _idxFileSize + _dataFile.getSize();
 }
 
 /*
@@ -890,15 +890,18 @@ WriteableFileChunk::unconditionallyFlushPendingChunks(const vespalib::LockGuard
         }
     }
     fastos::TimeStamp timeStamp(fastos::ClockSystem::now());
-    ssize_t wlen = _idxFile.Write2(os.c_str(), os.size());
+    auto idxFile = openIdx();
+    idxFile->SetPosition(idxFile->GetSize());
+    ssize_t wlen = idxFile->Write2(os.c_str(), os.size());
     updateCurrentDiskFootprint();
 
     if (wlen != static_cast<ssize_t>(os.size())) {
-        throw SummaryException(make_string("Failed writing %ld bytes to idx file. Only wrote %ld bytes ", os.size(), wlen), _idxFile, VESPA_STRLOC);
+        throw SummaryException(make_string("Failed writing %ld bytes to idx file. Only wrote %ld bytes ", os.size(), wlen), *idxFile, VESPA_STRLOC);
     }
-    if ( ! _idxFile.Sync()) {
-        throw SummaryException("Failed fsync of idx file", _idxFile, VESPA_STRLOC);
+    if ( ! idxFile->Sync()) {
+        throw SummaryException("Failed fsync of idx file", *idxFile, VESPA_STRLOC);
     }
+    _idxFileSize = idxFile->GetSize();
     if (_lastPersistedSerialNum < lastSerial) {
         _lastPersistedSerialNum = lastSerial;
     }
diff --git a/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.h b/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.h
index a89d88ac93e..86415e0cb40 100644
--- a/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.h
+++ b/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.h
@@ -90,7 +90,7 @@ private:
     void restart(uint32_t nextChunkId);
     ProcessedChunkQ drainQ();
     void readDataHeader();
-    void readIdxHeader();
+    void readIdxHeader(FastOS_FileInterface & idxFile);
     void writeDataHeader(const common::FileHeaderContext &fileHeaderContext);
     bool needFlushPendingChunks(uint64_t serialNum, uint64_t datFileLen);
     bool needFlushPendingChunks(const vespalib::MonitorGuard & guard, uint64_t serialNum, uint64_t datFileLen);
@@ -105,6 +105,7 @@ private:
     void updateChunkInfo(const ProcessedChunkQ & chunks, const ChunkMetaV & cmetaV, size_t sz);
     void updateCurrentDiskFootprint();
     size_t getDiskFootprint(const vespalib::MonitorGuard & guard) const;
+    std::unique_ptr<FastOS_FileInterface> openIdx();
 
     Config            _config;
     SerialNum         _serialNum;
@@ -114,13 +115,13 @@ private:
     vespalib::Lock    _writeLock;
     vespalib::Lock    _flushLock;
     FastOS_File       _dataFile;
-    FastOS_File       _idxFile;
     using ChunkMap = std::map<uint32_t, Chunk::UP>;
     ChunkMap          _chunkMap;
     using PendingChunks = std::deque<std::shared_ptr<PendingChunk>>;
     PendingChunks     _pendingChunks;
     uint64_t          _pendingIdx;
     uint64_t          _pendingDat;
+    uint64_t          _idxFileSize;
     uint64_t          _currentDiskFootprint;
     uint32_t          _nextChunkId;
     Chunk::UP         _active;