]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Tightened StoreEntry locking. Fixed entry touching and synchronization code:
authorAlex Rousskov <rousskov@measurement-factory.com>
Thu, 27 Jun 2013 21:26:57 +0000 (15:26 -0600)
committerAlex Rousskov <rousskov@measurement-factory.com>
Thu, 27 Jun 2013 21:26:57 +0000 (15:26 -0600)
Tightened StoreEntry locking code to use accessors instead of manipulating the
locking counter directly. Helps with locking bugs detection. Do not consider
STORE_PENDING and SWAPOUT_WRITING entries locked by default because it is
confusing and might even leave zero lock_count but locked() entries in the
global table. Entry users should lock them instead.

StoreController::get() is now the only place where we touch() a store entry.
We used to touch entries every time they were locked, which possibly did not
touch some entries often enough (e.g. during Vary mismatches and such where
the get() entry is discarded) and definitely touched some entries too often
(every time the entry was locked multiple times during the same master
transaction). This addresses a design bug marked RBC 20050104.

Fixed interpretation of IN_MEMORY status. The status means that the store
entry was, at some point, fully loaded into memory. And since we prohibit
trimming of IN_MEMORY entries, it should still be fully loaded. Collapsing
changes started to use IN_MEMORY for partially loaded entries, which helps
detecting entries associated with the [shared] memory cache, but goes against
old Squid code assumptions, triggering assertions.

Handle synchronization of entries the worker is writing. Normally, the writing
worker will not receive synchronization notifications (it will send them) but
a stale notification is possible and should not lead to asserts.  The worker
writing an entry will see a false mem_obj->smpCollapsed.

Do not re-anchor entries that were already anchored, fully loaded (ioDone),
and are now disassociated from the [shared] memory cache.

For shared caching to work reliably, StoreEntry::setReleaseFlag() should mark
cache entries for future release. We should not wait for release() time.
Waiting creates stuck entries because Squid sometimes changes the key from
public to private and collapsed forwarding broadcasts are incapable of
tracking such key changes (but they are capable of detecting entries abandoned
by their writers via the deletion mark in the transients table).

Polished debugging.

31 files changed:
src/MemStore.cc
src/MemStore.h
src/Server.cc
src/Store.h
src/StoreEntryStream.h
src/StoreSwapLogData.h
src/SwapDir.h
src/Transients.cc
src/Transients.h
src/acl/Asn.cc
src/client_side_reply.cc
src/client_side_request.cc
src/errorpage.cc
src/forward.cc
src/fs/rock/RockSwapDir.cc
src/fs/rock/RockSwapDir.h
src/fs/ufs/UFSSwapDir.cc
src/ftp.cc
src/gopher.cc
src/mgr/Forwarder.cc
src/neighbors.cc
src/peer_select.cc
src/ssl/ServerBump.cc
src/stat.cc
src/store.cc
src/store_client.cc
src/store_dir.cc
src/store_rebuild.cc
src/store_swapout.cc
src/urn.cc
src/whois.cc

index cc1aa0b57ddde2d83133f518f02b9ca9b817900b..c3b18ecc1fca3bc8ebcddeda5d39b36bcd9e45c5 100644 (file)
@@ -189,7 +189,6 @@ MemStore::get(const cache_key *key)
 
     // create a brand new store entry and initialize it with stored info
     StoreEntry *e = new StoreEntry();
-    e->lock_count = 0;
 
     // XXX: We do not know the URLs yet, only the key, but we need to parse and
     // store the response for the Root().get() callers to be happy because they
