[pve-devel] [PATCH-SERIES storage/manager] fix #3580: make preallocation mode selectable for qcow2 and raw images

[pve-devel] [PATCH-SERIES storage/manager] fix #3580: make preallocation mode selectable for qcow2 and raw images

[Lorenz Stechauner]

this series allows users to configure the `qemu-img` preallocation
mode per storage. this only applies to file-based storages and the
file types 'qcow2' and 'raw'.


pve-storage:
Lorenz Stechauner (1):
  fix #3580: plugins: make preallocation mode selectable for qcow2 and
    raw images

 PVE/Storage/BTRFSPlugin.pm     |  1 +
 PVE/Storage/CIFSPlugin.pm      |  1 +
 PVE/Storage/DirPlugin.pm       |  1 +
 PVE/Storage/GlusterfsPlugin.pm |  4 ++-
 PVE/Storage/NFSPlugin.pm       |  1 +
 PVE/Storage/Plugin.pm          | 46 +++++++++++++++++++++++++++++++++-
 6 files changed, 52 insertions(+), 2 deletions(-)


pve-manager:
Lorenz Stechauner (2):
  ui: add PreallocationSelector
  fix 3850: ui: storage: using PreallocationSelector for file based
    storage types

 www/manager6/Makefile                      |  1 +
 www/manager6/controller/StorageEdit.js     |  6 ++++++
 www/manager6/form/PreallocationSelector.js | 14 ++++++++++++++
 www/manager6/storage/Base.js               | 18 ++++++++++++++++++
 4 files changed, 39 insertions(+)
 create mode 100644 www/manager6/form/PreallocationSelector.js

-- 
2.30.2

[pve-devel] [PATCH storage 1/1] fix #3580: plugins: make preallocation mode selectable for qcow2 and raw images

[Lorenz Stechauner]

the plugins for file based storages
 * BTRFS
 * CIFS
 * Dir
 * Glusterfs
 * NFS
now allow the option 'preallocation'.

'preallocation' can have four values:
 * default
 * off
 * metadata
 * falloc
 * full
see man pages for `qemu-img` for what these mean exactly. [0]

the defualt value was chosen to be
 * qcow2: metadata (as previously)
 * raw: off (I was unable to find any documentation on this, so
    could only test this and found, that 'off' was the most
    fitting.)

when using 'metadata' as preallocation mode, for raw images 'off'
is used.

[0] https://qemu.readthedocs.io/en/latest/system/images.html#disk-image-file-formats

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 PVE/Storage/BTRFSPlugin.pm     |  1 +
 PVE/Storage/CIFSPlugin.pm      |  1 +
 PVE/Storage/DirPlugin.pm       |  1 +
 PVE/Storage/GlusterfsPlugin.pm |  4 ++-
 PVE/Storage/NFSPlugin.pm       |  1 +
 PVE/Storage/Plugin.pm          | 46 +++++++++++++++++++++++++++++++++-
 6 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
index fe42082..31a2954 100644
--- a/PVE/Storage/BTRFSPlugin.pm
+++ b/PVE/Storage/BTRFSPlugin.pm
@@ -73,6 +73,7 @@ sub options {
 	is_mountpoint => { optional => 1 },
 	nocow => { optional => 1 },
 	mkdir => { optional => 1 },
+	preallocation => { optional => 1 },
 	# TODO: The new variant of mkdir with  `populate` vs `create`...
     };
 }
diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
index 0221069..2d94413 100644
--- a/PVE/Storage/CIFSPlugin.pm
+++ b/PVE/Storage/CIFSPlugin.pm
@@ -140,6 +140,7 @@ sub options {
 	smbversion => { optional => 1},
 	mkdir => { optional => 1 },
 	bwlimit => { optional => 1 },
+	preallocation => { optional => 1 },
     };
 }
 
diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 2267f11..3eeec98 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -59,6 +59,7 @@ sub options {
 	mkdir => { optional => 1 },
 	is_mountpoint => { optional => 1 },
 	bwlimit => { optional => 1 },
+	preallocation => { optional => 1 },
    };
 }
 
diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
index ea4df82..d8d2b88 100644
--- a/PVE/Storage/GlusterfsPlugin.pm
+++ b/PVE/Storage/GlusterfsPlugin.pm
@@ -137,6 +137,7 @@ sub options {
 	format => { optional => 1 },
 	mkdir => { optional => 1 },
 	bwlimit => { optional => 1 },
+	preallocation => { optional => 1 },
     };
 }
 
@@ -260,7 +261,8 @@ sub alloc_image {
 
     my $cmd = ['/usr/bin/qemu-img', 'create'];
 
-    push @$cmd, '-o', 'preallocation=metadata' if $fmt eq 'qcow2';
+    my $prealloc_opt = PVE::Storage::Plugin::preallocation_cmd_option($scfg, $fmt);
+    push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
 
     push @$cmd, '-f', $fmt, $volumepath, "${size}K";
 
diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
index 39bf15a..21b288a 100644
--- a/PVE/Storage/NFSPlugin.pm
+++ b/PVE/Storage/NFSPlugin.pm
@@ -90,6 +90,7 @@ sub options {
 	format => { optional => 1 },
 	mkdir => { optional => 1 },
 	bwlimit => { optional => 1 },
+	preallocation => { optional => 1 },
     };
 }
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index b1865cb..4924525 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -41,6 +41,19 @@ our @SHARED_STORAGE = (
     'pbs',
 );
 
+our $QCOW2_PREALLOCATION = {
+    off => 1,
+    metadata => 1,
+    falloc => 1,
+    full => 1,
+};
+
+our $RAW_PREALLOCATION = {
+    off => 1,
+    falloc => 1,
+    full => 1,
+};
+
 our $MAX_VOLUMES_PER_GUEST = 1024;
 
 cfs_register_file ('storage.cfg',
@@ -150,6 +163,11 @@ my $defaultData = {
 	    type => 'string', format => 'pve-storage-format',
 	    optional => 1,
 	},
+	preallocation => {
+	    description => "Preallocation mode for raw and qcow2 images.",
+	    type => 'string', enum => ['default', 'off', 'metadata', 'falloc', 'full'],
+	    optional => 1,
+	},
     },
 };
 
