Do not hold _writeMonitor when restarting the fileWriter as it can block.

2 files changed, 27 insertions, 24 deletions

diff --git a/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.cpp b/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.cpp
index aa62c2bba9c..5cb11ede09c 100644
--- a/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.cpp
+++ b/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.cpp
@@ -46,13 +46,13 @@ class PendingChunk
 public:
     typedef std::shared_ptr<PendingChunk> SP;
     PendingChunk(uint64_t lastSerial, uint64_t dataOffset, uint32_t dataLen);
-    ~PendingChunk(void);
-    vespalib::nbostream & getSerializedIdx(void) { return _idx; }
-    const vespalib::nbostream & getSerializedIdx(void) const { return _idx; }
-    uint64_t getDataOffset(void) const { return _dataOffset; }
-    uint32_t getDataLen(void) const { return _dataLen; }
-    uint32_t getIdxLen(void) const { return _idx.size(); }
-    uint64_t getLastSerial(void) const { return _lastSerial; }
+    ~PendingChunk();
+    vespalib::nbostream & getSerializedIdx() { return _idx; }
+    const vespalib::nbostream & getSerializedIdx() const { return _idx; }
+    uint64_t getDataOffset() const { return _dataOffset; }
+    uint32_t getDataLen() const { return _dataLen; }
+    uint32_t getIdxLen() const { return _idx.size(); }
+    uint64_t getLastSerial() const { return _lastSerial; }
 };
 
 class ProcessedChunk
@@ -183,10 +183,8 @@ WriteableFileChunk::updateLidMap(const LockGuard &guard, ISetLid &ds, uint64_t s
 }
 
 void
-WriteableFileChunk::restart(const MonitorGuard & guard, uint32_t nextChunkId)
+WriteableFileChunk::restart(uint32_t nextChunkId)
 {
-    (void) guard;
-    _writeTaskIsRunning = true;
     _executor.execute(makeTask(makeClosure(this, &WriteableFileChunk::fileWriter, nextChunkId)));
 }
 
@@ -298,9 +296,14 @@ WriteableFileChunk::enque(ProcessedChunk::UP tmp)
     MonitorGuard guard(_writeMonitor);
     _writeQ.push_back(std::move(tmp));
     if (_writeTaskIsRunning == false) {
-        restart(guard, _firstChunkIdToBeWritten);
+        _writeTaskIsRunning = true;
+        uint32_t nextChunkId = _firstChunkIdToBeWritten;
+        guard.signal();
+        guard.unlock();
+        restart(nextChunkId);
+    } else {
+        guard.signal();
     }
-    guard.signal();
 }
 
 namespace {
@@ -507,8 +510,8 @@ WriteableFileChunk::fileWriter(const uint32_t firstChunkId)
     LOG(debug,
         "Stopping the filewriter with startchunkid = %d and ending chunkid = %d done=%d",
         firstChunkId, nextChunkId, done);
+    MonitorGuard guard(_writeMonitor);
     if (done) {
-        MonitorGuard guard(_writeMonitor);
         assert(_writeQ.empty());
         assert(_chunkMap.empty());
         for (const ChunkInfo & cm : _chunkInfo) {
@@ -518,12 +521,13 @@ WriteableFileChunk::fileWriter(const uint32_t firstChunkId)
         _writeTaskIsRunning = false;
         guard.broadcast();
     } else {
-        MonitorGuard guard(_writeMonitor);
         if (_writeQ.empty()) {
             _firstChunkIdToBeWritten = nextChunkId;
             _writeTaskIsRunning = false;
         } else {
-            restart(guard, nextChunkId);
+            _writeTaskIsRunning = true;
+            guard.unlock();
+            restart(nextChunkId);
         }
     }
 }
@@ -714,7 +718,7 @@ WriteableFileChunk::append(uint64_t serialNum,
 
 
 void
-WriteableFileChunk::readDataHeader(void)
+WriteableFileChunk::readDataHeader()
 {
     int64_t fSize(_dataFile.GetSize());
     try {
diff --git a/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.h b/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.h
index cbd7f780420..37bf102a868 100644
--- a/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.h
+++ b/searchlib/src/vespa/searchlib/docstore/writeablefilechunk.h
@@ -61,16 +61,16 @@ public:
     uint64_t   getSerialNum() const { return _serialNum; }
     void setSerialNum(uint64_t serialNum) { _serialNum = std::max(_serialNum, serialNum); }
 
-    virtual fastos::TimeStamp getModificationTime() const override;
+    fastos::TimeStamp getModificationTime() const override;
     void freeze();
     size_t getDiskFootprint() const override;
     size_t getMemoryFootprint() const override;
     size_t getMemoryMetaFootprint() const override;
-    virtual MemoryUsage getMemoryUsage() const override;
+    MemoryUsage getMemoryUsage() const override;
     size_t updateLidMap(const LockGuard &guard, ISetLid &lidMap, uint64_t serialNum, uint32_t docIdLimit) override;
     void waitForDiskToCatchUpToNow() const;
     void flushPendingChunks(uint64_t serialNum);
-    virtual DataStoreFileChunkStats getStats() const override;
+    DataStoreFileChunkStats getStats() const override;
 
     static uint64_t writeIdxHeader(const common::FileHeaderContext &fileHeaderContext, uint32_t docIdLimit, FastOS_FileInterface &file);
 private:
@@ -86,17 +86,17 @@ private:
     void internalFlush(uint32_t, uint64_t serialNum);
     void enque(ProcessedChunkUP);
     int32_t flushLastIfNonEmpty(bool force);
-    void restart(const vespalib::MonitorGuard & guard, uint32_t nextChunkId);
+    // _writeMonitor should not be held when calling restart
+    void restart(uint32_t nextChunkId);
     ProcessedChunkQ drainQ();
-    void readDataHeader(void);
-    void readIdxHeader(void);
+    void readDataHeader();
+    void readIdxHeader();
     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);
     fastos::TimeStamp unconditionallyFlushPendingChunks(const vespalib::LockGuard & flushGuard, uint64_t serialNum, uint64_t datFileLen);
     static void insertChunks(ProcessedChunkMap & orderedChunks, ProcessedChunkQ & newChunks, const uint32_t nextChunkId);
     static ProcessedChunkQ fetchNextChain(ProcessedChunkMap & orderedChunks, const uint32_t firstChunkId);
-    size_t computeDataLen(const ProcessedChunk & tmp, const Chunk & active);
     ChunkMeta computeChunkMeta(const vespalib::LockGuard & guard,
                                const vespalib::GenerationHandler::Guard & bucketizerGuard,
                                size_t offset, const ProcessedChunk & tmp, const Chunk & active);
@@ -137,4 +137,3 @@ private:
 };
 
 } // namespace search
-