[pbs-devel] [PATCH proxmox-backup v4 0/3] add 'sync-level' to datatore tuning options

[pbs-devel] [PATCH proxmox-backup v4 0/3] add 'sync-level' to datatore tuning options

[Dominik Csapak]

seems the first 3 parts of the previous series was already applied,
so the wanted changed from wolfgang as fixups + the last two (unchanged)
patches

Dominik Csapak (3):
  datastore: improve sync level code a bit
  docs: add documentation about the 'sync-level' tuning
  datastore: make 'filesystem' the default sync-level

 docs/storage.rst                 | 36 ++++++++++++++++++++++++++++++++
 pbs-api-types/src/datastore.rs   |  9 ++------
 pbs-datastore/src/chunk_store.rs |  5 ++---
 3 files changed, 40 insertions(+), 10 deletions(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v4 1/3] datastore: improve sync level code a bit

[Dominik Csapak]

fixups for DatastoreFSyncLevel:
* use derive for Default
* add some more derives (Clone, Copy)

chunk store:
* drop to_owned for chunk_dir_path

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-api-types/src/datastore.rs   | 9 ++-------
 pbs-datastore/src/chunk_store.rs | 5 ++---
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 865a7b55..4c9eda2f 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -169,7 +169,7 @@ pub enum ChunkOrder {
 }
 
 #[api]
