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

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.

Regards,
  Dmitry

> Regards,
> Dmitry
>
> >
> > Thank you,
> >
> > Alex.