[pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature

[pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature

[Maximiliano Sandoval]

Adds the ability to change the owner of a guest image.

Btrfs does not need special commands to rename a subvolume and this can
be achieved the same as in Storage/plugin.pm's rename_volume taking
special care of how the directory structure used by Btrfs.

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

diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
index 42815cb..b85956e 100644
--- a/src/PVE/Storage/BTRFSPlugin.pm
+++ b/src/PVE/Storage/BTRFSPlugin.pm
@@ -7,6 +7,7 @@ use base qw(PVE::Storage::Plugin);
 
 use Fcntl qw(S_ISDIR O_WRONLY O_CREAT O_EXCL);
 use File::Basename qw(basename dirname);
+use File::Copy 'move';
 use File::Path qw(mkpath);
 use IO::Dir;
 use POSIX qw(EEXIST);
@@ -618,6 +619,9 @@ sub volume_has_feature {
 	    base => { qcow2 => 1, raw => 1, vmdk => 1 },
 	    current => { qcow2 => 1, raw => 1, vmdk => 1 },
 	},
+	rename => {
+	    current => { raw => 1 },
+	},
     };
 
     my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) = $class->parse_volname($volname);
@@ -930,4 +934,40 @@ sub volume_import {
     return "$storeid:$volname";
 }
 
+sub rename_volume {
+    my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
+    die "no path found\n" if !$scfg->{path};
+
+    my (
+	$vtype,
+	$source_image,
+	$source_vmid,
+	$base_name,
+	$base_vmid,
+	undef,
+	$format
+    ) = $class->parse_volname($source_volname);
+
+    my $ppath = $class->filesystem_path($scfg, $source_volname);
+
+    $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1)
+	if !$target_volname;
+    $target_volname = "$target_vmid/$target_volname";
+
+    my $basedir = $class->get_subdir($scfg, 'images');
+
+    mkpath "${basedir}/${target_vmid}";
+    my $source_dir = raw_name_to_dir($source_volname);
+    my $target_dir = raw_name_to_dir($target_volname);
+
+    my $old_path = "${basedir}/${source_dir}";
+    my $new_path = "${basedir}/${target_dir}";
+
+    die "target volume '${target_volname}' already exists\n" if -e $new_path;
+    move $old_path, $new_path ||
+	die "rename '$old_path' to '$new_path' failed - $!\n";
+
+    return "${storeid}:$target_volname";
+}
+
 1
-- 
2.39.2



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

Re: [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature

[Maximiliano Sandoval]

ping 

Maximiliano Sandoval <m.sandoval@proxmox.com> writes:

> Adds the ability to change the owner of a guest image.
>
> Btrfs does not need special commands to rename a subvolume and this can
> be achieved the same as in Storage/plugin.pm's rename_volume taking
> special care of how the directory structure used by Btrfs.
>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  src/PVE/Storage/BTRFSPlugin.pm | 40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index 42815cb..b85956e 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -7,6 +7,7 @@ use base qw(PVE::Storage::Plugin);
>  
>  use Fcntl qw(S_ISDIR O_WRONLY O_CREAT O_EXCL);
>  use File::Basename qw(basename dirname);
> +use File::Copy 'move';
>  use File::Path qw(mkpath);
>  use IO::Dir;
>  use POSIX qw(EEXIST);
> @@ -618,6 +619,9 @@ sub volume_has_feature {
>  	    base => { qcow2 => 1, raw => 1, vmdk => 1 },
>  	    current => { qcow2 => 1, raw => 1, vmdk => 1 },
>  	},
> +	rename => {
> +	    current => { raw => 1 },
> +	},
>      };
>  
>      my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) = $class->parse_volname($volname);
> @@ -930,4 +934,40 @@ sub volume_import {
>      return "$storeid:$volname";
>  }
>  
> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
> +    die "no path found\n" if !$scfg->{path};
> +
> +    my (
> +	$vtype,
> +	$source_image,
> +	$source_vmid,
> +	$base_name,
> +	$base_vmid,
> +	undef,
> +	$format
> +    ) = $class->parse_volname($source_volname);
> +
> +    my $ppath = $class->filesystem_path($scfg, $source_volname);
> +
> +    $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1)
> +	if !$target_volname;
> +    $target_volname = "$target_vmid/$target_volname";
> +
> +    my $basedir = $class->get_subdir($scfg, 'images');
> +
> +    mkpath "${basedir}/${target_vmid}";
> +    my $source_dir = raw_name_to_dir($source_volname);
> +    my $target_dir = raw_name_to_dir($target_volname);
> +
> +    my $old_path = "${basedir}/${source_dir}";
> +    my $new_path = "${basedir}/${target_dir}";
> +
> +    die "target volume '${target_volname}' already exists\n" if -e $new_path;
> +    move $old_path, $new_path ||
> +	die "rename '$old_path' to '$new_path' failed - $!\n";
> +
> +    return "${storeid}:$target_volname";
> +}
> +
>  1

-- 
Maximiliano


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

