[pve-devel] [PATCH v2 storage] Use a common interface for find_free_diskname

Fabian Ebner f.ebner at proxmox.com
Wed Dec 11 10:25:49 CET 2019


We can use 'list_images' to get the desired volume IDs in
'find_free_diskname' for most plugins. For the two LVM plugins, 'list_images'
potentially skips untagged volumes, so we keep the custom version. For the
RBD plugin, 'list_images' is much more costly than the custom version, so we
keep the custom version.

Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
---

Changes from v1:
    * Keep the custom versions in LVMPlugin and RBDPlugin
    * Do not change the interface for get_next_vm_diskname

Thanks to Fabian for the suggestions!

 PVE/Storage/GlusterfsPlugin.pm | 15 ++-------------
 PVE/Storage/LVMPlugin.pm       | 10 +++++++---
 PVE/Storage/LvmThinPlugin.pm   |  8 ++------
 PVE/Storage/Plugin.pm          | 19 ++++++++++---------
 PVE/Storage/RBDPlugin.pm       | 10 +++++-----
 PVE/Storage/ZFSPlugin.pm       |  2 +-
 PVE/Storage/ZFSPoolPlugin.pm   | 16 +++-------------
 7 files changed, 30 insertions(+), 50 deletions(-)

diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
index b3e5553..2cf2da9 100644
--- a/PVE/Storage/GlusterfsPlugin.pm
+++ b/PVE/Storage/GlusterfsPlugin.pm
@@ -160,17 +160,6 @@ sub parse_name_dir {
     die "unable to parse volume filename '$name'\n";
 }
 