@@ -762,7 +780,8 @@ sub alloc_image {
     } else {
 	my $cmd = ['/usr/bin/qemu-img', 'create'];
 
-	push @$cmd, '-o', 'preallocation=metadata' if $fmt eq 'qcow2';
+	my $prealloc_opt = preallocation_cmd_option($scfg, $fmt);
+	push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
 
 	push @$cmd, '-f', $fmt, $path, "${size}K";
 
@@ -1484,4 +1503,29 @@ sub volume_import_formats {
     return ();
 }
 
+sub preallocation_cmd_option {
+    my ($scfg, $fmt) = @_;
+
+    my $prealloc = $scfg->{preallocation};
+
+    $prealloc = undef if $prealloc eq 'default';
+
+    if ($fmt eq 'qcow2') {
+	$prealloc = $prealloc // 'metadata';
+
+	die "preallocation mode '$prealloc' not supported by format '$fmt'\n" if !$QCOW2_PREALLOCATION->{$prealloc};
+
+	return "preallocation=$prealloc";
+    } elsif ($fmt eq 'raw') {
+	$prealloc = $prealloc // 'off';
+	$prealloc = 'off' if $prealloc eq 'metadata';
+
+	die "preallocation mode '$prealloc' not supported by format '$fmt'\n" if !$RAW_PREALLOCATION->{$prealloc};
+
+	return "preallocation=$prealloc";
+    }
+
+    return undef;
+}
+
 1;
-- 
2.30.2

[pve-devel] [PATCH manager 1/2] ui: add PreallocationSelector

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/Makefile                      |  1 +
 www/manager6/form/PreallocationSelector.js | 14 ++++++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 www/manager6/form/PreallocationSelector.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 75d355a5..f60fad06 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -49,6 +49,7 @@ JSSRC= 							\
 	form/PCISelector.js				\
 	form/PermPathSelector.js			\
 	form/PoolSelector.js				\
+	form/PreallocationSelector.js			\
 	form/PrivilegesSelector.js			\
 	form/QemuBiosSelector.js			\
 	form/SDNControllerSelector.js			\
diff --git a/www/manager6/form/PreallocationSelector.js b/www/manager6/form/PreallocationSelector.js
new file mode 100644
index 00000000..0a58ee87
--- /dev/null
+++ b/www/manager6/form/PreallocationSelector.js
@@ -0,0 +1,14 @@
+Ext.define('PVE.form.preallocationSelector', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: ['widget.pvePreallocationSelector'],
+    config: {
+	deleteEmpty: false,
+    },
+    comboItems: [
+	['default', 'Default'],
+	['off', 'Off'],
+	['metadata', 'Metadata'],
+	['falloc', 'Full (posix_fallocate)'],
+	['full', 'Full'],
+    ],
+});
-- 
2.30.2

[pve-devel] [PATCH manager 2/2] fix 3850: ui: storage: using PreallocationSelector for file based storage types

[Lorenz Stechauner]

Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
---
 www/manager6/controller/StorageEdit.js |  6 ++++++
 www/manager6/storage/Base.js           | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/www/manager6/controller/StorageEdit.js b/www/manager6/controller/StorageEdit.js
