Re: [RFC pve-container 1/1] mountpoint backup: allow changing backup flag at runtime

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Fri, Sep 04, 2026 at 12:11:19PM +0200, Thomas Ellmenreich wrote:
> On Mon Aug 31, 2026 at 12:36 PM CEST, Filip Schauer wrote:
> 
> [snip]
> 
> >> +# Checks whether the changed properties can be applied at runtime. It only
> >> +# returns true if all changes can be applied in this way. If even one
> >> +# property cannot be applied, returns false to ensure changes are applied
> >> +# as a unit.
> >> +my $mountpoint_config_fast_plug_safe = sub {

Newer code can use `my sub` instead of variables.

> >> +    my ($new, $old) = @_;
> >> +    my @fast_plug_safe_keys = ("backup");

↑ It's more convenient to have this as a hash, and quicker if it's
outside the sub.

my %FAST_PLUG_MP_OPTIONS = (backup => 1, replicate => 1);

> >> +
> >> +    for my $key (uniq(keys %$new, keys %$old)) {
> >> +        next if first { $_ eq $key } @fast_plug_safe_keys;
> >> +        return 0 if ($new->{$key} // '') ne ($old->{$key} // '');
> >
> >  From testing I noticed that when toggling `backup` on a mount point
> > specifying a custom `idmap`, the `backup` property is still marked as
> > pending.
> > The reason seems to be that this is comparing array references. Even if
> > the arrays both hold the same data, the references still differ.
> 
> Ack, thanks for this, stupid mistake.

Not really. And in perl quite annoying.

It should not be too difficult to implement a comparison, but on the
other hand, we *could* look at it this way:
- We have a fixed set of keys which can be changed.
- We do not care which value they get.
- But other keys need to remain the same.
So we could just *delete* those keys from both parsed mount point hashes
and then then call `print_volume` and string-compare the result.
It utilizes `print_property_string` which should be sorting the values
and thus produce a consistent result.

> 
> >
> >> +    }
> >> +
> >> +    1;
> >> +};
> >> +
> >>   sub apply_pending_mountpoint {
> >>       my ($class, $vmid, $conf, $opt, $storecfg, $running) = @_;
> >>   
> >>       my $mp = $class->parse_volume($opt, $conf->{pending}->{$opt});
> >>       my $old = $conf->{$opt};
> >> +
> >> +    if (exists($conf->{$opt}) && $running) {
> >> +        if ($mountpoint_config_fast_plug_safe->($mp, $class->parse_volume($opt, $old))) {
> >> +            return;
> >> +        }
> >> +        die "skip\n"; # don't try to hotplug over existing mp
> >
> > Doesn't this make the later `die "skip\n" if $running && defined($old);`
> > unreachable?
> 
> Good catch, yes. I think it will be unreachable, although I'm not sure what to
> do with the open #TODO, especially since I don't quite understand what the
> exact meaning of "changing" is in this context.

Yeah we can then drop that line.

The case above it is more obvious, the one below less so: they allow
allocating or adding-in a *new* disk. Anything else is a "change" to
something already mounted. But changes to mount points are tricky. If we
only want to change flags, like "read-only", it technically requires
making sure it gets applied to all mount *points* (from a VFS point of
view), unless we can apply the change to the superblock instead.
Changing file systems or replacing a disk is even worse, as then there
are open file handles as well.

> 
> ```perl
> if ($mp->{type} eq 'volume' && $mp->{volume} =~ $PVE::LXC::NEW_DISK_RE) {
>     # more code
> } else {
>     die "skip\n" if $running && defined($old); # TODO: "changing" mount points?
>     # more code
> }
> ```
> 
> [snip]
> 
> 
> 
> 
> 

--