From: Alex Rousskov Date: Thu, 27 Jun 2013 21:26:57 +0000 (-0600) Subject: Tightened StoreEntry locking. Fixed entry touching and synchronization code: X-Git-Tag: SQUID_3_5_0_1~444^2~43 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1bfe9adea57c6187029a014f0fa5c77ac39d74aa;p=thirdparty%2Fsquid.git Tightened StoreEntry locking. Fixed entry touching and synchronization code: 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. --- diff --git a/src/MemStore.cc b/src/MemStore.cc index cc1aa0b57d..c3b18ecc1f 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -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(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(e.key)); } diff --git a/src/MemStore.h b/src/MemStore.h index c875095ca1..527ae13030 100644 --- a/src/MemStore.h +++ b/src/MemStore.h @@ -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(); diff --git a/src/Server.cc b/src/Server.cc index 41d4a36c32..444311f8e8 100644 --- a/src/Server.cc +++ b/src/Server.cc @@ -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); diff --git a/src/Store.h b/src/Store.h index e689680f3d..446894d3b3 100644 --- a/src/Store.h +++ b/src/Store.h @@ -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(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); diff --git a/src/StoreEntryStream.h b/src/StoreEntryStream.h index 3f9732d5df..880405a41a 100644 --- a/src/StoreEntryStream.h +++ b/src/StoreEntryStream.h @@ -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: diff --git a/src/StoreSwapLogData.h b/src/StoreSwapLogData.h index 8a58046ab0..9cd86a9998 100644 --- a/src/StoreSwapLogData.h +++ b/src/StoreSwapLogData.h @@ -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; diff --git a/src/SwapDir.h b/src/SwapDir.h index 68fe0beeb2..fa08136f05 100644 --- a/src/SwapDir.h +++ b/src/SwapDir.h @@ -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); diff --git a/src/Transients.cc b/src/Transients.cc index c6967b8409..10a2480ec3 100644 --- a/src/Transients.cc +++ b/src/Transients.cc @@ -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) { diff --git a/src/Transients.h b/src/Transients.h index cbe2138f6c..0204853015 100644 --- a/src/Transients.h +++ b/src/Transients.h @@ -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(); diff --git a/src/acl/Asn.cc b/src/acl/Asn.cc index 43fe185973..a13ee13a62 100644 --- a/src/acl/Asn.cc +++ b/src/acl/Asn.cc @@ -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); } diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 8e4c4e1025..e353c73a9e 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -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()) { diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 88d535660a..d372ef42ca 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -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; } } diff --git a/src/errorpage.cc b/src/errorpage.cc index 1886342f6d..dfeb9a3211 100644 --- a/src/errorpage.cc +++ b/src/errorpage.cc @@ -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; } diff --git a/src/forward.cc b/src/forward.cc index d872d347c4..82671cc217 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -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); diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc index 60f41c7271..a992b17d6b 100644 --- a/src/fs/rock/RockSwapDir.cc +++ b/src/fs/rock/RockSwapDir.cc @@ -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) { diff --git a/src/fs/rock/RockSwapDir.h b/src/fs/rock/RockSwapDir.h index a9eb2a26fd..1a77a20855 100644 --- a/src/fs/rock/RockSwapDir.h +++ b/src/fs/rock/RockSwapDir.h @@ -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; diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc index 9a1094d7dd..599b8ad2be 100644 --- a/src/fs/ufs/UFSSwapDir.cc +++ b/src/fs/ufs/UFSSwapDir.cc @@ -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; diff --git a/src/ftp.cc b/src/ftp.cc index e031886aee..757db4e015 100644 --- a/src/ftp.cc +++ b/src/ftp.cc @@ -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 diff --git a/src/gopher.cc b/src/gopher.cc index bd1df5112e..63241914ff 100644 --- a/src/gopher.cc +++ b/src/gopher.cc @@ -173,7 +173,7 @@ gopherStateFree(const CommCloseCbParams ¶ms) 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; diff --git a/src/mgr/Forwarder.cc b/src/mgr/Forwarder.cc index d7a754dcfd..33481e9970 100644 --- a/src/mgr/Forwarder.cc +++ b/src/mgr/Forwarder.cc @@ -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(); } diff --git a/src/neighbors.cc b/src/neighbors.cc index da99ce3df6..562efc657e 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -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); diff --git a/src/peer_select.cc b/src/peer_select.cc index 897a06fe67..258960edab 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -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); } diff --git a/src/ssl/ServerBump.cc b/src/ssl/ServerBump.cc index 38e7234092..adaa640bae 100644 --- a/src/ssl/ServerBump.cc +++ b/src/ssl/ServerBump.cc @@ -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); } diff --git a/src/stat.cc b/src/stat.cc index db19c90ccc..715bef8310 100644 --- a/src/stat.cc +++ b/src/stat.cc @@ -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); diff --git a/src/store.cc b/src/store.cc index d4bbef8d9e..35f483f997 100644 --- a/src/store.cc +++ b/src/store.cc @@ -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 */ diff --git a/src/store_client.cc b/src/store_client.cc index 51a8acb1e7..f670286431 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -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); diff --git a/src/store_dir.cc b/src/store_dir.cc index 6584f360dc..74e336dcef 100644 --- a/src/store_dir.cc +++ b/src/store_dir.cc @@ -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) { diff --git a/src/store_rebuild.cc b/src/store_rebuild.cc index ba8478cffd..6f9a034bdc 100644 --- a/src/store_rebuild.cc +++ b/src/store_rebuild.cc @@ -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 { diff --git a/src/store_swapout.cc b/src/store_swapout.cc index 765aac2d3a..581a6db08a 100644 --- a/src/store_swapout.cc +++ b/src/store_swapout.cc @@ -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 diff --git a/src/urn.cc b/src/urn.cc index 28dfa1e5a3..a446545473 100644 --- a/src/urn.cc +++ b/src/urn.cc @@ -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; diff --git a/src/whois.cc b/src/whois.cc index f163fea2e2..a3b51d30ea 100644 --- a/src/whois.cc +++ b/src/whois.cc @@ -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 ¶ms) { WhoisState *p = (WhoisState *)params.data; debugs(75, 3, "whoisClose: FD " << params.fd); - p->entry->unlock(); + p->entry->unlock("whoisClose"); cbdataFree(p); }