index 4246d363..cb73b776 100644
--- a/www/manager6/controller/StorageEdit.js
+++ b/www/manager6/controller/StorageEdit.js
@@ -4,6 +4,12 @@ Ext.define('PVE.controller.StorageEdit', {
     control: {
 	'field[name=content]': {
 	    change: function(field, value) {
+		const hasImages = Ext.Array.contains(value, 'images');
+		const prealloc = field.up('form').getForm().findField('preallocation');
+		if (prealloc) {
+		    prealloc.setDisabled(!hasImages);
+		}
+
 		var hasBackups = Ext.Array.contains(value, 'backup');
 		var maxfiles = this.lookupReference('maxfiles');
 		if (!maxfiles) {
diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js
index ee8b54e8..457576a3 100644
--- a/www/manager6/storage/Base.js
+++ b/www/manager6/storage/Base.js
@@ -51,6 +51,24 @@ Ext.define('PVE.panel.StorageBase', {
 	    },
 	);
 
+	const fileBasedStorageTypes = ['dir', 'btrfs', 'nfs', 'cifs', 'glusterfs'];
+
+	if (fileBasedStorageTypes.includes(me.type)) {
+	    const preallocSelector = {
+		xtype: 'pvePreallocationSelector',
+		name: 'preallocation',
+		fieldLabel: gettext('Preallocation'),
+		allowBlank: false,
+		value: 'default',
+	    };
+
+	    // ensures, that the PreallocationSelector is always on the left side
+	    if (me.advancedColumn1) {
+		me.advancedColumn2 = me.advancedColumn1;
+	    }
+	    me.advancedColumn1 = [preallocSelector];
+	}
+
 	me.callParent();
     },
 });
-- 
2.30.2

Re: [pve-devel] [PATCH storage 1/1] fix #3580: plugins: make preallocation mode selectable for qcow2 and raw images

[alexandre derumier]

Hi,
it can be done too with ceph rbd with "rbd create ... –thick-provision"

Le lundi 06 septembre 2021 à 15:15 +0200, Lorenz Stechauner a écrit :
> the plugins for file based storages
>  * BTRFS
>  * CIFS
>  * Dir
>  * Glusterfs
>  * NFS
> now allow the option 'preallocation'.
> 
> 'preallocation' can have four values:
>  * default
>  * off
>  * metadata
>  * falloc
>  * full
> see man pages for `qemu-img` for what these mean exactly. [0]
> 
> the defualt value was chosen to be
>  * qcow2: metadata (as previously)
>  * raw: off (I was unable to find any documentation on this, so
>     could only test this and found, that 'off' was the most
>     fitting.)
> 
> when using 'metadata' as preallocation mode, for raw images 'off'
> is used.
> 
> [0]
> https://qemu.readthedocs.io/en/latest/system/images.html#disk-image-file-formats
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/Storage/BTRFSPlugin.pm     |  1 +
>  PVE/Storage/CIFSPlugin.pm      |  1 +
>  PVE/Storage/DirPlugin.pm       |  1 +
>  PVE/Storage/GlusterfsPlugin.pm |  4 ++-
>  PVE/Storage/NFSPlugin.pm       |  1 +
>  PVE/Storage/Plugin.pm          | 46
> +++++++++++++++++++++++++++++++++-
>  6 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
> index fe42082..31a2954 100644
> --- a/PVE/Storage/BTRFSPlugin.pm
> +++ b/PVE/Storage/BTRFSPlugin.pm
> @@ -73,6 +73,7 @@ sub options {
>         is_mountpoint => { optional => 1 },
>         nocow => { optional => 1 },
>         mkdir => { optional => 1 },
> +       preallocation => { optional => 1 },
>         # TODO: The new variant of mkdir with  `populate` vs
> `create`...
>      };
>  }
> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index 0221069..2d94413 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -140,6 +140,7 @@ sub options {
>         smbversion => { optional => 1},
>         mkdir => { optional => 1 },
>         bwlimit => { optional => 1 },
> +       preallocation => { optional => 1 },
>      };
>  }
>  
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 2267f11..3eeec98 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -59,6 +59,7 @@ sub options {
>         mkdir => { optional => 1 },
>         is_mountpoint => { optional => 1 },
>         bwlimit => { optional => 1 },
> +       preallocation => { optional => 1 },
>     };
>  }
>  
> diff --git a/PVE/Storage/GlusterfsPlugin.pm
> b/PVE/Storage/GlusterfsPlugin.pm
> index ea4df82..d8d2b88 100644
> --- a/PVE/Storage/GlusterfsPlugin.pm
> +++ b/PVE/Storage/GlusterfsPlugin.pm
> @@ -137,6 +137,7 @@ sub options {
>         format => { optional => 1 },
>         mkdir => { optional => 1 },
>         bwlimit => { optional => 1 },
> +       preallocation => { optional => 1 },
>      };
>  }
>  
> @@ -260,7 +261,8 @@ sub alloc_image {
>  
>      my $cmd = ['/usr/bin/qemu-img', 'create'];
>  
> -    push @$cmd, '-o', 'preallocation=metadata' if $fmt eq 'qcow2';
> +    my $prealloc_opt =
> PVE::Storage::Plugin::preallocation_cmd_option($scfg, $fmt);
> +    push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>  
>      push @$cmd, '-f', $fmt, $volumepath, "${size}K";
>  
> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 39bf15a..21b288a 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -90,6 +90,7 @@ sub options {
>         format => { optional => 1 },
>         mkdir => { optional => 1 },
>         bwlimit => { optional => 1 },
> +       preallocation => { optional => 1 },
>      };
>  }
>  
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index b1865cb..4924525 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -41,6 +41,19 @@ our @SHARED_STORAGE = (
>      'pbs',
>  );
>  
> +our $QCOW2_PREALLOCATION = {
> +    off => 1,
> +    metadata => 1,
> +    falloc => 1,
> +    full => 1,
> +};
> +
> +our $RAW_PREALLOCATION = {
> +    off => 1,
> +    falloc => 1,
> +    full => 1,
> +};
> +
>  our $MAX_VOLUMES_PER_GUEST = 1024;
>  
>  cfs_register_file ('storage.cfg',
> @@ -150,6 +163,11 @@ my $defaultData = {
>             type => 'string', format => 'pve-storage-format',
>             optional => 1,
>         },
> +       preallocation => {
> +           description => "Preallocation mode for raw and qcow2
> images.",
> +           type => 'string', enum => ['default', 'off', 'metadata',
> 'falloc', 'full'],
> +           optional => 1,
> +       },
>      },
>  };
>  
> @@ -762,7 +780,8 @@ sub alloc_image {
>      } else {
>         my $cmd = ['/usr/bin/qemu-img', 'create'];
>  
> -       push @$cmd, '-o', 'preallocation=metadata' if $fmt eq
> 'qcow2';
> +       my $prealloc_opt = preallocation_cmd_option($scfg, $fmt);
> +       push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>  
>         push @$cmd, '-f', $fmt, $path, "${size}K";
>  
> @@ -1484,4 +1503,29 @@ sub volume_import_formats {
>      return ();
>  }
>  
> +sub preallocation_cmd_option {
> +    my ($scfg, $fmt) = @_;
> +
> +    my $prealloc = $scfg->{preallocation};
> +
> +    $prealloc = undef if $prealloc eq 'default';
> +
> +    if ($fmt eq 'qcow2') {
> +       $prealloc = $prealloc // 'metadata';
> +
> +       die "preallocation mode '$prealloc' not supported by format
> '$fmt'\n" if !$QCOW2_PREALLOCATION->{$prealloc};
> +
> +       return "preallocation=$prealloc";
> +    } elsif ($fmt eq 'raw') {
> +       $prealloc = $prealloc // 'off';
> +       $prealloc = 'off' if $prealloc eq 'metadata';
> +
> +       die "preallocation mode '$prealloc' not supported by format
> '$fmt'\n" if !$RAW_PREALLOCATION->{$prealloc};
> +
> +       return "preallocation=$prealloc";
> +    }
> +
> +    return undef;
> +}
> +
>  1;

Re: [pve-devel] [PATCH storage 1/1] fix #3580: plugins: make preallocation mode selectable for qcow2 and raw images

[Fabian Ebner]

Am 08.09.21 um 10:11 schrieb alexandre derumier:
> Hi,
> it can be done too with ceph rbd with "rbd create ... –thick-provision"
> 

Hi,
there also is the 'sparse' storage config option (currently only used 
for ZFS plugins). If there is only thick or thin, re-using that one is 
probably nicer, because the newly proposed preallocation option seems to 
be closely tied to qemu-img.

> Le lundi 06 septembre 2021 à 15:15 +0200, Lorenz Stechauner a écrit :
>> the plugins for file based storages
>>   * BTRFS
>>   * CIFS
>>   * Dir
>>   * Glusterfs
>>   * NFS
>> now allow the option 'preallocation'.
>>
>> 'preallocation' can have four values:
>>   * default
>>   * off
>>   * metadata
>>   * falloc
>>   * full
>> see man pages for `qemu-img` for what these mean exactly. [0]
>>
>> the defualt value was chosen to be
>>   * qcow2: metadata (as previously)
>>   * raw: off (I was unable to find any documentation on this, so
>>      could only test this and found, that 'off' was the most
>>      fitting.)
>>
>> when using 'metadata' as preallocation mode, for raw images 'off'
>> is used.
>>
>> [0]
>> https://qemu.readthedocs.io/en/latest/system/images.html#disk-image-file-formats
>>
>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>> ---
>>   PVE/Storage/BTRFSPlugin.pm     |  1 +
>>   PVE/Storage/CIFSPlugin.pm      |  1 +
>>   PVE/Storage/DirPlugin.pm       |  1 +
>>   PVE/Storage/GlusterfsPlugin.pm |  4 ++-
>>   PVE/Storage/NFSPlugin.pm       |  1 +
>>   PVE/Storage/Plugin.pm          | 46
>> +++++++++++++++++++++++++++++++++-
>>   6 files changed, 52 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
>> index fe42082..31a2954 100644
>> --- a/PVE/Storage/BTRFSPlugin.pm
>> +++ b/PVE/Storage/BTRFSPlugin.pm
>> @@ -73,6 +73,7 @@ sub options {
>>          is_mountpoint => { optional => 1 },
>>          nocow => { optional => 1 },
>>          mkdir => { optional => 1 },
>> +       preallocation => { optional => 1 },
>>          # TODO: The new variant of mkdir with  `populate` vs
>> `create`...
>>       };
>>   }
>> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
>> index 0221069..2d94413 100644
>> --- a/PVE/Storage/CIFSPlugin.pm
>> +++ b/PVE/Storage/CIFSPlugin.pm
>> @@ -140,6 +140,7 @@ sub options {
>>          smbversion => { optional => 1},
>>          mkdir => { optional => 1 },
>>          bwlimit => { optional => 1 },
>> +       preallocation => { optional => 1 },
>>       };
>>   }
>>   
>> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>> index 2267f11..3eeec98 100644
>> --- a/PVE/Storage/DirPlugin.pm
>> +++ b/PVE/Storage/DirPlugin.pm
>> @@ -59,6 +59,7 @@ sub options {
>>          mkdir => { optional => 1 },
>>          is_mountpoint => { optional => 1 },
>>          bwlimit => { optional => 1 },
>> +       preallocation => { optional => 1 },
>>      };
>>   }
>>   
>> diff --git a/PVE/Storage/GlusterfsPlugin.pm
>> b/PVE/Storage/GlusterfsPlugin.pm
>> index ea4df82..d8d2b88 100644
>> --- a/PVE/Storage/GlusterfsPlugin.pm
>> +++ b/PVE/Storage/GlusterfsPlugin.pm
>> @@ -137,6 +137,7 @@ sub options {
>>          format => { optional => 1 },
>>          mkdir => { optional => 1 },
>>          bwlimit => { optional => 1 },
>> +       preallocation => { optional => 1 },
>>       };
>>   }
>>   
>> @@ -260,7 +261,8 @@ sub alloc_image {
>>   
>>       my $cmd = ['/usr/bin/qemu-img', 'create'];
>>   
>> -    push @$cmd, '-o', 'preallocation=metadata' if $fmt eq 'qcow2';
>> +    my $prealloc_opt =
>> PVE::Storage::Plugin::preallocation_cmd_option($scfg, $fmt);
>> +    push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>>   
>>       push @$cmd, '-f', $fmt, $volumepath, "${size}K";
>>   
>> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
>> index 39bf15a..21b288a 100644
>> --- a/PVE/Storage/NFSPlugin.pm
>> +++ b/PVE/Storage/NFSPlugin.pm
>> @@ -90,6 +90,7 @@ sub options {
>>          format => { optional => 1 },
>>          mkdir => { optional => 1 },
>>          bwlimit => { optional => 1 },
>> +       preallocation => { optional => 1 },
>>       };
>>   }
>>   
>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index b1865cb..4924525 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -41,6 +41,19 @@ our @SHARED_STORAGE = (
>>       'pbs',
>>   );
>>   
>> +our $QCOW2_PREALLOCATION = {
>> +    off => 1,
>> +    metadata => 1,
>> +    falloc => 1,
>> +    full => 1,
>> +};
>> +
>> +our $RAW_PREALLOCATION = {
>> +    off => 1,
>> +    falloc => 1,
>> +    full => 1,
>> +};
>> +
>>   our $MAX_VOLUMES_PER_GUEST = 1024;
>>   
>>   cfs_register_file ('storage.cfg',
>> @@ -150,6 +163,11 @@ my $defaultData = {
>>              type => 'string', format => 'pve-storage-format',
>>              optional => 1,
>>          },
>> +       preallocation => {
>> +           description => "Preallocation mode for raw and qcow2
>> images.",
>> +           type => 'string', enum => ['default', 'off', 'metadata',
>> 'falloc', 'full'],
>> +           optional => 1,
>> +       },
>>       },
>>   };
>>   
>> @@ -762,7 +780,8 @@ sub alloc_image {
>>       } else {
>>          my $cmd = ['/usr/bin/qemu-img', 'create'];
>>   
>> -       push @$cmd, '-o', 'preallocation=metadata' if $fmt eq
>> 'qcow2';
>> +       my $prealloc_opt = preallocation_cmd_option($scfg, $fmt);
>> +       push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>>   
>>          push @$cmd, '-f', $fmt, $path, "${size}K";
>>   
>> @@ -1484,4 +1503,29 @@ sub volume_import_formats {
>>       return ();
>>   }
>>   
>> +sub preallocation_cmd_option {
>> +    my ($scfg, $fmt) = @_;
>> +
>> +    my $prealloc = $scfg->{preallocation};
>> +
>> +    $prealloc = undef if $prealloc eq 'default';
>> +
>> +    if ($fmt eq 'qcow2') {
>> +       $prealloc = $prealloc // 'metadata';
>> +
>> +       die "preallocation mode '$prealloc' not supported by format
>> '$fmt'\n" if !$QCOW2_PREALLOCATION->{$prealloc};
>> +
>> +       return "preallocation=$prealloc";
>> +    } elsif ($fmt eq 'raw') {
>> +       $prealloc = $prealloc // 'off';
>> +       $prealloc = 'off' if $prealloc eq 'metadata';
>> +
>> +       die "preallocation mode '$prealloc' not supported by format
>> '$fmt'\n" if !$RAW_PREALLOCATION->{$prealloc};
>> +
>> +       return "preallocation=$prealloc";
>> +    }
>> +
>> +    return undef;
>> +}
>> +
>>   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 1/1] fix #3580: plugins: make preallocation mode selectable for qcow2 and raw images

[Lorenz Stechauner]

On 09.09.21 12:25, Fabian Ebner wrote:
> Am 08.09.21 um 10:11 schrieb alexandre derumier:
>> Hi,
>> it can be done too with ceph rbd with "rbd create ... –thick-provision"
>>
>
> Hi,
> there also is the 'sparse' storage config option (currently only used 
> for ZFS plugins). If there is only thick or thin, re-using that one is 
> probably nicer, because the newly proposed preallocation option seems 
> to be closely tied to qemu-img.

Sounds like a good idea. I doubt, that anyone would use full 
prellocation anyway, so simply using 'sparse' for prealloc=off and 
default remains prealloc=metadata sounds good.

>
>> Le lundi 06 septembre 2021 à 15:15 +0200, Lorenz Stechauner a écrit :
>>> the plugins for file based storages
>>>   * BTRFS
>>>   * CIFS
>>>   * Dir
>>>   * Glusterfs
>>>   * NFS
>>> now allow the option 'preallocation'.
>>>
>>> 'preallocation' can have four values:
>>>   * default
>>>   * off
>>>   * metadata
>>>   * falloc
>>>   * full
>>> see man pages for `qemu-img` for what these mean exactly. [0]
>>>
>>> the defualt value was chosen to be
>>>   * qcow2: metadata (as previously)
>>>   * raw: off (I was unable to find any documentation on this, so
>>>      could only test this and found, that 'off' was the most
>>>      fitting.)
>>>
>>> when using 'metadata' as preallocation mode, for raw images 'off'
>>> is used.
>>>
>>> [0]
>>> https://qemu.readthedocs.io/en/latest/system/images.html#disk-image-file-formats 
>>>
>>>
>>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>>> ---
>>>   PVE/Storage/BTRFSPlugin.pm     |  1 +
>>>   PVE/Storage/CIFSPlugin.pm      |  1 +
>>>   PVE/Storage/DirPlugin.pm       |  1 +
>>>   PVE/Storage/GlusterfsPlugin.pm |  4 ++-
>>>   PVE/Storage/NFSPlugin.pm       |  1 +
>>>   PVE/Storage/Plugin.pm          | 46
>>> +++++++++++++++++++++++++++++++++-
>>>   6 files changed, 52 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
>>> index fe42082..31a2954 100644
>>> --- a/PVE/Storage/BTRFSPlugin.pm
>>> +++ b/PVE/Storage/BTRFSPlugin.pm
>>> @@ -73,6 +73,7 @@ sub options {
>>>          is_mountpoint => { optional => 1 },
>>>          nocow => { optional => 1 },
>>>          mkdir => { optional => 1 },
>>> +       preallocation => { optional => 1 },
>>>          # TODO: The new variant of mkdir with  `populate` vs
>>> `create`...
>>>       };
>>>   }
>>> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
>>> index 0221069..2d94413 100644
>>> --- a/PVE/Storage/CIFSPlugin.pm
>>> +++ b/PVE/Storage/CIFSPlugin.pm
>>> @@ -140,6 +140,7 @@ sub options {
>>>          smbversion => { optional => 1},
>>>          mkdir => { optional => 1 },
>>>          bwlimit => { optional => 1 },
>>> +       preallocation => { optional => 1 },
>>>       };
>>>   }
>>>   diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>>> index 2267f11..3eeec98 100644
>>> --- a/PVE/Storage/DirPlugin.pm
>>> +++ b/PVE/Storage/DirPlugin.pm
>>> @@ -59,6 +59,7 @@ sub options {
>>>          mkdir => { optional => 1 },
>>>          is_mountpoint => { optional => 1 },
>>>          bwlimit => { optional => 1 },
>>> +       preallocation => { optional => 1 },
>>>      };
>>>   }
>>>   diff --git a/PVE/Storage/GlusterfsPlugin.pm
>>> b/PVE/Storage/GlusterfsPlugin.pm
>>> index ea4df82..d8d2b88 100644
>>> --- a/PVE/Storage/GlusterfsPlugin.pm
>>> +++ b/PVE/Storage/GlusterfsPlugin.pm
>>> @@ -137,6 +137,7 @@ sub options {
>>>          format => { optional => 1 },
>>>          mkdir => { optional => 1 },
>>>          bwlimit => { optional => 1 },
>>> +       preallocation => { optional => 1 },
>>>       };
>>>   }
>>>   @@ -260,7 +261,8 @@ sub alloc_image {
>>>         my $cmd = ['/usr/bin/qemu-img', 'create'];
>>>   -    push @$cmd, '-o', 'preallocation=metadata' if $fmt eq 'qcow2';
>>> +    my $prealloc_opt =
>>> PVE::Storage::Plugin::preallocation_cmd_option($scfg, $fmt);
>>> +    push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>>>         push @$cmd, '-f', $fmt, $volumepath, "${size}K";
>>>   diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
>>> index 39bf15a..21b288a 100644
>>> --- a/PVE/Storage/NFSPlugin.pm
>>> +++ b/PVE/Storage/NFSPlugin.pm
>>> @@ -90,6 +90,7 @@ sub options {
>>>          format => { optional => 1 },
>>>          mkdir => { optional => 1 },
>>>          bwlimit => { optional => 1 },
>>> +       preallocation => { optional => 1 },
>>>       };
>>>   }
>>>   diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>>> index b1865cb..4924525 100644
>>> --- a/PVE/Storage/Plugin.pm
>>> +++ b/PVE/Storage/Plugin.pm
>>> @@ -41,6 +41,19 @@ our @SHARED_STORAGE = (
>>>       'pbs',
>>>   );
>>>   +our $QCOW2_PREALLOCATION = {
>>> +    off => 1,
>>> +    metadata => 1,
>>> +    falloc => 1,
>>> +    full => 1,
>>> +};
>>> +
>>> +our $RAW_PREALLOCATION = {
>>> +    off => 1,
>>> +    falloc => 1,
>>> +    full => 1,
>>> +};
>>> +
>>>   our $MAX_VOLUMES_PER_GUEST = 1024;
>>>     cfs_register_file ('storage.cfg',
>>> @@ -150,6 +163,11 @@ my $defaultData = {
>>>              type => 'string', format => 'pve-storage-format',
>>>              optional => 1,
>>>          },
>>> +       preallocation => {
>>> +           description => "Preallocation mode for raw and qcow2
>>> images.",
>>> +           type => 'string', enum => ['default', 'off', 'metadata',
>>> 'falloc', 'full'],
>>> +           optional => 1,
>>> +       },
>>>       },
>>>   };
>>>   @@ -762,7 +780,8 @@ sub alloc_image {
>>>       } else {
>>>          my $cmd = ['/usr/bin/qemu-img', 'create'];
>>>   -       push @$cmd, '-o', 'preallocation=metadata' if $fmt eq
>>> 'qcow2';
>>> +       my $prealloc_opt = preallocation_cmd_option($scfg, $fmt);
>>> +       push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>>>            push @$cmd, '-f', $fmt, $path, "${size}K";
>>>   @@ -1484,4 +1503,29 @@ sub volume_import_formats {
>>>       return ();
>>>   }
>>>   +sub preallocation_cmd_option {
>>> +    my ($scfg, $fmt) = @_;
>>> +
>>> +    my $prealloc = $scfg->{preallocation};
>>> +
>>> +    $prealloc = undef if $prealloc eq 'default';
>>> +
>>> +    if ($fmt eq 'qcow2') {
>>> +       $prealloc = $prealloc // 'metadata';
>>> +
>>> +       die "preallocation mode '$prealloc' not supported by format
>>> '$fmt'\n" if !$QCOW2_PREALLOCATION->{$prealloc};
>>> +
>>> +       return "preallocation=$prealloc";
>>> +    } elsif ($fmt eq 'raw') {
>>> +       $prealloc = $prealloc // 'off';
>>> +       $prealloc = 'off' if $prealloc eq 'metadata';
>>> +
>>> +       die "preallocation mode '$prealloc' not supported by format
>>> '$fmt'\n" if !$RAW_PREALLOCATION->{$prealloc};
>>> +
>>> +       return "preallocation=$prealloc";
>>> +    }
>>> +
>>> +    return undef;
>>> +}
>>> +
>>>   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 1/1] fix #3580: plugins: make preallocation mode selectable for qcow2 and raw images

[Fabian Ebner]

Am 09.09.21 um 13:11 schrieb Lorenz Stechauner:
> 
> On 09.09.21 12:25, Fabian Ebner wrote:
>> Am 08.09.21 um 10:11 schrieb alexandre derumier:
>>> Hi,
>>> it can be done too with ceph rbd with "rbd create ... –thick-provision"
>>>
>>
>> Hi,
>> there also is the 'sparse' storage config option (currently only used 
>> for ZFS plugins). If there is only thick or thin, re-using that one is 
>> probably nicer, because the newly proposed preallocation option seems 
>> to be closely tied to qemu-img.
> 
> Sounds like a good idea. I doubt, that anyone would use full 
> prellocation anyway, so simply using 'sparse' for prealloc=off and 
> default remains prealloc=metadata sounds good.
> 

I actually only meant re-using 'sparse' for the RBD use case. But yes, 
it seems like re-using it for the qemu-img use case would be enough to 
fix the bug too. It might be a bit confusing though, because when sparse 
is not set, the images would still be mostly sparse (except for metadata).

>>
>>> Le lundi 06 septembre 2021 à 15:15 +0200, Lorenz Stechauner a écrit :
>>>> the plugins for file based storages
>>>>   * BTRFS
>>>>   * CIFS
>>>>   * Dir
>>>>   * Glusterfs
>>>>   * NFS
>>>> now allow the option 'preallocation'.
>>>>
>>>> 'preallocation' can have four values:
>>>>   * default
>>>>   * off
>>>>   * metadata
>>>>   * falloc
>>>>   * full
>>>> see man pages for `qemu-img` for what these mean exactly. [0]
>>>>
>>>> the defualt value was chosen to be
>>>>   * qcow2: metadata (as previously)
>>>>   * raw: off (I was unable to find any documentation on this, so
>>>>      could only test this and found, that 'off' was the most
>>>>      fitting.)
>>>>
>>>> when using 'metadata' as preallocation mode, for raw images 'off'
>>>> is used.
>>>>
>>>> [0]
>>>> https://qemu.readthedocs.io/en/latest/system/images.html#disk-image-file-formats 
>>>>
>>>>
>>>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
>>>> ---
>>>>   PVE/Storage/BTRFSPlugin.pm     |  1 +
>>>>   PVE/Storage/CIFSPlugin.pm      |  1 +
>>>>   PVE/Storage/DirPlugin.pm       |  1 +
>>>>   PVE/Storage/GlusterfsPlugin.pm |  4 ++-
>>>>   PVE/Storage/NFSPlugin.pm       |  1 +
>>>>   PVE/Storage/Plugin.pm          | 46
>>>> +++++++++++++++++++++++++++++++++-
>>>>   6 files changed, 52 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
>>>> index fe42082..31a2954 100644
>>>> --- a/PVE/Storage/BTRFSPlugin.pm
>>>> +++ b/PVE/Storage/BTRFSPlugin.pm
>>>> @@ -73,6 +73,7 @@ sub options {
>>>>          is_mountpoint => { optional => 1 },
>>>>          nocow => { optional => 1 },
>>>>          mkdir => { optional => 1 },
>>>> +       preallocation => { optional => 1 },
>>>>          # TODO: The new variant of mkdir with  `populate` vs
>>>> `create`...
>>>>       };
>>>>   }
>>>> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
>>>> index 0221069..2d94413 100644
>>>> --- a/PVE/Storage/CIFSPlugin.pm
>>>> +++ b/PVE/Storage/CIFSPlugin.pm
>>>> @@ -140,6 +140,7 @@ sub options {
>>>>          smbversion => { optional => 1},
>>>>          mkdir => { optional => 1 },
>>>>          bwlimit => { optional => 1 },
>>>> +       preallocation => { optional => 1 },
>>>>       };
>>>>   }
>>>>   diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>>>> index 2267f11..3eeec98 100644
>>>> --- a/PVE/Storage/DirPlugin.pm
>>>> +++ b/PVE/Storage/DirPlugin.pm
>>>> @@ -59,6 +59,7 @@ sub options {
>>>>          mkdir => { optional => 1 },
>>>>          is_mountpoint => { optional => 1 },
>>>>          bwlimit => { optional => 1 },
>>>> +       preallocation => { optional => 1 },
>>>>      };
>>>>   }
>>>>   diff --git a/PVE/Storage/GlusterfsPlugin.pm
>>>> b/PVE/Storage/GlusterfsPlugin.pm
>>>> index ea4df82..d8d2b88 100644
>>>> --- a/PVE/Storage/GlusterfsPlugin.pm
>>>> +++ b/PVE/Storage/GlusterfsPlugin.pm
>>>> @@ -137,6 +137,7 @@ sub options {
>>>>          format => { optional => 1 },
>>>>          mkdir => { optional => 1 },
>>>>          bwlimit => { optional => 1 },
>>>> +       preallocation => { optional => 1 },
>>>>       };
>>>>   }
>>>>   @@ -260,7 +261,8 @@ sub alloc_image {
>>>>         my $cmd = ['/usr/bin/qemu-img', 'create'];
>>>>   -    push @$cmd, '-o', 'preallocation=metadata' if $fmt eq 'qcow2';
>>>> +    my $prealloc_opt =
>>>> PVE::Storage::Plugin::preallocation_cmd_option($scfg, $fmt);
>>>> +    push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>>>>         push @$cmd, '-f', $fmt, $volumepath, "${size}K";
>>>>   diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
>>>> index 39bf15a..21b288a 100644
>>>> --- a/PVE/Storage/NFSPlugin.pm
>>>> +++ b/PVE/Storage/NFSPlugin.pm
>>>> @@ -90,6 +90,7 @@ sub options {
>>>>          format => { optional => 1 },
>>>>          mkdir => { optional => 1 },
>>>>          bwlimit => { optional => 1 },
>>>> +       preallocation => { optional => 1 },
>>>>       };
>>>>   }
>>>>   diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>>>> index b1865cb..4924525 100644
>>>> --- a/PVE/Storage/Plugin.pm
>>>> +++ b/PVE/Storage/Plugin.pm
>>>> @@ -41,6 +41,19 @@ our @SHARED_STORAGE = (
>>>>       'pbs',
>>>>   );
>>>>   +our $QCOW2_PREALLOCATION = {
>>>> +    off => 1,
>>>> +    metadata => 1,
>>>> +    falloc => 1,
>>>> +    full => 1,
>>>> +};
>>>> +
>>>> +our $RAW_PREALLOCATION = {
>>>> +    off => 1,
>>>> +    falloc => 1,
>>>> +    full => 1,
>>>> +};
>>>> +
>>>>   our $MAX_VOLUMES_PER_GUEST = 1024;
>>>>     cfs_register_file ('storage.cfg',
>>>> @@ -150,6 +163,11 @@ my $defaultData = {
>>>>              type => 'string', format => 'pve-storage-format',
>>>>              optional => 1,
>>>>          },
>>>> +       preallocation => {
>>>> +           description => "Preallocation mode for raw and qcow2
>>>> images.",
>>>> +           type => 'string', enum => ['default', 'off', 'metadata',
>>>> 'falloc', 'full'],
>>>> +           optional => 1,
>>>> +       },
>>>>       },
>>>>   };
>>>>   @@ -762,7 +780,8 @@ sub alloc_image {
>>>>       } else {
>>>>          my $cmd = ['/usr/bin/qemu-img', 'create'];
>>>>   -       push @$cmd, '-o', 'preallocation=metadata' if $fmt eq
>>>> 'qcow2';
>>>> +       my $prealloc_opt = preallocation_cmd_option($scfg, $fmt);
>>>> +       push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>>>>            push @$cmd, '-f', $fmt, $path, "${size}K";
>>>>   @@ -1484,4 +1503,29 @@ sub volume_import_formats {
>>>>       return ();
>>>>   }
>>>>   +sub preallocation_cmd_option {
>>>> +    my ($scfg, $fmt) = @_;
>>>> +
>>>> +    my $prealloc = $scfg->{preallocation};
>>>> +
>>>> +    $prealloc = undef if $prealloc eq 'default';
>>>> +
>>>> +    if ($fmt eq 'qcow2') {
>>>> +       $prealloc = $prealloc // 'metadata';
>>>> +
>>>> +       die "preallocation mode '$prealloc' not supported by format
>>>> '$fmt'\n" if !$QCOW2_PREALLOCATION->{$prealloc};
>>>> +
>>>> +       return "preallocation=$prealloc";
>>>> +    } elsif ($fmt eq 'raw') {
>>>> +       $prealloc = $prealloc // 'off';
>>>> +       $prealloc = 'off' if $prealloc eq 'metadata';
>>>> +
>>>> +       die "preallocation mode '$prealloc' not supported by format
>>>> '$fmt'\n" if !$RAW_PREALLOCATION->{$prealloc};
>>>> +
>>>> +       return "preallocation=$prealloc";
>>>> +    }
>>>> +
>>>> +    return undef;
>>>> +}
>>>> +
>>>>   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 1/1] fix #3580: plugins: make preallocation mode selectable for qcow2 and raw images

