[pve-devel] [PATCH common/container] fix syncfs usage for snapshot backup

[pve-devel] [PATCH common/container] fix syncfs usage for snapshot backup

[Stoiko Ivanov]

while investigating #2991 (and probably related #1911) I straced pvedaemon
noticing that the syncfs-calls all failed (errno EBADF).

This happens because we open the mountpoint path with O_PATH (open(2)), which
causes syncfs to fail.

Since I expect syncfs failing to be a potential cause for the inconstent
state of containers on ceph-storages while backing up in snapshot mode this
patchset hopefully mitigates the reported issues.

(if this is not the solution, I was also investigating calling
ioctl(fd, FIFREEZE) on the mountpoint before creating the snapshot)

The patch to pve-container is maybe not necessary, but should save us a few
syscalls, which don't help.

Would be grateful for review from more experienced eyes - I ran limted tests
with the patches applied - creating snapshot backups on a container with
lvmthin backed rootfs and on one with ceph backed rootfs.

pve-common
Stoiko Ivanov (1):
  sync_mountpoint: open path so that sync works

 src/PVE/Tools.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

pve-container:
Stoiko Ivanov (1):
  sync_container_namespace: skip virtual filesystems

 src/PVE/LXC.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.20.1

[pve-devel] [PATCH common 1/1] sync_mountpoint: open path so that sync works

[Stoiko Ivanov]

sync_mountpoint takes a path, gets an open filedescriptor and calls
syncfs(2) on it.
by opening with O_PATH the syncfs call fails with EBADF (see open(2)).

found by running:
```
pkill -f 'pvedaemon worker';
strace -yyttT -s 512 -o /tmp/trace -fp $(pgrep -f pvedaemon$)
```

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

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index e04b504..7eb1197 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -94,6 +94,7 @@ use constant {CLONE_NEWNS   => 0x00020000,
               CLONE_NEWNET  => 0x40000000};
 
 use constant {O_PATH    => 0x00200000,
+              O_CLOEXEC => 0x00080000,
               O_TMPFILE => 0x00410000}; # This includes O_DIRECTORY
 
 use constant {AT_EMPTY_PATH => 0x1000,
@@ -1432,7 +1433,7 @@ sub fsync($) {
 
 sub sync_mountpoint {
     my ($path) = @_;
-    sysopen my $fd, $path, O_PATH or die "failed to open $path: $!\n";
+    sysopen my $fd, $path, O_RDONLY|O_CLOEXEC or die "failed to open $path: $!\n";
     my $result = syncfs(fileno($fd));
     close($fd);
     return $result;
-- 
2.20.1

[pve-devel] applied: Re: [PATCH common 1/1] sync_mountpoint: open path so that sync works

[Thomas Lamprecht]

On 9/17/20 9:17 PM, Stoiko Ivanov wrote:
> sync_mountpoint takes a path, gets an open filedescriptor and calls
> syncfs(2) on it.
> by opening with O_PATH the syncfs call fails with EBADF (see open(2)).
> 
> found by running:
> ```
> pkill -f 'pvedaemon worker';
> strace -yyttT -s 512 -o /tmp/trace -fp $(pgrep -f pvedaemon$)
> ```
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PVE/Tools.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied, thanks! Followed up with a change in error propagation, after checking
the call sites (which is only one).

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 7eb1197..7d33683 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1434,9 +1434,12 @@ sub fsync($) {
 sub sync_mountpoint {
     my ($path) = @_;
     sysopen my $fd, $path, O_RDONLY|O_CLOEXEC or die "failed to open $path: $!\n";
-    my $result = syncfs(fileno($fd));
+    my $syncfs_err;
+    if (!syncfs(fileno($fd))) {
+       $syncfs_err = "$!";
+    }
     close($fd);
-    return $result;
+    die "syncfs '$path' failed - $syncfs_err\n" if defined $syncfs_err;
 }
 
 # support sending multi-part mail messages with a text and or a HTML part

[pve-devel] [PATCH container 1/1] sync_container_namespace: skip virtual filesystems

[Stoiko Ivanov]

skip additional virtual filesystems.

the list is taken from a running debian container's /proc/mounts

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

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index b3e3581..b67d872 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1107,7 +1107,7 @@ my $do_syncfs = sub {
     my $mounts = PVE::ProcFSTools::parse_mounts($mountdata);
     foreach my $mp (@$mounts) {
 	my ($what, $dir, $fs) = @$mp;
-	next if $fs eq 'fuse.lxcfs';
+	next if $fs =~ /cgroup|devtmpfs|devpts|fuse.lxcfs|mqueue|fusectl|proc|sysfs|tmpfs/;
 	eval { PVE::Tools::sync_mountpoint($dir); };
 	warn $@ if $@;
     }
-- 
2.20.1

Re: [pve-devel] [PATCH container 1/1] sync_container_namespace: skip virtual filesystems

[Thomas Lamprecht]

On 9/17/20 9:17 PM, Stoiko Ivanov wrote:
> skip additional virtual filesystems.
> 
> the list is taken from a running debian container's /proc/mounts
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PVE/LXC.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index b3e3581..b67d872 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1107,7 +1107,7 @@ my $do_syncfs = sub {
>      my $mounts = PVE::ProcFSTools::parse_mounts($mountdata);
>      foreach my $mp (@$mounts) {
>  	my ($what, $dir, $fs) = @$mp;
> -	next if $fs eq 'fuse.lxcfs';
> +	next if $fs =~ /cgroup|devtmpfs|devpts|fuse.lxcfs|mqueue|fusectl|proc|sysfs|tmpfs/;

the comparison isn't exact any more, maybe add ^ and $ anchors, or use a hash.

We could also use the presence of "nodev" in `cat /proc/filesystems`, albeit zfs
may need to be parsed separate, all others are either ram backed (sync not useful)
or really virtually computed once. Just as an idea...

>  	eval { PVE::Tools::sync_mountpoint($dir); };
>  	warn $@ if $@;
>      }
>