Hello,
The attached patch avoids memory corruption and segfaults during ufs
cache store maintenance by removing being-deleted StoreEntry from ufs
cache replacement policies.
SMP Store changes added a SwapDir::disconnect() method that notified
entry's cache_dir that its entry is being destroyed. I added that code
so that shared cache_dirs can update their index when idle store_table
entries are destroyed on cleanup (completely avoiding idle entries
proved to be too difficult with store_table still around).
However, I did not realize that non-shared cache_dirs have the same
problem: Their StoreEntries may be deleted without cache_dirs knowing
that. UFS cache_dir now implement the SwapDir::disconnect() API to stay
in sync. It is likely that the problem was introduced by SMP Store
changes, but I do not know whether there is a better fix for it.
I also added assertions to catch cases where a dirty StoreEntry or
MemObject (i.e., an entry or object that is still in some replacement
policy index) is being destroyed.
TODO: This patch breaks "make check". We need more STUBs somewhere!
Alex.
Avoid memory corruption and segfaults during ufs cache store maintenance
by removing being-deleted StoreEntry from ufs cache replacement policies.
SMP Store changes added a SwapDir::disconnect() method that notified
entry's cache_dir that its entry is being destroyed. I added that code so that
shared cache_dirs can update their index when idle store_table entries are
destroyed on cleanup (completely avoiding idle entries proved to be too
difficult with store_table still around).
However, I did not realize that non-shared cache_dirs have the same problem:
Their StoreEntries may be deleted without cache_dirs knowing that. UFS
cache_dir now implement the SwapDir::disconnect() API to stay in sync. It is
likely that the problem was introduced by SMP Store changes, but I do not know
whether there is a better fix for it.
I also added assertions to catch cases where a dirty StoreEntry or MemObject
(i.e., an entry or object that is still in some replacement policy index) is
being destroyed.
TODO: This patch breaks "make check". We need more STUBs.
=== modified file 'src/MemObject.cc'
--- src/MemObject.cc 2012-07-20 12:44:39 +0000
+++ src/MemObject.cc 2012-07-25 23:37:21 +0000
@@ -101,40 +101,42 @@
/* XXX account log_url */
swapout.decision = SwapOut::swNeedsCheck;
}
MemObject::~MemObject()
{
debugs(20, 3, HERE << "del MemObject " << this);
const Ctx ctx = ctx_enter(url);
#if URL_CHECKSUM_DEBUG
assert(chksum == url_checksum(url));
#endif
if (!shutting_down)
assert(swapout.sio == NULL);
data_hdr.freeContent();
+ assert(!repl.data); // we are not in some replacement policy queue
+
#if 0
/*
* There is no way to abort FD-less clients, so they might
* still have mem->clients set.
*/
assert(clients.head == NULL);
#endif
HTTPMSGUNLOCK(_reply);
HTTPMSGUNLOCK(request);
ctx_exit(ctx); /* must exit before we free mem->url */
safe_free(url);
safe_free(log_url); /* XXX account log_url */
safe_free(vary_headers);
=== modified file 'src/SwapDir.h'
--- src/SwapDir.h 2012-07-13 14:33:19 +0000
+++ src/SwapDir.h 2012-07-26 00:01:59 +0000
@@ -142,41 +142,41 @@
virtual void diskFull();
virtual StoreEntry * get(const cache_key *);
virtual void get(String const, STOREGETCLIENT, void * cbdata);
virtual uint64_t maxSize() const { return max_size;}
virtual uint64_t minSize() const;
virtual int64_t maxObjectSize() const { return max_objsize; }
virtual void getStats(StoreInfoStats &stats) const;
virtual void stat (StoreEntry &anEntry) const;
virtual StoreSearch *search(String const url, HttpRequest *) = 0;
/* 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 &) {}
+ virtual void disconnect(StoreEntry &e) = 0;
/// called when entry swap out is complete
virtual void swappedOut(const StoreEntry &e) = 0;
protected:
void parseOptions(int reconfiguring);
void dumpOptions(StoreEntry * e) const;
virtual ConfigOption *getOptionTree() const;
virtual bool allowOptionReconfigure(const char *const) const { return true; }
int64_t sizeInBlocks(const int64_t size) const { return (size + fs.blksize - 1) / fs.blksize; }
private:
bool optionReadOnlyParse(char const *option, const char *value, int reconfiguring);
void optionReadOnlyDump(StoreEntry * e) const;
bool optionObjectSizeParse(char const *option, const char *value, int reconfiguring);
void optionObjectSizeDump(StoreEntry * e) const;
char const *theType;
protected:
=== modified file 'src/fs/ufs/store_dir_ufs.cc'
--- src/fs/ufs/store_dir_ufs.cc 2012-07-23 15:34:12 +0000
+++ src/fs/ufs/store_dir_ufs.cc 2012-07-26 00:18:20 +0000
@@ -420,40 +420,60 @@
if (repl->Referenced)
repl->Referenced(repl, &e, &e.repl);
}
/*
* UFSSwapDir::dereference
* This routine is called whenever the last reference to an object is
* removed, to maintain replacement information within the storage fs.
*/
bool
UFSSwapDir::dereference(StoreEntry & e)
{
debugs(47, 3, "UFSSwapDir::dereference: referencing " << &e << " " << e.swap_dirn << "/" << e.swap_filen);
if (repl->Dereferenced)
repl->Dereferenced(repl, &e, &e.repl);
return true; // keep e in the global store_table
}
+void
+UFSSwapDir::disconnect(StoreEntry &e)
+{
+ assert(e.swap_dirn == index);
+ assert(e.swap_filen >= 0);
+
+ // We do not want to unlink(e) in this low-level method because it is
+ // called during Squid shutdown sequence (among other times).
+ if (e.swap_status == SWAPOUT_DONE) {
+ cur_size -= fs.blksize * sizeInBlocks(e.swap_file_sz);
+ --n_disk_objects;
+ }
+ replacementRemove(&e);
+ mapBitReset(e.swap_filen);
+
+ e.swap_dirn = -1;
+ e.swap_filen = -1;
+ e.swap_status = SWAPOUT_NONE;
+}
+
StoreIOState::Pointer
UFSSwapDir::createStoreIO(StoreEntry &e, StoreIOState::STFNCB * file_callback, StoreIOState::STIOCB * aCallback, void *callback_data)
{
return IO->create (this, &e, file_callback, aCallback, callback_data);
}
StoreIOState::Pointer
UFSSwapDir::openStoreIO(StoreEntry &e, StoreIOState::STFNCB * file_callback, StoreIOState::STIOCB * aCallback, void *callback_data)
{
return IO->open (this, &e, file_callback, aCallback, callback_data);
}
int
UFSSwapDir::mapBitTest(sfileno filn)
{
return map->testBit(filn);
}
void
UFSSwapDir::mapBitSet(sfileno filn)
=== modified file 'src/fs/ufs/ufscommon.h'
--- src/fs/ufs/ufscommon.h 2012-07-20 23:11:02 +0000
+++ src/fs/ufs/ufscommon.h 2012-07-26 00:02:33 +0000
@@ -50,40 +50,41 @@
public:
static int IsUFSDir(SwapDir* sd);
static int DirClean(int swap_index);
static int FilenoBelongsHere(int fn, int F0, int F1, int F2);
UFSSwapDir(char const *aType, const char *aModuleType);
virtual void init();
virtual void create();
virtual void dump(StoreEntry &) const;
~UFSSwapDir();
virtual StoreSearch *search(String const url, HttpRequest *);
virtual bool doubleCheck(StoreEntry &);
virtual bool unlinkdUseful() const;
virtual void unlink(StoreEntry &);
virtual void statfs(StoreEntry &)const;
virtual void maintain();
virtual bool canStore(const StoreEntry &e, int64_t diskSpaceNeeded, int &load) const;
virtual void reference(StoreEntry &);
virtual bool dereference(StoreEntry &);
+ virtual void disconnect(StoreEntry &e);
virtual StoreIOState::Pointer createStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *);
virtual StoreIOState::Pointer openStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *);
virtual void openLog();
virtual void closeLog();
virtual int writeCleanStart();
virtual void writeCleanDone();
virtual void logEntry(const StoreEntry & e, int op) const;
virtual void parse(int index, char *path);
virtual void reconfigure();
virtual int callback();
virtual void sync();
virtual void swappedOut(const StoreEntry &e);
virtual uint64_t currentSize() const { return cur_size; }
virtual uint64_t currentCount() const { return n_disk_objects; }
void unlinkFile(sfileno f);
// move down when unlink is a virtual method
//protected:
UFSStrategy *IO;
char *fullPath(sfileno, char *) const;
=== modified file 'src/store.cc'
--- src/store.cc 2012-07-23 07:02:06 +0000
+++ src/store.cc 2012-07-25 23:37:21 +0000
@@ -386,40 +386,42 @@
hidden_mem_obj(NULL),
swap_file_sz(0)
{
debugs(20, 3, HERE << "new StoreEntry " << this);
mem_obj = new MemObject(aUrl, aLogUrl);
expires = lastmod = lastref = timestamp = -1;
swap_status = SWAPOUT_NONE;
swap_filen = -1;
swap_dirn = -1;
}
StoreEntry::~StoreEntry()
{
if (swap_filen >= 0) {
SwapDir &sd = dynamic_cast<SwapDir&>(*store());
sd.disconnect(*this);
}
delete hidden_mem_obj;
+ assert(!mem_obj);
+ assert(!repl.data);
}
#if USE_ADAPTATION
void
StoreEntry::deferProducer(const AsyncCall::Pointer &producer)
{
if (!deferredProducer)
deferredProducer = producer;
else
debugs(20, 5, HERE << "Deferred producer call is allready set to: " <<
*deferredProducer << ", requested call: " << *producer);
}
void
StoreEntry::kickProducer()
{
if (deferredProducer != NULL) {
ScheduleCallHere(deferredProducer);
deferredProducer = NULL;
}
@@ -434,42 +436,42 @@
MemObject *mem = mem_obj;
mem_obj = NULL;
delete mem;
delete hidden_mem_obj;
hidden_mem_obj = NULL;
}
void
StoreEntry::hideMemObject()
{
debugs(20, 3, HERE << "hiding " << mem_obj);
assert(mem_obj);
assert(!hidden_mem_obj);
hidden_mem_obj = mem_obj;
mem_obj = NULL;
}
void
destroyStoreEntry(void *data)
{
- debugs(20, 3, HERE << "destroyStoreEntry: destroying " << data);
StoreEntry *e = static_cast<StoreEntry *>(static_cast<hash_link *>(data));
+ debugs(20, 3, HERE << "destroyStoreEntry: destroying " << e << " or " << data);
assert(e != NULL);
if (e == NullStoreEntry::getInstance())
return;
e->destroyMemObject();
e->hashDelete();
assert(e->key == NULL);
delete e;
}
/* ----- INTERFACE BETWEEN STORAGE MANAGER AND HASH TABLE FUNCTIONS --------- */
void
StoreEntry::hashInsert(const cache_key * someKey)
{
debugs(20, 3, "StoreEntry::hashInsert: Inserting Entry " << this << " key '" << storeKeyText(someKey) << "'");
