[pve-devel] [PATCH proxmox-backup 0/3] file-restore: set arc limits correctly

[pve-devel] [PATCH proxmox-backup 0/3] file-restore: set arc limits correctly

[Stoiko Ivanov]

The first two patches fix the setting of zfs_arc_min and zfs_arc_max via
the kernel commandline - afaics they should not have been honored until
now - verification was done in the debug-image by inspecting
`grep '^c_' /proc/spl/kstat/zfs/arcstats`

the first 2 patches can  be squashed - it explicitly split them out for
ease of reviewing

the last patch is more as an rfc, because it puzzled my why a debug-log
message was not printed when running with debug.


Stoiko Ivanov (3):
  file-restore: add zfs. prefix to arc_min/max settings
  file-restore: set zfs_arc_min to current minimum of 32M
  file-restore: set loglevel considering PBS_QEMU_DEBUG

 proxmox-file-restore/src/main.rs        | 7 ++++++-
 proxmox-file-restore/src/qemu_helper.rs | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH proxmox-backup 1/3] file-restore: add zfs. prefix to arc_min/max settings

[Stoiko Ivanov]

Currently the values set for zfs_arc_min and zfs_arc_max are ignored
by the kernel:
```
Unknown kernel command line parameters... will be passed to user space
```
module parameters provided on the commandline usually need to be
prefixed with the modulename (e.g. zfs.zfs_arc_min, see [0] for a bit
on related information (the issue itself is not related)).

Paradoxically currently ZFS will print spurious warnings about
settings being ignored when they are actually set - see [1].

Booting the debug image and connecting the shell on the serial console
confirmed that the values did not seem to be set:
`grep '^c_' /proc/spl/kstat/zfs/arcstats` showed half of the memory
for c_max.

[0] https://github.com/openzfs/zfs/issues/698
[1] https://github.com/openzfs/zfs/issues/12504

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 proxmox-file-restore/src/qemu_helper.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/proxmox-file-restore/src/qemu_helper.rs b/proxmox-file-restore/src/qemu_helper.rs
index c49f80d9..12c8cabf 100644
--- a/proxmox-file-restore/src/qemu_helper.rs
+++ b/proxmox-file-restore/src/qemu_helper.rs
@@ -260,7 +260,7 @@ pub async fn start_vm(
         // NOTE: ZFS requires that the ARC can at least grow to the max transaction size of 64MB
         // also: setting any of min/max to zero will rather do the opposite of what one wants here
         &format!(
-            "{} panic=1 zfs_arc_min=16777216 zfs_arc_max=67108864 memhp_default_state=online_kernel",
+            "{} panic=1 zfs.zfs_arc_min=16777216 zfs.zfs_arc_max=67108864 memhp_default_state=online_kernel",
             if debug { "debug" } else { "quiet" }
         ),
         "-daemonize",
-- 
2.30.2

[pve-devel] [PATCH proxmox-backup 2/3] file-restore: set zfs_arc_min to current minimum of 32M

[Stoiko Ivanov]

zfs_arc_min was raised to 32M (for linux) in zfs-commit
121b3cae742a0670d902a51bc61d49dc4a3e4445

while the current logic would still set the min_size to 32M (it's
max(32M, allmem/32), which results to 32M for memory sizes up to
1024M), setting it explicitly to the minimum makes it clear, and will
still be kept should the restore vm have more than 1G of memory at
some point.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 proxmox-file-restore/src/qemu_helper.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/proxmox-file-restore/src/qemu_helper.rs b/proxmox-file-restore/src/qemu_helper.rs
index 12c8cabf..0219a310 100644
--- a/proxmox-file-restore/src/qemu_helper.rs
+++ b/proxmox-file-restore/src/qemu_helper.rs
@@ -260,7 +260,7 @@ pub async fn start_vm(
         // NOTE: ZFS requires that the ARC can at least grow to the max transaction size of 64MB
         // also: setting any of min/max to zero will rather do the opposite of what one wants here
         &format!(
-            "{} panic=1 zfs.zfs_arc_min=16777216 zfs.zfs_arc_max=67108864 memhp_default_state=online_kernel",
+            "{} panic=1 zfs.zfs_arc_min=33554432 zfs.zfs_arc_max=67108864 memhp_default_state=online_kernel",
             if debug { "debug" } else { "quiet" }
         ),
         "-daemonize",
-- 
2.30.2

[pve-devel] [PATCH proxmox-backup 3/3] file-restore: set loglevel considering PBS_QEMU_DEBUG

[Stoiko Ivanov]

during some tests recently I wondered why a debug log-message was not
printed, despite running with PBS_QEMU_DEBUG.

This patch sets the loglevel for the cli logger to debug if the
variable is present and not-empty (see qemu_helper.rs for the other
usage).

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 proxmox-file-restore/src/main.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
index 87caadc4..74ef1979 100644
--- a/proxmox-file-restore/src/main.rs
+++ b/proxmox-file-restore/src/main.rs
@@ -585,7 +585,12 @@ where
 }
 
 fn main() {
-    init_cli_logger("PBS_LOG", "info");
+    let loglevel = match std::env::var("PBS_QEMU_DEBUG") {
+        Ok(val) if !val.is_empty() => "debug",
+        _ => "info"
+    };
+
+    init_cli_logger("PBS_LOG", loglevel);
 
     let list_cmd_def = CliCommand::new(&API_METHOD_LIST)
         .arg_param(&["snapshot", "path"])
-- 
2.30.2

[pve-devel] applied: [PATCH proxmox-backup 0/3] file-restore: set arc limits correctly

[Thomas Lamprecht]

Am 15/06/2023 um 12:42 schrieb Stoiko Ivanov:
> The first two patches fix the setting of zfs_arc_min and zfs_arc_max via
> the kernel commandline - afaics they should not have been honored until
> now - verification was done in the debug-image by inspecting
> `grep '^c_' /proc/spl/kstat/zfs/arcstats`
> 
> the first 2 patches can  be squashed - it explicitly split them out for
> ease of reviewing
> 
> the last patch is more as an rfc, because it puzzled my why a debug-log
> message was not printed when running with debug.
> 
> 
> Stoiko Ivanov (3):
>   file-restore: add zfs. prefix to arc_min/max settings
>   file-restore: set zfs_arc_min to current minimum of 32M
>   file-restore: set loglevel considering PBS_QEMU_DEBUG
> 
>  proxmox-file-restore/src/main.rs        | 7 ++++++-
>  proxmox-file-restore/src/qemu_helper.rs | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 


applied, thanks!

I made a follow-up to factor out the is-debug-mode logic in file-restore, not that
it gets out of sync when exactly it gets enabled (e.g., for some tools it's enough
if such a variable is set, even if empty, so if one would change one side but not
the other it could get confusing).