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

[pve-devel] [PATCH storage v4 1/7] 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 v3:
 - Rename regex
 - Delete now useless foreach_suvbol helper

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..e5c3e08 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -419,6 +419,20 @@ my sub foreach_subvol : prototype($$) {
     })
 }
 
+# Calls `$code->($snapshot)` 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, $snapshot) = @_;
+	return if $name ne $basename;
+	return if !defined $snapshot;
+	$code->($snapshot);
+    });
+}
+
 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 v4 2/7] 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 e5c3e08..8f4691b 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 v4 3/7] 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 8f4691b..7dec6e8 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 v4 4/7] 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 7dec6e8..9fc51ef 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

[pve-devel] [PATCH storage v4 5/7] btrfs: rename volume regex

[Maximiliano Sandoval]

The regex only lists snapshots.

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

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 9fc51ef..bd93cc0 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -405,14 +405,14 @@ my sub path_is_subvolume : prototype($) {
     return S_ISDIR($mode) && $ino == BTRFS_FIRST_FREE_OBJECTID;
 }
 
-my $BTRFS_VOL_REGEX = qr/((?:vm|base|subvol)-\d+-disk-\d+(?:\.subvol)?)(?:\@(\S+))$/;
+my $BTRFS_SNAPSHOT_REGEX = qr/((?:vm|base|subvol)-\d+-disk-\d+(?:\.subvol)?)(?:\@(\S+))$/;
 
 # Calls `$code->($volume, $name, $snapshot)` for each subvol in a directory matching our volume
 # regex.
 my sub foreach_subvol : prototype($$) {
     my ($dir, $code) = @_;
 
-    dir_glob_foreach($dir, $BTRFS_VOL_REGEX, sub {
+    dir_glob_foreach($dir, $BTRFS_SNAPSHOT_REGEX, sub {
 	my ($volume, $name, $snapshot) = ($1, $2, $3);
 	return if !path_is_subvolume("$dir/$volume");
 	$code->($volume, $name, $snapshot);
@@ -869,7 +869,7 @@ sub volume_import {
 	$dh->rewind;
 	while (defined(my $entry = $dh->read)) {
 	    next if $entry eq '.' || $entry eq '..';
-	    next if $entry !~ /^$BTRFS_VOL_REGEX$/;
+	    next if $entry !~ /^$BTRFS_SNAPSHOT_REGEX$/;
 	    my ($cur_diskname, $cur_snapshot) = ($1, $2);
 
 	    die "send stream included a non-snapshot subvolume\n"
-- 
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 v4 6/7] btrfs: remove foreach_subvol

[Maximiliano Sandoval]

This method is not used anymore.

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

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index bd93cc0..c82d5f7 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -407,26 +407,15 @@ my sub path_is_subvolume : prototype($) {
 
 my $BTRFS_SNAPSHOT_REGEX = qr/((?:vm|base|subvol)-\d+-disk-\d+(?:\.subvol)?)(?:\@(\S+))$/;
 
-# Calls `$code->($volume, $name, $snapshot)` for each subvol in a directory matching our volume
-# regex.
-my sub foreach_subvol : prototype($$) {
-    my ($dir, $code) = @_;
-
-    dir_glob_foreach($dir, $BTRFS_SNAPSHOT_REGEX, sub {
-	my ($volume, $name, $snapshot) = ($1, $2, $3);
-	return if !path_is_subvolume("$dir/$volume");
-	$code->($volume, $name, $snapshot);
-    })
-}
-
 # Calls `$code->($snapshot)` 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, $snapshot) = @_;
+    dir_glob_foreach($dir, $BTRFS_SNAPSHOT_REGEX, sub {
+	my ($volume, $name, $snapshot) = ($1, $2, $3);
+	return if !path_is_subvolume("$dir/$volume");
 	return if $name ne $basename;
 	return if !defined $snapshot;
 	$code->($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 v4 7/7] btrfs: rename snapshot parameter

[Maximiliano Sandoval]

It was originally introduced as $snapshot to match the argument name of
foreach_subvol, we rename it here to make it clear that it only contains
the snapshot name.

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

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index c82d5f7..a28996c 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -407,18 +407,18 @@ my sub path_is_subvolume : prototype($) {
 
 my $BTRFS_SNAPSHOT_REGEX = qr/((?:vm|base|subvol)-\d+-disk-\d+(?:\.subvol)?)(?:\@(\S+))$/;
 
-# Calls `$code->($snapshot)` for each snapshot of the BTRFS subvolume.
+# Calls `$code->($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);
     dir_glob_foreach($dir, $BTRFS_SNAPSHOT_REGEX, sub {
-	my ($volume, $name, $snapshot) = ($1, $2, $3);
+	my ($volume, $name, $snap_name) = ($1, $2, $3);
 	return if !path_is_subvolume("$dir/$volume");
 	return if $name ne $basename;
-	return if !defined $snapshot;
-	$code->($snapshot);
+	return if !defined $snap_name;
+	$code->($snap_name);
     });
 }
 
-- 
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 v4 4/7] btrfs: forcefully set image to readwrite

[Fiona Ebner]

I want to apply the series with a small follow-up, i.e.

>     btrfs: avoid superfluous check in foreach_snapshot_of_subvol() helper
>     
>     The helper iterates with the BTRFS_SNAPSHOT_REGEX regular expression,
>     so there will always be a snapshot name.
>     
>     Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/Storage/BTRFSPlugin.pm | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index a28996c..e47f25e 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -417,7 +417,6 @@ my sub foreach_snapshot_of_subvol : prototype($$) {
>         my ($volume, $name, $snap_name) = ($1, $2, $3);
>         return if !path_is_subvolume("$dir/$volume");
>         return if $name ne $basename;
> -       return if !defined $snap_name;
>         $code->($snap_name);
>      });
>  }

but I need to ask about this patch first.

Am 19.02.25 um 12:17 schrieb 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.

What do you mean exactly with "add the received_uuid property by force"?
What would the command for that be? Just want to make sure we don't
block our way to support incremental export/import later (it's broken
right now [0], but we might want to implement it at some point).

[0]:
https://lore.proxmox.com/pve-devel/20250221154111.73194-1-f.ebner@proxmox.com/T/

> 
> 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 7dec6e8..9fc51ef 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",



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

Re: [pve-devel] [PATCH storage v4 4/7] btrfs: forcefully set image to readwrite

[Maximiliano Sandoval]

Fiona Ebner <f.ebner@proxmox.com> writes:

> I want to apply the series with a small follow-up, i.e.
>
>>     btrfs: avoid superfluous check in foreach_snapshot_of_subvol() helper
>>
>>     The helper iterates with the BTRFS_SNAPSHOT_REGEX regular expression,
>>     so there will always be a snapshot name.
>>
>>     Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>  src/PVE/Storage/BTRFSPlugin.pm | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
>> index a28996c..e47f25e 100644
>> --- a/src/PVE/Storage/BTRFSPlugin.pm
>> +++ b/src/PVE/Storage/BTRFSPlugin.pm
>> @@ -417,7 +417,6 @@ my sub foreach_snapshot_of_subvol : prototype($$) {
>>         my ($volume, $name, $snap_name) = ($1, $2, $3);
>>         return if !path_is_subvolume("$dir/$volume");
>>         return if $name ne $basename;
>> -       return if !defined $snap_name;
>>         $code->($snap_name);
>>      });
>>  }
>
> but I need to ask about this patch first.

Requiring the argument to be defined sounds sensible for a helper imo.

> Am 19.02.25 um 12:17 schrieb 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.
>
> What do you mean exactly with "add the received_uuid property by force"?
> What would the command for that be? Just want to make sure we don't
> block our way to support incremental export/import later (it's broken
> right now [0], but we might want to implement it at some point).
>
> [0]:
> https://lore.proxmox.com/pve-devel/20250221154111.73194-1-f.ebner@proxmox.com/T/
>

There is the BTRFS_IOC_SET_RECEIVED_SUBVOL ioctl. See [1].

[1] https://btrfs.readthedocs.io/en/latest/btrfs-ioctl.html#overview

>>
>> 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 7dec6e8..9fc51ef 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",



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

[pve-devel] applied-series: [PATCH storage v4 1/7] btrfs: add helper to iterate over snapshots of a subvolume

[Fiona Ebner]

Am 19.02.25 um 12:17 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>

applied series, thanks!


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