Hi there,
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. With this patch, we correctly account for both kinds of
entries.
The patch also removes some code duplication and fixes counts.objcount
maintenance.
TODO: Do not enter the validation loop in storeCleanup unless running
with -S. The loop does nothing useful when storeCleanupDoubleCheck() is
not called. I am not planning to work on that though so please feel free
to tackle it.
Thank you,
Alex.
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<UFSSwapDir *>(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<double>(++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<UFSSwapDir *>(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<UFSSwapDir *>(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 */
