Re: [PATCH] Ignore and warn about attempts to reconfigure static Rock store options.

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?

Thank you,

Alex.
Received on Tue Sep 20 2011 - 05:08:30 MDT