On 09/20/2011 10:17 AM, Dmitry Kurochkin wrote: > On Tue, 20 Sep 2011 19:41:31 +0400, Dmitry Kurochkin > <dmitry.kuroch...@measurement-factory.com> wrote: >> 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. >> > > Oops, forgot to remove unneeded changes in src/fs/rock/RockSwapDir.h.
Committed as trunk r11754. Thank you, Alex.