[Lorenz Stechauner]

On 09.09.21 14:04, Fabian Ebner wrote:
> Am 09.09.21 um 13:11 schrieb Lorenz Stechauner:
>>
>> On 09.09.21 12:25, Fabian Ebner wrote:
>>> Am 08.09.21 um 10:11 schrieb alexandre derumier:
>>>> Hi,
>>>> it can be done too with ceph rbd with "rbd create ... 
>>>> –thick-provision"
>>>>
>>>
>>> Hi,
>>> there also is the 'sparse' storage config option (currently only 
>>> used for ZFS plugins). If there is only thick or thin, re-using that 
>>> one is probably nicer, because the newly proposed preallocation 
>>> option seems to be closely tied to qemu-img.
>>
>> Sounds like a good idea. I doubt, that anyone would use full 
>> prellocation anyway, so simply using 'sparse' for prealloc=off and 
>> default remains prealloc=metadata sounds good.
>>
>
> I actually only meant re-using 'sparse' for the RBD use case. But yes, 
> it seems like re-using it for the qemu-img use case would be enough to 
> fix the bug too. It might be a bit confusing though, because when 
> sparse is not set, the images would still be mostly sparse (except for 
> metadata).

makes sense, I got a bit confused by the rbd stuff. Then I won't update 
the patch to use 'sparse' :)

