Merge pull request #3064 from yahoo/balder/handle-truncate-with-later-write

Avoid opening idxfile to early for writing. Especially as it might ge…

5 files changed, 51 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/filechunk.cpp b/searchlib/src/vespa/searchlib/docstore/filechunk.cpp
index 0f753b4914c..f271e6f320b 100644
--- a/searchlib/src/vespa/searchlib/docstore/filechunk.cpp
+++ b/searchlib/src/vespa/searchlib/docstore/filechunk.cpp
@@ -194,6 +194,7 @@ FileChunk::updateLidMap(const LockGuard &guard, ISetLid &ds, uint64_t serialNum,
                     } else {
                         throw SummaryException("Open for truncation failed.", toTruncate, VESPA_STRLOC);
                     }
+                    break;
                 }
             }
             if ( ! tempVector.empty()) {
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;