-#[derive(PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(rename_all = "lowercase")]
 /// The level of syncing that is done when writing into a datastore.
 pub enum DatastoreFSyncLevel {
@@ -181,6 +181,7 @@ pub enum DatastoreFSyncLevel {
     /// which reduces IO pressure.
     /// But it may cause losing data on powerloss or system crash without any uninterruptible power
     /// supply.
+    #[default]
     None,
     /// Triggers a fsync after writing any chunk on the datastore. While this can slow down
     /// backups significantly, depending on the underlying file system and storage used, it
@@ -198,12 +199,6 @@ pub enum DatastoreFSyncLevel {
     Filesystem,
 }
 
-impl Default for DatastoreFSyncLevel {
-    fn default() -> Self {
-        DatastoreFSyncLevel::None
-    }
-}
-
 #[api(
     properties: {
         "chunk-order": {
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index a8582485..75880331 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -470,11 +470,10 @@ impl ChunkStore {
 
         let chunk_dir_path = chunk_path
             .parent()
-            .ok_or_else(|| format_err!("unable to get chunk dir"))?
-            .to_owned();
+            .ok_or_else(|| format_err!("unable to get chunk dir"))?;
 
         proxmox_sys::fs::replace_file(
-            chunk_path,
+            &chunk_path,
             raw_data,
             CreateOptions::new(),
             self.sync_level == DatastoreFSyncLevel::File,
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup v4 1/3] datastore: improve sync level code a bit

[Thomas Lamprecht]

On 28/10/2022 09:34, Dominik Csapak wrote:
> fixups for DatastoreFSyncLevel:
> * use derive for Default
> * add some more derives (Clone, Copy)
> 
> chunk store:
> * drop to_owned for chunk_dir_path
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs   | 9 ++-------
>  pbs-datastore/src/chunk_store.rs | 5 ++---
>  2 files changed, 4 insertions(+), 10 deletions(-)
> 
>

applied, thanks!

[pbs-devel] [PATCH proxmox-backup v4 2/3] docs: add documentation about the 'sync-level' tuning

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 docs/storage.rst | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/docs/storage.rst b/docs/storage.rst
index 1909bd84..c4e44c72 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -338,3 +338,39 @@ and only available on the CLI:
 
   # proxmox-backup-manager datastore update <storename> --tuning 'chunk-order=none'
 
+* ``sync-level``: Datastore fsync level:
+
+  You can set the level of syncing on the datastore for chunks, which influences
+  the crash resistance of backups in case of a powerloss or hard shutoff.
+  There are currently three levels:
+
+  - `none` (default): Does not do any syncing when writing chunks. This is fast
+    and normally OK, since the kernel eventually flushes writes onto the disk.
+    The kernel sysctls `dirty_expire_centisecs` and `dirty_writeback_centisecs`
+    are used to tune that behaviour, while the default is to flush old data
+    after ~30s.
+
+  - `filesystem` : This triggers a ``syncfs(2)`` after a backup, but before
+    the task returns `OK`. This way it is ensured that the written backups
+    are on disk. This is a good balance between speed and consistency.
+    Note that the underlying storage device still needs to protect itself against
+    powerloss to flush its internal ephemeral caches to the permanent storage layer.
+
+  - `file` With this mode, a fsync is triggered on every chunk insertion, which
+    makes sure each and every chunk reaches the disk as soon as possible. While
+    this reaches the highest level of consistency, for many storages (especially
+    slower ones) this comes at the cost of speed. For many users the `filesystem`
+    mode is better suited, but for very fast storages this mode can be OK.
+
+  This can be set with:
+
+.. code-block:: console
+
+  # proxmox-backup-manager datastore update <storename> --tuning 'sync-level=filesystem'
+
+If you want to set multiple tuning options simultaneously, you can separate them
+with a comma, like this:
+
+.. code-block:: console
+
+  # proxmox-backup-manager datastore update <storename> --tuning 'sync-level=filesystem,chunk-order=none'
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup v4 2/3] docs: add documentation about the 'sync-level' tuning

[Thomas Lamprecht]

On 28/10/2022 09:34, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  docs/storage.rst | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
>

applied, thanks!

[pbs-devel] [PATCH proxmox-backup v4 3/3] datastore: make 'filesystem' the default sync-level

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 docs/storage.rst               | 4 ++--
 pbs-api-types/src/datastore.rs | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/storage.rst b/docs/storage.rst
index c4e44c72..d61c3a40 100644
--- a/docs/storage.rst
+++ b/docs/storage.rst
@@ -344,13 +344,13 @@ and only available on the CLI:
   the crash resistance of backups in case of a powerloss or hard shutoff.
   There are currently three levels:
 
-  - `none` (default): Does not do any syncing when writing chunks. This is fast
+  - `none` : Does not do any syncing when writing chunks. This is fast
     and normally OK, since the kernel eventually flushes writes onto the disk.
     The kernel sysctls `dirty_expire_centisecs` and `dirty_writeback_centisecs`
     are used to tune that behaviour, while the default is to flush old data
     after ~30s.
 
-  - `filesystem` : This triggers a ``syncfs(2)`` after a backup, but before
+  - `filesystem` (default): This triggers a ``syncfs(2)`` after a backup, but before
     the task returns `OK`. This way it is ensured that the written backups
     are on disk. This is a good balance between speed and consistency.
     Note that the underlying storage device still needs to protect itself against
diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 4c9eda2f..ff8fe55f 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -181,7 +181,6 @@ pub enum DatastoreFSyncLevel {
     /// which reduces IO pressure.
     /// But it may cause losing data on powerloss or system crash without any uninterruptible power
     /// supply.
-    #[default]
     None,
     /// Triggers a fsync after writing any chunk on the datastore. While this can slow down
     /// backups significantly, depending on the underlying file system and storage used, it
@@ -196,6 +195,7 @@ pub enum DatastoreFSyncLevel {
     /// Depending on the setup, it might have a negative impact on unrelated write operations
     /// of the underlying filesystem, but it is generally a good compromise between performance
     /// and consitency.
+    #[default]
     Filesystem,
 }
 
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup v4 3/3] datastore: make 'filesystem' the default sync-level

[Thomas Lamprecht]

Updated stats would be nice, maybe with the following parings:

(v)Disk backed by
* nvme
* spinner

FS of:
- ext4
- XFS
- ZFS

Each combined with each disk backing type and current newest default
software (i.e. kernel 5.15).

Additionally maybe also NFS or CIFS. Those results would then maybe
again nice to have in the commit message.

Would help to find a more clear verdict, as currently I'm close to, but
not yet fully 100% sure if we just want to switch plainly, maybe making
it easier to change by adding it in the web interface, to the datastore
options tab and emphasizing it in the upcoming 2.3 changelog could be
already enough; depending on the numbers.

On 28/10/2022 09:34, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  docs/storage.rst               | 4 ++--
>  pbs-api-types/src/datastore.rs | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/storage.rst b/docs/storage.rst
> index c4e44c72..d61c3a40 100644
> --- a/docs/storage.rst
> +++ b/docs/storage.rst
> @@ -344,13 +344,13 @@ and only available on the CLI:
>    the crash resistance of backups in case of a powerloss or hard shutoff.
>    There are currently three levels:
>  
> -  - `none` (default): Does not do any syncing when writing chunks. This is fast
> +  - `none` : Does not do any syncing when writing chunks. This is fast
>      and normally OK, since the kernel eventually flushes writes onto the disk.
>      The kernel sysctls `dirty_expire_centisecs` and `dirty_writeback_centisecs`
>      are used to tune that behaviour, while the default is to flush old data
>      after ~30s.
>  
> -  - `filesystem` : This triggers a ``syncfs(2)`` after a backup, but before
> +  - `filesystem` (default): This triggers a ``syncfs(2)`` after a backup, but before
>      the task returns `OK`. This way it is ensured that the written backups
>      are on disk. This is a good balance between speed and consistency.
>      Note that the underlying storage device still needs to protect itself against
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 4c9eda2f..ff8fe55f 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -181,7 +181,6 @@ pub enum DatastoreFSyncLevel {
>      /// which reduces IO pressure.
>      /// But it may cause losing data on powerloss or system crash without any uninterruptible power
>      /// supply.
> -    #[default]
>      None,
>      /// Triggers a fsync after writing any chunk on the datastore. While this can slow down
>      /// backups significantly, depending on the underlying file system and storage used, it
> @@ -196,6 +195,7 @@ pub enum DatastoreFSyncLevel {
>      /// Depending on the setup, it might have a negative impact on unrelated write operations
>      /// of the underlying filesystem, but it is generally a good compromise between performance
>      /// and consitency.
> +    #[default]
>      Filesystem,
>  }
>