From f58bb2f4789906320d4e3bdba8bf4e32dfc1b0ff Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 4 Feb 2011 15:18:41 -0700 Subject: [PATCH] Revised Core/Store cooperation w.r.t. SwapDir map slot locks. When StoreEntry is deleted, we need to release the SwapDir map slot locks it holds, if any. This is difficult because SwapDir maintains the locks while Squid Core maintains the entry swap_status. The Core gets swap_status-related notifications using async calls so it is easy for swap_status to get out of sync if SwapDir updates the map slot proactively. The new code no longer releases the slot lock until the associated StoreEntry is unlinked or gone, even if the slot is known to be unusable and waiting to be deleted. We also do not rely on swap_status to guess which lock to release; we use slot state to determine that instead. Removed rock-specific code from StoreEntry destructor by introducing a general SwapDir::disconnect(StoreEntry&) API. Polished StoreEntry initialization. Polished and fixed Rock::SwapDir debugging. --- src/SwapDir.h | 3 +++ src/fs/rock/RockDirMap.cc | 18 ++++++++++++++++++ src/fs/rock/RockDirMap.h | 6 ++++-- src/fs/rock/RockSwapDir.cc | 36 ++++++++++++++++++++++++------------ src/fs/rock/RockSwapDir.h | 3 +-- src/store.cc | 28 ++++++++++++---------------- src/store_swapin.cc | 1 + src/store_swapout.cc | 7 ++----- 8 files changed, 65 insertions(+), 37 deletions(-) diff --git a/src/SwapDir.h b/src/SwapDir.h index 8e8187c48e..e5a4cddd06 100644 --- a/src/SwapDir.h +++ b/src/SwapDir.h @@ -139,6 +139,9 @@ public: /* migrated from store_dir.cc */ bool objectSizeIsAcceptable(int64_t objsize) const; + /// called when the entry is about to forget its association with cache_dir + virtual void disconnect(StoreEntry &) {} + protected: void parseOptions(int reconfiguring); void dumpOptions(StoreEntry * e) const; diff --git a/src/fs/rock/RockDirMap.cc b/src/fs/rock/RockDirMap.cc index b014bdce5f..3d716fdc56 100644 --- a/src/fs/rock/RockDirMap.cc +++ b/src/fs/rock/RockDirMap.cc @@ -68,6 +68,7 @@ Rock::DirMap::closeForWriting(const sfileno fileno) s.switchExclusiveToSharedLock(); } +/// terminate writing the entry, freeing its slot for others to use void Rock::DirMap::abortWriting(const sfileno fileno) { @@ -79,6 +80,22 @@ Rock::DirMap::abortWriting(const sfileno fileno) freeLocked(s); } +void +Rock::DirMap::abortIo(const sfileno fileno) +{ + debugs(79, 5, HERE << " abort I/O for slot at " << fileno << + " in map [" << path << ']'); + assert(valid(fileno)); + Slot &s = shared->slots[fileno]; + + // The caller is a lock holder. Thus, if we are Writeable, then the + // caller must be the writer; otherwise the caller must be the reader. + if (s.state == Slot::Writeable) + abortWriting(fileno); + else + closeForReading(fileno); +} + bool Rock::DirMap::putAt(const StoreEntry &e, const sfileno fileno) { @@ -176,6 +193,7 @@ Rock::DirMap::closeForReading(const sfileno fileno) "map [" << path << ']'); assert(valid(fileno)); Slot &s = shared->slots[fileno]; + assert(s.state == Slot::Readable); s.releaseSharedLock(); freeIfNeeded(s); } diff --git a/src/fs/rock/RockDirMap.h b/src/fs/rock/RockDirMap.h index 586800b63e..fcd48ab4de 100644 --- a/src/fs/rock/RockDirMap.h +++ b/src/fs/rock/RockDirMap.h @@ -64,8 +64,6 @@ public: StoreEntryBasics *openForWriting(const cache_key *const key, sfileno &fileno); /// successfully finish writing the entry, leaving it opened for reading void closeForWriting(const sfileno fileno); - /// terminate writing the entry, freeing its slot for others to use - void abortWriting(const sfileno fileno); /// stores entry info at the requested slot or returns false bool putAt(const StoreEntry &e, const sfileno fileno); @@ -80,6 +78,9 @@ public: /// close slot after reading, decrements read level void closeForReading(const sfileno fileno); + /// called by lock holder to terminate either slot writing or reading + void abortIo(const sfileno fileno); + bool full() const; ///< there are no empty slots left bool valid(int n) const; ///< whether n is a valid slot coordinate int entryCount() const; ///< number of used slots @@ -100,6 +101,7 @@ private: int slotIdx(const cache_key *const key) const; Slot &slot(const cache_key *const key); const StoreEntryBasics *openForReading(Slot &s); + void abortWriting(const sfileno fileno); void freeIfNeeded(Slot &s); void freeLocked(Slot &s); String sharedMemoryName(); diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc index 6d0b0de56c..b11c9ba0ed 100644 --- a/src/fs/rock/RockSwapDir.cc +++ b/src/fs/rock/RockSwapDir.cc @@ -76,14 +76,21 @@ Rock::SwapDir::get(const cache_key *key) // the disk entry remains open for reading, protected from modifications } -void -Rock::SwapDir::closeForReading(StoreEntry &e) +void Rock::SwapDir::disconnect(StoreEntry &e) { - assert(index == e.swap_dirn); + assert(e.swap_dirn == index); assert(e.swap_filen >= 0); - map->closeForReading(e.swap_filen); + // cannot have SWAPOUT_NONE entry with swap_filen >= 0 + assert(e.swap_status != SWAPOUT_NONE); + + // do not rely on e.swap_status here because there is an async delay + // before it switches from SWAPOUT_WRITING to SWAPOUT_DONE. + + // since e has swap_filen, its slot is locked for either reading or writing + map->abortIo(e.swap_filen); e.swap_dirn = -1; e.swap_filen = -1; + e.swap_status = SWAPOUT_NONE; } // TODO: encapsulate as a tool; identical to CossSwapDir::create() @@ -475,11 +482,15 @@ Rock::SwapDir::writeCompleted(int errflag, size_t rlen, RefCount< ::WriteRequest assert(request->sio != NULL); IoState &sio = *request->sio; - if (errflag != DISK_OK) - map->free(sio.swap_filen); // TODO: test by forcing failure - // else sio.offset_ += rlen; - - map->closeForWriting(sio.swap_filen); // assume we only write once + if (errflag == DISK_OK) { + // close, assuming we only write once; the entry gets the read lock + map->closeForWriting(sio.swap_filen); + // and sio.offset_ += rlen; + } else { + // Do not abortWriting here. The entry should keep the write lock + // instead of losing association with the store and confusing core. + map->free(sio.swap_filen); // will mark as unusable, just in case + } // TODO: always compute cur_size based on map, do not store it cur_size = (HeaderSize + max_objsize * map->entryCount()) >> 10; @@ -584,15 +595,16 @@ Rock::SwapDir::dereference(StoreEntry &e) void Rock::SwapDir::unlink(StoreEntry &e) { - debugs(47, 5, HERE << &e << ' ' << e.swap_dirn << ' ' << e.swap_filen); + debugs(47, 5, HERE << e); ignoreReferences(e); map->free(e.swap_filen); + disconnect(e); } void Rock::SwapDir::trackReferences(StoreEntry &e) { - debugs(47, 5, HERE << *e); + debugs(47, 5, HERE << e); repl->Add(repl, &e, &e.repl); } @@ -600,7 +612,7 @@ Rock::SwapDir::trackReferences(StoreEntry &e) void Rock::SwapDir::ignoreReferences(StoreEntry &e) { - debugs(47, 5, HERE << *e); + debugs(47, 5, HERE << e); repl->Remove(repl, &e, &e.repl); } diff --git a/src/fs/rock/RockSwapDir.h b/src/fs/rock/RockSwapDir.h index fb1336898b..375fd530d1 100644 --- a/src/fs/rock/RockSwapDir.h +++ b/src/fs/rock/RockSwapDir.h @@ -25,8 +25,7 @@ public: virtual void reconfigure(int, char *); virtual StoreSearch *search(String const url, HttpRequest *); virtual StoreEntry *get(const cache_key *key); - - void closeForReading(StoreEntry &e); + virtual void disconnect(StoreEntry &e); protected: /* protected ::SwapDir API */ diff --git a/src/store.cc b/src/store.cc index 6284558a83..4c70ad6388 100644 --- a/src/store.cc +++ b/src/store.cc @@ -53,7 +53,6 @@ #include "SquidTime.h" #include "swap_log_op.h" #include "mgr/StoreIoAction.h" -#include "fs/rock/RockSwapDir.h" static STMCB storeWriteComplete; @@ -372,6 +371,7 @@ StoreEntry::StoreEntry(): expires = lastmod = lastref = timestamp = -1; + swap_status = SWAPOUT_NONE; swap_filen = -1; swap_dirn = -1; } @@ -384,6 +384,7 @@ StoreEntry::StoreEntry(const char *aUrl, const char *aLogUrl): expires = lastmod = lastref = timestamp = -1; + swap_status = SWAPOUT_NONE; swap_filen = -1; swap_dirn = -1; } @@ -391,10 +392,8 @@ StoreEntry::StoreEntry(const char *aUrl, const char *aLogUrl): StoreEntry::~StoreEntry() { if (swap_filen >= 0) { - // XXX: support cache types other than Rock - Rock::SwapDir &rockSwapDir = - dynamic_cast(*store()); - rockSwapDir.closeForReading(*this); + SwapDir &sd = dynamic_cast(*store()); + sd.disconnect(*this); } } @@ -798,9 +797,6 @@ storeCreateEntry(const char *url, const char *log_url, request_flags flags, cons e->store_status = STORE_PENDING; e->setMemStatus(NOT_IN_MEMORY); - e->swap_status = SWAPOUT_NONE; - e->swap_filen = -1; - e->swap_dirn = -1; e->refcount = 0; e->lastref = squid_curtime; e->timestamp = -1; /* set in StoreEntry::timestampsSet() */ @@ -1279,21 +1275,18 @@ StoreEntry::release() storeLog(STORE_LOG_RELEASE, this); if (swap_filen > -1) { - unlink(); + // update size before unlink() below clears swap_status + // TODO: the store/SwapDir::unlink should update the size! if (swap_status == SWAPOUT_DONE) if (EBIT_TEST(flags, ENTRY_VALIDATED)) store()->updateSize(swap_file_sz, -1); + // log before unlink() below clears swap_filen if (!EBIT_TEST(flags, KEY_PRIVATE)) storeDirSwapLog(this, SWAP_LOG_DEL); -#if 0 - /* From 2.4. I think we do this in storeUnlink? */ - storeSwapFileNumberSet(this, -1); - -#endif - + unlink(); } setMemStatus(NOT_IN_MEMORY); @@ -2013,7 +2006,10 @@ StoreEntry::store() const void StoreEntry::unlink() { - store()->unlink(*this); + store()->unlink(*this); // implies disconnect() + swap_filen = -1; + swap_dirn = -1; + swap_status = SWAPOUT_NONE; } /* diff --git a/src/store_swapin.cc b/src/store_swapin.cc index b6ca9b759f..d993b62d06 100644 --- a/src/store_swapin.cc +++ b/src/store_swapin.cc @@ -97,6 +97,7 @@ storeSwapInFileNotify(void *data, int errflag, StoreIOState::Pointer self) e->swap_dirn << " to " << sc->swapin_sio->swap_filen << "/" << sc->swapin_sio->swap_dirn); + assert(e->swap_filen < 0); // if this fails, call SwapDir::disconnect(e) e->swap_filen = sc->swapin_sio->swap_filen; e->swap_dirn = sc->swapin_sio->swap_dirn; } diff --git a/src/store_swapout.cc b/src/store_swapout.cc index 8397139897..b0981da071 100644 --- a/src/store_swapout.cc +++ b/src/store_swapout.cc @@ -115,6 +115,7 @@ storeSwapOutFileNotify(void *data, int errflag, StoreIOState::Pointer self) assert(mem); assert(mem->swapout.sio == self); assert(errflag == 0); + assert(e->swap_filen < 0); // if this fails, call SwapDir::disconnect(e) e->swap_filen = mem->swapout.sio->swap_filen; e->swap_dirn = mem->swapout.sio->swap_dirn; } @@ -340,11 +341,7 @@ storeSwapOutFileClosed(void *data, int errflag, StoreIOState::Pointer self) if (e->swap_filen > 0) e->unlink(); - e->swap_filen = -1; - - e->swap_dirn = -1; - - e->swap_status = SWAPOUT_NONE; + assert(e->swap_status == SWAPOUT_NONE); e->releaseRequest(); } else { -- 2.47.2