Ignore and warn about attempts to reconfigure static Rock store options. Some Rock store options cannot be changed dynamically: path, size, and max-size. Before the change, there were no checks during reconfigure to prevent changing these options. This may lead to Rock cache corruption and other bugs. The patch adds necessary checks to Rock store code. If user tries to change an option that cannot be updated dynamically, a warning is reported and the value is left unchanged. === modified file 'src/SwapDir.cc' --- src/SwapDir.cc 2011-05-12 03:58:16 +0000 +++ src/SwapDir.cc 2011-09-19 22:15:05 +0000 @@ -293,42 +293,51 @@ SwapDir::optionReadOnlyDump(StoreEntry * if (flags.read_only) storeAppendPrintf(e, " no-store"); } bool SwapDir::optionObjectSizeParse(char const *option, const char *value, int isaReconfig) { int64_t *val; if (strcmp(option, "max-size") == 0) { val = &max_objsize; } else if (strcmp(option, "min-size") == 0) { val = &min_objsize; } else return false; if (!value) self_destruct(); int64_t size = strtoll(value, NULL, 10); - if (isaReconfig && *val != size) - debugs(3, 1, "Cache dir '" << path << "' object " << option << " now " << size); + if (isaReconfig && *val != size) { + if (allowOptionReconfigure(option)) { + debugs(3, DBG_IMPORTANT, "cache_dir '" << path << "' object " << + option << " now " << size << " Bytes"); + } else { + debugs(3, DBG_IMPORTANT, "WARNING: cache_dir '" << path << "' " + "object " << option << " cannot be changed dynamically, " << + "value left unchanged (" << *val << " Bytes)"); + return true; + } + } *val = size; return true; } void SwapDir::optionObjectSizeDump(StoreEntry * e) const { if (min_objsize != 0) storeAppendPrintf(e, " min-size=%"PRId64, min_objsize); if (max_objsize != -1) storeAppendPrintf(e, " max-size=%"PRId64, max_objsize); } // some SwapDirs may maintain their indexes and be able to lookup an entry key StoreEntry * SwapDir::get(const cache_key *key) { === modified file 'src/SwapDir.h' --- src/SwapDir.h 2011-05-12 03:58:16 +0000 +++ src/SwapDir.h 2011-09-20 16:14:19 +0000 @@ -145,40 +145,41 @@ public: virtual uint64_t minSize() const; virtual int64_t maxObjectSize() const { return max_objsize; } 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 &) {} /// 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: uint64_t max_size; ///< maximum allocatable size of the storage area public: char *path; int index; /* This entry's index into the swapDirs array */ int64_t min_objsize; int64_t max_objsize; RemovalPolicy *repl; int removals; === modified file 'src/fs/rock/RockSwapDir.cc' --- src/fs/rock/RockSwapDir.cc 2011-09-14 19:59:30 +0000 +++ src/fs/rock/RockSwapDir.cc 2011-09-20 15:39:33 +0000 @@ -237,80 +237,93 @@ Rock::SwapDir::init() } bool Rock::SwapDir::needsDiskStrand() const { return true; } void Rock::SwapDir::parse(int anIndex, char *aPath) { index = anIndex; path = xstrdup(aPath); // cache store is located at path/db String fname(path); fname.append("/rock"); filePath = xstrdup(fname.termedBuf()); - parseSize(); + parseSize(false); parseOptions(0); // Current openForWriting() code overwrites the old slot if needed // and possible, so proactively removing old slots is probably useless. assert(!repl); // repl = createRemovalPolicy(Config.replPolicy); validateOptions(); } void -Rock::SwapDir::reconfigure(int, char *) +Rock::SwapDir::reconfigure(int, char *aPath) { - // TODO: do not update a parameter if we cannot propagate that change - // TODO: warn if reconfigure changes any parameter that we cannot update - parseSize(); + parseSize(true); parseOptions(1); // TODO: can we reconfigure the replacement policy (repl)? validateOptions(); } /// parse maximum db disk size void -Rock::SwapDir::parseSize() +Rock::SwapDir::parseSize(const bool reconfiguring) { const int i = GetInteger(); if (i < 0) fatal("negative Rock cache_dir size value"); - max_size = static_cast(i) << 20; // MBytes to Bytes + const uint64_t new_max_size = + static_cast(i) << 20; // MBytes to Bytes + if (!reconfiguring) + max_size = new_max_size; + else if (new_max_size != max_size) { + debugs(3, DBG_IMPORTANT, "WARNING: cache_dir '" << path << "' size " + "cannot be changed dynamically, value left unchanged (" << + (max_size >> 20) << " MB)"); + } } ConfigOption * Rock::SwapDir::getOptionTree() const { ConfigOptionVector *vector = dynamic_cast(::SwapDir::getOptionTree()); assert(vector); vector->options.push_back(new ConfigOptionAdapter(*const_cast(this), &SwapDir::parseTimeOption, &SwapDir::dumpTimeOption)); return vector; } +bool +Rock::SwapDir::allowOptionReconfigure(const char *const option) const +{ + return strcmp(option, "max-size") != 0 && + ::SwapDir::allowOptionReconfigure(option); +} + /// parses time-specific options; mimics ::SwapDir::optionObjectSizeParse() bool Rock::SwapDir::parseTimeOption(char const *option, const char *value, int reconfiguring) { // TODO: ::SwapDir or, better, Config should provide time-parsing routines, // including time unit handling. Same for size. time_msec_t *storedTime; if (strcmp(option, "swap-timeout") == 0) storedTime = &fileConfig.ioTimeout; else return false; if (!value) self_destruct(); // TODO: handle time units and detect parsing errors better const int64_t parsedValue = strtoll(value, NULL, 10); if (parsedValue < 0) { debugs(3, DBG_CRITICAL, "FATAL: cache_dir " << path << ' ' << option << " must not be negative but is: " << parsedValue); === modified file 'src/fs/rock/RockSwapDir.h' --- src/fs/rock/RockSwapDir.h 2011-09-14 16:34:40 +0000 +++ src/fs/rock/RockSwapDir.h 2011-09-20 16:14:39 +0000 @@ -28,58 +28,59 @@ 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 disconnect(StoreEntry &e); virtual uint64_t currentSize() const; virtual uint64_t currentCount() const; virtual bool doReportStat() const; virtual void swappedOut(const StoreEntry &e); int64_t entryLimitHigh() const { return SwapFilenMax; } ///< Core limit int64_t entryLimitAllowed() const; typedef Ipc::StoreMapWithExtras DirMap; protected: /* protected ::SwapDir API */ virtual bool needsDiskStrand() const; virtual void create(); virtual void init(); virtual ConfigOption *getOptionTree() const; + virtual bool allowOptionReconfigure(const char *const option) const; virtual bool canStore(const StoreEntry &e, int64_t diskSpaceNeeded, int &load) const; virtual StoreIOState::Pointer createStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *); virtual StoreIOState::Pointer openStoreIO(StoreEntry &, StoreIOState::STFNCB *, StoreIOState::STIOCB *, void *); virtual void maintain(); virtual void diskFull(); virtual void reference(StoreEntry &e); virtual bool dereference(StoreEntry &e); virtual void unlink(StoreEntry &e); virtual void statfs(StoreEntry &e) const; /* IORequestor API */ virtual void ioCompletedNotification(); virtual void closeCompleted(); virtual void readCompleted(const char *buf, int len, int errflag, RefCount< ::ReadRequest>); virtual void writeCompleted(int errflag, size_t len, RefCount< ::WriteRequest>); virtual void parse(int index, char *path); - void parseSize(); ///< parses anonymous cache_dir size option + void parseSize(const bool reconfiguring); ///< parses anonymous cache_dir size option void validateOptions(); ///< warns of configuration problems; may quit bool parseTimeOption(char const *option, const char *value, int reconfiguring); void dumpTimeOption(StoreEntry * e) const; void rebuild(); ///< starts loading and validating stored entry metadata ///< used to add entries successfully loaded during rebuild bool addEntry(const int fileno, const DbCellHeader &header, const StoreEntry &from); bool full() const; ///< no more entries can be stored without purging void trackReferences(StoreEntry &e); ///< add to replacement policy scope void ignoreReferences(StoreEntry &e); ///< delete from repl policy scope int64_t diskOffset(int filen) const; int64_t diskOffsetLimit() const; int entryLimit() const { return map->entryLimit(); } friend class Rebuild; const char *filePath; ///< location of cache storage file inside path/ private: