From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; Fri, 12 Apr 2024 12:26:31 +0200 (CEST) Date: Fri, 12 Apr 2024 12:26:30 +0200 From: Christoph Heiss To: Maximiliano Sandoval Cc: Proxmox VE development discussion Message-ID: References: <20240207142824.2613933-1-c.heiss@proxmox.com> <20240207142824.2613933-4-c.heiss@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 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 > > --- > > ... > > ... > > + > > + # 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 > >