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

On Wed, Jul 29, 2026 at 12:16 PM Sergey Senozhatsky
<[email protected]> wrote:
>
> 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.

You're right, the scenario I had in mind was a bit far-fetched.  It requires
loading dict_a first, then trying to replace it with dict_b before init, with
the second read failing:

    echo "algo=zstd dict=/path/dict_a" > algorithm_params
    echo "algo=zstd dict=/path/dict_b" > algorithm_params  # fails

comp_params_reset() would free dict_a before dict_b fails to load. But in
practice params are set once before init and never change.

I'll simplify this to just the sz <= 0 change in v3.