>
>>>
>>>> Le lundi 06 septembre 2021 à 15:15 +0200, Lorenz Stechauner a écrit :
>>>>> the plugins for file based storages
>>>>>   * BTRFS
>>>>>   * CIFS
>>>>>   * Dir
>>>>>   * Glusterfs
>>>>>   * NFS
>>>>> now allow the option 'preallocation'.
>>>>>
>>>>> 'preallocation' can have four values:
>>>>>   * default
>>>>>   * off
>>>>>   * metadata
>>>>>   * falloc
>>>>>   * full
>>>>> see man pages for `qemu-img` for what these mean exactly. [0]
>>>>>
>>>>> the defualt value was chosen to be
>>>>>   * qcow2: metadata (as previously)
>>>>>   * raw: off (I was unable to find any documentation on this, so
>>>>>      could only test this and found, that 'off' was the most
>>>>>      fitting.)
>>>>>
>>>>> when using 'metadata' as preallocation mode, for raw images 'off'
>>>>> is used.
>>>>>
>>>>> [0]
>>>>> https://qemu.readthedocs.io/en/latest/system/images.html#disk-image-file-formats 
>>>>>
>>>>>
>>>>> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>

Re: [pve-devel] [PATCH storage 1/1] fix #3580: plugins: make preallocation mode selectable for qcow2 and raw images

[Thomas Lamprecht]

On 09.09.21 14:04, Fabian Ebner wrote:
> Am 09.09.21 um 13:11 schrieb Lorenz Stechauner:
>>
>> On 09.09.21 12:25, Fabian Ebner wrote:
>>> Am 08.09.21 um 10:11 schrieb alexandre derumier:
>>>> Hi,
>>>> it can be done too with ceph rbd with "rbd create ... –thick-provision"
>>>>
>>>
>>> Hi,
>>> there also is the 'sparse' storage config option (currently only used for ZFS plugins). If there is only thick or thin, re-using that one is probably nicer, because the newly proposed preallocation option seems to be closely tied to qemu-img.
>>
>> Sounds like a good idea. I doubt, that anyone would use full prellocation anyway, so simply using 'sparse' for prealloc=off and default remains prealloc=metadata sounds good.
>>
> 
> I actually only meant re-using 'sparse' for the RBD use case. But yes, it seems like re-using it for the qemu-img use case would be enough to fix the bug too. It might be a bit confusing though, because when sparse is not set, the images would still be mostly sparse (except for metadata).

Yeah, I'm not sure that just deriving all from the sparse storage option would
be good UX - I think it should be per creation and if we want to expand the scope
of such a value we should rather see it as fallback.

But IMO this is a bit of an edge case anyway, so I'd not rush the latter without
user demand.

Re: [pve-devel] [PATCH storage 1/1] fix #3580: plugins: make preallocation mode selectable for qcow2 and raw images

[Fabian Ebner]

