Re: [PATCH 2/3] zram: add per-backend capability flags and validate parameters early

On Tue, Jul 28, 2026 at 9:19 AM Sergey Senozhatsky
<[email protected]> wrote:
>
> On (26/07/28 00:50), haoqin huang wrote:
> > On Fri, Jul 24, 2026 at 1:28 PM Sergey Senozhatsky
> > <[email protected]> wrote:
> > >
> > > On (26/06/27 15:02), Haoqin Huang wrote:
> > > > Writing dict or level parameters for algorithms that don't support
> > > > them was silently accepted but had no effect.  Out-of-range levels
> > > > were silently clamped by the underlying library.  Dict read failures
> > > > always lost the real error from kernel_read_file_from_path().
> > > >
> > > > Add caps, level_min and level_max to zcomp_ops and validate
> > > > user-supplied parameters in algorithm_params_store() before storing,
> > > > giving immediate error feedback. Also fix comp_params_store() to
> > > > read the new dict into a temporary buffer before resetting old
> > > > parameters, making the update atomic.
> > >
> > > I probably would prefer not to add this.  Again, zram setup is almost
> > > always automated, you figure out what you need to put into your init
> > > script once and you never touch it again.
> > >
> >
> > That's a fair point. The sysfs validation is admittedly defensive.
> >
> > My thinking was just that without any feedback, a misconfiguration can
> > be hard to notice. For instance:
> >
> >  algo=lzo dict=/data/dict    -> setup_params() silently discards it
> >  algo=deflate dict=/data/dict -> same, silently ignored
> >  algo=zstd level=999         -> silently clamped to 22 by the library
> >
> > All of these succeed without a peep -- not a crash, but also no
> > indication that anything was wrong.
>
> We were thinking about moving from zcomp to crypto API acomp.  So I'm
> not sure if we want to invest a lot of time into zcomp.  If you want
> to have it then I'd ask to split caps and kernel_read_file_from_path()
> zero i_size check into separate patches.  We also probably want some
> pr_err() on failed validation, because just returning -EINVAL doesn't
> explain the error and doesn't help the (supposedly) clueless user
> who attempts to configure zram by throwing random numbers at it.
>

Sounds good.  I'll add pr_err() on validation failures in the caps
patch and post v2 with the split series.

> > > The 0 i_size for CD-dict is something that simply should not happen.
> > > If you insist on handling that then we can replace "sz < 0" with "sz <= 0",
> > > but that 0 len dictionary case is something purely theoretical.
> >
> > That's fine, I'll use sz <= 0 in v2.
> >
> > Anyway, let me split the original patch so the non-controversial part
> > stands on its own:
> >
> >   [2/3] zram: make dict update in comp_params_store() atomic
> >
> >     Move comp_params_reset() after kernel_read_file_from_path(), so an
> >     I/O error doesn't leave the old dict freed with nothing to replace
> >     it.  Also preserve the actual error code.
> >
> >   [3/3] zram: add per-backend caps and validate parameters early
> >
> >     Optional -- happy to drop if you still feel it's unnecessary.
>
> Sounds good.