[pve-devel] [PATCH common/storage/container] add fsfreeze/thaw for rbd snapshots

[pve-devel] [PATCH common/storage/container] add fsfreeze/thaw for rbd snapshots

[Stoiko Ivanov]

this patchset addresses #2991 and #2528.

As discussed in #2991 (and off-list with Wolfgang B. and Dominik) - it does
not address the fundamental problem of the snapshot being created outside of
the open krbd block-device, by an independend 'rbd' call (which is most likely
the reason for the inconsistency).

However according to the reporter in #2991 it does help in their case to
actually get backups of their containers.

I put the ioctl call inside sync_container_namespace since it:
* should happen shortly after the syncfs call
* needs to happen inside the container's mount namespace (else we'd need to
  mount the filesystem in order to freeze/thaw it - see the proposed patch
  in #2528)

and I wanted to avoid to fork+nsenter for each volume twice (in
__snapshot_create_vol_snapshs_hook)

Would be grateful for feedback if this approach is ok (reading containerconfig
+ storage config in __snapshot_freeze) or if some other way would be nicer.

Tested on my testsetup with a ceph-backed container (and 2 additional
mountpoints (one ceph, one on LVM thin).


pve-common:
Stoiko Ivanov (1):
  add fsfreeze helper:

 src/PVE/Tools.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

pve-storage:
Stoiko Ivanov (1):
  add check for fsfreeze before snapshot

 PVE/Storage.pm           | 12 ++++++++++++
 PVE/Storage/Plugin.pm    |  4 ++++
 PVE/Storage/RBDPlugin.pm |  5 +++++
 3 files changed, 21 insertions(+)

pve-container:
Stoiko Ivanov (1):
  snapshot creation: fsfreeze mountpoints, if needed

 src/PVE/LXC.pm            | 21 +++++++++++++++++----
 src/PVE/LXC/Config.pm     | 15 ++++++++++++++-
 src/test/snapshot-test.pm | 12 +++++++++++-
 3 files changed, 42 insertions(+), 6 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH common] add fsfreeze helper:

[Stoiko Ivanov]

fsfreeze_mountpoint issues the same ioctl's as fsfreeze(8) on the provided
directory (the $thaw parameter deciding between '--freeze' and '--unfreeze')

This is used for container backups on RBD, where snapshots on containers, which
are heavy on IO, are not mountable readonly, because the ext4 is not consistent

Needed to fix #2991 and #2528.

