[pve-devel] [PATCH storage v3 1/4] btrfs: add helper to iterate over snapshots of a subvolume

[pve-devel] [PATCH storage v3 1/4] btrfs: add helper to iterate over snapshots of a subvolume

[Maximiliano Sandoval]

In this context a subvolume means a BTRFS subvolume.
`$volume\@$snap_name` would be for example
`btrfs_volume/images/102/vm-102-disk-0@snap_name`.

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

Differences from v2:
 - Split into multiple commits
 - Simplify patch
 - Use only the snapshot name in the callback of the helper

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

 src/PVE/Storage/BTRFSPlugin.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index cadd9d1..43f797a 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 BTRFS 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->($snap);
+   });
+}
+
 sub free_image {
     my ($class, $storeid, $scfg, $volname, $isBase, $_format) = @_;
 
-- 
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 v3 2/4] fix #3873: btrfs: use foreach_snapshot_of_subvol helper in volume_export

[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.

By using the helper we only iterate over the snapshots of the guest.

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

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 43f797a..1a877f7 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -796,8 +796,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 ($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 v3 3/4] btrfs: use foreach_snapshot_of_subvol helper in free_image

[Maximiliano Sandoval]

Replaces the current use without changes. The `$dir` variable is not
used anymore at that moment so it is defined later.

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

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 1a877f7..e554894 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -450,19 +450,16 @@ sub free_image {
 	$subvol = raw_file_to_subvol($path);
     }
 
-    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;
-	push @snapshot_vols, "$dir/$volume";
+    foreach_snapshot_of_subvol($subvol, sub {
+	my ($snap_name) = @_;
+	push @snapshot_vols, "$subvol\@$snap_name";
     });
 
     $class->btrfs_cmd(['subvolume', 'delete', '--', @snapshot_vols, $subvol]);
     # try to cleanup directory to not clutter storage with empty $vmid dirs if
     # all images from a guest got deleted
+    my $dir = dirname($subvol);
     rmdir($dir);
 
     return undef;
-- 
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 v3 4/4] 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 e554894..f4a7479 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -895,7 +895,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",
@@ -917,7 +917,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 v3 1/4] btrfs: add helper to iterate over snapshots of a subvolume

[Fiona Ebner]

Am 19.02.25 um 10:58 schrieb Maximiliano Sandoval:
> In this context a subvolume means a BTRFS subvolume.
> `$volume\@$snap_name` would be for example
> `btrfs_volume/images/102/vm-102-disk-0@snap_name`.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 
> Differences from v2:
>  - Split into multiple commits
>  - Simplify patch
>  - Use only the snapshot name in the callback of the helper
> 
> Differences from v1:
>  - Add a helper to run callback on each snapshot.
> 
>  src/PVE/Storage/BTRFSPlugin.pm | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index cadd9d1..43f797a 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 BTRFS subvolume.
> +my sub foreach_snapshot_of_subvol : prototype($$) {
> +    my ($subvol, $code) = @_;
> +
> +    my $basename = basename($subvol);
> +    my $dir = dirname($subvol);
> +    foreach_subvol($dir, sub {

We're getting there, but:

This helper still is named incorrectly. I'd like to have the helper and
the regex be named correctly (not your fault, but it just gets more
confusing if there are more users). You can change the helper to only
operate on a given subvolume, because that is the only use case we have.
I.e. merge your wrapper and the original helper.

I'd also name it foreach_snapshot_of_subvolume() to better differentiate
it from the 'subvol' format, and path_is_subvolume() already uses the
word 'subvolume'.

> +	my ($volume, $name, $snap) = @_;
> +	return if $name ne $basename;
> +	return if !defined $snap;
> +	$code->($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