Am 06.09.21 um 15:15 schrieb Lorenz Stechauner:
> the plugins for file based storages
>   * BTRFS
>   * CIFS
>   * Dir
>   * Glusterfs
>   * NFS
> now allow the option 'preallocation'.
> 
> 'preallocation' can have four values:
>   * default
>   * off
>   * metadata
>   * falloc
>   * full
> see man pages for `qemu-img` for what these mean exactly. [0]
> 
> the defualt value was chosen to be
>   * qcow2: metadata (as previously)
>   * raw: off (I was unable to find any documentation on this, so
>      could only test this and found, that 'off' was the most
>      fitting.)
> 
> when using 'metadata' as preallocation mode, for raw images 'off'
> is used.
> 
> [0] https://qemu.readthedocs.io/en/latest/system/images.html#disk-image-file-formats
> 
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   PVE/Storage/BTRFSPlugin.pm     |  1 +
>   PVE/Storage/CIFSPlugin.pm      |  1 +
>   PVE/Storage/DirPlugin.pm       |  1 +
>   PVE/Storage/GlusterfsPlugin.pm |  4 ++-
>   PVE/Storage/NFSPlugin.pm       |  1 +
>   PVE/Storage/Plugin.pm          | 46 +++++++++++++++++++++++++++++++++-
>   6 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm
> index fe42082..31a2954 100644
> --- a/PVE/Storage/BTRFSPlugin.pm
> +++ b/PVE/Storage/BTRFSPlugin.pm
> @@ -73,6 +73,7 @@ sub options {
>   	is_mountpoint => { optional => 1 },
>   	nocow => { optional => 1 },
>   	mkdir => { optional => 1 },
> +	preallocation => { optional => 1 },
>   	# TODO: The new variant of mkdir with  `populate` vs `create`...
>       };
>   }
> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index 0221069..2d94413 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -140,6 +140,7 @@ sub options {
>   	smbversion => { optional => 1},
>   	mkdir => { optional => 1 },
>   	bwlimit => { optional => 1 },
> +	preallocation => { optional => 1 },
>       };
>   }
>   
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 2267f11..3eeec98 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -59,6 +59,7 @@ sub options {
>   	mkdir => { optional => 1 },
>   	is_mountpoint => { optional => 1 },
>   	bwlimit => { optional => 1 },
> +	preallocation => { optional => 1 },
>      };
>   }
>   
> diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
> index ea4df82..d8d2b88 100644
> --- a/PVE/Storage/GlusterfsPlugin.pm
> +++ b/PVE/Storage/GlusterfsPlugin.pm
> @@ -137,6 +137,7 @@ sub options {
>   	format => { optional => 1 },
>   	mkdir => { optional => 1 },
>   	bwlimit => { optional => 1 },
> +	preallocation => { optional => 1 },
>       };
>   }
>   
> @@ -260,7 +261,8 @@ sub alloc_image {
>   
>       my $cmd = ['/usr/bin/qemu-img', 'create'];
>   
> -    push @$cmd, '-o', 'preallocation=metadata' if $fmt eq 'qcow2';
> +    my $prealloc_opt = PVE::Storage::Plugin::preallocation_cmd_option($scfg, $fmt);
> +    push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>   
>       push @$cmd, '-f', $fmt, $volumepath, "${size}K";
>   
> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 39bf15a..21b288a 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -90,6 +90,7 @@ sub options {
>   	format => { optional => 1 },
>   	mkdir => { optional => 1 },
>   	bwlimit => { optional => 1 },
> +	preallocation => { optional => 1 },
>       };
>   }
>   
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index b1865cb..4924525 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -41,6 +41,19 @@ our @SHARED_STORAGE = (
>       'pbs',
>   );
>   
> +our $QCOW2_PREALLOCATION = {
> +    off => 1,
> +    metadata => 1,
> +    falloc => 1,
> +    full => 1,
> +};
> +
> +our $RAW_PREALLOCATION = {
> +    off => 1,
> +    falloc => 1,
> +    full => 1,
> +};
> +
>   our $MAX_VOLUMES_PER_GUEST = 1024;
>   
>   cfs_register_file ('storage.cfg',
> @@ -150,6 +163,11 @@ my $defaultData = {
>   	    type => 'string', format => 'pve-storage-format',
>   	    optional => 1,
>   	},
> +	preallocation => {
> +	    description => "Preallocation mode for raw and qcow2 images.",
> +	    type => 'string', enum => ['default', 'off', 'metadata', 'falloc', 'full'],
> +	    optional => 1,

It might be worth to mention that 'metadata' for raw means 'off' in the 
description.

Any reason for an explicit 'default' rather then having 'option not set 
at all' be the default? You could also write
     default => 'metadata',
as part of the option's properties.

> +	},
>       },
>   };
>   
> @@ -762,7 +780,8 @@ sub alloc_image {
>       } else {
>   	my $cmd = ['/usr/bin/qemu-img', 'create'];
>   
> -	push @$cmd, '-o', 'preallocation=metadata' if $fmt eq 'qcow2';
> +	my $prealloc_opt = preallocation_cmd_option($scfg, $fmt);
> +	push @$cmd, '-o', $prealloc_opt if defined($prealloc_opt);
>   
>   	push @$cmd, '-f', $fmt, $path, "${size}K";
>   
> @@ -1484,4 +1503,29 @@ sub volume_import_formats {
>       return ();
>   }
>   
> +sub preallocation_cmd_option {

I suppose this function should not be part of the Plugin API, so it 
should either be above the
     # Storage implementation
comment or made into a private helper using
     my $preallocation_cmd_option = sub

> +    my ($scfg, $fmt) = @_;
> +
> +    my $prealloc = $scfg->{preallocation};
> +
> +    $prealloc = undef if $prealloc eq 'default';

Will lead to a warning when $prealloc is already undef:
     Use of uninitialized value $prealloc in string eq

> +
> +    if ($fmt eq 'qcow2') {
> +	$prealloc = $prealloc // 'metadata';
> +
> +	die "preallocation mode '$prealloc' not supported by format '$fmt'\n" if !$QCOW2_PREALLOCATION->{$prealloc};

Style nit: line too long

Nit: I'm wondering if the check is worth it. It should not be possible 
to trigger with the current implementation, and even if there were a bad 
value and no check here, qemu-img would still complain afterwards. But 
I'm also fine with the check.

> +
> +	return "preallocation=$prealloc";
> +    } elsif ($fmt eq 'raw') {
> +	$prealloc = $prealloc // 'off';
> +	$prealloc = 'off' if $prealloc eq 'metadata';
> +
> +	die "preallocation mode '$prealloc' not supported by format '$fmt'\n" if !$RAW_PREALLOCATION->{$prealloc};

Same here.

> +
> +	return "preallocation=$prealloc";
> +    }
> +
> +    return undef;

Nit: 'return;' should be preferred over 'return undef;' (behaves 
differently when called in list context).

> +}
> +
>   1;
> 

Re: [pve-devel] [PATCH manager 2/2] fix 3850: ui: storage: using PreallocationSelector for file based storage types

[Fabian Ebner]

Am 06.09.21 um 15:15 schrieb Lorenz Stechauner:
> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>   www/manager6/controller/StorageEdit.js |  6 ++++++
>   www/manager6/storage/Base.js           | 18 ++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/www/manager6/controller/StorageEdit.js b/www/manager6/controller/StorageEdit.js
> index 4246d363..cb73b776 100644
> --- a/www/manager6/controller/StorageEdit.js
> +++ b/www/manager6/controller/StorageEdit.js
> @@ -4,6 +4,12 @@ Ext.define('PVE.controller.StorageEdit', {
>       control: {
>   	'field[name=content]': {
>   	    change: function(field, value) {
> +		const hasImages = Ext.Array.contains(value, 'images');
> +		const prealloc = field.up('form').getForm().findField('preallocation');
> +		if (prealloc) {
> +		    prealloc.setDisabled(!hasImages);
> +		}
> +
>   		var hasBackups = Ext.Array.contains(value, 'backup');
>   		var maxfiles = this.lookupReference('maxfiles');
>   		if (!maxfiles) {
> diff --git a/www/manager6/storage/Base.js b/www/manager6/storage/Base.js
> index ee8b54e8..457576a3 100644
> --- a/www/manager6/storage/Base.js
> +++ b/www/manager6/storage/Base.js
> @@ -51,6 +51,24 @@ Ext.define('PVE.panel.StorageBase', {
>   	    },
>   	);
>   
> +	const fileBasedStorageTypes = ['dir', 'btrfs', 'nfs', 'cifs', 'glusterfs'];

Nit: CephFS is also file-based. What about using qemuImgStorageTypes or 
if (['dir',...].includes(me.type)) directly?

> +
> +	if (fileBasedStorageTypes.includes(me.type)) {
> +	    const preallocSelector = {
> +		xtype: 'pvePreallocationSelector',
> +		name: 'preallocation',
> +		fieldLabel: gettext('Preallocation'),
> +		allowBlank: false,
> +		value: 'default',
> +	    };
> +
> +	    // ensures, that the PreallocationSelector is always on the left side
> +	    if (me.advancedColumn1) {
> +		me.advancedColumn2 = me.advancedColumn1;

Why does it need to be on the left side? What if there already is an 
advancedColumn2?

> +	    }
> +	    me.advancedColumn1 = [preallocSelector];
> +	}
> +
>   	me.callParent();
>       },
>   });
>