-my $find_free_diskname = sub {
-    my ($imgdir, $vmid, $fmt, $scfg) = @_;
-
-    my $disk_list = [];
-
-    my $dh = IO::Dir->new ($imgdir);
-    @$disk_list = $dh->read() if defined($dh);
-
-    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $imgdir, $vmid, $fmt, $scfg, 1);
-};
-
 sub path {
     my ($class, $scfg, $volname, $storeid, $snapname) = @_;
 
@@ -225,7 +214,7 @@ sub clone_image {
 
     mkpath $imagedir;
 
-    my $name = $find_free_diskname->($imagedir, $vmid, "qcow2", $scfg);
+    my $name = $class->find_free_diskname($imagedir, $scfg, $vmid, "qcow2", 1);
 
     warn "clone $volname: $vtype, $name, $vmid to $name (base=../$basevmid/$basename)\n";
 
@@ -253,7 +242,7 @@ sub alloc_image {
 
     mkpath $imagedir;
 
-    $name = $find_free_diskname->($imagedir, $vmid, $fmt, $scfg) if !$name;
+    $name = $class->find_free_diskname($imagedir, $scfg, $vmid, $fmt, 1) if !$name;
 
     my (undef, $tmpfmt) = parse_name_dir($name);
 
diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
index c652e2a..f02c110 100644
--- a/PVE/Storage/LVMPlugin.pm
+++ b/PVE/Storage/LVMPlugin.pm
@@ -303,8 +303,12 @@ sub clone_image {
     die "can't clone images in lvm storage\n";
 }
 
-sub lvm_find_free_diskname {
-    my ($lvs, $vg, $storeid, $vmid, $scfg) = @_;
+sub find_free_diskname {
+    my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
+
+    my $vg = $scfg->{vgname};
+
+    my $lvs = lvm_list_volumes($vg);
 
     my $disk_list = [ keys %{$lvs->{$vg}} ];
 
@@ -345,7 +349,7 @@ sub alloc_image {
 
     die "not enough free space ($free < $size)\n" if $free < $size;
 
-    $name = lvm_find_free_diskname(lvm_list_volumes($vg), $vg, $storeid, $vmid, $scfg)
+    $name = $class->find_free_diskname($storeid, $scfg, $vmid)
 	if !$name;
 
     lvcreate($vg, $name, $size, ["pve-vm-$vmid"]);
diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
index aafc202..88060c7 100644
--- a/PVE/Storage/LvmThinPlugin.pm
+++ b/PVE/Storage/LvmThinPlugin.pm
@@ -95,9 +95,7 @@ sub alloc_image {
 
     die "no such volume group '$vg'\n" if !defined ($vgs->{$vg});
 
-    my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
-
-    $name = PVE::Storage::LVMPlugin::lvm_find_free_diskname($lvs, $vg, $storeid, $vmid, $scfg)
+    $name = $class->find_free_diskname($storeid, $scfg, $vmid)
 	if !$name;
 
     my $cmd = ['/sbin/lvcreate', '-aly', '-V', "${size}k", '--name', $name,
@@ -268,9 +266,7 @@ sub clone_image {
 	$lv = "$vg/$volname";
     }
 
-    my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
-
-    my $name =  PVE::Storage::LVMPlugin::lvm_find_free_diskname($lvs, $vg, $storeid, $vmid, $scfg);
+    my $name = $class->find_free_diskname($storeid, $scfg, $vmid);
 
     my $cmd = ['/sbin/lvcreate', '-n', $name, '-prw', '-kn', '-s', $lv];
     run_command($cmd, errmsg => "clone image '$lv' error");
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index d0f1df6..73f80af 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -572,18 +572,19 @@ sub get_next_vm_diskname {
     die "unable to allocate an image name for VM $vmid in storage '$storeid'\n"
 }
 
-my $find_free_diskname = sub {
-    my ($imgdir, $vmid, $fmt, $scfg) = @_;
+sub find_free_diskname {
+    my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
+
+    my $disks = $class->list_images($storeid, $scfg, $vmid);
 
     my $disk_list = [];
 
-    if (defined(my $dh = IO::Dir->new($imgdir))) {
-	@$disk_list = $dh->read();
-	$dh->close();
+    foreach my $disk (@{$disks}) {
+	push @{$disk_list}, $disk->{volid};
     }
 
-    return  get_next_vm_diskname($disk_list, $imgdir, $vmid, $fmt, $scfg, 1);
-};
+    return get_next_vm_diskname($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix);
+}
 
 sub clone_image {
     my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
@@ -607,7 +608,7 @@ sub clone_image {
 
     mkpath $imagedir;
 
-    my $name = $find_free_diskname->($imagedir, $vmid, "qcow2", $scfg);
+    my $name = $class->find_free_diskname($imagedir, $scfg, $vmid, "qcow2", 1);
 
     warn "clone $volname: $vtype, $name, $vmid to $name (base=../$basevmid/$basename)\n";
 
@@ -639,7 +640,7 @@ sub alloc_image {
 
     mkpath $imagedir;
 
-    $name = $find_free_diskname->($imagedir, $vmid, $fmt, $scfg) if !$name;
+    $name = $class->find_free_diskname($imagedir, $scfg, $vmid, $fmt, 1) if !$name;
 
     my (undef, $tmpfmt) = parse_name_dir($name);
 
diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index 538d6bb..5d796bd 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -362,8 +362,8 @@ sub path {
     return ($path, $vmid, $vtype);
 }
 
-my $find_free_diskname = sub {
-    my ($storeid, $scfg, $vmid) = @_;
+sub find_free_diskname {
+    my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
 
     my $cmd = &$rbd_cmd($scfg, $storeid, 'ls');
     my $disk_list = [];
@@ -383,7 +383,7 @@ my $find_free_diskname = sub {
     die $err if $err && $err !~ m/doesn't contain rbd images/;
 
     return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, undef, $scfg);
-};
+}
 
 sub create_base {
     my ($class, $storeid, $scfg, $volname) = @_;
@@ -438,7 +438,7 @@ sub clone_image {
     die "$volname is not a base image and snapname is not provided\n" 
 	if !$isBase && !length($snapname);
 
-    my $name = $find_free_diskname->($storeid, $scfg, $vmid);
+    my $name = $class->find_free_diskname($storeid, $scfg, $vmid);
 
     warn "clone $volname: $basename snapname $snap to $name\n";
 
@@ -469,7 +469,7 @@ sub alloc_image {
     die "illegal name '$name' - should be 'vm-$vmid-*'\n"
 	if  $name && $name !~ m/^vm-$vmid-/;
 
-    $name = $find_free_diskname->($storeid, $scfg, $vmid) if !$name;
+    $name = $class->find_free_diskname($storeid, $scfg, $vmid) if !$name;
 
     my $cmd = &$rbd_cmd($scfg, $storeid, 'create', '--image-format' , 2, '--size', int(($size+1023)/1024), $name);
     run_rbd_command($cmd, errmsg => "rbd create $name' error");
diff --git a/PVE/Storage/ZFSPlugin.pm b/PVE/Storage/ZFSPlugin.pm
index 8c6709c..383f0a0 100644
--- a/PVE/Storage/ZFSPlugin.pm
+++ b/PVE/Storage/ZFSPlugin.pm
@@ -294,7 +294,7 @@ sub alloc_image {
 
     my $volname = $name;
 
-    $volname = $class->zfs_find_free_diskname($storeid, $scfg, $vmid, $fmt) if !$volname;
+    $volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt) if !$volname;
     
     $class->zfs_create_zvol($scfg, $volname, $size);
  
diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index 456fb40..12dc2be 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -240,7 +240,7 @@ sub alloc_image {
 
 	die "illegal name '$volname' - should be 'vm-$vmid-*'\n"
 	    if $volname && $volname !~ m/^vm-$vmid-/;
-	$volname = $class->zfs_find_free_diskname($storeid, $scfg, $vmid, $fmt)
+	$volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt)
 	    if !$volname;
 
 	$class->zfs_create_zvol($scfg, $volname, $size);
@@ -250,7 +250,7 @@ sub alloc_image {
 
 	die "illegal name '$volname' - should be 'subvol-$vmid-*'\n"
 	    if $volname && $volname !~ m/^subvol-$vmid-/;
-	$volname = $class->zfs_find_free_diskname($storeid, $scfg, $vmid, $fmt)
+	$volname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt)
 	    if !$volname;
 
 	die "illegal name '$volname' - should be 'subvol-$vmid-*'\n"
@@ -423,16 +423,6 @@ sub zfs_list_zvol {
     return $list;
 }
 
-sub zfs_find_free_diskname {
-    my ($class, $storeid, $scfg, $vmid, $format) = @_;
-
-    my $volumes = $class->zfs_list_zvol($scfg);
-    my $dat = $volumes->{$scfg->{pool}};
-
-    my $disk_list = [ keys %$dat ];
-    return PVE::Storage::Plugin::get_next_vm_diskname($disk_list, $storeid, $vmid, $format, $scfg);
-}
-
 sub zfs_get_latest_snapshot {
     my ($class, $scfg, $volname) = @_;
 
@@ -612,7 +602,7 @@ sub clone_image {
 
     die "clone_image only works on base images\n" if !$isBase;
 
-    my $name = $class->zfs_find_free_diskname($storeid, $scfg, $vmid, $format);
+    my $name = $class->find_free_diskname($storeid, $scfg, $vmid, $format);
 
     if ($format eq 'subvol') {
 	my $size = $class->zfs_request($scfg, undef, 'list', '-H', '-o', 'refquota', "$scfg->{pool}/$basename");
-- 
2.20.1




More information about the pve-devel mailing list