Re: [pve-devel] [PATCH installer v2 3/3] proxinstall: expose arc size setting for zfs bootdisks for all products

[Christoph Heiss <c.heiss@proxmox.com>]

Thanks for the review!

On Fri, Feb 23, 2024 at 04:37:16PM +0100, Maximiliano Sandoval wrote:
>
> Some comments bellow.
>
> Christoph Heiss <c.heiss@proxmox.com> writes:
>
> > For non-PVE products, simply use the ZFS defaults (aka. 50%) and leave
> > unset, if the user never touches that setting.
> >
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> > ...
> > ...
> > +
> > +    # GTKs SpinButton does not support a placeholder value, unfortunaly. That means we also
>
> There is a typo on unfortunately. What exactly do you mean by a
> placeholder value? Like the initial value? If so see comment bellow.

Placeholder as in the `Gtk.Entry:placeholder-text` [0]. Effectively a
separate value that is displayed to the user if the field is empty.

[0] https://docs.gtk.org/gtk3/property.Entry.placeholder-text.html

>
> > +    # have to set the value normally for non-PVE products, where the ZFS default should be used,
> > +    # in case the user does not explicitly set a value.
> > +    # But due to the signal-based nature of GTK, as long as the user never touches the spinbutton,
> > +    # `Proxmox::Install::Config::set_zfs_opt('arc_max', ..)` never gets called, thus the default
> > +    # value (which is 0 for non-PVE) won't get overwritten and no modprobe file is written.
> > +    if ($arc_max > 0) {
> >  	$spinbutton_arc_max->set_value($arc_max);
> > -	push @$labeled_widgets, ['ARC max size', $spinbutton_arc_max, 'MiB'];
> > +    } else {
> > +	# .. but we need to display the "real" value to the user
> > +	$spinbutton_arc_max->set_value($total_memory * 0.5);
> >      }
> >
> > +    # We need to connect the signal afterwards, to avoid triggering it using ->set_value() above.
> >
>
> Alternatively one could init the spin button with the correct values
> e.g.
>
>     my $total_memory = Proxmox::Install::RunEnv::get('total_memory');
>     my $arc_max = Proxmox::Install::Config::get_zfs_opt('arc_max');
>     my $arc_max_value = $total_memory * 0.5;
>     $arc_max_value = $arc_max if $arc_max > 0;
>     my $arc_max_adjusment = Gtk3::Adjustment->new($arc_max_value, $Proxmox::Install::RunEnv::ZFS_ARC_MIN_SIZE_MIB, $total_memory, 1, 10, 0);
>     my $spinbutton_arc_max = Gtk3::SpinButton->new($arc_max_adjusment, 1, 0);
>     $spinbutton_arc_max->set_tooltip_text('Maximum ARC size in megabytes');
>     $spinbutton_arc_max->signal_connect('value-changed' => sub {
>         my $w = shift;
>         Proxmox::Install::Config::set_zfs_opt('arc_max', $w->get_value_as_int());
>     });
>
> Here the numerical values 1, 10, 0, and 1, 0 come from a quick
> inspection of the source for gtk_spin_button_new_with_range and
> gtk_spin_button_new (in the docs [1, 2] there is a [src] link next to
> the Description header).


Well, after testing, this does not really create the wanted behaviour
for PBS and PMG.
For these two products, we want to display the ZFS default value, used
when the `zfs_arc_max` module parameter is unset (aka. 50% of memory).

But the widget itself should return 0 (aka what
Proxmox::Install::RunEnv::default_zfs_arc_max() returns) in case the
user never changed it, such that we can skip writing the file (resulting
in the previous behaviour).

That's why the "workaround" was needed to support that.

>
> [1] https://docs.gtk.org/gtk3/ctor.SpinButton.new.html
> [2] https://docs.gtk.org/gtk3/ctor.SpinButton.new_with_range.html
>
> > +    $spinbutton_arc_max->signal_connect('value-changed' => sub {
> > +	my $w = shift;
> > +	Proxmox::Install::Config::set_zfs_opt('arc_max', $w->get_value_as_int());
> > +    });
> > +
> > +    push @$labeled_widgets, ['ARC max size', $spinbutton_arc_max, 'MiB'];
> > +
> >      push @$labeled_widgets, ['hdsize', $hdsize_btn, 'GB'];
> >      return $create_label_widget_grid->($labeled_widgets);;
> >  };
>
>
> --
> Maximiliano
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>