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

On 09/20/2011 10:17 AM, Dmitry Kurochkin wrote:
> On Tue, 20 Sep 2011 19:41:31 +0400, Dmitry Kurochkin <dmitry.kurochkin_at_measurement-factory.com> wrote:
>> On Tue, 20 Sep 2011 09:08:33 -0600, Alex Rousskov <rousskov_at_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 <rousskov_at_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.
>>
>
> Oops, forgot to remove unneeded changes in src/fs/rock/RockSwapDir.h.

Committed as trunk r11754.

Thank you,

Alex.
Received on Thu Sep 22 2011 - 21:43:27 MDT