[pve-devel] [PATCH storage v2 1/2] fix #3873: btrfs: check for correct subvolume taking snapshot

[pve-devel] [PATCH storage v2 1/2] fix #3873: btrfs: check for correct subvolume taking snapshot

[Maximiliano Sandoval]

Suppose we are taking a snapshot of VM 100's disk-0. The
dir_glob_foreach runs over $path=/subvolume/images/100, lists all
snapshot names and appends their names to the path of the disk, e.g.
/subvolume/images/vm-100-disk-0@SNAP_NAME, but the original directory
$path might contain a second disk `vm-100-disk-1` which is also listed
by the dir_glib_foreach.

This change adds a helper foreach_snapshot_of_subvol which only lists
snapshots for the current volume.

The helper is also implemented in the other place where we iterate over
snapshots.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---

Differences from v1:
 - Add a helper to run callback on each snapshot.

 src/PVE/Storage/BTRFSPlugin.pm | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index cadd9d1..d21c831 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -419,6 +419,20 @@ my sub foreach_subvol : prototype($$) {
     })
 }
 
+# Calls `$code->($volume, $snap_name)` for each snapshot of the subvolume.
+my sub foreach_snapshot_of_subvol : prototype($$) {
+    my ($subvol, $code) = @_;
+
+    my $basename = basename($subvol);
+    my $dir = dirname($subvol);
+    foreach_subvol($dir, sub {
+	my ($volume, $name, $snap) = @_;
+	return if $name ne $basename;
+	return if !defined $snap;
+	$code->($volume, $snap);
+   });
+}
+
 sub free_image {
     my ($class, $storeid, $scfg, $volname, $isBase, $_format) = @_;
 
@@ -439,10 +453,8 @@ sub free_image {
     my $dir = dirname($subvol);
     my $basename = basename($subvol);
     my @snapshot_vols;
-    foreach_subvol($dir, sub {
-	my ($volume, $name, $snapshot) = @_;
-	return if $name ne $basename;
-	return if !defined $snapshot;
+    foreach_snapshot_of_subvol($dir, sub {
+	my ($volume, $snap_name) = @_;
 	push @snapshot_vols, "$dir/$volume";
     });
 
@@ -782,8 +794,9 @@ sub volume_export {
     if (ref($with_snapshots) eq 'ARRAY') {
 	push @$cmd, (map { "$path\@$_" } ($with_snapshots // [])->@*), $path;
     } else {
-	dir_glob_foreach(dirname($path), $BTRFS_VOL_REGEX, sub {
-	    push @$cmd, "$path\@$_[2]" if !(defined($snapshot) && $_[2] eq $snapshot);
+	foreach_snapshot_of_subvol($path, sub {
+	    my ($volume, $snap_name) = @_;
+	    push @$cmd, "$path\@$snap_name" if !(defined($snapshot) && $snap_name eq $snapshot);
 	});
     }
     $path .= "\@$snapshot" if defined($snapshot);
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH storage v2 2/2] btrfs: forcefully set image to readwrite

[Maximiliano Sandoval]

When a subvolume is transferred via btrfs send/receive the resulting
image contains the received_uuid property set. This property is required
to do incremental snapshots.

A downside though is that once the received_uuid property is set, it is
not possible to make the image readwrite again without the force (-f)
flag, and in such case the received_uuid property is lost. Since we know
the images are only set to rw for the duration of the move, it is safe
to set the flag forcefully and then in a future commit add the
received_uuid property by force.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/PVE/Storage/BTRFSPlugin.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index d21c831..c44f591 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -896,7 +896,7 @@ sub volume_import {
 	# Rotate the disk into place, first the current state:
 	# Note that read-only subvolumes cannot be moved into different directories, but for the
 	# "current" state we also want a writable copy, so start with that:
-	$class->btrfs_cmd(['property', 'set', "$tmppath/$diskname\@$snapshot", 'ro', 'false']);
+	$class->btrfs_cmd(['property', 'set', '-f', "$tmppath/$diskname\@$snapshot", 'ro', 'false']);
 	PVE::Tools::renameat2(
 	    -1,
 	    "$tmppath/$diskname\@$snapshot",
@@ -918,7 +918,7 @@ sub volume_import {
 
 	# Now go through the remaining snapshots (if any)
 	foreach my $snap (@snapshots) {
-	    $class->btrfs_cmd(['property', 'set', "$tmppath/$diskname\@$snap", 'ro', 'false']);
+	    $class->btrfs_cmd(['property', 'set', '-f', "$tmppath/$diskname\@$snap", 'ro', 'false']);
 	    PVE::Tools::renameat2(
 		-1,
 		"$tmppath/$diskname\@$snap",
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH storage v2 1/2] fix #3873: btrfs: check for correct subvolume taking snapshot

[Fiona Ebner]

Am 18.02.25 um 17:28 schrieb Maximiliano Sandoval:
> Suppose we are taking a snapshot of VM 100's disk-0. The
> dir_glob_foreach runs over $path=/subvolume/images/100, lists all
> snapshot names and appends their names to the path of the disk, e.g.
> /subvolume/images/vm-100-disk-0@SNAP_NAME, but the original directory
> $path might contain a second disk `vm-100-disk-1` which is also listed
> by the dir_glib_foreach.
> 
> This change adds a helper foreach_snapshot_of_subvol which only lists
> snapshots for the current volume.
> 
> The helper is also implemented in the other place where we iterate over
> snapshots.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 
> Differences from v1:
>  - Add a helper to run callback on each snapshot.
> 
>  src/PVE/Storage/BTRFSPlugin.pm | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index cadd9d1..d21c831 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -419,6 +419,20 @@ my sub foreach_subvol : prototype($$) {
>      })
>  }
>  
> +# Calls `$code->($volume, $snap_name)` for each snapshot of the subvolume.
> +my sub foreach_snapshot_of_subvol : prototype($$) {

We can just rename foreach_subvol() and the $BTRFS_VOL_REGEX instead of
adding this new wrapper. As already written in the review of v1, the
regex requires a snapshot to be present. This means the regex is a
misnomer and the foreach_subvol() helper already operates only on
snapshots and can also be renamed. It'd be more concise/straightforward
and otherwise, those remain confusing.

> +    my ($subvol, $code) = @_;
> +
> +    my $basename = basename($subvol);
> +    my $dir = dirname($subvol);
> +    foreach_subvol($dir, sub {
> +	my ($volume, $name, $snap) = @_;
> +	return if $name ne $basename;
> +	return if !defined $snap;
> +	$code->($volume, $snap);
> +   });
> +}
> +
>  sub free_image {
>      my ($class, $storeid, $scfg, $volname, $isBase, $_format) = @_;
>  


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH storage v2 1/2] fix #3873: btrfs: check for correct subvolume taking snapshot

[Fiona Ebner]

Am 19.02.25 um 10:24 schrieb Fiona Ebner:
> Am 18.02.25 um 17:28 schrieb Maximiliano Sandoval:
>> +# Calls `$code->($volume, $snap_name)` for each snapshot of the subvolume.
>> +my sub foreach_snapshot_of_subvol : prototype($$) {
> 
> We can just rename foreach_subvol() and the $BTRFS_VOL_REGEX instead of
> adding this new wrapper. As already written in the review of v1, the
> regex requires a snapshot to be present. This means the regex is a
> misnomer and the foreach_subvol() helper already operates only on
> snapshots and can also be renamed. It'd be more concise/straightforward
> and otherwise, those remain confusing.
> 

I mean, it still makes sense to have the helper only operate on the
snapshots of a given subvolume, because both the existing user in
free_image() and your new user want that.

>> +    my ($subvol, $code) = @_;
>> +
>> +    my $basename = basename($subvol);
>> +    my $dir = dirname($subvol);
>> +    foreach_subvol($dir, sub {
>> +	my ($volume, $name, $snap) = @_;
>> +	return if $name ne $basename;
>> +	return if !defined $snap;
>> +	$code->($volume, $snap);
>> +   });
>> +}
>> +
>>  sub free_image {
>>      my ($class, $storeid, $scfg, $volname, $isBase, $_format) = @_;
>>  


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel