From 8cd8de0496f96570d7f8de111c378461e7048300 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Thu, 23 Feb 2023 17:57:58 +0300 Subject: [PATCH 01/12] Optimize freeMemorySpace() to not loop in vain We should not perform this loop if the total number of idle memory pages is less than required number of pages. --- src/MemObject.cc | 2 ++ src/MemObject.h | 6 ++++++ src/store/Controller.cc | 7 +++++++ 3 files changed, 15 insertions(+) diff --git a/src/MemObject.cc b/src/MemObject.cc index bb1292108f2..9d019a16c1e 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -43,6 +43,8 @@ url_checksum(const char *url) RemovalPolicy * mem_policy = nullptr; +size_t MemObject::IdlePagesCount = 0; + size_t MemObject::inUseCount() { diff --git a/src/MemObject.h b/src/MemObject.h index adc786e8208..f49d398a71a 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -165,6 +165,12 @@ class MemObject static constexpr Io ioWriting = Store::ioWriting; static constexpr Io ioDone = Store::ioDone; + size_t pages() const { return (endOffset() + SM_PAGE_SIZE-1) / SM_PAGE_SIZE; } + void markPagesIdle() { IdlePagesCount += pages(); } + void markPagesBusy() { assert(IdlePagesCount > pages()); IdlePagesCount -= pages(); } + + static size_t IdlePagesCount; + /// State of an entry with regards to the [shared] in-transit table. class XitTable { diff --git a/src/store/Controller.cc b/src/store/Controller.cc index 03eaaa15b20..06dedcbc9bf 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -253,6 +253,7 @@ Store::Controller::referenceBusy(StoreEntry &e) if (e.mem_obj) { if (mem_policy->Referenced) mem_policy->Referenced(mem_policy, &e, &e.mem_obj->repl); + e.mem_obj->markPagesBusy(); } } @@ -534,6 +535,9 @@ Store::Controller::freeMemorySpace(const int bytesRequired) if (memoryCacheHasSpaceFor(pagesRequired)) return; + if (MemObject::IdlePagesCount < static_cast(pagesRequired)) + return; + // XXX: When store_pages_max is smaller than pagesRequired, we should not // look for more space (but we do because we want to abandon idle entries?). @@ -677,6 +681,9 @@ Store::Controller::handleIdleEntry(StoreEntry &e) if (keepInLocalMemory) { e.setMemStatus(IN_MEMORY); e.mem_obj->unlinkRequest(); + if (!EBIT_TEST(e.flags, ENTRY_SPECIAL)) { + e.mem_obj->markPagesIdle(); + } return; } From 03e1af327a4c51cc14251ff8f97cfe713daa0fda Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Thu, 2 Mar 2023 02:36:28 +0300 Subject: [PATCH 02/12] Mark idle pages in StoreEntry lock()/unlock() methods Also update the global idle page counter when the number of entry's pages changes. Fortunately, there were only few mem_hdr methods (responsible for adding/removing memory nodes) to be adjusted. Also removed a couple of unused mem_hdr methods. --- src/MemObject.cc | 7 ++-- src/MemObject.h | 8 +--- src/MemStore.cc | 2 +- src/mem_node.cc | 2 + src/mem_node.h | 3 ++ src/stmem.cc | 83 ++++++++++++++++++++++++++--------------- src/stmem.h | 14 ++++--- src/store.cc | 8 +++- src/store/Controller.cc | 6 +-- src/store_client.cc | 2 +- src/tests/stub_stmem.cc | 2 +- 11 files changed, 81 insertions(+), 56 deletions(-) diff --git a/src/MemObject.cc b/src/MemObject.cc index 9d019a16c1e..66b6d38a1be 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -43,8 +43,6 @@ url_checksum(const char *url) RemovalPolicy * mem_policy = nullptr; -size_t MemObject::IdlePagesCount = 0; - size_t MemObject::inUseCount() { @@ -134,8 +132,9 @@ MemObject::replaceBaseReply(const HttpReplyPointer &r) updatedReply_ = nullptr; } +/// TODO: remove the second argument if only locked entries do write() calls void -MemObject::write(const StoreIOBuffer &writeBuffer) +MemObject::write(const StoreIOBuffer &writeBuffer, const bool locked) { debugs(19, 6, "memWrite: offset " << writeBuffer.offset << " len " << writeBuffer.length); @@ -144,7 +143,7 @@ MemObject::write(const StoreIOBuffer &writeBuffer) */ assert (data_hdr.endOffset() || writeBuffer.offset == 0); - assert (data_hdr.write (writeBuffer)); + data_hdr.write(writeBuffer, locked); } void diff --git a/src/MemObject.h b/src/MemObject.h index f49d398a71a..a780d6f40bb 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -51,7 +51,7 @@ class MemObject /// whether setUris() has been called bool hasUris() const; - void write(const StoreIOBuffer &buf); + void write(const StoreIOBuffer &buf, bool locked); void unlinkRequest() { request = nullptr; } /// HTTP response before 304 (Not Modified) updates @@ -165,12 +165,6 @@ class MemObject static constexpr Io ioWriting = Store::ioWriting; static constexpr Io ioDone = Store::ioDone; - size_t pages() const { return (endOffset() + SM_PAGE_SIZE-1) / SM_PAGE_SIZE; } - void markPagesIdle() { IdlePagesCount += pages(); } - void markPagesBusy() { assert(IdlePagesCount > pages()); IdlePagesCount -= pages(); } - - static size_t IdlePagesCount; - /// State of an entry with regards to the [shared] in-transit table. class XitTable { diff --git a/src/MemStore.cc b/src/MemStore.cc index 44dc0dcb983..f4f96d1607f 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -569,7 +569,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof) // local memory stores both headers and body so copy regardless of pstate const int64_t offBefore = e.mem_obj->endOffset(); - assert(e.mem_obj->data_hdr.write(buf)); // from MemObject::write() + e.mem_obj->data_hdr.write(buf, e.locked()); // from MemObject::write() const int64_t offAfter = e.mem_obj->endOffset(); // expect to write the entire buf because StoreEntry::write() never fails assert(offAfter >= 0 && offBefore <= offAfter && diff --git a/src/mem_node.cc b/src/mem_node.cc index 0b75ed2ddec..8ac28a8a1ec 100644 --- a/src/mem_node.cc +++ b/src/mem_node.cc @@ -16,6 +16,8 @@ static ptrdiff_t makeMemNodeDataOffset(); static ptrdiff_t _mem_node_data_offset = makeMemNodeDataOffset(); +size_t mem_node::IdleNodes = 0; + /* * Calculate the offset between the start of a mem_node and * its 'data' member diff --git a/src/mem_node.h b/src/mem_node.h index 439957531a2..807735a9a8a 100644 --- a/src/mem_node.h +++ b/src/mem_node.h @@ -31,6 +31,9 @@ class mem_node bool contains (int64_t const &location) const; bool canAccept (int64_t const &location) const; bool operator < (mem_node const & rhs) const; + /// the total number of pages belonging to unlocked StoreEntries + static size_t IdleNodes; + /* public */ StoreIOBuffer nodeBuffer; /* Private */ diff --git a/src/stmem.cc b/src/stmem.cc index 146cf956563..12f0ebefb28 100644 --- a/src/stmem.cc +++ b/src/stmem.cc @@ -58,7 +58,10 @@ mem_hdr::endOffset () const void mem_hdr::freeContent() { + const auto initialNodes = size(); nodes.destroy(); + if (isIdle) + updateIdleNodesNumber(initialNodes); inmem_hi = 0; debugs(19, 9, this << " hi: " << inmem_hi); } @@ -79,6 +82,16 @@ mem_hdr::unlink(mem_node *aNode) int64_t mem_hdr::freeDataUpto(int64_t target_offset) +{ + const auto initialNodes = size(); + const auto offset = freeDataUptoImpl(target_offset); + if (isIdle) + updateIdleNodesNumber(initialNodes); + return offset; +} + +int64_t +mem_hdr::freeDataUptoImpl(int64_t target_offset) { debugs(19, 8, this << " up to " << target_offset); /* keep the last one to avoid change to other part of code */ @@ -139,35 +152,6 @@ mem_hdr::appendNode (mem_node *aNode) nodes.insert (aNode, NodeCompare); } -void -mem_hdr::makeAppendSpace() -{ - if (!nodes.size()) { - appendNode (new mem_node (0)); - return; - } - - if (!nodes.finish()->data->space()) - appendNode (new mem_node (endOffset())); - - assert (nodes.finish()->data->space()); -} - -void -mem_hdr::internalAppend(const char *data, int len) -{ - debugs(19, 6, "memInternalAppend: " << this << " len " << len); - - while (len > 0) { - makeAppendSpace(); - int copied = appendToNode (nodes.finish()->data, data, len); - assert (copied); - - len -= copied; - data += copied; - } -} - /* returns a mem_node that contains location.. * If no node contains the start, it returns NULL. */ @@ -337,8 +321,17 @@ mem_hdr::nodeToRecieve(int64_t offset) return candidate; } +void +mem_hdr::write(const StoreIOBuffer &writeBuffer, const bool locked) +{ + const auto initialNodes = size(); + assert(writeImpl(writeBuffer)); + if (!locked) + updateIdleNodesNumber(initialNodes); +} + bool -mem_hdr::write (StoreIOBuffer const &writeBuffer) +mem_hdr::writeImpl(StoreIOBuffer const &writeBuffer) { debugs(19, 6, "mem_hdr::write: " << this << " " << writeBuffer.range() << " object end " << endOffset()); @@ -367,7 +360,7 @@ mem_hdr::write (StoreIOBuffer const &writeBuffer) return true; } -mem_hdr::mem_hdr() : inmem_hi(0) +mem_hdr::mem_hdr() : inmem_hi(0), isIdle(false) { debugs(19, 9, this << " hi: " << inmem_hi); } @@ -423,3 +416,31 @@ mem_hdr::getNodes() const return nodes; } +void +mem_hdr::markBusy() +{ + isIdle = false; + assert(mem_node::IdleNodes >= size()); + mem_node::IdleNodes -= size(); +} + +void +mem_hdr::markIdle() +{ + isIdle = true; + mem_node::IdleNodes += size(); +} + +void +mem_hdr::updateIdleNodesNumber(const size_t oldSize) +{ + const auto newSize = size(); + const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize; + if (newSize > oldSize) { + mem_node::IdleNodes += delta; + } else if (newSize < oldSize) { + assert(mem_node::IdleNodes >= delta); + mem_node::IdleNodes -= delta; + } +} + diff --git a/src/stmem.h b/src/stmem.h index d5e82d18df6..bd156a79086 100644 --- a/src/stmem.h +++ b/src/stmem.h @@ -29,35 +29,39 @@ class mem_hdr ssize_t copy (StoreIOBuffer const &) const; bool hasContigousContentRange(Range const &range) const; /* success or fail */ - bool write (StoreIOBuffer const &); + void write(StoreIOBuffer const &, bool locked); void dump() const; size_t size() const; /* Not an iterator - thus the start, not begin() */ mem_node const *start() const; mem_node *getBlockContainingLocation (int64_t location) const; + /// called when the possessing entry becomes busy/locked + void markBusy(); + /// called when the possessing entry becomes idle + void markIdle(); /* access the contained nodes - easier than punning * as a container ourselves */ const Splay &getNodes() const; char * NodeGet(mem_node * aNode); - /* Only for use of MemObject */ - void internalAppend(const char *data, int len); - static Splay::SPLAYCMP NodeCompare; private: void debugDump() const; bool unlink(mem_node *aNode); - void makeAppendSpace(); int appendToNode(mem_node *aNode, const char *data, int maxLength); void appendNode (mem_node *aNode); size_t copyAvailable(mem_node *aNode, int64_t location, size_t amount, char *target) const; bool unionNotEmpty (StoreIOBuffer const &); mem_node *nodeToRecieve(int64_t offset); size_t writeAvailable(mem_node *aNode, int64_t location, size_t amount, char const *source); + int64_t freeDataUptoImpl(int64_t); + bool writeImpl(const StoreIOBuffer &); + void updateIdleNodesNumber(const size_t oldSize); int64_t inmem_hi; Splay nodes; + bool isIdle; ///< whether the associated pages belong to an unlocked StoreEntry }; #endif /* SQUID_STMEM_H */ diff --git a/src/store.cc b/src/store.cc index 44b866582ea..85e77b1077e 100644 --- a/src/store.cc +++ b/src/store.cc @@ -418,6 +418,8 @@ StoreEntry::hashDelete() void StoreEntry::lock(const char *context) { + if (!lock_count && mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL)) + mem_obj->data_hdr.markBusy(); ++lock_count; debugs(20, 3, context << " locked key " << getMD5Text() << ' ' << *this); } @@ -450,6 +452,10 @@ StoreEntry::unlock(const char *context) if (lock_count) return (int) lock_count; + if (!EBIT_TEST(flags, ENTRY_SPECIAL)) { + mem_obj->data_hdr.markIdle(); + } + abandon(context); return 0; } @@ -762,7 +768,7 @@ StoreEntry::write (StoreIOBuffer writeBuffer) debugs(20, 5, "storeWrite: writing " << writeBuffer.length << " bytes for '" << getMD5Text() << "'"); storeGetMemSpace(writeBuffer.length); - mem_obj->write(writeBuffer); + mem_obj->write(writeBuffer, locked()); if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT) && !mem_obj->readAheadPolicyCanRead()) { debugs(20, 3, "allow Store clients to get entry content after buffering too much for " << *this); diff --git a/src/store/Controller.cc b/src/store/Controller.cc index 06dedcbc9bf..801266c7ecf 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -253,7 +253,6 @@ Store::Controller::referenceBusy(StoreEntry &e) if (e.mem_obj) { if (mem_policy->Referenced) mem_policy->Referenced(mem_policy, &e, &e.mem_obj->repl); - e.mem_obj->markPagesBusy(); } } @@ -535,7 +534,7 @@ Store::Controller::freeMemorySpace(const int bytesRequired) if (memoryCacheHasSpaceFor(pagesRequired)) return; - if (MemObject::IdlePagesCount < static_cast(pagesRequired)) + if (mem_node::IdleNodes < static_cast(pagesRequired)) return; // XXX: When store_pages_max is smaller than pagesRequired, we should not @@ -681,9 +680,6 @@ Store::Controller::handleIdleEntry(StoreEntry &e) if (keepInLocalMemory) { e.setMemStatus(IN_MEMORY); e.mem_obj->unlinkRequest(); - if (!EBIT_TEST(e.flags, ENTRY_SPECIAL)) { - e.mem_obj->markPagesIdle(); - } return; } diff --git a/src/store_client.cc b/src/store_client.cc index b213c89d340..faf889a7b16 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -539,7 +539,7 @@ store_client::readBody(const char *, ssize_t len) * copyInto.offset includes headers, which is what mem cache needs */ if (copyInto.offset == entry->mem_obj->endOffset()) { - entry->mem_obj->write(StoreIOBuffer(len, copyInto.offset, copyInto.data)); + entry->mem_obj->write(StoreIOBuffer(len, copyInto.offset, copyInto.data), entry->locked()); } } } diff --git a/src/tests/stub_stmem.cc b/src/tests/stub_stmem.cc index ab329b5eb62..e45f5df4312 100644 --- a/src/tests/stub_stmem.cc +++ b/src/tests/stub_stmem.cc @@ -16,5 +16,5 @@ mem_hdr::mem_hdr() STUB mem_hdr::~mem_hdr() STUB size_t mem_hdr::size() const STUB_RETVAL(0) int64_t mem_hdr::endOffset () const STUB_RETVAL(0) -bool mem_hdr::write (StoreIOBuffer const &) STUB_RETVAL(false) +void mem_hdr::write (StoreIOBuffer const &) STUB_RETVAL(false) From ac3cf7c4a0b0e454133e6244876db33e6c86a26e Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 13 Mar 2023 17:59:59 +0300 Subject: [PATCH 03/12] Removed 'locked' parameter from write() methods --- src/MemObject.cc | 5 ++-- src/MemObject.h | 2 +- src/MemStore.cc | 2 +- src/Store.h | 7 ++++++ src/stmem.cc | 51 +++++++++++++---------------------------- src/stmem.h | 13 ++++------- src/store.cc | 26 ++++++++++++++++----- src/store/Controller.cc | 2 +- src/store_client.cc | 2 +- src/tests/stub_stmem.cc | 2 +- 10 files changed, 55 insertions(+), 57 deletions(-) diff --git a/src/MemObject.cc b/src/MemObject.cc index 66b6d38a1be..87e8c666686 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -132,9 +132,8 @@ MemObject::replaceBaseReply(const HttpReplyPointer &r) updatedReply_ = nullptr; } -/// TODO: remove the second argument if only locked entries do write() calls void -MemObject::write(const StoreIOBuffer &writeBuffer, const bool locked) +MemObject::write(const StoreIOBuffer &writeBuffer) { debugs(19, 6, "memWrite: offset " << writeBuffer.offset << " len " << writeBuffer.length); @@ -143,7 +142,7 @@ MemObject::write(const StoreIOBuffer &writeBuffer, const bool locked) */ assert (data_hdr.endOffset() || writeBuffer.offset == 0); - data_hdr.write(writeBuffer, locked); + assert(data_hdr.write(writeBuffer)); } void diff --git a/src/MemObject.h b/src/MemObject.h index a780d6f40bb..adc786e8208 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -51,7 +51,7 @@ class MemObject /// whether setUris() has been called bool hasUris() const; - void write(const StoreIOBuffer &buf, bool locked); + void write(const StoreIOBuffer &buf); void unlinkRequest() { request = nullptr; } /// HTTP response before 304 (Not Modified) updates diff --git a/src/MemStore.cc b/src/MemStore.cc index f4f96d1607f..8f8abdeb75b 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -569,7 +569,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof) // local memory stores both headers and body so copy regardless of pstate const int64_t offBefore = e.mem_obj->endOffset(); - e.mem_obj->data_hdr.write(buf, e.locked()); // from MemObject::write() + e.writeData(buf); // from MemObject::write() const int64_t offAfter = e.mem_obj->endOffset(); // expect to write the entire buf because StoreEntry::write() never fails assert(offAfter >= 0 && offBefore <= offAfter && diff --git a/src/Store.h b/src/Store.h index 3b74f07a6ef..5f97a8a17aa 100644 --- a/src/Store.h +++ b/src/Store.h @@ -56,6 +56,10 @@ class StoreEntry : public hash_link, public Packable /// \see MemObject::freshestReply() const HttpReply *hasFreshestReply() const { return mem_obj ? &mem_obj->freshestReply() : nullptr; } + /// writes to local memory store + void writeData(StoreIOBuffer); + + /// writeData() and calls awaiting handlers void write(StoreIOBuffer); /** Check if the Store entry is empty @@ -67,6 +71,9 @@ class StoreEntry : public hash_link, public Packable bool isAccepting() const; size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; + /// whether write() can be called + bool isLocalWriter() const { return locked() && isAccepting(); } + /// Signals that the entire response has been stored and no more append() /// calls should be expected; cf. completeTruncated(). void completeSuccessfully(const char *whyWeAreSureWeStoredTheWholeReply); diff --git a/src/stmem.cc b/src/stmem.cc index ac91a465166..b792d76c0fc 100644 --- a/src/stmem.cc +++ b/src/stmem.cc @@ -60,8 +60,7 @@ mem_hdr::freeContent() { const auto initialNodes = size(); nodes.destroy(); - if (isIdle) - updateIdleNodesNumber(initialNodes); + updateIdleNodes(initialNodes); inmem_hi = 0; debugs(19, 9, this << " hi: " << inmem_hi); } @@ -75,23 +74,15 @@ mem_hdr::unlink(mem_node *aNode) } debugs(19, 8, this << " removing " << aNode); + const auto initialNodes = size(); nodes.remove (aNode, NodeCompare); delete aNode; + updateIdleNodes(initialNodes); return true; } int64_t mem_hdr::freeDataUpto(int64_t target_offset) -{ - const auto initialNodes = size(); - const auto offset = freeDataUptoImpl(target_offset); - if (isIdle) - updateIdleNodesNumber(initialNodes); - return offset; -} - -int64_t -mem_hdr::freeDataUptoImpl(int64_t target_offset) { debugs(19, 8, this << " up to " << target_offset); /* keep the last one to avoid change to other part of code */ @@ -314,17 +305,8 @@ mem_hdr::nodeToRecieve(int64_t offset) return candidate; } -void -mem_hdr::write(const StoreIOBuffer &writeBuffer, const bool locked) -{ - const auto initialNodes = size(); - assert(writeImpl(writeBuffer)); - if (!locked) - updateIdleNodesNumber(initialNodes); -} - bool -mem_hdr::writeImpl(StoreIOBuffer const &writeBuffer) +mem_hdr::write(const StoreIOBuffer &writeBuffer) { debugs(19, 6, "mem_hdr::write: " << this << " " << writeBuffer.range() << " object end " << endOffset()); @@ -353,7 +335,7 @@ mem_hdr::writeImpl(StoreIOBuffer const &writeBuffer) return true; } -mem_hdr::mem_hdr() : inmem_hi(0), isIdle(false) +mem_hdr::mem_hdr() : inmem_hi(0), isIdle(true) { debugs(19, 9, this << " hi: " << inmem_hi); } @@ -399,23 +381,22 @@ mem_hdr::getNodes() const } void -mem_hdr::markBusy() +mem_hdr::setIdleness(bool idle) { - isIdle = false; - assert(mem_node::IdleNodes >= size()); - mem_node::IdleNodes -= size(); -} - -void -mem_hdr::markIdle() -{ - isIdle = true; - mem_node::IdleNodes += size(); + // TODO: rework to assert + if (idle == isIdle) + return; + if (isIdle) + mem_node::IdleNodes += size(); + else + mem_node::IdleNodes -= size(); } void -mem_hdr::updateIdleNodesNumber(const size_t oldSize) +mem_hdr::updateIdleNodes(const size_t oldSize) { + if (!isIdle) + return; const auto newSize = size(); const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize; if (newSize > oldSize) { diff --git a/src/stmem.h b/src/stmem.h index e8bced43b09..803dabe3448 100644 --- a/src/stmem.h +++ b/src/stmem.h @@ -29,14 +29,14 @@ class mem_hdr ssize_t copy (StoreIOBuffer const &) const; bool hasContigousContentRange(Range const &range) const; /* success or fail */ - void write(StoreIOBuffer const &, bool locked); + bool write (StoreIOBuffer const &); void dump() const; size_t size() const; mem_node *getBlockContainingLocation (int64_t location) const; - /// called when the possessing entry becomes busy/locked - void markBusy(); - /// called when the possessing entry becomes idle - void markIdle(); + /// switches the 'idleness' status of or all nodes + void setIdleness(bool idle); + /// adjust idle nodes counter + void updateIdleNodes(const size_t oldSize); /* access the contained nodes - easier than punning * as a container ourselves */ @@ -53,9 +53,6 @@ class mem_hdr bool unionNotEmpty (StoreIOBuffer const &); mem_node *nodeToRecieve(int64_t offset); size_t writeAvailable(mem_node *aNode, int64_t location, size_t amount, char const *source); - int64_t freeDataUptoImpl(int64_t); - bool writeImpl(const StoreIOBuffer &); - void updateIdleNodesNumber(const size_t oldSize); int64_t inmem_hi; Splay nodes; bool isIdle; ///< whether the associated pages belong to an unlocked StoreEntry diff --git a/src/store.cc b/src/store.cc index 85e77b1077e..c0387d77367 100644 --- a/src/store.cc +++ b/src/store.cc @@ -419,7 +419,7 @@ void StoreEntry::lock(const char *context) { if (!lock_count && mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL)) - mem_obj->data_hdr.markBusy(); + mem_obj->data_hdr.setIdleness(false); ++lock_count; debugs(20, 3, context << " locked key " << getMD5Text() << ' ' << *this); } @@ -452,8 +452,8 @@ StoreEntry::unlock(const char *context) if (lock_count) return (int) lock_count; - if (!EBIT_TEST(flags, ENTRY_SPECIAL)) { - mem_obj->data_hdr.markIdle(); + if (mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL)) { + mem_obj->data_hdr.setIdleness(true); } abandon(context); @@ -761,14 +761,14 @@ StoreEntry::write (StoreIOBuffer writeBuffer) { assert(mem_obj != nullptr); /* This assert will change when we teach the store to update */ - assert(store_status == STORE_PENDING); + assert(isLocalWriter()); // XXX: caller uses content offset, but we also store headers writeBuffer.offset += mem_obj->baseReply().hdr_sz; debugs(20, 5, "storeWrite: writing " << writeBuffer.length << " bytes for '" << getMD5Text() << "'"); storeGetMemSpace(writeBuffer.length); - mem_obj->write(writeBuffer, locked()); + writeData(writeBuffer); if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT) && !mem_obj->readAheadPolicyCanRead()) { debugs(20, 3, "allow Store clients to get entry content after buffering too much for " << *this); @@ -778,6 +778,15 @@ StoreEntry::write (StoreIOBuffer writeBuffer) invokeHandlers(); } +void +StoreEntry::writeData(StoreIOBuffer writeBuffer) +{ + const auto oldSize = mem_obj->data_hdr.size(); + mem_obj->write(writeBuffer); + if (!locked()) + mem_obj->data_hdr.updateIdleNodes(oldSize); +} + /* Append incoming data from a primary server to an entry. */ void StoreEntry::append(char const *buf, int len) @@ -1550,6 +1559,8 @@ StoreEntry::createMemObject() { assert(!mem_obj); mem_obj = new MemObject(); + if (locked()) + mem_obj->data_hdr.setIdleness(false); } void @@ -1562,8 +1573,11 @@ StoreEntry::createMemObject(const char *aUrl, const char *aLogUrl, const HttpReq void StoreEntry::ensureMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod) { - if (!mem_obj) + if (!mem_obj) { mem_obj = new MemObject(); + if (locked()) + mem_obj->data_hdr.setIdleness(false); + } mem_obj->setUris(aUrl, aLogUrl, aMethod); } diff --git a/src/store/Controller.cc b/src/store/Controller.cc index 801266c7ecf..ea9e1b04487 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -339,7 +339,7 @@ Store::Controller::checkFoundCandidate(const StoreEntry &entry) const // Transients do check when they setCollapsingRequirement(). } else { // a local writer must hold a lock on its writable entry - if (!(entry.locked() && entry.isAccepting())) + if (!(entry.isLocalWriter())) throw TextException("no local writer", Here()); } } diff --git a/src/store_client.cc b/src/store_client.cc index faf889a7b16..ec6e1a078e5 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -539,7 +539,7 @@ store_client::readBody(const char *, ssize_t len) * copyInto.offset includes headers, which is what mem cache needs */ if (copyInto.offset == entry->mem_obj->endOffset()) { - entry->mem_obj->write(StoreIOBuffer(len, copyInto.offset, copyInto.data), entry->locked()); + entry->writeData(StoreIOBuffer(len, copyInto.offset, copyInto.data)); } } } diff --git a/src/tests/stub_stmem.cc b/src/tests/stub_stmem.cc index e45f5df4312..ab329b5eb62 100644 --- a/src/tests/stub_stmem.cc +++ b/src/tests/stub_stmem.cc @@ -16,5 +16,5 @@ mem_hdr::mem_hdr() STUB mem_hdr::~mem_hdr() STUB size_t mem_hdr::size() const STUB_RETVAL(0) int64_t mem_hdr::endOffset () const STUB_RETVAL(0) -void mem_hdr::write (StoreIOBuffer const &) STUB_RETVAL(false) +bool mem_hdr::write (StoreIOBuffer const &) STUB_RETVAL(false) From 33a2c2c5cb3ef55914a5b821f3f3490c1f21e8d7 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 14 Mar 2023 01:26:04 +0300 Subject: [PATCH 04/12] Addressed a TODO in stmem.cc Also properly initialize mem_hdr::isIdle in constructor instead of supplying the correct value later. Also added more invariants (as assertions). --- src/MemObject.cc | 4 ++-- src/MemObject.h | 3 ++- src/MemStore.cc | 2 +- src/stmem.cc | 15 ++++++++------- src/stmem.h | 3 ++- src/store.cc | 12 ++++-------- 6 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/MemObject.cc b/src/MemObject.cc index 87e8c666686..955ebc1f0a8 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -94,7 +94,7 @@ MemObject::setUris(char const *aStoreId, char const *aLogUri, const HttpRequestM #endif } -MemObject::MemObject() +MemObject::MemObject(const bool locked) : data_hdr(locked) { debugs(20, 3, "MemObject constructed, this=" << this); ping_reply_callback = nullptr; @@ -142,7 +142,7 @@ MemObject::write(const StoreIOBuffer &writeBuffer) */ assert (data_hdr.endOffset() || writeBuffer.offset == 0); - assert(data_hdr.write(writeBuffer)); + assert (data_hdr.write (writeBuffer)); } void diff --git a/src/MemObject.h b/src/MemObject.h index adc786e8208..c96d2fb228c 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -37,7 +37,8 @@ class MemObject static size_t inUseCount(); void dump() const; - MemObject(); + /// \param locked whether the associated StoreEntry is locked + MemObject(bool locked); ~MemObject(); /// Sets store ID, log URI, and request method (unless already set). Does diff --git a/src/MemStore.cc b/src/MemStore.cc index 8f8abdeb75b..263cc55892d 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -569,7 +569,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof) // local memory stores both headers and body so copy regardless of pstate const int64_t offBefore = e.mem_obj->endOffset(); - e.writeData(buf); // from MemObject::write() + e.writeData(buf); const int64_t offAfter = e.mem_obj->endOffset(); // expect to write the entire buf because StoreEntry::write() never fails assert(offAfter >= 0 && offBefore <= offAfter && diff --git a/src/stmem.cc b/src/stmem.cc index b792d76c0fc..99facd17a38 100644 --- a/src/stmem.cc +++ b/src/stmem.cc @@ -335,7 +335,7 @@ mem_hdr::write(const StoreIOBuffer &writeBuffer) return true; } -mem_hdr::mem_hdr() : inmem_hi(0), isIdle(true) +mem_hdr::mem_hdr(const bool locked) : inmem_hi(0), isIdle(!locked) { debugs(19, 9, this << " hi: " << inmem_hi); } @@ -381,15 +381,16 @@ mem_hdr::getNodes() const } void -mem_hdr::setIdleness(bool idle) +mem_hdr::setIdleness(const bool idle) { - // TODO: rework to assert - if (idle == isIdle) - return; - if (isIdle) + assert(idle != isIdle); + isIdle = idle; + if (isIdle) { mem_node::IdleNodes += size(); - else + } else { + assert(mem_node::IdleNodes >= size()); mem_node::IdleNodes -= size(); + } } void diff --git a/src/stmem.h b/src/stmem.h index 803dabe3448..9121ea64977 100644 --- a/src/stmem.h +++ b/src/stmem.h @@ -20,7 +20,8 @@ class mem_hdr { public: - mem_hdr(); + /// \param locked whether the associated StoreEntry is locked + mem_hdr(bool locked); ~mem_hdr(); void freeContent(); int64_t lowestOffset () const; diff --git a/src/store.cc b/src/store.cc index c0387d77367..79b3e6d1d24 100644 --- a/src/store.cc +++ b/src/store.cc @@ -783,6 +783,7 @@ StoreEntry::writeData(StoreIOBuffer writeBuffer) { const auto oldSize = mem_obj->data_hdr.size(); mem_obj->write(writeBuffer); + // the entry may be still unlocked when it is (initially) copied from a shared storage if (!locked()) mem_obj->data_hdr.updateIdleNodes(oldSize); } @@ -1558,9 +1559,7 @@ void StoreEntry::createMemObject() { assert(!mem_obj); - mem_obj = new MemObject(); - if (locked()) - mem_obj->data_hdr.setIdleness(false); + mem_obj = new MemObject(locked()); } void @@ -1573,11 +1572,8 @@ StoreEntry::createMemObject(const char *aUrl, const char *aLogUrl, const HttpReq void StoreEntry::ensureMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod) { - if (!mem_obj) { - mem_obj = new MemObject(); - if (locked()) - mem_obj->data_hdr.setIdleness(false); - } + if (!mem_obj) + createMemObject(); mem_obj->setUris(aUrl, aLogUrl, aMethod); } From 919a6df276a1b62eb00eee7913954c748e7deabf Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 14 Mar 2023 02:03:03 +0300 Subject: [PATCH 05/12] Partially fixed unit tests --- src/Store.h | 2 +- src/tests/stub_MemObject.cc | 2 +- src/tests/stub_stmem.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Store.h b/src/Store.h index 5f97a8a17aa..836a282245f 100644 --- a/src/Store.h +++ b/src/Store.h @@ -71,7 +71,7 @@ class StoreEntry : public hash_link, public Packable bool isAccepting() const; size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; - /// whether write() can be called + /// the entry is being written to the local memory store bool isLocalWriter() const { return locked() && isAccepting(); } /// Signals that the entire response has been stored and no more append() diff --git a/src/tests/stub_MemObject.cc b/src/tests/stub_MemObject.cc index 2be51b6ee56..0a0a5311896 100644 --- a/src/tests/stub_MemObject.cc +++ b/src/tests/stub_MemObject.cc @@ -29,7 +29,7 @@ MemObject::endOffset() const void MemObject::trimSwappable() STUB void MemObject::trimUnSwappable() STUB int64_t MemObject::policyLowestOffsetToKeep(bool) const STUB_RETVAL(-1) -MemObject::MemObject() { +MemObject::MemObject(bool locked) : data_hdr(locked) { ping_reply_callback = nullptr; memset(&start_ping, 0, sizeof(start_ping)); } // NOP instead of elided due to Store diff --git a/src/tests/stub_stmem.cc b/src/tests/stub_stmem.cc index ab329b5eb62..5bc8f18b31a 100644 --- a/src/tests/stub_stmem.cc +++ b/src/tests/stub_stmem.cc @@ -12,7 +12,7 @@ #define STUB_API "stmem.cc" #include "tests/STUB.h" -mem_hdr::mem_hdr() STUB +mem_hdr::mem_hdr(bool) STUB mem_hdr::~mem_hdr() STUB size_t mem_hdr::size() const STUB_RETVAL(0) int64_t mem_hdr::endOffset () const STUB_RETVAL(0) From df95793dc9171f35af97a5f2a7c1829e792d4977 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 14 Mar 2023 15:54:12 +0300 Subject: [PATCH 06/12] Fixed the rest of unit tests --- src/tests/testStore.cc | 1 + src/tests/testStoreController.cc | 1 + src/tests/testStoreHashIndex.cc | 1 + 3 files changed, 3 insertions(+) diff --git a/src/tests/testStore.cc b/src/tests/testStore.cc index 7538f0d8d28..a83696b2ae0 100644 --- a/src/tests/testStore.cc +++ b/src/tests/testStore.cc @@ -112,6 +112,7 @@ TestStore::testStats() Store::Init(aStore); CPPUNIT_ASSERT_EQUAL(false, aStore->statsCalled); StoreEntry entry; + entry.lock("TestStore::testStats"); Store::Stats(&entry); CPPUNIT_ASSERT_EQUAL(true, aStore->statsCalled); Store::FreeMemory(); diff --git a/src/tests/testStoreController.cc b/src/tests/testStoreController.cc index ff528f135f4..d2cd2294dd2 100644 --- a/src/tests/testStoreController.cc +++ b/src/tests/testStoreController.cc @@ -30,6 +30,7 @@ TestStoreController::testStats() { Store::Init(); StoreEntry *logEntry = new StoreEntry; + logEntry->lock("TestStoreController::testStats"); logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; TestSwapDirPointer aStore (new TestSwapDir); diff --git a/src/tests/testStoreHashIndex.cc b/src/tests/testStoreHashIndex.cc index 7ddf48f6676..90d08814d62 100644 --- a/src/tests/testStoreHashIndex.cc +++ b/src/tests/testStoreHashIndex.cc @@ -29,6 +29,7 @@ void TestStoreHashIndex::testStats() { StoreEntry *logEntry = new StoreEntry; + logEntry->lock("TestStoreHashIndex::testStats"); logEntry->createMemObject("dummy_storeId", nullptr, HttpRequestMethod()); logEntry->store_status = STORE_PENDING; Store::Init(); From b9d2033ad071170f72c3124f92f207ca19c84de7 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 14 Mar 2023 16:46:28 +0300 Subject: [PATCH 07/12] Reduced diff by moving IdleNodes to stmem.cc Also adjusted documentation. --- src/MemObject.h | 2 ++ src/Store.h | 5 +++-- src/mem_node.cc | 2 -- src/mem_node.h | 3 --- src/stmem.cc | 14 ++++++++------ src/stmem.h | 6 +++++- src/store.cc | 2 +- src/store/Controller.cc | 2 +- 8 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/MemObject.h b/src/MemObject.h index c96d2fb228c..bb86659c4e9 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -52,6 +52,8 @@ class MemObject /// whether setUris() has been called bool hasUris() const; + /// \copydoc StoreEntry::writeData() + /// Do not call directly - use StoreEntry::writeData() instead. void write(const StoreIOBuffer &buf); void unlinkRequest() { request = nullptr; } diff --git a/src/Store.h b/src/Store.h index 836a282245f..9d06dd30600 100644 --- a/src/Store.h +++ b/src/Store.h @@ -59,7 +59,8 @@ class StoreEntry : public hash_link, public Packable /// writes to local memory store void writeData(StoreIOBuffer); - /// writeData() and calls awaiting handlers + /// writeData() and calls awaiting handlers. + /// Should be called for non-completed (STORE_PENDING) entries only. void write(StoreIOBuffer); /** Check if the Store entry is empty @@ -71,7 +72,7 @@ class StoreEntry : public hash_link, public Packable bool isAccepting() const; size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; - /// the entry is being written to the local memory store + /// whether a non-completed (STORE_PENDING) entry is being written to the local memory store bool isLocalWriter() const { return locked() && isAccepting(); } /// Signals that the entire response has been stored and no more append() diff --git a/src/mem_node.cc b/src/mem_node.cc index 8ac28a8a1ec..0b75ed2ddec 100644 --- a/src/mem_node.cc +++ b/src/mem_node.cc @@ -16,8 +16,6 @@ static ptrdiff_t makeMemNodeDataOffset(); static ptrdiff_t _mem_node_data_offset = makeMemNodeDataOffset(); -size_t mem_node::IdleNodes = 0; - /* * Calculate the offset between the start of a mem_node and * its 'data' member diff --git a/src/mem_node.h b/src/mem_node.h index 807735a9a8a..439957531a2 100644 --- a/src/mem_node.h +++ b/src/mem_node.h @@ -31,9 +31,6 @@ class mem_node bool contains (int64_t const &location) const; bool canAccept (int64_t const &location) const; bool operator < (mem_node const & rhs) const; - /// the total number of pages belonging to unlocked StoreEntries - static size_t IdleNodes; - /* public */ StoreIOBuffer nodeBuffer; /* Private */ diff --git a/src/stmem.cc b/src/stmem.cc index 99facd17a38..f8d43893d22 100644 --- a/src/stmem.cc +++ b/src/stmem.cc @@ -15,6 +15,8 @@ #include "MemObject.h" #include "stmem.h" +size_t mem_hdr::IdleNodes = 0; + /* * NodeGet() is called to get the data buffer to pass to storeIOWrite(). * By setting the write_pending flag here we are assuming that there @@ -386,10 +388,10 @@ mem_hdr::setIdleness(const bool idle) assert(idle != isIdle); isIdle = idle; if (isIdle) { - mem_node::IdleNodes += size(); + IdleNodes += size(); } else { - assert(mem_node::IdleNodes >= size()); - mem_node::IdleNodes -= size(); + assert(IdleNodes >= size()); + IdleNodes -= size(); } } @@ -401,10 +403,10 @@ mem_hdr::updateIdleNodes(const size_t oldSize) const auto newSize = size(); const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize; if (newSize > oldSize) { - mem_node::IdleNodes += delta; + IdleNodes += delta; } else if (newSize < oldSize) { - assert(mem_node::IdleNodes >= delta); - mem_node::IdleNodes -= delta; + assert(IdleNodes >= delta); + IdleNodes -= delta; } } diff --git a/src/stmem.h b/src/stmem.h index 9121ea64977..9281c6232c3 100644 --- a/src/stmem.h +++ b/src/stmem.h @@ -36,7 +36,8 @@ class mem_hdr mem_node *getBlockContainingLocation (int64_t location) const; /// switches the 'idleness' status of or all nodes void setIdleness(bool idle); - /// adjust idle nodes counter + /// Adjusts IdleNodes counter by the difference + /// between the current size() and oldSize. void updateIdleNodes(const size_t oldSize); /* access the contained nodes - easier than punning * as a container ourselves @@ -46,6 +47,9 @@ class mem_hdr static Splay::SPLAYCMP NodeCompare; + /// the total number of pages belonging to unlocked StoreEntries + static size_t IdleNodes; + private: void debugDump() const; bool unlink(mem_node *aNode); diff --git a/src/store.cc b/src/store.cc index 79b3e6d1d24..f4e88de331f 100644 --- a/src/store.cc +++ b/src/store.cc @@ -783,7 +783,7 @@ StoreEntry::writeData(StoreIOBuffer writeBuffer) { const auto oldSize = mem_obj->data_hdr.size(); mem_obj->write(writeBuffer); - // the entry may be still unlocked when it is (initially) copied from a shared storage + // the entry may be still unlocked when it is being copied from a shared storage if (!locked()) mem_obj->data_hdr.updateIdleNodes(oldSize); } diff --git a/src/store/Controller.cc b/src/store/Controller.cc index ea9e1b04487..aeb1248ac56 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -534,7 +534,7 @@ Store::Controller::freeMemorySpace(const int bytesRequired) if (memoryCacheHasSpaceFor(pagesRequired)) return; - if (mem_node::IdleNodes < static_cast(pagesRequired)) + if (mem_hdr::IdleNodes < static_cast(pagesRequired)) return; // XXX: When store_pages_max is smaller than pagesRequired, we should not From 4843b19a40cc99aa0604538d2c7370f58b072c4d Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 14 Mar 2023 20:56:06 +0300 Subject: [PATCH 08/12] Removed MemObject::write() We need to minimize the chance of calling mem_hdr::write() directly, i.e., bypassing updating mem_hdr::IdleNodes counter. The only caller now is StoreEntry::writeData(). Ideally, mem_hdr::write() should update IdleNodes itself, but but that would require passing StoreEntry pointer there (to check required conditions), which does not look as a good design. Also fixed test-suite tests. --- src/MemObject.cc | 13 ------------- src/MemObject.h | 3 --- src/Store.h | 3 +++ src/stmem.h | 3 ++- src/store.cc | 18 ++++++++++-------- src/tests/stub_MemObject.cc | 1 - test-suite/mem_hdr_test.cc | 4 ++-- 7 files changed, 17 insertions(+), 28 deletions(-) diff --git a/src/MemObject.cc b/src/MemObject.cc index 955ebc1f0a8..1195e59e564 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -132,19 +132,6 @@ MemObject::replaceBaseReply(const HttpReplyPointer &r) updatedReply_ = nullptr; } -void -MemObject::write(const StoreIOBuffer &writeBuffer) -{ - debugs(19, 6, "memWrite: offset " << writeBuffer.offset << " len " << writeBuffer.length); - - /* We don't separate out mime headers yet, so ensure that the first - * write is at offset 0 - where they start - */ - assert (data_hdr.endOffset() || writeBuffer.offset == 0); - - assert (data_hdr.write (writeBuffer)); -} - void MemObject::dump() const { diff --git a/src/MemObject.h b/src/MemObject.h index bb86659c4e9..8403d61eb51 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -52,9 +52,6 @@ class MemObject /// whether setUris() has been called bool hasUris() const; - /// \copydoc StoreEntry::writeData() - /// Do not call directly - use StoreEntry::writeData() instead. - void write(const StoreIOBuffer &buf); void unlinkRequest() { request = nullptr; } /// HTTP response before 304 (Not Modified) updates diff --git a/src/Store.h b/src/Store.h index 9d06dd30600..5f8496204c1 100644 --- a/src/Store.h +++ b/src/Store.h @@ -327,6 +327,9 @@ class StoreEntry : public hash_link, public Packable /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it void lengthWentBad(const char *reason); + /// whether methods such as lock()/unlock() also require mem_hdr::IdleNodes counter updating + bool needsIdlePagesUpdating() const { return !lock_count && mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL); } + static Mem::Allocator *pool; unsigned short lock_count; /* Assume < 65536! */ diff --git a/src/stmem.h b/src/stmem.h index 9281c6232c3..24b8379f501 100644 --- a/src/stmem.h +++ b/src/stmem.h @@ -29,7 +29,8 @@ class mem_hdr int64_t freeDataUpto (int64_t); ssize_t copy (StoreIOBuffer const &) const; bool hasContigousContentRange(Range const &range) const; - /* success or fail */ + /// Saves the buffer into the internal storage. + /// Do not call directly - use StoreEntry::writeData() instead. bool write (StoreIOBuffer const &); void dump() const; size_t size() const; diff --git a/src/store.cc b/src/store.cc index f4e88de331f..fd5f25a5eaf 100644 --- a/src/store.cc +++ b/src/store.cc @@ -418,7 +418,7 @@ StoreEntry::hashDelete() void StoreEntry::lock(const char *context) { - if (!lock_count && mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL)) + if (needsIdlePagesUpdating()) mem_obj->data_hdr.setIdleness(false); ++lock_count; debugs(20, 3, context << " locked key " << getMD5Text() << ' ' << *this); @@ -452,9 +452,8 @@ StoreEntry::unlock(const char *context) if (lock_count) return (int) lock_count; - if (mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL)) { + if (needsIdlePagesUpdating()) mem_obj->data_hdr.setIdleness(true); - } abandon(context); return 0; @@ -781,11 +780,14 @@ StoreEntry::write (StoreIOBuffer writeBuffer) void StoreEntry::writeData(StoreIOBuffer writeBuffer) { - const auto oldSize = mem_obj->data_hdr.size(); - mem_obj->write(writeBuffer); - // the entry may be still unlocked when it is being copied from a shared storage - if (!locked()) - mem_obj->data_hdr.updateIdleNodes(oldSize); + assert(mem_obj); + debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length); + auto &dataHdr = mem_obj->data_hdr; + assert (dataHdr.endOffset() || writeBuffer.offset == 0); + const auto oldSize = dataHdr.size(); + assert(dataHdr.write(writeBuffer)); + if (needsIdlePagesUpdating()) + dataHdr.updateIdleNodes(oldSize); } /* Append incoming data from a primary server to an entry. */ diff --git a/src/tests/stub_MemObject.cc b/src/tests/stub_MemObject.cc index 0a0a5311896..e0c4a1fce97 100644 --- a/src/tests/stub_MemObject.cc +++ b/src/tests/stub_MemObject.cc @@ -46,7 +46,6 @@ int MemObject::mostBytesWanted(int, bool) const STUB_RETVAL(-1) #if USE_DELAY_POOLS DelayId MemObject::mostBytesAllowed() const STUB_RETVAL(DelayId()) #endif -void MemObject::write(const StoreIOBuffer &) STUB int64_t MemObject::lowestMemReaderOffset() const STUB_RETVAL(0) void MemObject::kickReads() STUB int64_t MemObject::objectBytesOnDisk() const STUB_RETVAL(0) diff --git a/test-suite/mem_hdr_test.cc b/test-suite/mem_hdr_test.cc index 7f46c1aa6cb..fc801e78733 100644 --- a/test-suite/mem_hdr_test.cc +++ b/test-suite/mem_hdr_test.cc @@ -19,7 +19,7 @@ static void testLowAndHigh() { - mem_hdr aHeader; + mem_hdr aHeader(true); assert (aHeader.lowestOffset() == 0); assert (aHeader.write (StoreIOBuffer())); assert (aHeader.lowestOffset() == 0); @@ -74,7 +74,7 @@ testSplayOfNodes() static void testHdrVisit() { - mem_hdr aHeader; + mem_hdr aHeader(true); char * sampleData = xstrdup ("A"); assert (aHeader.write (StoreIOBuffer(1, 100, sampleData))); safe_free (sampleData); From 2340315944c4ab4dac5ef47667090fc19af8d6a0 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 20 Mar 2023 16:25:20 +0300 Subject: [PATCH 09/12] Adjusted the 'idle pages' scope These are unlocked pages that can be purged by the removal policy, i.e., by RemovalPurgeWalker. --- src/MemObject.cc | 3 ++- src/MemObject.h | 3 +-- src/Store.h | 3 +-- src/stmem.cc | 43 ++++++++++++++++++++++++++--------------- src/stmem.h | 17 +++++++--------- src/store.cc | 35 +++++++++++++++++++++++---------- src/store/Controller.cc | 2 +- 7 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/MemObject.cc b/src/MemObject.cc index 1195e59e564..0332258ec7c 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -94,12 +94,13 @@ MemObject::setUris(char const *aStoreId, char const *aLogUri, const HttpRequestM #endif } -MemObject::MemObject(const bool locked) : data_hdr(locked) +MemObject::MemObject() { debugs(20, 3, "MemObject constructed, this=" << this); ping_reply_callback = nullptr; memset(&start_ping, 0, sizeof(start_ping)); reply_ = new HttpReply; + assert(!repl.data); } MemObject::~MemObject() diff --git a/src/MemObject.h b/src/MemObject.h index 8403d61eb51..ceec8be17e6 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -37,8 +37,7 @@ class MemObject static size_t inUseCount(); void dump() const; - /// \param locked whether the associated StoreEntry is locked - MemObject(bool locked); + MemObject(); ~MemObject(); /// Sets store ID, log URI, and request method (unless already set). Does diff --git a/src/Store.h b/src/Store.h index 5f8496204c1..9580c065d2e 100644 --- a/src/Store.h +++ b/src/Store.h @@ -327,8 +327,7 @@ class StoreEntry : public hash_link, public Packable /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it void lengthWentBad(const char *reason); - /// whether methods such as lock()/unlock() also require mem_hdr::IdleNodes counter updating - bool needsIdlePagesUpdating() const { return !lock_count && mem_obj && !EBIT_TEST(flags, ENTRY_SPECIAL); } + bool hasReplPolicy() const; static Mem::Allocator *pool; diff --git a/src/stmem.cc b/src/stmem.cc index f8d43893d22..9c64f53b2f1 100644 --- a/src/stmem.cc +++ b/src/stmem.cc @@ -15,7 +15,7 @@ #include "MemObject.h" #include "stmem.h" -size_t mem_hdr::IdleNodes = 0; +size_t mem_hdr::ReplPolicyIdleNodesCount = 0; /* * NodeGet() is called to get the data buffer to pass to storeIOWrite(). @@ -308,7 +308,7 @@ mem_hdr::nodeToRecieve(int64_t offset) } bool -mem_hdr::write(const StoreIOBuffer &writeBuffer) +mem_hdr::write (StoreIOBuffer const &writeBuffer) { debugs(19, 6, "mem_hdr::write: " << this << " " << writeBuffer.range() << " object end " << endOffset()); @@ -326,6 +326,7 @@ mem_hdr::write(const StoreIOBuffer &writeBuffer) char *currentSource = writeBuffer.data; size_t len = writeBuffer.length; + const auto initialNodes = size(); while (len && (target = nodeToRecieve(currentOffset))) { size_t wrote = writeAvailable(target, currentOffset, len, currentSource); assert (wrote); @@ -333,11 +334,12 @@ mem_hdr::write(const StoreIOBuffer &writeBuffer) currentOffset += wrote; currentSource += wrote; } + updateIdleNodes(initialNodes); return true; } -mem_hdr::mem_hdr(const bool locked) : inmem_hi(0), isIdle(!locked) +mem_hdr::mem_hdr() : inmem_hi(0), removableByReplPolicy(false) { debugs(19, 9, this << " hi: " << inmem_hi); } @@ -383,30 +385,39 @@ mem_hdr::getNodes() const } void -mem_hdr::setIdleness(const bool idle) +mem_hdr::allowedToFreeWithReplPolicy(const bool allowed) { - assert(idle != isIdle); - isIdle = idle; - if (isIdle) { - IdleNodes += size(); - } else { - assert(IdleNodes >= size()); - IdleNodes -= size(); + if (removableByReplPolicy == allowed) + return; + removableByReplPolicy = allowed; + const auto oldCount = ReplPolicyIdleNodesCount; + if (removableByReplPolicy) + ReplPolicyIdleNodesCount += size(); + else { + assert(ReplPolicyIdleNodesCount >= size()); + ReplPolicyIdleNodesCount -= size(); } + debugs(19, 5, this << " modified ReplPolicyIdleNodesCount from " << oldCount << " to " << ReplPolicyIdleNodesCount); } +/// Adjusts the ReplPolicyIdleNodesCount counter by the difference +/// between the current size() and oldSize. void mem_hdr::updateIdleNodes(const size_t oldSize) { - if (!isIdle) + if (!removableByReplPolicy) return; const auto newSize = size(); const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize; + if (newSize == oldSize) + return; + const auto oldCount = ReplPolicyIdleNodesCount; if (newSize > oldSize) { - IdleNodes += delta; - } else if (newSize < oldSize) { - assert(IdleNodes >= delta); - IdleNodes -= delta; + ReplPolicyIdleNodesCount += delta; + } else { + assert(ReplPolicyIdleNodesCount >= delta); + ReplPolicyIdleNodesCount -= delta; } + debugs(19, 5, this << " updated ReplPolicyIdleNodesCount from " << oldCount << " to " << ReplPolicyIdleNodesCount); } diff --git a/src/stmem.h b/src/stmem.h index 24b8379f501..91758b727b7 100644 --- a/src/stmem.h +++ b/src/stmem.h @@ -20,8 +20,7 @@ class mem_hdr { public: - /// \param locked whether the associated StoreEntry is locked - mem_hdr(bool locked); + mem_hdr(); ~mem_hdr(); void freeContent(); int64_t lowestOffset () const; @@ -35,11 +34,7 @@ class mem_hdr void dump() const; size_t size() const; mem_node *getBlockContainingLocation (int64_t location) const; - /// switches the 'idleness' status of or all nodes - void setIdleness(bool idle); - /// Adjusts IdleNodes counter by the difference - /// between the current size() and oldSize. - void updateIdleNodes(const size_t oldSize); + void allowedToFreeWithReplPolicy(const bool allowed); /* access the contained nodes - easier than punning * as a container ourselves */ @@ -48,8 +43,9 @@ class mem_hdr static Splay::SPLAYCMP NodeCompare; - /// the total number of pages belonging to unlocked StoreEntries - static size_t IdleNodes; + /// the total number of pages that allowed to be purged by + /// the associated replacement policy + static size_t ReplPolicyIdleNodesCount; private: void debugDump() const; @@ -59,9 +55,10 @@ class mem_hdr bool unionNotEmpty (StoreIOBuffer const &); mem_node *nodeToRecieve(int64_t offset); size_t writeAvailable(mem_node *aNode, int64_t location, size_t amount, char const *source); + void updateIdleNodes(const size_t oldSize); int64_t inmem_hi; Splay nodes; - bool isIdle; ///< whether the associated pages belong to an unlocked StoreEntry + bool removableByReplPolicy; ///< whether nodes allowed to be purged by the associated replacement policy }; #endif /* SQUID_STMEM_H */ diff --git a/src/store.cc b/src/store.cc index fd5f25a5eaf..99b8979aeb6 100644 --- a/src/store.cc +++ b/src/store.cc @@ -418,8 +418,8 @@ StoreEntry::hashDelete() void StoreEntry::lock(const char *context) { - if (needsIdlePagesUpdating()) - mem_obj->data_hdr.setIdleness(false); + if (!lock_count && hasReplPolicy()) + mem_obj->data_hdr.allowedToFreeWithReplPolicy(false); ++lock_count; debugs(20, 3, context << " locked key " << getMD5Text() << ' ' << *this); } @@ -452,8 +452,8 @@ StoreEntry::unlock(const char *context) if (lock_count) return (int) lock_count; - if (needsIdlePagesUpdating()) - mem_obj->data_hdr.setIdleness(true); + if (hasReplPolicy()) + mem_obj->data_hdr.allowedToFreeWithReplPolicy(true); abandon(context); return 0; @@ -784,10 +784,7 @@ StoreEntry::writeData(StoreIOBuffer writeBuffer) debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length); auto &dataHdr = mem_obj->data_hdr; assert (dataHdr.endOffset() || writeBuffer.offset == 0); - const auto oldSize = dataHdr.size(); assert(dataHdr.write(writeBuffer)); - if (needsIdlePagesUpdating()) - dataHdr.updateIdleNodes(oldSize); } /* Append incoming data from a primary server to an entry. */ @@ -1531,6 +1528,8 @@ StoreEntry::setMemStatus(mem_status_t new_status) } else { mem_policy->Add(mem_policy, this, &mem_obj->repl); debugs(20, 4, "inserted " << *this << " key: " << getMD5Text()); + if (!locked()) + mem_obj->data_hdr.allowedToFreeWithReplPolicy(true); } ++hot_obj_count; // TODO: maintain for the shared hot cache as well @@ -1538,8 +1537,11 @@ StoreEntry::setMemStatus(mem_status_t new_status) if (EBIT_TEST(flags, ENTRY_SPECIAL)) { debugs(20, 4, "not removing special " << *this << " from policy"); } else { - mem_policy->Remove(mem_policy, this, &mem_obj->repl); - debugs(20, 4, "removed " << *this); + if (hasReplPolicy()) { + mem_obj->data_hdr.allowedToFreeWithReplPolicy(false); + mem_policy->Remove(mem_policy, this, &mem_obj->repl); + debugs(20, 4, "removed " << *this); + } } --hot_obj_count; @@ -1561,7 +1563,7 @@ void StoreEntry::createMemObject() { assert(!mem_obj); - mem_obj = new MemObject(locked()); + mem_obj = new MemObject(); } void @@ -1967,6 +1969,19 @@ StoreEntry::checkDisk() const } } +/// whether the entry was added to a memory replacement policy +bool +StoreEntry::hasReplPolicy() const +{ + if (!mem_obj) + return false; + if (mem_obj->repl.data) { + assert(!EBIT_TEST(flags, ENTRY_SPECIAL)); + return true; + } + return false; +} + /* * return true if the entry is in a state where * it can accept more data (ie with write() method) diff --git a/src/store/Controller.cc b/src/store/Controller.cc index aeb1248ac56..fa51773c5cc 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -534,7 +534,7 @@ Store::Controller::freeMemorySpace(const int bytesRequired) if (memoryCacheHasSpaceFor(pagesRequired)) return; - if (mem_hdr::IdleNodes < static_cast(pagesRequired)) + if (mem_hdr::ReplPolicyIdleNodesCount < static_cast(pagesRequired)) return; // XXX: When store_pages_max is smaller than pagesRequired, we should not From ec5917ed0efade3c17d2d7d2fb2d8257788838a4 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 20 Mar 2023 18:01:22 +0300 Subject: [PATCH 10/12] Fixed unit tests --- src/tests/stub_MemObject.cc | 2 +- src/tests/stub_stmem.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/stub_MemObject.cc b/src/tests/stub_MemObject.cc index e0c4a1fce97..2018d65d742 100644 --- a/src/tests/stub_MemObject.cc +++ b/src/tests/stub_MemObject.cc @@ -29,7 +29,7 @@ MemObject::endOffset() const void MemObject::trimSwappable() STUB void MemObject::trimUnSwappable() STUB int64_t MemObject::policyLowestOffsetToKeep(bool) const STUB_RETVAL(-1) -MemObject::MemObject(bool locked) : data_hdr(locked) { +MemObject::MemObject() { ping_reply_callback = nullptr; memset(&start_ping, 0, sizeof(start_ping)); } // NOP instead of elided due to Store diff --git a/src/tests/stub_stmem.cc b/src/tests/stub_stmem.cc index 5bc8f18b31a..ab329b5eb62 100644 --- a/src/tests/stub_stmem.cc +++ b/src/tests/stub_stmem.cc @@ -12,7 +12,7 @@ #define STUB_API "stmem.cc" #include "tests/STUB.h" -mem_hdr::mem_hdr(bool) STUB +mem_hdr::mem_hdr() STUB mem_hdr::~mem_hdr() STUB size_t mem_hdr::size() const STUB_RETVAL(0) int64_t mem_hdr::endOffset () const STUB_RETVAL(0) From 7820d21ef69fc399cbf3af5c12ba970a80b43af0 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Tue, 21 Mar 2023 19:41:01 +0300 Subject: [PATCH 11/12] Removed questionable (helper) methods and code duplication Also safe numeric operations with Less() and IncreaseSum(). --- src/Store.h | 3 --- src/stmem.cc | 41 +++++++++++++++++++------------------- src/store.cc | 21 +++++++------------ src/store/Controller.cc | 5 +++-- test-suite/mem_hdr_test.cc | 4 ++-- 5 files changed, 33 insertions(+), 41 deletions(-) diff --git a/src/Store.h b/src/Store.h index 9580c065d2e..6612fa6b5f3 100644 --- a/src/Store.h +++ b/src/Store.h @@ -72,9 +72,6 @@ class StoreEntry : public hash_link, public Packable bool isAccepting() const; size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; - /// whether a non-completed (STORE_PENDING) entry is being written to the local memory store - bool isLocalWriter() const { return locked() && isAccepting(); } - /// Signals that the entire response has been stored and no more append() /// calls should be expected; cf. completeTruncated(). void completeSuccessfully(const char *whyWeAreSureWeStoredTheWholeReply); diff --git a/src/stmem.cc b/src/stmem.cc index 9c64f53b2f1..193902b080c 100644 --- a/src/stmem.cc +++ b/src/stmem.cc @@ -13,6 +13,7 @@ #include "HttpReply.h" #include "mem_node.h" #include "MemObject.h" +#include "SquidMath.h" #include "stmem.h" size_t mem_hdr::ReplPolicyIdleNodesCount = 0; @@ -384,20 +385,31 @@ mem_hdr::getNodes() const return nodes; } +static void +UpdateIdleNodesCounter(const size_t oldSize, const size_t newSize) +{ + if (newSize == oldSize) + return; + const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize; + const auto oldCount = mem_hdr::ReplPolicyIdleNodesCount; + if (newSize > oldSize) { + mem_hdr::ReplPolicyIdleNodesCount = IncreaseSum(mem_hdr::ReplPolicyIdleNodesCount, delta).value(); + } else { + assert(mem_hdr::ReplPolicyIdleNodesCount >= delta); + mem_hdr::ReplPolicyIdleNodesCount -= delta; + } + debugs(19, 5, "Updated ReplPolicyIdleNodesCount from " << oldCount << " to " << mem_hdr::ReplPolicyIdleNodesCount); +} + void mem_hdr::allowedToFreeWithReplPolicy(const bool allowed) { if (removableByReplPolicy == allowed) return; removableByReplPolicy = allowed; - const auto oldCount = ReplPolicyIdleNodesCount; - if (removableByReplPolicy) - ReplPolicyIdleNodesCount += size(); - else { - assert(ReplPolicyIdleNodesCount >= size()); - ReplPolicyIdleNodesCount -= size(); - } - debugs(19, 5, this << " modified ReplPolicyIdleNodesCount from " << oldCount << " to " << ReplPolicyIdleNodesCount); + const auto oldSize = removableByReplPolicy ? 0 : size(); + const auto newSize = removableByReplPolicy ? size() : 0; + UpdateIdleNodesCounter(oldSize, newSize); } /// Adjusts the ReplPolicyIdleNodesCount counter by the difference @@ -407,17 +419,6 @@ mem_hdr::updateIdleNodes(const size_t oldSize) { if (!removableByReplPolicy) return; - const auto newSize = size(); - const auto delta = newSize > oldSize ? newSize - oldSize : oldSize - newSize; - if (newSize == oldSize) - return; - const auto oldCount = ReplPolicyIdleNodesCount; - if (newSize > oldSize) { - ReplPolicyIdleNodesCount += delta; - } else { - assert(ReplPolicyIdleNodesCount >= delta); - ReplPolicyIdleNodesCount -= delta; - } - debugs(19, 5, this << " updated ReplPolicyIdleNodesCount from " << oldCount << " to " << ReplPolicyIdleNodesCount); + UpdateIdleNodesCounter(oldSize, size()); } diff --git a/src/store.cc b/src/store.cc index 99b8979aeb6..dcebc2e49ae 100644 --- a/src/store.cc +++ b/src/store.cc @@ -418,7 +418,7 @@ StoreEntry::hashDelete() void StoreEntry::lock(const char *context) { - if (!lock_count && hasReplPolicy()) + if (!lock_count && mem_obj && mem_obj->repl.data) mem_obj->data_hdr.allowedToFreeWithReplPolicy(false); ++lock_count; debugs(20, 3, context << " locked key " << getMD5Text() << ' ' << *this); @@ -452,7 +452,7 @@ StoreEntry::unlock(const char *context) if (lock_count) return (int) lock_count; - if (hasReplPolicy()) + if (mem_obj && mem_obj->repl.data) mem_obj->data_hdr.allowedToFreeWithReplPolicy(true); abandon(context); @@ -760,7 +760,7 @@ StoreEntry::write (StoreIOBuffer writeBuffer) { assert(mem_obj != nullptr); /* This assert will change when we teach the store to update */ - assert(isLocalWriter()); + assert(locked()); // XXX: caller uses content offset, but we also store headers writeBuffer.offset += mem_obj->baseReply().hdr_sz; @@ -1537,11 +1537,10 @@ StoreEntry::setMemStatus(mem_status_t new_status) if (EBIT_TEST(flags, ENTRY_SPECIAL)) { debugs(20, 4, "not removing special " << *this << " from policy"); } else { - if (hasReplPolicy()) { + if (mem_obj->repl.data) mem_obj->data_hdr.allowedToFreeWithReplPolicy(false); - mem_policy->Remove(mem_policy, this, &mem_obj->repl); - debugs(20, 4, "removed " << *this); - } + mem_policy->Remove(mem_policy, this, &mem_obj->repl); + debugs(20, 4, "removed " << *this); } --hot_obj_count; @@ -1973,13 +1972,7 @@ StoreEntry::checkDisk() const bool StoreEntry::hasReplPolicy() const { - if (!mem_obj) - return false; - if (mem_obj->repl.data) { - assert(!EBIT_TEST(flags, ENTRY_SPECIAL)); - return true; - } - return false; + return mem_obj && mem_obj->repl.data; } /* diff --git a/src/store/Controller.cc b/src/store/Controller.cc index fa51773c5cc..865c06fd943 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -339,7 +339,7 @@ Store::Controller::checkFoundCandidate(const StoreEntry &entry) const // Transients do check when they setCollapsingRequirement(). } else { // a local writer must hold a lock on its writable entry - if (!(entry.isLocalWriter())) + if (!(entry.locked() && entry.isAccepting())) throw TextException("no local writer", Here()); } } @@ -534,7 +534,8 @@ Store::Controller::freeMemorySpace(const int bytesRequired) if (memoryCacheHasSpaceFor(pagesRequired)) return; - if (mem_hdr::ReplPolicyIdleNodesCount < static_cast(pagesRequired)) + // do not free anything if freeing everything freeable would not be enough + if (Less(mem_hdr::ReplPolicyIdleNodesCount, pagesRequired)) return; // XXX: When store_pages_max is smaller than pagesRequired, we should not diff --git a/test-suite/mem_hdr_test.cc b/test-suite/mem_hdr_test.cc index fc801e78733..7f46c1aa6cb 100644 --- a/test-suite/mem_hdr_test.cc +++ b/test-suite/mem_hdr_test.cc @@ -19,7 +19,7 @@ static void testLowAndHigh() { - mem_hdr aHeader(true); + mem_hdr aHeader; assert (aHeader.lowestOffset() == 0); assert (aHeader.write (StoreIOBuffer())); assert (aHeader.lowestOffset() == 0); @@ -74,7 +74,7 @@ testSplayOfNodes() static void testHdrVisit() { - mem_hdr aHeader(true); + mem_hdr aHeader; char * sampleData = xstrdup ("A"); assert (aHeader.write (StoreIOBuffer(1, 100, sampleData))); safe_free (sampleData); From 6423c13c43d33e9ce27fde40760b99d1c464e8b5 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Wed, 22 Mar 2023 16:46:47 +0300 Subject: [PATCH 12/12] Simplified, removing the 'mem_obj has memory policy' checks Also restored MemObject::write() and undone other write()-related refactoring. --- src/MemObject.cc | 13 +++++++++++++ src/MemObject.h | 1 + src/MemStore.cc | 2 +- src/Store.h | 7 ------- src/stmem.h | 3 +-- src/store.cc | 37 ++++++++++--------------------------- src/store_client.cc | 2 +- src/tests/stub_MemObject.cc | 1 + 8 files changed, 28 insertions(+), 38 deletions(-) diff --git a/src/MemObject.cc b/src/MemObject.cc index 0332258ec7c..b07dc3672e0 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -133,6 +133,19 @@ MemObject::replaceBaseReply(const HttpReplyPointer &r) updatedReply_ = nullptr; } +void +MemObject::write(const StoreIOBuffer &writeBuffer) +{ + debugs(19, 6, "memWrite: offset " << writeBuffer.offset << " len " << writeBuffer.length); + + /* We don't separate out mime headers yet, so ensure that the first + * write is at offset 0 - where they start + */ + assert (data_hdr.endOffset() || writeBuffer.offset == 0); + + assert (data_hdr.write (writeBuffer)); +} + void MemObject::dump() const { diff --git a/src/MemObject.h b/src/MemObject.h index ceec8be17e6..adc786e8208 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -51,6 +51,7 @@ class MemObject /// whether setUris() has been called bool hasUris() const; + void write(const StoreIOBuffer &buf); void unlinkRequest() { request = nullptr; } /// HTTP response before 304 (Not Modified) updates diff --git a/src/MemStore.cc b/src/MemStore.cc index 263cc55892d..44dc0dcb983 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -569,7 +569,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof) // local memory stores both headers and body so copy regardless of pstate const int64_t offBefore = e.mem_obj->endOffset(); - e.writeData(buf); + assert(e.mem_obj->data_hdr.write(buf)); // from MemObject::write() const int64_t offAfter = e.mem_obj->endOffset(); // expect to write the entire buf because StoreEntry::write() never fails assert(offAfter >= 0 && offBefore <= offAfter && diff --git a/src/Store.h b/src/Store.h index 6612fa6b5f3..3b74f07a6ef 100644 --- a/src/Store.h +++ b/src/Store.h @@ -56,11 +56,6 @@ class StoreEntry : public hash_link, public Packable /// \see MemObject::freshestReply() const HttpReply *hasFreshestReply() const { return mem_obj ? &mem_obj->freshestReply() : nullptr; } - /// writes to local memory store - void writeData(StoreIOBuffer); - - /// writeData() and calls awaiting handlers. - /// Should be called for non-completed (STORE_PENDING) entries only. void write(StoreIOBuffer); /** Check if the Store entry is empty @@ -324,8 +319,6 @@ class StoreEntry : public hash_link, public Packable /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it void lengthWentBad(const char *reason); - bool hasReplPolicy() const; - static Mem::Allocator *pool; unsigned short lock_count; /* Assume < 65536! */ diff --git a/src/stmem.h b/src/stmem.h index 91758b727b7..58d8bab15b8 100644 --- a/src/stmem.h +++ b/src/stmem.h @@ -28,8 +28,7 @@ class mem_hdr int64_t freeDataUpto (int64_t); ssize_t copy (StoreIOBuffer const &) const; bool hasContigousContentRange(Range const &range) const; - /// Saves the buffer into the internal storage. - /// Do not call directly - use StoreEntry::writeData() instead. + /* success or fail */ bool write (StoreIOBuffer const &); void dump() const; size_t size() const; diff --git a/src/store.cc b/src/store.cc index dcebc2e49ae..e3886ab3ad5 100644 --- a/src/store.cc +++ b/src/store.cc @@ -418,9 +418,9 @@ StoreEntry::hashDelete() void StoreEntry::lock(const char *context) { - if (!lock_count && mem_obj && mem_obj->repl.data) - mem_obj->data_hdr.allowedToFreeWithReplPolicy(false); ++lock_count; + if (mem_obj) + mem_obj->data_hdr.allowedToFreeWithReplPolicy(false); debugs(20, 3, context << " locked key " << getMD5Text() << ' ' << *this); } @@ -452,8 +452,8 @@ StoreEntry::unlock(const char *context) if (lock_count) return (int) lock_count; - if (mem_obj && mem_obj->repl.data) - mem_obj->data_hdr.allowedToFreeWithReplPolicy(true); + if (mem_obj) + mem_obj->data_hdr.allowedToFreeWithReplPolicy(mem_obj->repl.data); abandon(context); return 0; @@ -760,6 +760,7 @@ StoreEntry::write (StoreIOBuffer writeBuffer) { assert(mem_obj != nullptr); /* This assert will change when we teach the store to update */ + assert(store_status == STORE_PENDING); assert(locked()); // XXX: caller uses content offset, but we also store headers @@ -767,7 +768,7 @@ StoreEntry::write (StoreIOBuffer writeBuffer) debugs(20, 5, "storeWrite: writing " << writeBuffer.length << " bytes for '" << getMD5Text() << "'"); storeGetMemSpace(writeBuffer.length); - writeData(writeBuffer); + mem_obj->write(writeBuffer); if (EBIT_TEST(flags, ENTRY_FWD_HDR_WAIT) && !mem_obj->readAheadPolicyCanRead()) { debugs(20, 3, "allow Store clients to get entry content after buffering too much for " << *this); @@ -777,16 +778,6 @@ StoreEntry::write (StoreIOBuffer writeBuffer) invokeHandlers(); } -void -StoreEntry::writeData(StoreIOBuffer writeBuffer) -{ - assert(mem_obj); - debugs(20, 6, "offset " << writeBuffer.offset << " len " << writeBuffer.length); - auto &dataHdr = mem_obj->data_hdr; - assert (dataHdr.endOffset() || writeBuffer.offset == 0); - assert(dataHdr.write(writeBuffer)); -} - /* Append incoming data from a primary server to an entry. */ void StoreEntry::append(char const *buf, int len) @@ -1528,8 +1519,8 @@ StoreEntry::setMemStatus(mem_status_t new_status) } else { mem_policy->Add(mem_policy, this, &mem_obj->repl); debugs(20, 4, "inserted " << *this << " key: " << getMD5Text()); - if (!locked()) - mem_obj->data_hdr.allowedToFreeWithReplPolicy(true); + // only idle entries can be freed by the replacement policy + mem_obj->data_hdr.allowedToFreeWithReplPolicy(!locked()); } ++hot_obj_count; // TODO: maintain for the shared hot cache as well @@ -1537,8 +1528,7 @@ StoreEntry::setMemStatus(mem_status_t new_status) if (EBIT_TEST(flags, ENTRY_SPECIAL)) { debugs(20, 4, "not removing special " << *this << " from policy"); } else { - if (mem_obj->repl.data) - mem_obj->data_hdr.allowedToFreeWithReplPolicy(false); + mem_obj->data_hdr.allowedToFreeWithReplPolicy(false); mem_policy->Remove(mem_policy, this, &mem_obj->repl); debugs(20, 4, "removed " << *this); } @@ -1576,7 +1566,7 @@ void StoreEntry::ensureMemObject(const char *aUrl, const char *aLogUrl, const HttpRequestMethod &aMethod) { if (!mem_obj) - createMemObject(); + mem_obj = new MemObject(); mem_obj->setUris(aUrl, aLogUrl, aMethod); } @@ -1968,13 +1958,6 @@ StoreEntry::checkDisk() const } } -/// whether the entry was added to a memory replacement policy -bool -StoreEntry::hasReplPolicy() const -{ - return mem_obj && mem_obj->repl.data; -} - /* * return true if the entry is in a state where * it can accept more data (ie with write() method) diff --git a/src/store_client.cc b/src/store_client.cc index ec6e1a078e5..b213c89d340 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -539,7 +539,7 @@ store_client::readBody(const char *, ssize_t len) * copyInto.offset includes headers, which is what mem cache needs */ if (copyInto.offset == entry->mem_obj->endOffset()) { - entry->writeData(StoreIOBuffer(len, copyInto.offset, copyInto.data)); + entry->mem_obj->write(StoreIOBuffer(len, copyInto.offset, copyInto.data)); } } } diff --git a/src/tests/stub_MemObject.cc b/src/tests/stub_MemObject.cc index 2018d65d742..2be51b6ee56 100644 --- a/src/tests/stub_MemObject.cc +++ b/src/tests/stub_MemObject.cc @@ -46,6 +46,7 @@ int MemObject::mostBytesWanted(int, bool) const STUB_RETVAL(-1) #if USE_DELAY_POOLS DelayId MemObject::mostBytesAllowed() const STUB_RETVAL(DelayId()) #endif +void MemObject::write(const StoreIOBuffer &) STUB int64_t MemObject::lowestMemReaderOffset() const STUB_RETVAL(0) void MemObject::kickReads() STUB int64_t MemObject::objectBytesOnDisk() const STUB_RETVAL(0)