From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <c.heiss@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id BB1B7957A0
for <pve-devel@lists.proxmox.com>; Fri, 12 Apr 2024 12:27:02 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id A35898810
for <pve-devel@lists.proxmox.com>; Fri, 12 Apr 2024 12:26:32 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS
for <pve-devel@lists.proxmox.com>; Fri, 12 Apr 2024 12:26:31 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id BEE98450DF
for <pve-devel@lists.proxmox.com>; Fri, 12 Apr 2024 12:26:31 +0200 (CEST)
Date: Fri, 12 Apr 2024 12:26:30 +0200
From: Christoph Heiss <c.heiss@proxmox.com>
To: Maximiliano Sandoval <m.sandoval@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Message-ID: <tsawevmthckio7njfgkyhmr5njgjt5tsnkuumwdnyqenbzenh6@ewyyldk4awue>
References: <20240207142824.2613933-1-c.heiss@proxmox.com>
<20240207142824.2613933-4-c.heiss@proxmox.com>
<s8owmqvtc5a.fsf@proxmox.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <s8owmqvtc5a.fsf@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.000 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH installer v2 3/3] proxinstall: expose arc
size setting for zfs bootdisks for all products
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 12 Apr 2024 10:27:02 -0000
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
>
>