Subtract sizes of added-then-rejected entries while loading ufs cache index. Before SMP changes, the ufs code was incorrectly ignoring the size of loaded-but-not-yet-validated entries, leading to cache disk overflows. After SMP changes, the ufs code was not subtracting sizes of loaded-but-then-rejected entries, leading to bogus "Disk space over limit" warnings. Now we correctly account for both kinds of entries. TODO: Do not enter the validation loop in storeCleanup unless running with -S. The loop does nothing useful when storeCleanupDoubleCheck() is not called. === modified file 'src/fs/ufs/store_dir_ufs.cc' --- src/fs/ufs/store_dir_ufs.cc 2012-06-22 03:49:38 +0000 +++ src/fs/ufs/store_dir_ufs.cc 2012-07-18 23:27:27 +0000 @@ -712,40 +712,53 @@ e->lastref = lastref; e->timestamp = timestamp; e->expires = expires; e->lastmod = lastmod; e->refcount = refcount; e->flags = newFlags; EBIT_SET(e->flags, ENTRY_CACHABLE); EBIT_CLR(e->flags, RELEASE_REQUEST); EBIT_CLR(e->flags, KEY_PRIVATE); e->ping_status = PING_NONE; EBIT_CLR(e->flags, ENTRY_VALIDATED); mapBitSet(e->swap_filen); cur_size += fs.blksize * sizeInBlocks(e->swap_file_sz); ++n_disk_objects; e->hashInsert(key); /* do it after we clear KEY_PRIVATE */ replacementAdd (e); return e; } void +UFSSwapDir::undoAddDiskRestore(StoreEntry *e) +{ + debugs(47, 5, HERE << *e); + replacementRemove(e); // checks swap_dirn so do it before we invalidate it + // Do not unlink the file as it might be used by a subsequent entry. + mapBitReset(e->swap_filen); + e->swap_filen = -1; + e->swap_dirn = -1; + cur_size -= fs.blksize * sizeInBlocks(e->swap_file_sz); + --n_disk_objects; +} + +void UFSSwapDir::rebuild() { ++StoreController::store_dirs_rebuilding; eventAdd("storeRebuild", RebuildState::RebuildStep, new RebuildState(this), 0.0, 1); } void UFSSwapDir::closeTmpSwapLog() { char *swaplog_path = xstrdup(logFile(NULL)); char *new_path = xstrdup(logFile(".new")); int fd; file_close(swaplog_fd); if (xrename(new_path, swaplog_path) < 0) { debugs(50, DBG_IMPORTANT, "ERROR: " << swaplog_path << ": " << xstrerror()); fatalf("Failed to rename log file %s to %s.new", swaplog_path, swaplog_path); } fd = file_open(swaplog_path, O_WRONLY | O_CREAT | O_BINARY); @@ -1288,41 +1301,41 @@ { debugs(79, 3, "UFSSwapDir::unlinkFile: unlinking fileno " << std::setfill('0') << std::hex << std::uppercase << std::setw(8) << f << " '" << fullPath(f,NULL) << "'"); /* commonUfsDirMapBitReset(this, f); */ IO->unlinkFile(fullPath(f,NULL)); } bool UFSSwapDir::unlinkdUseful() const { // unlinkd may be useful only in workers return IamWorkerProcess() && IO->io->unlinkdUseful(); } void UFSSwapDir::unlink(StoreEntry & e) { debugs(79, 3, "storeUfsUnlink: dirno " << index << ", fileno "<< std::setfill('0') << std::hex << std::uppercase << std::setw(8) << e.swap_filen); - if (e.swap_status == SWAPOUT_DONE && EBIT_TEST(e.flags, ENTRY_VALIDATED)) { + if (e.swap_status == SWAPOUT_DONE) { cur_size -= fs.blksize * sizeInBlocks(e.swap_file_sz); --n_disk_objects; } replacementRemove(&e); mapBitReset(e.swap_filen); UFSSwapDir::unlinkFile(e.swap_filen); } /* * Add and remove the given StoreEntry from the replacement policy in * use. */ void UFSSwapDir::replacementAdd(StoreEntry * e) { debugs(47, 4, "UFSSwapDir::replacementAdd: added node " << e << " to dir " << index); repl->Add(repl, e, &e->repl); } === modified file 'src/fs/ufs/ufscommon.cc' --- src/fs/ufs/ufscommon.cc 2012-04-11 00:15:57 +0000 +++ src/fs/ufs/ufscommon.cc 2012-07-18 23:15:55 +0000 @@ -550,59 +550,41 @@ debugs(47, 3, "commonUfsDirRebuildFromSwapLog: " << swap_log_op_str[(int) swapData.op] << " " << storeKeyText(swapData.key) << " "<< std::setfill('0') << std::hex << std::uppercase << std::setw(8) << swapData.swap_filen); if (swapData.op == SWAP_LOG_ADD) { (void) 0; } else if (swapData.op == SWAP_LOG_DEL) { /* Delete unless we already have a newer copy anywhere in any store */ /* this needs to become * 1) unpack url * 2) make synthetic request with headers ?? or otherwise search * for a matching object in the store * TODO FIXME change to new async api */ currentEntry (Store::Root().get(swapData.key)); if (currentEntry() != NULL && swapData.lastref >= e->lastref) { - /* - * Make sure we don't unlink the file, it might be - * in use by a subsequent entry. Also note that - * we don't have to subtract from cur_size because - * adding to cur_size happens in the cleanup procedure. - */ - currentEntry()->expireNow(); - currentEntry()->releaseRequest(); - - if (currentEntry()->swap_filen > -1) { - UFSSwapDir *sdForThisEntry = dynamic_cast(INDEXSD(currentEntry()->swap_dirn)); - assert (sdForThisEntry); - sdForThisEntry->replacementRemove(currentEntry()); - sdForThisEntry->mapBitReset(currentEntry()->swap_filen); - currentEntry()->swap_filen = -1; - currentEntry()->swap_dirn = -1; - } - - currentEntry()->release(); + undoAdd(); counts.objcount--; counts.cancelcount++; } return; } else { const double x = ::log(static_cast(++counts.bad_log_op)) / ::log(10.0); if (0.0 == x - (double) (int) x) debugs(47, 1, "WARNING: " << counts.bad_log_op << " invalid swap log entries found"); counts.invalid++; return; } ++counts.scancount; // XXX: should not this be incremented earlier? if (!sd->validFileno(swapData.swap_filen, 0)) { counts.invalid++; @@ -665,76 +647,86 @@ /* I'm tempted to remove the swapfile here just to be safe, * but there is a bad race condition in the NOVM version if * the swapfile has recently been opened for writing, but * not yet opened for reading. Because we can't map * swapfiles back to StoreEntrys, we don't know the state * of the entry using that file. */ /* We'll assume the existing entry is valid, probably because * were in a slow rebuild and the the swap file number got taken * and the validation procedure hasn't run. */ assert(flags.need_to_validate); counts.clashcount++; return; } else if (currentEntry() && !disk_entry_newer) { /* key already exists, current entry is newer */ /* keep old, ignore new */ counts.dupcount++; return; } else if (currentEntry()) { /* key already exists, this swapfile not being used */ /* junk old, load new */ - currentEntry()->expireNow(); - currentEntry()->releaseRequest(); - - if (currentEntry()->swap_filen > -1) { - UFSSwapDir *sdForThisEntry = dynamic_cast(INDEXSD(currentEntry()->swap_dirn)); - sdForThisEntry->replacementRemove(currentEntry()); - /* Make sure we don't actually unlink the file */ - sdForThisEntry->mapBitReset(currentEntry()->swap_filen); - currentEntry()->swap_filen = -1; - currentEntry()->swap_dirn = -1; - } - - currentEntry()->release(); + undoAdd(); + counts.objcount--; counts.dupcount++; } else { /* URL doesnt exist, swapfile not in use */ /* load new */ (void) 0; } counts.objcount++; currentEntry(sd->addDiskRestore(swapData.key, swapData.swap_filen, swapData.swap_file_sz, swapData.expires, swapData.timestamp, swapData.lastref, swapData.lastmod, swapData.refcount, swapData.flags, (int) flags.clean)); storeDirSwapLog(currentEntry(), SWAP_LOG_ADD); } +/// undo the effects of adding an entry in rebuildFromSwapLog() +void +RebuildState::undoAdd() +{ + StoreEntry *added = currentEntry(); + assert(added); + currentEntry(NULL); + + // TODO: Why bother with these two if we are going to release?! + added->expireNow(); + added->releaseRequest(); + + if (added->swap_filen > -1) { + UFSSwapDir *sde = dynamic_cast(INDEXSD(added->swap_dirn)); + assert(sde); + sde->undoAddDiskRestore(added); + } + + added->release(); +} + int RebuildState::getNextFile(sfileno * filn_p, int *size) { int fd = -1; int dirs_opened = 0; debugs(47, 3, "commonUfsDirGetNextFile: flag=" << flags.init << ", " << sd->index << ": /"<< std::setfill('0') << std::hex << std::uppercase << std::setw(2) << curlvl1 << "/" << std::setw(2) << curlvl2); if (done) return -2; while (fd < 0 && done == 0) { fd = -1; if (0 == flags.init) { /* initialize, open first file */ done = 0; curlvl1 = 0; curlvl2 = 0; === modified file 'src/fs/ufs/ufscommon.h' --- src/fs/ufs/ufscommon.h 2012-01-26 04:51:21 +0000 +++ src/fs/ufs/ufscommon.h 2012-07-18 23:27:27 +0000 @@ -87,40 +87,42 @@ //protected: UFSStrategy *IO; char *fullPath(sfileno, char *) const; /* temp */ void closeTmpSwapLog(); FILE *openTmpSwapLog(int *clean_flag, int *zero_flag); char *swapSubDir(int subdirn) const; int mapBitTest(sfileno filn); void mapBitReset(sfileno filn); void mapBitSet(sfileno filn); StoreEntry *addDiskRestore(const cache_key * key, sfileno file_number, uint64_t swap_file_sz, time_t expires, time_t timestamp, time_t lastref, time_t lastmod, uint32_t refcount, uint16_t flags, int clean); + /// Undo the effects of UFSSwapDir::addDiskRestore(). + void undoAddDiskRestore(StoreEntry *e); int validFileno(sfileno filn, int flag) const; int mapBitAllocate(); virtual ConfigOption *getOptionTree() const; void *fsdata; bool validL2(int) const; bool validL1(int) const; void replacementAdd(StoreEntry *e); void replacementRemove(StoreEntry *e); protected: FileMap *map; int suggest; int l1; int l2; private: void parseSizeL1L2(); @@ -387,36 +389,37 @@ unsigned int need_to_validate:1; unsigned int clean:1; unsigned int init:1; } flags; int in_dir; int done; int fn; dirent_t *entry; DIR *td; char fullpath[MAXPATHLEN]; char fullfilename[MAXPATHLEN]; struct _store_rebuild_data counts; private: CBDATA_CLASS2(RebuildState); void rebuildFromDirectory(); void rebuildFromSwapLog(); void rebuildStep(); + void undoAdd(); int getNextFile(sfileno *, int *size); StoreEntry *currentEntry() const; void currentEntry(StoreEntry *); StoreEntry *e; bool fromLog; bool _done; /// \bug (callback) should be hidden behind a proper human readable name void (callback)(void *cbdata); void *cbdata; }; #if _USE_INLINE_ #include "ufscommon.cci" #endif #endif /* SQUID_UFSCOMMON_H */