Re: [PATCH v2 2/5] zram: make dict update in comp_params_store() atomic

On (26/07/29 12:06), haoqin huang wrote:
> > On (26/07/28 17:29), Haoqin Huang wrote:
[..]
> > > @@ -1699,21 +1699,23 @@ static int comp_params_store(struct zram *zram, u32 prio, s32 level,
> > >                            const char *dict_path,
> > >                            struct deflate_params *deflate_params)
> > >  {
> > > +     void *new_dict = NULL;
> > >       ssize_t sz = 0;
> > >
> > > -     comp_params_reset(zram, prio);
> >
> > I don't see why is that a problem.  All you wanted to do here is to
> > handle zero i_size.  Why do we need dict setting to be atomic?
> >
> 
> comp_params_reset() calls vfree() on the old dict and resets level/
> winbits to NOT_SET before reading the new dict

But what is the scenario here?  Who would have several dicts?

  echo "dict=/etc/dict.foo prio=1" > algorithm_params

and if that fails then

  echo "dict=/etc/dict.bar prio=1" > algorithm_params

I don't think this is something that we need to consider.  What am I
missing?

[..]
> The "atomic" in the subject is about all-or-nothing semantics: don't
> destroy valid state until the replacement is confirmed good.

I don't think that "valid state configuration replacement" ever happens.