On Tue, 20 Sep 2011 09:08:33 -0600, Alex Rousskov
<rouss...@measurement-factory.com> wrote:
> On 09/20/2011 04:30 AM, Dmitry Kurochkin wrote:
> > Hi Alex, Amos.
> >
> > On Mon, 19 Sep 2011 23:07:43 -0600, Alex Rousskov
> > <rouss...@measurement-factory.com> wrote:
> >> On 09/19/2011 08:10 PM, Amos Jeffries wrote:
> >>> On Tue, 20 Sep 2011 02:40:13 +0400, Dmitry Kurochkin wrote:
> >>>> Attached patch does some polishing for Rock store cache_dir
> >>>> reconfiguration.
> >>>>
> >>>>
> >>>> 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.
> >>
> >>
> >>
> >>> Of itself the patch looks okay.
> >>>
> >>> BUT... IMO _path_ should never be re-configurable.
> >>
> >> Good point! I wonder whether admins expect cache_dir lines to be tied to
> >> paths (e.g., changing cache_dir order changes nothing) or to position
> >> (i.e., changing path can be supported). We should document what we
> >> actually support (but that documentation change, if needed, is outside
> >> this patch scope).
> >>
> >>
> >>> If rock store can't handle multiple cache_dir of type rock with unique
> >>> paths there is something deeper wrong.
> >>
> >> It can. Nothing special here.
> >>
> >>
> >>> AFAIK path should be the unique key to identify different cache_dir.
> >>> Changing the path means a completely different dir is now being
> >>> referenced. If not already loaded the dir needs to be loaded as if on
> >>> startup. If any existing are no longer referenced the old cache_dir
> >>> details needs discarding.
> >>
> >> I tend to agree: Path-matching is probably better than position-matching.
> >>
> >> Dmitry, could you please check whether current code maps cache_dir lines
> >> to SwapDir objects using cache_dir position in squid.conf or using
> >> cache_dir paths? If it is the latter, you can remove the path-related
> >> warning from the patch, right?
> >>
> >
> > Cache dir mapping uses paths (though it ignores case). So it works as
> > you would expect. I knew that when I was working on the patch and I
> > still included the path check. Because it feels to me like we should
> > check the path if it is passed to SwapDir::reconfigure(). Otherwise we
> > should not pass path to SwapDir::reconfigure() at all. (There is also
> > index parameter which should be checked or removed as well following the
> > same arguments.) What do you think about:
> >
> > * remove path check from this patch
> >
> > * prepare another patch that removes index and path parameters from
> > SwapDir::reconfigure()
>
>
> Sounds like a good plan to me.
>
Attached patch without the path check.
Regards,
Dmitry
>
> Thank you,
>
> Alex.
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-19 17:57:28 +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<uint64_t>(i) << 20; // MBytes to Bytes
+ const uint64_t new_max_size =
+ static_cast<uint64_t>(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<ConfigOptionVector*>(::SwapDir::getOptionTree());
assert(vector);
vector->options.push_back(new ConfigOptionAdapter<SwapDir>(*const_cast<SwapDir *>(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-19 17:57:36 +0000
@@ -7,79 +7,80 @@
#include "fs/rock/RockDbCell.h"
#include "ipc/StoreMap.h"
class DiskIOStrategy;
class ReadRequest;
class WriteRequest;
namespace Rock
{
class Rebuild;
/// \ingroup Rock
class SwapDir: public ::SwapDir, public IORequestor
{
public:
SwapDir();
virtual ~SwapDir();
/* public ::SwapDir API */
- virtual void reconfigure(int, char *);
+ virtual void reconfigure(int, char *aPath);
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<DbCellHeader> 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:
