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

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

[Stoiko Ivanov]

this patchset addresses #2991 and #2528.

v2->v3:
* incoroprated Wolfgang's feedback (huge thanks!!):
** /proc/$pid/root contains as magic-link [0] to the container's rootfs -
   so use these for the FIFREEZE/FITHAW ioctls instead of fork+nsenter
** thus moved the fsfreeze_mountpoint to PVE::LXC::Config (it's only needed
   there)

[0] https://www.kernel.org/doc/html/latest/filesystems/path-lookup.html

original cover-letter for the v2:
v1->v2:
mostly incorporated Thomas' feedback (huge thanks!!):
* moved fsfreeze from pve-common to pve-container (it's only used here, and
  it avoids one versioned dependency bump).
* for this needed to drop O_CLOEXEC (only defined in PVE::Tools) flag from
  sysopen (fsfreeze(8) does not use it either...)
* increased APIVER and APIAGE in PVE::Storage
* don't use sync_container_namespace for unfreezing (no need to parse
  /proc/mounts and pass it to the child process) - moved that part to a new
  sub
* found a tiny typo in a comment (patch 1/2 for pve-storage)

original cover-letter for v1:

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-storage:
Stoiko Ivanov (2):
  fix typo in comment
  add check for fsfreeze before snapshot

 PVE/Storage.pm           | 18 +++++++++++++++---
 PVE/Storage/Plugin.pm    |  4 ++++
 PVE/Storage/RBDPlugin.pm |  5 +++++
 3 files changed, 24 insertions(+), 3 deletions(-)
i
pve-container:
Stoiko Ivanov (2):
  add fsfreeze helper:
  snapshot creation: fsfreeze mountpoints, if needed

 src/PVE/LXC/Config.pm     | 47 +++++++++++++++++++++++++++++++++++++++
 src/test/snapshot-test.pm | 12 +++++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

-- 
2.20.1

[pve-devel] [PATCH storage v3 1/2] fix typo in comment

[Stoiko Ivanov]

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 PVE/Storage.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index cd7b5ff..ad10827 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -40,7 +40,7 @@ use PVE::Storage::ZFSPlugin;
 use PVE::Storage::DRBDPlugin;
 use PVE::Storage::PBSPlugin;
 
-# Storage API version. Icrement it on changes in storage API interface.
+# Storage API version. Increment it on changes in storage API interface.
 use constant APIVER => 6;
 # Age is the number of versions we're backward compatible with.
 # This is like having 'current=APIVER' and age='APIAGE' in libtool,
-- 
2.20.1

[pve-devel] [PATCH storage v3 2/2] 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.

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           | 16 ++++++++++++++--
 PVE/Storage/Plugin.pm    |  4 ++++
 PVE/Storage/RBDPlugin.pm |  5 +++++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index ad10827..18eef46 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -41,11 +41,11 @@ use PVE::Storage::DRBDPlugin;
 use PVE::Storage::PBSPlugin;
 
 # Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 6;
+use constant APIVER => 7;
 # Age is the number of versions we're backward compatible with.
 # This is like having 'current=APIVER' and age='APIAGE' in libtool,
 # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
-use constant APIAGE => 5;
+use constant APIAGE => 6;
 
 # load standard plugins
 PVE::Storage::DirPlugin->register();
@@ -287,6 +287,18 @@ sub volume_snapshot_delete {
     }
 }
 
+# check if a filesystem on top of a volume needs to flush its journal for
+# consistency (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 v3 1/2] add fsfreeze helper:

[Stoiko Ivanov]

fsfreeze_mountpoint issues the same ioctls 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 verified with the
kernel documentation).

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

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 49f599b..0528d51 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -13,6 +13,9 @@ use PVE::Tools;
 
 use base qw(PVE::AbstractConfig);
 
+use constant {FIFREEZE => 0xc0045877,
+              FITHAW   => 0xc0045878};
+
 my $nodename = PVE::INotify::nodename();
 my $lock_handles =  {};
 my $lockdir = "/run/lock/lxc";
@@ -109,6 +112,22 @@ sub __snapshot_check_freeze_needed {
     return ($ret, $ret);
 }
 
+# implements similar functionality to fsfreeze(8)
+sub fsfreeze_mountpoint {
+    my ($path, $thaw) = @_;
+
+    my $op = $thaw ? 'thaw' : 'freeze';
+    my $ioctl = $thaw ? FITHAW : FIFREEZE;
+
+    sysopen my $fd, $path, O_RDONLY 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;
+}
+
 sub __snapshot_freeze {
     my ($class, $vmid, $unfreeze) = @_;
 
-- 
2.20.1

[pve-devel] [PATCH container v3 2/2] 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 the freeze to sync_container_namespace, since it needs to run inside the
container's mount namespace.

unfreezing happens in a sub of its own.

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

needs a versioned dependency bump on pve-storage

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PVE/LXC/Config.pm     | 28 ++++++++++++++++++++++++++++
 src/test/snapshot-test.pm | 12 +++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0528d51..9834866 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -2,6 +2,7 @@ package PVE::LXC::Config;
 
 use strict;
 use warnings;
+use Fcntl qw(O_RDONLY);
 
 use PVE::AbstractConfig;
 use PVE::Cluster qw(cfs_register_file);
@@ -131,12 +132,39 @@ sub fsfreeze_mountpoint {
 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};
+	}
+    });
+
+    my $freeze_mountpoints = sub {
+	my ($thaw) = @_;
+
+	return if scalar(@$freeze_mps) == 0;
+
+	my $pid = PVE::LXC::find_lxc_pid($vmid);
+
+	for my $mp (@$freeze_mps) {
+	    eval{ fsfreeze_mountpoint("/proc/${pid}/root/${mp}", $thaw); };
+	    warn $@ if $@;
+	}
+    };
+
     if ($unfreeze) {
 	eval { PVE::LXC::thaw($vmid); };
 	warn $@ if $@;
+	$freeze_mountpoints->(1);
     } else {
 	PVE::LXC::freeze($vmid);
 	PVE::LXC::sync_container_namespace($vmid);
+	$freeze_mountpoints->(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

[pve-devel] applied-series: [PATCH container/storage v3] add fsfreeze/thaw for rbd snapshots

[Thomas Lamprecht]

On 06.11.20 15:19, Stoiko Ivanov wrote:
> this patchset addresses #2991 and #2528.
> 
> v2->v3:
> * incoroprated Wolfgang's feedback (huge thanks!!):
> ** /proc/$pid/root contains as magic-link [0] to the container's rootfs -
>    so use these for the FIFREEZE/FITHAW ioctls instead of fork+nsenter
> ** thus moved the fsfreeze_mountpoint to PVE::LXC::Config (it's only needed
>    there)
> 
> [0] https://www.kernel.org/doc/html/latest/filesystems/path-lookup.html


applied series, thanks!