@@ -237,7 +236,6 @@ MemStore::anchorCollapsed(StoreEntry &collapsed, bool &inSync)
 bool
 MemStore::updateCollapsed(StoreEntry &collapsed)
 {
-    assert(collapsed.mem_status == IN_MEMORY);
     assert(collapsed.mem_obj);
 
     const sfileno index = collapsed.mem_obj->memCache.index; 
@@ -281,11 +279,12 @@ MemStore::anchorEntry(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnc
     if (anchor.complete()) {
         e.store_status = STORE_OK;
         e.mem_obj->object_sz = e.swap_file_sz;
+        e.setMemStatus(IN_MEMORY);
     } else {
         e.store_status = STORE_PENDING;
         assert(e.mem_obj->object_sz < 0);
+        e.setMemStatus(NOT_IN_MEMORY);
     }
-    e.setMemStatus(IN_MEMORY);
     assert(e.swap_status == SWAPOUT_NONE); // set in StoreEntry constructor
     e.ping_status = PING_NONE;
 
@@ -364,6 +363,7 @@ MemStore::copyFromShm(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnc
     // from StoreEntry::complete()
     e.mem_obj->object_sz = e.mem_obj->endOffset();
     e.store_status = STORE_OK;
+    e.setMemStatus(IN_MEMORY);
 
     assert(e.mem_obj->object_sz >= 0);
     assert(static_cast<uint64_t>(e.mem_obj->object_sz) == anchor.basics.swap_file_sz);
@@ -692,16 +692,23 @@ MemStore::completeWriting(StoreEntry &e)
     Store::Root().transientsCompleteWriting(e);
 }
 
+void
+MemStore::markForUnlink(StoreEntry &e)
+{
+    assert(e.mem_obj);
+    if (e.mem_obj->memCache.index >= 0)
+        map->freeEntry(e.mem_obj->memCache.index);
+}
+
 void
 MemStore::unlink(StoreEntry &e)
 {
-    assert(e.mem_status == IN_MEMORY);
     assert(e.mem_obj);
     if (e.mem_obj->memCache.index >= 0) {
         map->freeEntry(e.mem_obj->memCache.index);
         disconnect(*e.mem_obj);
     } else {
-        // the entry was loaded and then disconnected from the memory cache
+        // the entry may bave been loaded and then disconnected from the cache
         map->freeEntryByKey(reinterpret_cast<cache_key*>(e.key));
     }
         
index c875095ca19c4e977a6d9f4a9d5b003c09f78252..527ae13030ae30f274a9a38209512d968d7cb3ef 100644 (file)
@@ -48,6 +48,7 @@ public:
     virtual void getStats(StoreInfoStats &stats) const;
     virtual void stat(StoreEntry &) const;
     virtual StoreSearch *search(String const url, HttpRequest *);
+    virtual void markForUnlink(StoreEntry &e);
     virtual void reference(StoreEntry &);
     virtual bool dereference(StoreEntry &, bool);
     virtual void maintain();
index 41d4a36c32bc8f9013c86148e282efcf19a90918..444311f8e807f3487d787f7c923b6e37578e6373 100644 (file)
@@ -73,7 +73,7 @@ ServerStateData::ServerStateData(FwdState *theFwdState): AsyncJob("ServerStateDa
     fwd = theFwdState;
     entry = fwd->entry;
 
-    entry->lock();
+    entry->lock("ServerStateData");
 
     request = fwd->request;
     HTTPMSGLOCK(request);
@@ -88,7 +88,7 @@ ServerStateData::~ServerStateData()
     assert(!adaptedBodySource);
 #endif
 
-    entry->unlock();
+    entry->unlock("ServerStateData");
 
     HTTPMSGUNLOCK(request);
     HTTPMSGUNLOCK(theVirginReply);
index e689680f3dd0c728ff67bd5b9cf4f2ffa87f188d..446894d3b3442f967e129cc7337ac0d3af8c9e64 100644 (file)
@@ -165,8 +165,6 @@ public:
 
     sdirno swap_dirn:7;
 
-    unsigned short lock_count;         /* Assume < 65536! */
-
     mem_status_t mem_status:3;
 
     ping_status_t ping_status:3;
@@ -201,21 +199,16 @@ public:
     virtual int64_t objectLen() const;
     virtual int64_t contentLen() const;
 
-    /** deprecated: lock() in anonymous context folowed by touch()
-     * RBC 20050104 this is wrong- memory ref counting
-     * is not at all equivalent to the store 'usage' concept
-     * which the replacement policies should be acting upon.
-     * specifically, object iteration within stores needs
-     * memory ref counting to prevent race conditions,
-     * but this should not influence store replacement.
-     */
-    void lock() { lock("somebody"); touch(); }
-
     /// claim shared ownership of this entry (for use in a given context)
     void lock(const char *context);
+
     /// disclaim shared ownership; may remove entry from store and delete it
     /// returns remaning lock level (zero for unlocked and possibly gone entry)
     int unlock(const char *context = "somebody");
+
+    /// returns a local concurrent use counter, for debugging
+    int locks() const { return static_cast<int>(lock_count); }
+
     /// update last reference timestamp and related Store metadata
     void touch();
 
@@ -231,6 +224,8 @@ public:
 private:
     static MemAllocator *pool;
 
+    unsigned short lock_count;         /* Assume < 65536! */
+
 #if USE_ADAPTATION
     /// producer callback registered with deferProducer
     AsyncCall::Pointer deferredProducer;
@@ -362,6 +357,11 @@ public:
 
     virtual void maintain() = 0; /* perform regular maintenance should be private and self registered ... */
 
+    // XXX: This method belongs to Store::Root/StoreController, but it is here
+    // to avoid casting Root() to StoreController until Root() API is fixed.
+    /// informs stores that this entry will be eventually unlinked
+    virtual void markForUnlink(StoreEntry &e) {}
+
     // XXX: This method belongs to Store::Root/StoreController, but it is here
     // because test cases use non-StoreController derivatives as Root
     /// called when the entry is no longer needed by any transaction
@@ -445,8 +445,13 @@ StoreEntry *storeGetPublicByRequest(HttpRequest * request);
 StoreEntry *storeGetPublicByRequestMethod(HttpRequest * request, const HttpRequestMethod& method);
 
 /// \ingroup StoreAPI
+/// Like storeCreatePureEntry(), but also locks the entry and sets entry key.
 StoreEntry *storeCreateEntry(const char *, const char *, const RequestFlags &, const HttpRequestMethod&);
 
+/// \ingroup StoreAPI
+/// Creates a new StoreEntry with mem_obj and sets initial flags/states.
+StoreEntry *storeCreatePureEntry(const char *storeId, const char *logUrl, const RequestFlags &, const HttpRequestMethod&);
+
 /// \ingroup StoreAPI
 void storeInit(void);
 
index 3f9732d5df195987b39cb92c89fa3e96d8d08bdb..880405a41a421dbf7025fce24f15f5c085db38de 100644 (file)
@@ -48,13 +48,12 @@ class StoreEntryStreamBuf : public std::streambuf
 
 public:
     StoreEntryStreamBuf(StoreEntry *anEntry) : theEntry(anEntry) {
-
-        theEntry->lock();
+        theEntry->lock("StoreEntryStreamBuf");
         theEntry->buffer();
     }
 
     ~StoreEntryStreamBuf() {
-        theEntry->unlock();
+        theEntry->unlock("StoreEntryStreamBuf");
     }
 
 protected:
index 8a58046ab0d655bac442b853d52411dc5c414159..9cd86a9998c1e166d28dd930656c82932fc7c95c 100644 (file)
@@ -152,8 +152,6 @@ public:
 
     /**
      * The last time that a client requested this object.
-     * Strictly speaking, this time is set whenever the StoreEntry
-     * is locked (via storeLockObject()).
      */
     SwappedTime lastref;
 
index 68fe0beeb29d8152a7d7f26c2037c13b352f1a3e..fa08136f05a67b9faa6bbefec392809a4c8d781e 100644 (file)
@@ -61,6 +61,7 @@ public:
     virtual void get(String const, STOREGETCLIENT, void * cbdata);
 
     /* Store parent API */
+    virtual void markForUnlink(StoreEntry &e);
     virtual void handleIdleEntry(StoreEntry &e);
     virtual void transientsCompleteWriting(StoreEntry &e);
     virtual void transientsAbandon(StoreEntry &e);
@@ -102,6 +103,7 @@ public:
 
 private:
     void createOneStore(Store &aStore);
+    StoreEntry *find(const cache_key *key);
     bool keepForLocalMemoryCache(const StoreEntry &e) const;
     bool anchorCollapsed(StoreEntry &collapsed, bool &inSync);
     bool anchorCollapsedOnDisk(StoreEntry &collapsed, bool &inSync);
index c6967b84092bd2d4a353228b70fde5c089158a82..10a2480ec3c196c749d60e5a4a34534f3bde5544 100644 (file)
@@ -4,6 +4,7 @@
  */
 
 #include "squid.h"
+#include "CollapsedForwarding.h" /* XXX: who should broadcast and when? */
 #include "base/RunnersRegistry.h"
 #include "HttpReply.h"
 #include "ipc/mem/Page.h"
@@ -152,13 +153,19 @@ Transients::get(const cache_key *key)
         return NULL;
 
     sfileno index;
-    if (!map->openForReading(key, index))
+    const Ipc::StoreMapAnchor *anchor = map->openForReading(key, index);
+    if (!anchor)
         return NULL;
 
-    if (StoreEntry *e = copyFromShm(index))
+    // Without a writer, either the response has been cached already or we will
+    // get stuck waiting for it to be cached (because nobody will cache it).
+    if (!anchor->writing()) {
+        debugs(20, 5, "ignoring writer-less entry " << index);
+       } else if (StoreEntry *e = copyFromShm(index)) {
         return e; // keep read lock to receive updates from others
+    }
 
-    // loading failure
+    // missing writer or loading failure
     map->closeForReading(index);
     return NULL;
 }
@@ -169,20 +176,14 @@ Transients::copyFromShm(const sfileno index)
     const TransientsMap::Extras &extras = map->extras(index);
 
     // create a brand new store entry and initialize it with stored info
-    StoreEntry *e = storeCreateEntry(extras.url, extras.url,
+    StoreEntry *e = storeCreatePureEntry(extras.url, extras.url,
                                      extras.reqFlags, extras.reqMethod);
-    // XXX: overwriting storeCreateEntry() because we are expected to return an unlocked entry
-    // TODO: move locking from storeCreateEntry to callers as a mid-term solution
-    e->lock_count = 0;
 
     assert(e->mem_obj);
     e->mem_obj->method = extras.reqMethod;
     e->mem_obj->xitTable.io = MemObject::ioReading;
     e->mem_obj->xitTable.index = index;
 
-    // XXX: overwriting storeCreateEntry() which calls setPrivateKey() if
-    // neighbors_do_private_keys (which is true in most cases and by default).
-    // This is nothing but waste of CPU cycles. Need a better API to avoid it.
     e->setPublicKey();
     assert(e->key);
 
@@ -275,6 +276,7 @@ Transients::abandon(const StoreEntry &e)
 {
     assert(e.mem_obj && map);
     map->freeEntry(e.mem_obj->xitTable.index); // just marks the locked entry
+    CollapsedForwarding::Broadcast(e);
     // We do not unlock the entry now because the problem is most likely with
     // the server resource rather than a specific cache writer, so we want to
     // prevent other readers from collapsing requests for that resource.
@@ -316,6 +318,13 @@ Transients::readers(const StoreEntry &e) const
     return 0;
 }
 
+void
+Transients::markForUnlink(StoreEntry &e)
+{
+    if (e.mem_obj && e.mem_obj->xitTable.io == MemObject::ioWriting)
+        abandon(e);
+}
+
 void
 Transients::disconnect(MemObject &mem_obj)
 {
index cbe2138f6ce205937934b78bb9b158fb6f24d2e2..0204853015f7feda1e5fada25bdb16f796ac2de9 100644 (file)
@@ -57,6 +57,7 @@ public:
     virtual StoreSearch *search(String const url, HttpRequest *);
     virtual void reference(StoreEntry &);
     virtual bool dereference(StoreEntry &, bool);
+    virtual void markForUnlink(StoreEntry &e);
     virtual void maintain();
 
     static int64_t EntryLimit();
index 43fe185973c94158cd8ff22f5846b7462819223e..a13ee13a6211467b58417b11aaf94bf0122a190b 100644 (file)
@@ -252,8 +252,7 @@ asnCacheStart(int as)
         asState->sc = storeClientListAdd(e, asState);
         FwdState::fwdStart(Comm::ConnectionPointer(), e, asState->request);
     } else {
-
-        e->lock();
+        e->lock("Asn");
         asState->sc = storeClientListAdd(e, asState);
     }
 
@@ -383,7 +382,7 @@ asStateFree(void *data)
     ASState *asState = (ASState *)data;
     debugs(53, 3, "asnStateFree: " << asState->entry->url()  );
     storeUnregister(asState->sc, asState->entry, asState);
-    asState->entry->unlock();
+    asState->entry->unlock("Asn");
     HTTPMSGUNLOCK(asState->request);
     cbdataFree(asState);
 }
index 8e4c4e10257fb412457b9022d86e921e81a999b8..e353c73a9e3d47a926bc883ef6ce4ad49090f582 100644 (file)
@@ -141,7 +141,7 @@ void clientReplyContext::setReplyToError(const HttpRequestMethod& method, ErrorS
 
 void clientReplyContext::setReplyToStoreEntry(StoreEntry *entry)
 {
-    entry->lock(); // removeClientStoreReference() unlocks
+    entry->lock("clientReplyContext::setReplyToStoreEntry"); // removeClientStoreReference() unlocks
     sc = storeClientListAdd(entry, this);
 #if USE_DELAY_POOLS
     sc->setDelayId(DelayId::DelayClient(http));
@@ -163,7 +163,7 @@ clientReplyContext::removeStoreReference(store_client ** scp,
         *ep = NULL;
         storeUnregister(sc_tmp, e, this);
         *scp = NULL;
-        e->unlock();
+        e->unlock("clientReplyContext::removeStoreReference");
     }
 }
 
@@ -802,7 +802,7 @@ purgeEntriesByUrl(HttpRequest * req, const char *url)
     for (HttpRequestMethod m(Http::METHOD_NONE); m != Http::METHOD_ENUM_END; ++m) {
         if (m.respMaybeCacheable()) {
             if (StoreEntry *entry = storeGetPublic(url, m)) {
-                debugs(88, 5, "purging " << RequestMethodStr(m) << ' ' << url);
+                debugs(88, 5, "purging " << *entry << ' ' << RequestMethodStr(m) << ' ' << url);
 #if USE_HTCP
                 neighborsHtcpClear(entry, url, req, m, HTCP_CLR_INVALIDATION);
                 if (m == Http::METHOD_GET || m == Http::METHOD_HEAD) {
@@ -879,7 +879,7 @@ clientReplyContext::purgeFoundObject(StoreEntry *entry)
     /* Swap in the metadata */
     http->storeEntry(entry);
 
-    http->storeEntry()->lock();
+    http->storeEntry()->lock("clientReplyContext::purgeFoundObject");
     http->storeEntry()->createMemObject(storeId(), http->log_uri,
                                         http->request->method);
 
@@ -1182,7 +1182,7 @@ clientReplyContext::replyStatus()
     }
 
     if ((done = checkTransferDone()) != 0 || flags.complete) {
-        debugs(88, 5, "clientReplyStatus: transfer is DONE");
+        debugs(88, 5, "clientReplyStatus: transfer is DONE: " << done << flags.complete);
         /* Ok we're finished, but how? */
 
         const int64_t expectedBodySize =
@@ -1552,9 +1552,9 @@ clientReplyContext::forgetHit()
     // now so that we can unlock two lines later (and trigger cleanup).
     // Ideally, ClientHttpRequest::storeEntry() should lock/unlock, but it is
     // used so inconsistently that simply adding locking there leads to bugs.
-    e->lock();
+    e->lock("clientReplyContext::forgetHit");
     http->storeEntry(NULL);
-    e->unlock(); // may delete e (and release resources associated with it)
+    e->unlock("clientReplyContext::forgetHit"); // may delete e
 }
 
 void
@@ -1736,7 +1736,7 @@ clientReplyContext::doGetMoreData()
         /* someone found the object in the cache for us */
         StoreIOBuffer localTempBuffer;
 
-        http->storeEntry()->lock();
+        http->storeEntry()->lock("clientReplyContext::doGetMoreData");
 
         MemObject *mem_obj = http->storeEntry()->makeMemObject();
         if (!mem_obj->hasUris()) {
index 88d535660a3252bb5bb775b02a168ae9a6f9c913..d372ef42cad789fe75ac25fdc51507b66d433efe 100644 (file)
@@ -1655,12 +1655,12 @@ void
 ClientHttpRequest::loggingEntry(StoreEntry *newEntry)
 {
     if (loggingEntry_)
-        loggingEntry_->unlock();
+        loggingEntry_->unlock("ClientHttpRequest::loggingEntry");
 
     loggingEntry_ = newEntry;
 
     if (loggingEntry_)
-        loggingEntry_->lock();
+        loggingEntry_->lock("ClientHttpRequest::loggingEntry");
 }
 
 /*
@@ -1828,7 +1828,7 @@ ClientHttpRequest::doCallouts()
             errorAppendEntry(e, calloutContext->error);
             calloutContext->error = NULL;
             getConn()->setServerBump(srvBump);
-            e->unlock();
+            e->unlock("ClientHttpRequest::doCallouts+sslBumpNeeded");
         } else
 #endif
         {
@@ -1843,7 +1843,7 @@ ClientHttpRequest::doCallouts()
                 getConn()->flags.readMore = true; // resume any pipeline reads.
             node = (clientStreamNode *)client_stream.tail->data;
             clientStreamRead(node, this, node->readBuffer);
-            e->unlock();
+            e->unlock("ClientHttpRequest::doCallouts-sslBumpNeeded");
             return;
         }
     }
index 1886342f6ded166bb5cee1f274d1206de2d6286c..dfeb9a321194d3094a1442babf1c9cb54eff31a4 100644 (file)
@@ -634,14 +634,14 @@ errorAppendEntry(StoreEntry * entry, ErrorState * err)
         }
     }
 
-    entry->lock();
+    entry->lock("errorAppendEntry");
     entry->buffer();
     entry->replaceHttpReply( err->BuildHttpReply() );
     entry->flush();
     entry->complete();
     entry->negativeCache();
     entry->releaseRequest();
-    entry->unlock();
+    entry->unlock("errorAppendEntry");
     delete err;
 }
 
index d872d347c413aaefc6ea78c4b14936d3c19fe251..82671cc217e3fb6137c60ef6773947ec7ceac969 100644 (file)
@@ -129,7 +129,7 @@ FwdState::FwdState(const Comm::ConnectionPointer &client, StoreEntry * e, HttpRe
     pconnRace = raceImpossible;
     start_t = squid_curtime;
     serverDestinations.reserve(Config.forward_max_tries);
-    e->lock();
+    e->lock("FwdState");
     EBIT_SET(e->flags, ENTRY_FWD_HDR_WAIT);
 }
 
@@ -262,7 +262,7 @@ FwdState::~FwdState()
 
     entry->unregisterAbort();
 
-    entry->unlock();
+    entry->unlock("FwdState");
 
     entry = NULL;
 
@@ -1232,7 +1232,7 @@ FwdState::dispatch()
     /*assert(!EBIT_TEST(entry->flags, ENTRY_DISPATCHED)); */
     assert(entry->ping_status != PING_WAITING);
 
-    assert(entry->lock_count);
+    assert(entry->locked());
 
     EBIT_SET(entry->flags, ENTRY_DISPATCHED);
 
index 60f41c7271b6ceccb581b14a1334061ba5206a20..a992b17d6bd71addc536a2243403ac57a75eb7ab 100644 (file)
@@ -71,7 +71,6 @@ Rock::SwapDir::get(const cache_key *key)
 
     // create a brand new store entry and initialize it with stored basics
     StoreEntry *e = new StoreEntry();
-    e->lock_count = 0;
     anchorEntry(*e, filen, *slot);
 
     e->hashInsert(key);
@@ -896,6 +895,13 @@ Rock::SwapDir::unlink(StoreEntry &e)
     disconnect(e);
 }
 
+void
+Rock::SwapDir::markForUnlink(StoreEntry &e)
+{
+    debugs(47, 5, e);
+    map->freeEntry(e.swap_filen);
+}
+
 void
 Rock::SwapDir::trackReferences(StoreEntry &e)
 {
index a9eb2a26fd15b2c9ae242927694799be47d18089..1a77a2085551e659d278ae1b10bbf77b87904724 100644 (file)
@@ -32,6 +32,7 @@ public:
     virtual StoreSearch *search(String const url, HttpRequest *);
     virtual StoreEntry *get(const cache_key *key);
     virtual void get(String const, STOREGETCLIENT, void * cbdata);
+    virtual void markForUnlink(StoreEntry &e);
     virtual void disconnect(StoreEntry &e);
     virtual uint64_t currentSize() const;
     virtual uint64_t currentCount() const;
index 9a1094d7dd9829ce8d73f32dfe9e0512927b93f2..599b8ad2be92cae13e80659129fd668f0b209d9e 100644 (file)
@@ -754,7 +754,6 @@ Fs::Ufs::UFSSwapDir::addDiskRestore(const cache_key * key,
     e->swap_filen = file_number;
     e->swap_dirn = index;
     e->swap_file_sz = swap_file_sz;
-    e->lock_count = 0;
     e->lastref = lastref;
     e->timestamp = timestamp;
     e->expires = expires;
index e031886aeee170a6d757ed461688ed8e4c7f81ba..757db4e01525329117bb9a3f4ee549d1b6a2653a 100644 (file)
@@ -3277,7 +3277,7 @@ void
 FtpStateData::completedListing()
 {
     assert(entry);
-    entry->lock();
+    entry->lock("FtpStateData");
     ErrorState ferr(ERR_DIR_LISTING, Http::scOkay, request);
     ferr.ftp.listing = &listing;
     ferr.ftp.cwd_msg = xstrdup(cwd_message.size()? cwd_message.termedBuf() : "");
@@ -3286,7 +3286,7 @@ FtpStateData::completedListing()
     entry->replaceHttpReply( ferr.BuildHttpReply() );
     EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
     entry->flush();
-    entry->unlock();
+    entry->unlock("FtpStateData");
 }
 
 /// \ingroup ServerProtocolFTPInternal
index bd1df5112e359af6e7ab8216965d1b2565a46f88..63241914ff1da1cf4b9de3f28202527025897cea 100644 (file)
@@ -173,7 +173,7 @@ gopherStateFree(const CommCloseCbParams &params)
         return;
 
     if (gopherState->entry) {
-        gopherState->entry->unlock();
+        gopherState->entry->unlock("gopherState");
     }
 
     HTTPMSGUNLOCK(gopherState->req);
@@ -962,7 +962,7 @@ gopherStart(FwdState * fwd)
     gopherState = cbdataAlloc(GopherStateData);
     gopherState->buf = (char *)memAllocate(MEM_4K_BUF);
 
-    entry->lock();
+    entry->lock("gopherState");
     gopherState->entry = entry;
 
     gopherState->fwd = fwd;
index d7a754dcfd52753cac3e4351fc18850a6bd99537..33481e99707ad66d48a171a50bfc88050300eb5f 100644 (file)
@@ -31,7 +31,7 @@ Mgr::Forwarder::Forwarder(const Comm::ConnectionPointer &aConn, const ActionPara
     Must(entry != NULL);
 
     HTTPMSGLOCK(httpRequest);
-    entry->lock();
+    entry->lock("Mgr::Forwarder");
     EBIT_SET(entry->flags, ENTRY_FWD_HDR_WAIT);
 
     closer = asyncCall(16, 5, "Mgr::Forwarder::noteCommClosed",
@@ -47,7 +47,7 @@ Mgr::Forwarder::~Forwarder()
 
     HTTPMSGUNLOCK(httpRequest);
     entry->unregisterAbort();
-    entry->unlock();
+    entry->unlock("Mgr::Forwarder");
     cleanup();
 }
 
index da99ce3df60b80c9aa32c76046e42c5a62e26957..562efc657e51bd0044ddb397174c04cbea5d807a 100644 (file)
@@ -1027,7 +1027,7 @@ neighborsUdpAck(const cache_key * key, icp_common_t * header, const Ip::Address
         return;
     }
 
-    if (entry->lock_count == 0) {
+    if (!entry->locked()) {
         // TODO: many entries are unlocked; why is this reported at level 1?
         debugs(12, DBG_IMPORTANT, "neighborsUdpAck: '" << storeKeyText(key) << "' has no locks");
         neighborCountIgnored(p);
@@ -1733,7 +1733,7 @@ neighborsHtcpReply(const cache_key * key, HtcpReplyData * htcp, const Ip::Addres
         return;
     }
 
-    if (e->lock_count == 0) {
+    if (!e->locked()) {
         // TODO: many entries are unlocked; why is this reported at level 1?
         debugs(12, DBG_IMPORTANT, "neighborsUdpAck: '" << storeKeyText(key) << "' has no locks");
         neighborCountIgnored(p);
index 897a06fe67166d4692139cae1a8784eac3c6db30..258960edab04dda886d3c1d3c7f5af36c7ee0245 100644 (file)
@@ -110,7 +110,7 @@ peerSelectStateFree(ps_state * psstate)
 
     if (psstate->entry) {
         assert(psstate->entry->ping_status != PING_WAITING);
-        psstate->entry->unlock();
+        psstate->entry->unlock("peerSelect");
         psstate->entry = NULL;
     }
 
@@ -175,7 +175,7 @@ peerSelect(Comm::ConnectionList * paths,
 #endif
 
     if (psstate->entry)
-        psstate->entry->lock();
+        psstate->entry->lock("peerSelect");
 
     peerSelectFoo(psstate);
 }
index 38e7234092a04412e6bd3489e3b1fdd12eb1cd01..adaa640baea6d925d4216365fde146df8095eac6 100644 (file)
@@ -22,7 +22,7 @@ Ssl::ServerBump::ServerBump(HttpRequest *fakeRequest, StoreEntry *e):
     const char *uri = urlCanonical(request.getRaw());
     if (e) {
         entry = e;
-        entry->lock();
+        entry->lock("Ssl::ServerBump");
     } else
         entry = storeCreateEntry(uri, uri, request->flags, request->method);
     // We do not need to be a client because the error contents will be used
@@ -36,7 +36,7 @@ Ssl::ServerBump::~ServerBump()
     if (entry) {
         debugs(33, 4, HERE << *entry);
         storeUnregister(sc, entry, this);
-        entry->unlock();
+        entry->unlock("Ssl::ServerBump");
     }
     cbdataReferenceDone(sslErrors);
 }
index db19c90ccc11b5d240c3e6fc066332273efbe28f..715bef8310735c9ec53628d6af832ea9f9d872ce 100644 (file)
@@ -371,7 +371,7 @@ statStoreEntry(MemBuf * mb, StoreEntry * e)
     mb->Printf("\t%s\n", storeEntryFlags(e));
     mb->Printf("\t%s\n", describeTimestamps(e));
     mb->Printf("\t%d locks, %d clients, %d refs\n",
-               (int) e->lock_count,
+               (int) e->locks(),
                storePendingNClients(e),
                (int) e->refcount);
     mb->Printf("\tSwap Dir %d, File %#08X\n",
@@ -394,11 +394,11 @@ statObjects(void *data)
         if (UsingSmp())
             storeAppendPrintf(state->sentry, "} by kid%d\n\n", KidIdentifier);
         state->sentry->complete();
-        state->sentry->unlock();
+        state->sentry->unlock("statObjects+isDone");
         cbdataFree(state);
         return;
     } else if (EBIT_TEST(state->sentry->flags, ENTRY_ABORTED)) {
-        state->sentry->unlock();
+        state->sentry->unlock("statObjects+aborted");
         cbdataFree(state);
         return;
     } else if (state->sentry->checkDeferRead(-1)) {
@@ -436,7 +436,7 @@ statObjectsStart(StoreEntry * sentry, STOBJFLT * filter)
     state->sentry = sentry;
     state->filter = filter;
 
-    sentry->lock();
+    sentry->lock("statObjects");
     state->theSearch = Store::Root().search(NULL, NULL);
 
     eventAdd("statObjects", statObjects, state, 0.0, 1);
index d4bbef8d9ebf13264c811964daea7f48fa53a413..35f483f9970f4128cc72b89a610ba9874eec6916 100644 (file)
@@ -389,11 +389,11 @@ StoreEntry::StoreEntry() :
         flags(0),
         swap_filen(-1),
         swap_dirn(-1),
-        lock_count(0),
         mem_status(NOT_IN_MEMORY),
         ping_status(PING_NONE),
         store_status(STORE_PENDING),
-        swap_status(SWAPOUT_NONE)
+        swap_status(SWAPOUT_NONE),
+        lock_count(0)
 {
     debugs(20, 3, HERE << "new StoreEntry " << this);
 }
@@ -513,6 +513,8 @@ StoreEntry::setReleaseFlag()
     debugs(20, 3, "StoreEntry::setReleaseFlag: '" << getMD5Text() << "'");
 
     EBIT_SET(flags, RELEASE_REQUEST);
+
+    Store::Root().markForUnlink(*this);
 }
 
 void
@@ -756,7 +758,7 @@ StoreEntry::setPublicKey()
 
             pe->complete();
 
-            pe->unlock();
+            pe->unlock("StoreEntry::setPublicKey+Vary");
         }
 
         newkey = storeKeyPublicByRequest(mem_obj->request);
@@ -786,21 +788,15 @@ StoreEntry::setPublicKey()
 }
 
 StoreEntry *
-storeCreateEntry(const char *url, const char *log_url, const RequestFlags &flags, const HttpRequestMethod& method)
+storeCreatePureEntry(const char *url, const char *log_url, const RequestFlags &flags, const HttpRequestMethod& method)
 {
     StoreEntry *e = NULL;
     debugs(20, 3, "storeCreateEntry: '" << url << "'");
 
     e = new StoreEntry();
-    e->lock_count = 1;          /* Note lock here w/o calling storeLock() */
     e->makeMemObject();
     e->mem_obj->setUris(url, log_url, method);
 
-    if (neighbors_do_private_keys || !flags.hierarchical)
-        e->setPrivateKey();
-    else
-        e->setPublicKey();
-
     if (flags.cachable) {
         EBIT_SET(e->flags, ENTRY_CACHABLE);
         EBIT_CLR(e->flags, RELEASE_REQUEST);
@@ -810,7 +806,6 @@ storeCreateEntry(const char *url, const char *log_url, const RequestFlags &flags
     }
 
     e->store_status = STORE_PENDING;
-    e->setMemStatus(NOT_IN_MEMORY);
     e->refcount = 0;
     e->lastref = squid_curtime;
     e->timestamp = -1;          /* set in StoreEntry::timestampsSet() */
@@ -819,6 +814,20 @@ storeCreateEntry(const char *url, const char *log_url, const RequestFlags &flags
     return e;
 }
 
+StoreEntry *
+storeCreateEntry(const char *url, const char *logUrl, const RequestFlags &flags, const HttpRequestMethod& method)
+{
+    StoreEntry *e = storeCreatePureEntry(url, logUrl, flags, method);
+    e->lock("storeCreateEntry");
+
+    if (neighbors_do_private_keys || !flags.hierarchical)
+        e->setPrivateKey();
+    else
+        e->setPublicKey();
+
+    return e;
+}
+
 /* Mark object as expired */
 void
 StoreEntry::expireNow()
@@ -1077,7 +1086,7 @@ StoreEntry::abort()
     assert(mem_obj != NULL);
     debugs(20, 6, "storeAbort: " << getMD5Text());
 
-    lock();         /* lock while aborting */
+    lock("StoreEntry::abort");         /* lock while aborting */
     negativeCache();
 
     releaseRequest();
@@ -1114,7 +1123,7 @@ StoreEntry::abort()
     // abort swap out, invalidating what was created so far (release follows)
     swapOutFileClose(StoreIOState::writerGone);
 
-    unlock();       /* unlock */
+    unlock("StoreEntry::abort");       /* unlock */
 }
 
 /**
@@ -1217,7 +1226,7 @@ void
 StoreEntry::release()
 {
     PROF_start(storeRelease);
-    debugs(20, 3, "storeRelease: Releasing: '" << getMD5Text() << "'");
+    debugs(20, 3, "releasing " << *this << ' ' << getMD5Text());
     /* If, for any reason we can't discard this object because of an
      * outstanding request, mark it for pending release */
 
@@ -1239,7 +1248,7 @@ StoreEntry::release()
              * Fake a call to StoreEntry->lock()  When rebuilding is done,
              * we'll just call StoreEntry->unlock() on these.
              */
-            ++lock_count;
+            lock("StoreEntry::release+rebuilding");
             setReleaseFlag();
             LateReleaseStack.push_back(this);
         } else {
@@ -1286,7 +1295,7 @@ storeLateRelease(void *unused)
             return;
         }
 
-        e->unlock();
+        e->unlock("storeLateRelease");
         ++n;
     }
 
@@ -1300,14 +1309,9 @@ StoreEntry::locked() const
     if (lock_count)
         return 1;
 
-    if (swap_status == SWAPOUT_WRITING)
-        return 1;
-
-    if (store_status == STORE_PENDING)
-        return 1;
-
     /*
-     * SPECIAL, PUBLIC entries should be "locked"
+     * SPECIAL, PUBLIC entries should be "locked";
+     * XXX: Their owner should lock them then instead of relying on this hack.
      */
     if (EBIT_TEST(flags, ENTRY_SPECIAL))
         if (!EBIT_TEST(flags, KEY_PRIVATE))
@@ -2019,7 +2023,7 @@ std::ostream &operator <<(std::ostream &os, const StoreEntry &e)
     if (e.mem_obj && e.mem_obj->smpCollapsed)
         os << 'O';
 
-    return os << '/' << &e << '*' << e.lock_count;
+    return os << '/' << &e << '*' << e.locks();
 }
 
 /* NullStoreEntry */
index 51a8acb1e7bf99e4838ac6ac35cfe667c0c39dfa..f670286431b719fdd4daa42917a4e7ef35622f71 100644 (file)
@@ -723,7 +723,7 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
 
     delete sc;
 
-    assert(e->lock_count > 0);
+    assert(e->locked());
 
     if (mem->nclients == 0)
         CheckQuickAbort(e);
index 6584f360dcc315f350428f9a838e7bd73129534e..74e336dcef20d781047d1fdaad008c38fc48000c 100644 (file)
@@ -753,6 +753,19 @@ StoreController::dereference(StoreEntry &e, bool wantsLocalMemory)
 
 StoreEntry *
 StoreController::get(const cache_key *key)
+{
+    if (StoreEntry *e = find(key)) {
+        // this is not very precise: some get()s are not initiated by clients
+        e->touch(); 
+        return e;
+    }
+    return NULL;
+}
+
+/// Internal method to implements the guts of the Store::get() API:
+/// returns an in-transit or cached object with a given key, if any.
+StoreEntry *
+StoreController::find(const cache_key *key)
 {
     if (StoreEntry *e = swapDir->get(key)) {
         // TODO: ignore and maybe handleIdleEntry() unlocked intransit entries
@@ -844,6 +857,16 @@ StoreController::anchorCollapsedOnDisk(StoreEntry &collapsed, bool &inSync)
     return false;
 }
 
+void StoreController::markForUnlink(StoreEntry &e)
+{
+    if (transients && e.mem_obj && e.mem_obj->xitTable.index >= 0)
+        transients->markForUnlink(e);
+    if (memStore && e.mem_obj && e.mem_obj->memCache.index >= 0)
+        memStore->markForUnlink(e);
+    if (e.swap_filen >= 0)
+        e.store()->markForUnlink(e);
+}
+
 // move this into [non-shared] memory cache class when we have one
 /// whether e should be kept in local RAM for possible future caching
 bool
@@ -881,9 +904,6 @@ StoreController::memoryOut(StoreEntry &e, const bool preserveSwappable)
 void
 StoreController::memoryUnlink(StoreEntry &e)
 {
-    if (e.mem_status != IN_MEMORY)
-        return;
-
     if (memStore)
         memStore->unlink(e);
     else // TODO: move into [non-shared] memory cache class when we have one
@@ -1000,6 +1020,15 @@ StoreController::syncCollapsed(const cache_key *key)
         return;
     }
 
+    // this must be done before the abandoned() check because we may be looking
+    // at an entry we are writing while abandoned() requires a reading lock.
+    if (!collapsed->mem_obj->smpCollapsed) {
+        // this happens, e.g., when we tried collapsing but rejected the hit
+        // or a stale notification was received (and we are now the writer)
+        debugs(20, 7, "not SMP-syncing not-SMP-collapsed " << *collapsed);
+        return;
+    }
+
     assert(transients);
     if (transients->abandoned(*collapsed)) {
         debugs(20, 3, "aborting abandoned " << *collapsed);
@@ -1007,17 +1036,15 @@ StoreController::syncCollapsed(const cache_key *key)
         return;
     }
 
-    if (!collapsed->mem_obj->smpCollapsed) {
-        // this happens, e.g., when we tried collapsing but rejected the hit
-        debugs(20, 7, "not SMP-syncing not-SMP-collapsed " << *collapsed);
-        return;
-    }
-
     debugs(20, 7, "syncing " << *collapsed);
 
     bool found = false;
     bool inSync = false;
-    if (memStore && collapsed->mem_status == IN_MEMORY) {
+    if (memStore && collapsed->mem_obj->memCache.io == MemObject::ioDone) {
+        found = true;
+        inSync = true;
+        debugs(20, 7, "fully mem-loaded " << *collapsed);
+    } else if (memStore && collapsed->mem_obj->memCache.index >= 0) {
         found = true;
         inSync = memStore->updateCollapsed(*collapsed);
     } else if (collapsed->swap_filen >= 0) {
index ba8478cffdbb441b37d36a1bb51c47948c558618..6f9a034bdc68e2c028122fed003aecf3e172e189 100644 (file)
@@ -413,8 +413,8 @@ storeRebuildKeepEntry(const StoreEntry &tmpe, const cache_key *key, StoreRebuild
 
             // For some stores, get() creates/unpacks a store entry. Signal
             // such stores that we will no longer use the get() result:
-            e->lock();
-            e->unlock();
+            e->lock("storeRebuildKeepEntry");
+            e->unlock("storeRebuildKeepEntry");
 
             return false;
         } else {
index 765aac2d3aecd575a9f5f4dd2d281478acbc549c..581a6db08ae200d50bc0778f17929dcdb77397ff 100644 (file)
@@ -98,7 +98,7 @@ storeSwapOutStart(StoreEntry * e)
     /* Don't lock until after create, or the replacement
      * code might get confused */
 
-    e->lock();
+    e->lock("storeSwapOutStart");
     /* Pick up the file number if it was assigned immediately */
     e->swap_filen = mem->swapout.sio->swap_filen;
 
@@ -356,7 +356,7 @@ storeSwapOutFileClosed(void *data, int errflag, StoreIOState::Pointer self)
 
     debugs(20, 3, "storeSwapOutFileClosed: " << __FILE__ << ":" << __LINE__);
     mem->swapout.sio = NULL;
-    e->unlock();
+    e->unlock("storeSwapOutFileClosed");
 }
 
 bool
index 28dfa1e5a33f68ba090a6e4a6630565a1dcb5504..a4465454739c02b38af9435de2beb92ecdc25ae2 100644 (file)
@@ -242,7 +242,7 @@ UrnState::start(HttpRequest * r, StoreEntry * e)
     request = r;
     HTTPMSGLOCK(request);
 
-    entry->lock();
+    entry->lock("UrnState::start");
     setUriResFromRequest(r);
 
     if (urlres_r == NULL)
@@ -262,7 +262,7 @@ UrnState::created(StoreEntry *newEntry)
         FwdState::fwdStart(Comm::ConnectionPointer(), urlres_e, urlres_r);
     } else {
 
-        urlres_e->lock();
+        urlres_e->lock("UrnState::created");
         sc = storeClientListAdd(urlres_e, this);
     }
 
@@ -303,8 +303,8 @@ url_entry_sort(const void *A, const void *B)
 static void
 urnHandleReplyError(UrnState *urnState, StoreEntry *urlres_e)
 {
-    urlres_e->unlock();
-    urnState->entry->unlock();
+    urlres_e->unlock("urnHandleReplyError+res");
+    urnState->entry->unlock("urnHandleReplyError+prime");
     HTTPMSGUNLOCK(urnState->request);
     HTTPMSGUNLOCK(urnState->urlres_r);
     delete urnState;
index f163fea2e2a8b4b495d2db0e63f9a20206416c86..a3b51d30ea85013a42f90305f234206b7ccc5c37 100644 (file)
@@ -96,7 +96,7 @@ whoisStart(FwdState * fwd)
     p->fwd = fwd;
     p->dataWritten = false;
 
-    p->entry->lock();
+    p->entry->lock("whoisStart");
     comm_add_close_handler(fwd->serverConnection()->fd, whoisClose, p);
 
     l = p->request->urlpath.size() + 3;
@@ -205,6 +205,6 @@ whoisClose(const CommCloseCbParams &params)
 {
     WhoisState *p = (WhoisState *)params.data;
     debugs(75, 3, "whoisClose: FD " << params.fd);
-    p->entry->unlock();
+    p->entry->unlock("whoisClose");
     cbdataFree(p);
 }