Re: [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature

[Aaron Lauterer]

Works overall. I did not test edge cases like working around the BTRFS 
plugin to have qcow2 files instead of the raw files in a subvol.

On  2024-05-27  13:42, Maximiliano Sandoval wrote:
> Adds the ability to change the owner of a guest image.
> 
> Btrfs does not need special commands to rename a subvolume and this can
> be achieved the same as in Storage/plugin.pm's rename_volume taking
> special care of how the directory structure used by Btrfs.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>   src/PVE/Storage/BTRFSPlugin.pm | 40 ++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm
> index 42815cb..b85956e 100644
> --- a/src/PVE/Storage/BTRFSPlugin.pm
> +++ b/src/PVE/Storage/BTRFSPlugin.pm
> @@ -7,6 +7,7 @@ use base qw(PVE::Storage::Plugin);
>   
>   use Fcntl qw(S_ISDIR O_WRONLY O_CREAT O_EXCL);
>   use File::Basename qw(basename dirname);
> +use File::Copy 'move';

No knowing BTRFS too well, do we need that or could the built-in 
'rename' work as well? A quick test indicates that it does, but I am not 
sure if there are no other reasons I am not aware of.

>   use File::Path qw(mkpath);
>   use IO::Dir;
>   use POSIX qw(EEXIST);
> @@ -618,6 +619,9 @@ sub volume_has_feature {
>   	    base => { qcow2 => 1, raw => 1, vmdk => 1 },
>   	    current => { qcow2 => 1, raw => 1, vmdk => 1 },
>   	},
> +	rename => {
> +	    current => { raw => 1 },
> +	},
>       };
>   
>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) = $class->parse_volname($volname);
> @@ -930,4 +934,40 @@ sub volume_import {
>       return "$storeid:$volname";
>   }
>   

I am not too familiar with how BTRFS and our plugin works, but looking 
at other functions, like 'alloc_image' or 'clone_image' it seems that we 
do have checks regarding the format in place. If it is not a 'raw' or 
'subvol' we call the SUPER from the main Plugin.pm instead of continuing 
in the BTRFS specific implementations that use subvols.

This seems to be missing here, but it might be that it is fine if the 
"move" is working in any way.

> +sub rename_volume {
> +    my ($class, $scfg, $storeid, $source_volname, $target_vmid, $target_volname) = @_;
> +    die "no path found\n" if !$scfg->{path};
> +
> +    my (
> +	$vtype,
> +	$source_image,
> +	$source_vmid,
> +	$base_name,
> +	$base_vmid,
> +	undef,
> +	$format
> +    ) = $class->parse_volname($source_volname);

You only seem to be using '$format' from the returned values? Then the 
others could also be 'undef' if not used as we don't need to allocate them.

> +
> +    my $ppath = $class->filesystem_path($scfg, $source_volname);
> +
> +    $target_volname = $class->find_free_diskname($storeid, $scfg, $target_vmid, $format, 1)
> +	if !$target_volname;
> +    $target_volname = "$target_vmid/$target_volname";
> +
> +    my $basedir = $class->get_subdir($scfg, 'images');
> +
> +    mkpath "${basedir}/${target_vmid}";
> +    my $source_dir = raw_name_to_dir($source_volname);
> +    my $target_dir = raw_name_to_dir($target_volname);
> +
> +    my $old_path = "${basedir}/${source_dir}";
> +    my $new_path = "${basedir}/${target_dir}";
> +
> +    die "target volume '${target_volname}' already exists\n" if -e $new_path;
> +    move $old_path, $new_path ||
> +	die "rename '$old_path' to '$new_path' failed - $!\n";
> +
> +    return "${storeid}:$target_volname";
> +}
> +
>   1


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

Re: [pve-devel] [PATCH storage] fix-4272: btrfs: add rename feature

[Aaron Lauterer]

On  2024-07-03  14:32, Maximiliano Sandoval wrote:
> Aaron Lauterer <a.lauterer@proxmox.com> writes:
> 
>> Works overall. I did not test edge cases like working around the BTRFS plugin to
>> have qcow2 files instead of the raw files in a subvol.
> 
> Thanks for testing!
> 
[...]
>> I am not too familiar with how BTRFS and our plugin works, but looking at other
>> functions, like 'alloc_image' or 'clone_image' it seems that we do have checks
>> regarding the format in place. If it is not a 'raw' or 'subvol' we call the
>> SUPER from the main Plugin.pm instead of continuing in the BTRFS specific
>> implementations that use subvols.
>>
>> This seems to be missing here, but it might be that it is fine if the "move" is
>> working in any way.
> 
> While it is true that others methods call the SUPER method, I didn't
> find any other implementation using SUPER::rename_volume.

Not surprising, as this behavior seems to be a BTRFS specific one for 
the edge case that we are dealing with a regular file (non .raw) instead 
of how the plugin usually wants it. AFAICT this is a safety mechanism, 
just in case.

For example (IIRC): on BTRFS the layout if usually:
images/{vmid}/vm-{vmid}-disk-X/disk.raw

where the `vm-{vmid}-disk-X` part is a BTRFS subvolume. But it would 
also be possible that someone (manually) builds it like a regular 
directory based storage plugin:

images/{vmid}/vm-{vmid}-disk-X.qcow2

where everything is just a path/file. How well does the rename_volume 
code handle this? Or should it call the SUPER method?


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