The ioctl numbers were found via strace -X verbose (and compared with the
kernel source).

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PVE/Tools.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 4b445ea..c61fc49 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -100,6 +100,9 @@ use constant {O_PATH    => 0x00200000,
 use constant {AT_EMPTY_PATH => 0x1000,
               AT_FDCWD => -100};
 
+use constant {FIFREEZE => 0xc0045877,
+              FITHAW   => 0xc0045878};
+
 sub run_with_timeout {
     my ($timeout, $code, @param) = @_;
 
@@ -1442,6 +1445,21 @@ sub sync_mountpoint {
     die "syncfs '$path' failed - $syncfs_err\n" if defined $syncfs_err;
 }
 
+sub fsfreeze_mountpoint {
+    my ($path, $thaw) = @_;
+
+    my $op = $thaw ? 'thaw' : 'freeze';
+    my $ioctl = $thaw ? FITHAW : FIFREEZE;
+
+    sysopen my $fd, $path, O_RDONLY|O_CLOEXEC or die "failed to open $path: $!\n";
+    my $ioctl_err;
+    if (!ioctl($fd, $ioctl, 0)) {
+	$ioctl_err = "$!";
+    }
+    close($fd);
+    die "fs$op '$path' failed - $ioctl_err\n" if defined $ioctl_err;
+}
+
 # support sending multi-part mail messages with a text and or a HTML part
 # mailto may be a single email string or an array of receivers
 sub sendmail {
-- 
2.20.1

[pve-devel] [PATCH storage 1/1] add check for fsfreeze before snapshot

[Stoiko Ivanov]

In order to take a snapshot of a container volume, which can be mounted
read-only with RBD, the volume needs to be frozen (fsfreeze (8)) before taking
the snapshot. Only RBD is affected by this (for LVMThin this is handled
by the devicemapper).

This commit adds helpers to determine if the FIFREEZE ioctl needs to be called
for the volume.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 PVE/Storage.pm           | 12 ++++++++++++
 PVE/Storage/Plugin.pm    |  4 ++++
 PVE/Storage/RBDPlugin.pm |  5 +++++
 3 files changed, 21 insertions(+)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index cd7b5ff..27223b6 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -287,6 +287,18 @@ sub volume_snapshot_delete {
     }
 }
 
+# check if a filesystem on top of a volume needs to flush it's journal for
+# constency (see fsfreeze(8)) before a snapshot is taken - needed for
+# container mountpoints
+sub volume_snapshot_needs_fsfreeze {
+    my ($cfg, $volid) = @_;
+
+    my ($storeid, $volname) = parse_volume_id($volid);
+    my $scfg = storage_config($cfg, $storeid);
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+    return $plugin->volume_snapshot_needs_fsfreeze();
+}
+
 # check if a volume or snapshot supports a given feature
 # $feature - one of:
 #            clone - linked clone is possible
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 63f68da..a046640 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -888,6 +888,10 @@ sub volume_snapshot_delete {
     return undef;
 }
 
+sub volume_snapshot_needs_fsfreeze {
+
+    return 0;
+}
 sub storage_can_replicate {
     my ($class, $scfg, $storeid, $format) = @_;
 
diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index 38f2b46..94df89d 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -694,6 +694,11 @@ sub volume_snapshot_delete {
     return undef;
 }
 
+sub volume_snapshot_needs_fsfreeze {
+
+    return 1;
+}
+
 sub volume_has_feature {
     my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
 
-- 
2.20.1

[pve-devel] [PATCH container 1/1] snapshot creation: fsfreeze mountpoints, if needed

[Stoiko Ivanov]

fixes #2991, #2528.

creating a snapshot with rbd, after the syncfs finished successfully does not
guarantee that the snapshot has the state of the filesystem after syncfs.

suggestion taken from #2528 (running fsfreeze -f/-u before snapshotting on
the mountpoints)

added helper PVE::Storage::volume_snapshot_needs_fsfreeze, to indicate
which volumes need to be frozen/thawed. (and mocked it in the tests here).

Added to sync_container_namespace, since fsfreeze needs to run inside the
container's mount namespace.

tests in #2991 seem to indicate that this helps to successfully create backups.

needs a versioned dependency bump on pve-common and pve-storage

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PVE/LXC.pm            | 21 +++++++++++++++++----
 src/PVE/LXC/Config.pm     | 15 ++++++++++++++-
 src/test/snapshot-test.pm | 12 +++++++++++-
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 9c150d9..6f87c96 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1091,7 +1091,7 @@ my $get_container_namespace = sub {
 };
 
 my $do_syncfs = sub {
-    my ($vmid, $pid, $socket) = @_;
+    my ($vmid, $pid, $socket, $freeze_mountpoints, $thaw) = @_;
 
     &$enter_namespace($vmid, $pid, 'mnt', PVE::Tools::CLONE_NEWNS);
 
@@ -1121,13 +1121,26 @@ my $do_syncfs = sub {
     foreach my $mp (@$mounts) {
 	my ($what, $dir, $fs) = @$mp;
 	next if $nosyncfs{$fs};
-	eval { PVE::Tools::sync_mountpoint($dir); };
+	eval {
+
+	    PVE::Tools::sync_mountpoint($dir) if !$thaw;
+	};
+
+	warn $@ if $@;
+    }
+
+    # freeze/unfreeze the mountpoint which need it
+    foreach my $dir (@$freeze_mountpoints) {
+
+	eval {
+	    PVE::Tools::fsfreeze_mountpoint($dir, $thaw);
+	};
 	warn $@ if $@;
     }
 };
 
 sub sync_container_namespace {
-    my ($vmid) = @_;
+    my ($vmid, $freeze_mountpoints, $thaw) = @_;
     my $pid = find_lxc_pid($vmid);
 
     # SOCK_DGRAM is nicer for barriers but cannot be slurped
@@ -1140,7 +1153,7 @@ sub sync_container_namespace {
     if (!$child) {
 	eval {
 	    close $pfd;
-	    &$do_syncfs($vmid, $pid, $cfd);
+	    &$do_syncfs($vmid, $pid, $cfd, $freeze_mountpoints, $thaw);
 	};
 	if (my $err = $@) {
 	    warn $err;
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 49f599b..f7b763b 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -112,12 +112,25 @@ sub __snapshot_check_freeze_needed {
 sub __snapshot_freeze {
     my ($class, $vmid, $unfreeze) = @_;
 
+    my $conf = $class->load_config($vmid);
+    my $storagecfg = PVE::Storage::config();
+
+    my $freeze_mps = [];
+    $class->foreach_volume($conf, sub {
+	my ($ms, $mountpoint) = @_;
+
+	if (PVE::Storage::volume_snapshot_needs_fsfreeze($storagecfg, $mountpoint->{volume})) {
+	    push @$freeze_mps, $mountpoint->{mp};
+	}
+    });
+
     if ($unfreeze) {
 	eval { PVE::LXC::thaw($vmid); };
 	warn $@ if $@;
+	PVE::LXC::sync_container_namespace($vmid, $freeze_mps, 1);
     } else {
 	PVE::LXC::freeze($vmid);
-	PVE::LXC::sync_container_namespace($vmid);
+	PVE::LXC::sync_container_namespace($vmid, $freeze_mps, 0);
     }
 }
 
diff --git a/src/test/snapshot-test.pm b/src/test/snapshot-test.pm
index bfb8551..91a2af9 100644
--- a/src/test/snapshot-test.pm
+++ b/src/test/snapshot-test.pm
@@ -113,6 +113,15 @@ sub mocked_volume_rollback_is_possible {
     die "volume_rollback_is_possible failed\n";
 }
 
+sub mocked_volume_snapshot_needs_fsfreeze {
+    my ($storecfg, $volid) = @_;
+    die "Storage config not mocked! aborting\n"
+	if defined($storecfg);
+    die "volid undefined\n"
+	if !defined($volid);
+    return 0;
+}
+
 sub mocked_vm_stop {
     if ($kill_possible) {
 	$running = 0;
@@ -386,7 +395,8 @@ $storage_module->mock('volume_snapshot', \&mocked_volume_snapshot);
 $storage_module->mock('volume_snapshot_delete', \&mocked_volume_snapshot_delete);
 $storage_module->mock('volume_snapshot_rollback', \&mocked_volume_snapshot_rollback);
 $storage_module->mock('volume_rollback_is_possible', \&mocked_volume_rollback_is_possible);
-printf("\tconfig(), volume_snapshot(), volume_snapshot_delete(), volume_snapshot_rollback() and volume_rollback_is_possible() mocked\n");
+$storage_module->mock('volume_snapshot_needs_fsfreeze', \&mocked_volume_snapshot_needs_fsfreeze);
+printf("\tconfig(), volume_snapshot(), volume_snapshot_delete(), volume_snapshot_rollback(), volume_rollback_is_possible() and volume_snapshot_needs_fsfreeze() mocked\n");
 
 printf("\n");
 printf("Setting up Mocking for PVE::Tools\n");
-- 
2.20.1

Re: [pve-devel] [PATCH storage 1/1] add check for fsfreeze before snapshot

[Thomas Lamprecht]

On 05.11.20 17:06, Stoiko Ivanov wrote:
> In order to take a snapshot of a container volume, which can be mounted
> read-only with RBD, the volume needs to be frozen (fsfreeze (8)) before taking
> the snapshot. Only RBD is affected by this (for LVMThin this is handled
> by the devicemapper).
> 
> This commit adds helpers to determine if the FIFREEZE ioctl needs to be called
> for the volume.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  PVE/Storage.pm           | 12 ++++++++++++
>  PVE/Storage/Plugin.pm    |  4 ++++
>  PVE/Storage/RBDPlugin.pm |  5 +++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index cd7b5ff..27223b6 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -287,6 +287,18 @@ sub volume_snapshot_delete {
>      }
>  }
>  
> +# check if a filesystem on top of a volume needs to flush it's journal for
> +# constency (see fsfreeze(8)) before a snapshot is taken - needed for
> +# container mountpoints
> +sub volume_snapshot_needs_fsfreeze {
> +    my ($cfg, $volid) = @_;
> +
> +    my ($storeid, $volname) = parse_volume_id($volid);
> +    my $scfg = storage_config($cfg, $storeid);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +    return $plugin->volume_snapshot_needs_fsfreeze();
> +}
> +
>  # check if a volume or snapshot supports a given feature
>  # $feature - one of:
>  #            clone - linked clone is possible
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 63f68da..a046640 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -888,6 +888,10 @@ sub volume_snapshot_delete {
>      return undef;
>  }
>  
> +sub volume_snapshot_needs_fsfreeze {

expanding the storage plugin ABI should normally be recorded via the version/
age mechanism. New additions which do not break existing plugins should bump
both, max age and version. Else, external plugins cannot know if they can rely
on that method.

> +
> +    return 0;
> +}
>  sub storage_can_replicate {
>      my ($class, $scfg, $storeid, $format) = @_;
>  
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 38f2b46..94df89d 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -694,6 +694,11 @@ sub volume_snapshot_delete {
>      return undef;
>  }
>  
> +sub volume_snapshot_needs_fsfreeze {
> +
> +    return 1;
> +}
> +
>  sub volume_has_feature {
>      my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
>  
>