[pbs-devel] [PATCH proxmox/proxmox-backup v3] tape: implement multithreaded read

[pbs-devel] [PATCH proxmox/proxmox-backup v3] tape: implement multithreaded read

[Dominik Csapak]

this is a replacement series for my previous patches[0]

I dropped the verification patches again, and moved the thread config to
the tape jobs/tape backup api. That way it's much nearer the actual
issue, and we don't run into problems with resource constraints, since
one job cannot run multiple times (if a user configures many jobs in
parallel, that must be deliberate then).

changes from v2:
* slight rewording of the commit message
* rebase on master (include pbs-api-types repo change)
* keep default to 1
* keep minimum of channel size 3

changes from v1:
* reordered code from verify,tape to tape,verify
* marked verify patches as rfc
* use a struct for saving the values in the datastore (as thomas
  suggested)
* introduces another verify patch to merge the chunk loading into the
  worker threads

0: https://lore.proxmox.com/pbs-devel/20240507072955.364206-1-d.csapak@proxmox.com/

proxmox:

Dominik Csapak (1):
  pbs api types: tape backup job: add worker threads option

 pbs-api-types/src/jobs.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

proxmox-backup:

Dominik Csapak (1):
  tape: introduce a tape backup job worker thread option

 src/api2/config/tape_backup_job.rs          |  8 ++++
 src/api2/tape/backup.rs                     |  4 ++
 src/tape/pool_writer/mod.rs                 | 14 ++++++-
 src/tape/pool_writer/new_chunks_iterator.rs | 44 +++++++++++++--------
 www/tape/window/TapeBackup.js               | 12 ++++++
 www/tape/window/TapeBackupJob.js            | 14 +++++++
 6 files changed, 79 insertions(+), 17 deletions(-)

-- 
2.39.5



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

[pbs-devel] [PATCH proxmox v3 1/1] pbs api types: tape backup job: add worker threads option

[Dominik Csapak]

Sometimes it's useful to give the user control over how much
parallelized the job is, so introduce a worker thread option.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-api-types/src/jobs.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 04631d92..d0b94a24 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -306,6 +306,13 @@ pub struct VerificationJobStatus {
             schema: crate::NS_MAX_DEPTH_SCHEMA,
             optional: true,
         },
+        "worker-threads": {
+            type: Integer,
+            optional: true,
+            minimum: 1,
+            maximum: 32,
+            default: 1,
+        },
     }
 )]
 #[derive(Serialize, Deserialize, Clone, Updater, PartialEq)]
@@ -332,6 +339,9 @@ pub struct TapeBackupJobSetup {
     pub ns: Option<BackupNamespace>,
     #[serde(skip_serializing_if = "Option::is_none", default)]
     pub max_depth: Option<usize>,
+    /// Set the number of worker threads to use for the job
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub worker_threads: Option<u64>,
 }
 
 #[api(
-- 
2.39.5



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

[pbs-devel] [PATCH proxmox-backup v3 1/1] tape: introduce a tape backup job worker thread option

[Dominik Csapak]

Using a single thread for reading is not optimal in some cases, e.g.
when the underlying storage can handle reads from multiple threads in
parallel.

We use the ParallelHandler to handle the actual reads. Make the
sync_channel buffer size depend on the number of threads so we have
space for two chunks per thread. (But keep the minimum to 3 like
before).

How this impacts the backup speed largely depends on the underlying
storage and how the backup is laid out on it.

I benchmarked the following setups:

* Setup A: relatively spread out backup on a virtualized pbs on single HDDs
* Setup B: mostly sequential chunks on a virtualized pbs on single HDDs
* Setup C: backup on virtualized pbs on a fast NVME
* Setup D: backup on bare metal pbs with ZFS in a RAID10 with 6 HDDs
  and 2 fast special devices in a mirror

(values are reported in MB/s as seen in the task log, caches were
cleared between runs, backups were bigger than the memory available)

setup  1 thread  2 threads  4 threads  8 threads
A      55        70         80         95
B      110       89         100        108
C      294       294        294        294
D      118       180        300        300

So there are cases where multiple read threads speed up the tape backup
(dramatically). On the other hand there are situations where reading
from a single thread is actually faster, probably because we can read
from the HDD sequentially.

I left the default value of '1' to not change the default behavior.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v2:
* move the ui config to the tape backup jobs and tape backup window
* use pool writer to store the thread count, not datastore
* keep minimum of channel size 3
* keep default of one thread

i left the benchmark data intact, since the actual code that does
the multithreading is the same as before, and i could not find a
virtual setup to replicate the performance of a hdd very well
(limiting virtual disks iops does not really do the trick due to
disk caching, etc.)

If wanted i can of course setup a physical testbed with multiple hdds
again.

 src/api2/config/tape_backup_job.rs          |  8 ++++
 src/api2/tape/backup.rs                     |  4 ++
 src/tape/pool_writer/mod.rs                 | 14 ++++++-
 src/tape/pool_writer/new_chunks_iterator.rs | 44 +++++++++++++--------
 www/tape/window/TapeBackup.js               | 12 ++++++
 www/tape/window/TapeBackupJob.js            | 14 +++++++
 6 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/src/api2/config/tape_backup_job.rs b/src/api2/config/tape_backup_job.rs
index b6db92998..f2abf652b 100644
--- a/src/api2/config/tape_backup_job.rs
+++ b/src/api2/config/tape_backup_job.rs
@@ -140,6 +140,8 @@ pub enum DeletableProperty {
     MaxDepth,
     /// Delete the 'ns' property
     Ns,
+    /// Delete the 'worker-threads' property
+    WorkerThreads,
 }
 
 #[api(
@@ -222,6 +224,9 @@ pub fn update_tape_backup_job(
                 DeletableProperty::Ns => {
                     data.setup.ns = None;
                 }
+                DeletableProperty::WorkerThreads => {
+                    data.setup.worker_threads = None;
+                }
             }
         }
     }
@@ -260,6 +265,9 @@ pub fn update_tape_backup_job(
     if update.setup.max_depth.is_some() {
         data.setup.max_depth = update.setup.max_depth;
     }
+    if update.setup.worker_threads.is_some() {
+        data.setup.worker_threads = update.setup.worker_threads;
+    }
 
     let schedule_changed = data.schedule != update.schedule;
     if update.schedule.is_some() {
diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
index cf5a01897..31293a9a9 100644
--- a/src/api2/tape/backup.rs
+++ b/src/api2/tape/backup.rs
@@ -387,6 +387,10 @@ fn backup_worker(
         ns_magic,
     )?;
 
+    if let Some(threads) = setup.worker_threads {
+        pool_writer.set_read_thread_count(threads as usize);
+    }
+
     let mut group_list = Vec::new();
     let namespaces = datastore.recursive_iter_backup_ns_ok(root_namespace, setup.max_depth)?;
     for ns in namespaces {
diff --git a/src/tape/pool_writer/mod.rs b/src/tape/pool_writer/mod.rs
index 3114ec061..540844212 100644
--- a/src/tape/pool_writer/mod.rs
+++ b/src/tape/pool_writer/mod.rs
@@ -56,6 +56,7 @@ pub struct PoolWriter {
     notification_mode: TapeNotificationMode,
     ns_magic: bool,
     used_tapes: HashSet<Uuid>,
+    read_threads: usize,
 }
 
 impl PoolWriter {
@@ -93,9 +94,15 @@ impl PoolWriter {
             notification_mode,
             ns_magic,
             used_tapes: HashSet::new(),
+            read_threads: 1,
         })
     }
 
+    /// Set the read threads to use when writing a backup to tape
+    pub fn set_read_thread_count(&mut self, read_threads: usize) {
+        self.read_threads = read_threads;
+    }
+
     pub fn pool(&mut self) -> &mut MediaPool {
         &mut self.pool
     }
@@ -541,7 +548,12 @@ impl PoolWriter {
         datastore: Arc<DataStore>,
         snapshot_reader: Arc<Mutex<SnapshotReader>>,
     ) -> Result<(std::thread::JoinHandle<()>, NewChunksIterator), Error> {
-        NewChunksIterator::spawn(datastore, snapshot_reader, Arc::clone(&self.catalog_set))
+        NewChunksIterator::spawn(
+            datastore,
+            snapshot_reader,
+            Arc::clone(&self.catalog_set),
+            self.read_threads,
+        )
     }
 
     pub(crate) fn catalog_version(&self) -> [u8; 8] {
diff --git a/src/tape/pool_writer/new_chunks_iterator.rs b/src/tape/pool_writer/new_chunks_iterator.rs
index 1454b33d2..de847b3c9 100644
--- a/src/tape/pool_writer/new_chunks_iterator.rs
+++ b/src/tape/pool_writer/new_chunks_iterator.rs
@@ -6,8 +6,9 @@ use anyhow::{format_err, Error};
 use pbs_datastore::{DataBlob, DataStore, SnapshotReader};
 
 use crate::tape::CatalogSet;
+use crate::tools::parallel_handler::ParallelHandler;
 
-/// Chunk iterator which use a separate thread to read chunks
+/// Chunk iterator which uses separate threads to read chunks
 ///
 /// The iterator skips duplicate chunks and chunks already in the
 /// catalog.
@@ -24,8 +25,11 @@ impl NewChunksIterator {
         datastore: Arc<DataStore>,
         snapshot_reader: Arc<Mutex<SnapshotReader>>,
         catalog_set: Arc<Mutex<CatalogSet>>,
+        read_threads: usize,
     ) -> Result<(std::thread::JoinHandle<()>, Self), Error> {
-        let (tx, rx) = std::sync::mpsc::sync_channel(3);
+        // use twice the threadcount for the channel, so the read thread can already send another
+        // one when the previous one was not consumed yet, but keep the minimum at 3
+        let (tx, rx) = std::sync::mpsc::sync_channel((read_threads * 2).max(3));
 
         let reader_thread = std::thread::spawn(move || {
             let snapshot_reader = snapshot_reader.lock().unwrap();
@@ -35,36 +39,44 @@ impl NewChunksIterator {
             let datastore_name = snapshot_reader.datastore_name().to_string();
 
             let result: Result<(), Error> = proxmox_lang::try_block!({
-                let mut chunk_iter = snapshot_reader.chunk_iterator(move |digest| {
+                let chunk_iter = snapshot_reader.chunk_iterator(move |digest| {
                     catalog_set
                         .lock()
                         .unwrap()
                         .contains_chunk(&datastore_name, digest)
                 })?;
 
-                loop {
-                    let digest = match chunk_iter.next() {
-                        None => {
-                            let _ = tx.send(Ok(None)); // ignore send error
-                            break;
+                let reader_pool =
+                    ParallelHandler::new("tape backup chunk reader pool", read_threads, {
+                        let tx = tx.clone();
+                        move |digest| {
+                            let blob = datastore.load_chunk(&digest)?;
+                            //println!("LOAD CHUNK {}", hex::encode(&digest));
+
+                            tx.send(Ok(Some((digest, blob)))).map_err(|err| {
+                                format_err!("error sending result from reader thread: {err}")
+                            })?;
+
+                            Ok(())
                         }
-                        Some(digest) => digest?,
-                    };
+                    });
+
+                for digest in chunk_iter {
+                    let digest = digest?;
 
                     if chunk_index.contains(&digest) {
                         continue;
                     }
 
-                    let blob = datastore.load_chunk(&digest)?;
-                    //println!("LOAD CHUNK {}", hex::encode(&digest));
-                    if let Err(err) = tx.send(Ok(Some((digest, blob)))) {
-                        eprintln!("could not send chunk to reader thread: {err}");
-                        break;
-                    }
+                    reader_pool.send(digest)?;
 
                     chunk_index.insert(digest);
                 }
 
+                reader_pool.complete()?;
+
+                let _ = tx.send(Ok(None)); // ignore send error
+
                 Ok(())
             });
             if let Err(err) = result {
diff --git a/www/tape/window/TapeBackup.js b/www/tape/window/TapeBackup.js
index 7a45e388c..fc8be80e3 100644
--- a/www/tape/window/TapeBackup.js
+++ b/www/tape/window/TapeBackup.js
@@ -119,6 +119,18 @@ Ext.define('PBS.TapeManagement.TapeBackupWindow', {
 		    renderer: Ext.String.htmlEncode,
 		},
 	    ],
+
+	    advancedColumn1: [
+		{
+		    xtype: 'proxmoxintegerfield',
+		    name: 'worker-threads',
+		    fieldLabel: gettext('# of Worker Threads'),
+		    emptyText: '1',
+		    skipEmptyText: true,
+		    minValue: 1,
+		    maxValue: 32,
+		},
+	    ],
 	},
     ],
 
diff --git a/www/tape/window/TapeBackupJob.js b/www/tape/window/TapeBackupJob.js
index 12623712a..24afb9fb6 100644
--- a/www/tape/window/TapeBackupJob.js
+++ b/www/tape/window/TapeBackupJob.js
@@ -216,6 +216,20 @@ Ext.define('PBS.TapeManagement.BackupJobEdit', {
 			},
 		    },
 		],
+
+		advancedColumn1: [
+		    {
+			xtype: 'proxmoxintegerfield',
+			name: 'worker-threads',
+			fieldLabel: gettext('# of Worker Threads'),
+			emptyText: '1',
+			minValue: 1,
+			maxValue: 32,
+			cbind: {
+			    deleteEmpty: '{!isCreate}',
+			},
+		    },
+		],
 	    },
 	    {
 		xtype: 'inputpanel',
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox/proxmox-backup v3] tape: implement multithreaded read

[Dominik Csapak]

On 2/21/25 16:06, Dominik Csapak wrote:
> this is a replacement series for my previous patches[0]
> 
> I dropped the verification patches again, and moved the thread config to
> the tape jobs/tape backup api. That way it's much nearer the actual
> issue, and we don't run into problems with resource constraints, since
> one job cannot run multiple times (if a user configures many jobs in
> parallel, that must be deliberate then).
> 
> changes from v2:
> * slight rewording of the commit message
> * rebase on master (include pbs-api-types repo change)
> * keep default to 1
> * keep minimum of channel size 3
> 
> changes from v1:
> * reordered code from verify,tape to tape,verify
> * marked verify patches as rfc
> * use a struct for saving the values in the datastore (as thomas
>    suggested)
> * introduces another verify patch to merge the chunk loading into the
>    worker threads
> 
> 0: https://lore.proxmox.com/pbs-devel/20240507072955.364206-1-d.csapak@proxmox.com/
> 

ping, would be nice to get feedback on this, since some users are asking for this on the forum


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

Re: [pbs-devel] [PATCH proxmox/proxmox-backup v3] tape: implement multithreaded read

[Dietmar Maurer]

This does not apply cleanly. I guess this needs a rebase...


> On 19.3.2025 15:12 CET Dominik Csapak <d.csapak@proxmox.com> wrote:
> 
>  
> On 2/21/25 16:06, Dominik Csapak wrote:
> > this is a replacement series for my previous patches[0]
> > 
> > I dropped the verification patches again, and moved the thread config to
> > the tape jobs/tape backup api. That way it's much nearer the actual
> > issue, and we don't run into problems with resource constraints, since
> > one job cannot run multiple times (if a user configures many jobs in
> > parallel, that must be deliberate then).
> > 
> > changes from v2:
> > * slight rewording of the commit message
> > * rebase on master (include pbs-api-types repo change)
> > * keep default to 1
> > * keep minimum of channel size 3
> > 
> > changes from v1:
> > * reordered code from verify,tape to tape,verify
> > * marked verify patches as rfc
> > * use a struct for saving the values in the datastore (as thomas
> >    suggested)
> > * introduces another verify patch to merge the chunk loading into the
> >    worker threads
> > 
> > 0: https://lore.proxmox.com/pbs-devel/20240507072955.364206-1-d.csapak@proxmox.com/
> > 
> 
> ping, would be nice to get feedback on this, since some users are asking for this on the forum
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


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

Re: [pbs-devel] [PATCH proxmox/proxmox-backup v3] tape: implement multithreaded read

[Dietmar Maurer]

Sorry, just noticed it applies without problems - will test again...

> This does not apply cleanly. I guess this needs a rebase...


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

Re: [pbs-devel] [PATCH proxmox-backup v3 1/1] tape: introduce a tape backup job worker thread option

[Thomas Lamprecht]

Am 21.02.25 um 16:06 schrieb Dominik Csapak:
> Using a single thread for reading is not optimal in some cases, e.g.
> when the underlying storage can handle reads from multiple threads in
> parallel.
> 
> We use the ParallelHandler to handle the actual reads. Make the
> sync_channel buffer size depend on the number of threads so we have
> space for two chunks per thread. (But keep the minimum to 3 like
> before).
> 
> How this impacts the backup speed largely depends on the underlying
> storage and how the backup is laid out on it.

And the amount of tasks going on at the same time, which can wildly
influence the result and is not fully under the admin control as the
duration of tasks is not exactly fixed.

FWIW, I think this is an OK stop-gap as it's really simple and if the
admin is somewhat careful with configuring this and the tasks schedules
it might help them already quite a bit as your benchmark shows, and
again, it _really_ is simple, and the whole tape subsystem is a bit more
specific and contained.

That said, in the long-term this would probably be better replaced with
a global scheduling approach that respects this and other tasks workloads
and resource usage, which is certainly not an easy thing to do as there
are many aspects one needs to think through and schedulers are not really
a done thing in academic research either, especially not general ones.
I'm mostly mentioning this to avoid proliferation of such a mechanism to
other tasks, as that would result in a configuration hell for admin where
they hardly can tune for their workloads sanely anymore.

Code looks OK to me, one tiny nit about a comment inline though.

> diff --git a/src/tape/pool_writer/new_chunks_iterator.rs b/src/tape/pool_writer/new_chunks_iterator.rs
> index 1454b33d2..de847b3c9 100644
> --- a/src/tape/pool_writer/new_chunks_iterator.rs
> +++ b/src/tape/pool_writer/new_chunks_iterator.rs
> @@ -6,8 +6,9 @@ use anyhow::{format_err, Error};
>  use pbs_datastore::{DataBlob, DataStore, SnapshotReader};
>  
>  use crate::tape::CatalogSet;
> +use crate::tools::parallel_handler::ParallelHandler;
>  
> -/// Chunk iterator which use a separate thread to read chunks
> +/// Chunk iterator which uses separate threads to read chunks
>  ///
>  /// The iterator skips duplicate chunks and chunks already in the
>  /// catalog.
> @@ -24,8 +25,11 @@ impl NewChunksIterator {
>          datastore: Arc<DataStore>,
>          snapshot_reader: Arc<Mutex<SnapshotReader>>,
>          catalog_set: Arc<Mutex<CatalogSet>>,
> +        read_threads: usize,
>      ) -> Result<(std::thread::JoinHandle<()>, Self), Error> {
> -        let (tx, rx) = std::sync::mpsc::sync_channel(3);
> +        // use twice the threadcount for the channel, so the read thread can already send another
> +        // one when the previous one was not consumed yet, but keep the minimum at 3

this reads a bit confusing like the channel could go up to 2 x thread
count, while that does not make much sense if one is well acquainted
with the matter at hand it'd be IMO still nicer to clarify that for
others stumbling into this, maybe something like:


// set the buffer size of the channel queues to twice the number of threads or 3, whichever
// is greater, to reduce the chance of a reader thread (producer) being blocked.

Can be fixed up on applying though, if you agree (or propose something better).

> +        let (tx, rx) = std::sync::mpsc::sync_channel((read_threads * 2).max(3));




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

Re: [pbs-devel] [PATCH proxmox-backup v3 1/1] tape: introduce a tape backup job worker thread option

[Dominik Csapak]

On 3/20/25 17:30, Thomas Lamprecht wrote:
> Am 21.02.25 um 16:06 schrieb Dominik Csapak:
>> Using a single thread for reading is not optimal in some cases, e.g.
>> when the underlying storage can handle reads from multiple threads in
>> parallel.
>>
>> We use the ParallelHandler to handle the actual reads. Make the
>> sync_channel buffer size depend on the number of threads so we have
>> space for two chunks per thread. (But keep the minimum to 3 like
>> before).
>>
>> How this impacts the backup speed largely depends on the underlying
>> storage and how the backup is laid out on it.
> 
> And the amount of tasks going on at the same time, which can wildly
> influence the result and is not fully under the admin control as the
> duration of tasks is not exactly fixed.
> 
> FWIW, I think this is an OK stop-gap as it's really simple and if the
> admin is somewhat careful with configuring this and the tasks schedules
> it might help them already quite a bit as your benchmark shows, and
> again, it _really_ is simple, and the whole tape subsystem is a bit more
> specific and contained.
> 
> That said, in the long-term this would probably be better replaced with
> a global scheduling approach that respects this and other tasks workloads
> and resource usage, which is certainly not an easy thing to do as there
> are many aspects one needs to think through and schedulers are not really
> a done thing in academic research either, especially not general ones.
> I'm mostly mentioning this to avoid proliferation of such a mechanism to
> other tasks, as that would result in a configuration hell for admin where
> they hardly can tune for their workloads sanely anymore.
> 

I fully agree here with you, I sent it this way again because we have users running into this now
and don't want/can't wait for the proper fix with a scheduler.

> Code looks OK to me, one tiny nit about a comment inline though.
> 
>> diff --git a/src/tape/pool_writer/new_chunks_iterator.rs b/src/tape/pool_writer/new_chunks_iterator.rs
>> index 1454b33d2..de847b3c9 100644
>> --- a/src/tape/pool_writer/new_chunks_iterator.rs
>> +++ b/src/tape/pool_writer/new_chunks_iterator.rs
>> @@ -6,8 +6,9 @@ use anyhow::{format_err, Error};
>>   use pbs_datastore::{DataBlob, DataStore, SnapshotReader};
>>   
>>   use crate::tape::CatalogSet;
>> +use crate::tools::parallel_handler::ParallelHandler;
>>   
>> -/// Chunk iterator which use a separate thread to read chunks
>> +/// Chunk iterator which uses separate threads to read chunks
>>   ///
>>   /// The iterator skips duplicate chunks and chunks already in the
>>   /// catalog.
>> @@ -24,8 +25,11 @@ impl NewChunksIterator {
>>           datastore: Arc<DataStore>,
>>           snapshot_reader: Arc<Mutex<SnapshotReader>>,
>>           catalog_set: Arc<Mutex<CatalogSet>>,
>> +        read_threads: usize,
>>       ) -> Result<(std::thread::JoinHandle<()>, Self), Error> {
>> -        let (tx, rx) = std::sync::mpsc::sync_channel(3);
>> +        // use twice the threadcount for the channel, so the read thread can already send another
>> +        // one when the previous one was not consumed yet, but keep the minimum at 3
> 
> this reads a bit confusing like the channel could go up to 2 x thread
> count, while that does not make much sense if one is well acquainted
> with the matter at hand it'd be IMO still nicer to clarify that for
> others stumbling into this, maybe something like:
> 
> 
> // set the buffer size of the channel queues to twice the number of threads or 3, whichever
> // is greater, to reduce the chance of a reader thread (producer) being blocked.
> 
> Can be fixed up on applying though, if you agree (or propose something better).

sound fine to me

> 
>> +        let (tx, rx) = std::sync::mpsc::sync_channel((read_threads * 2).max(3));
> 
> 



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

Re: [pbs-devel] [PATCH proxmox-backup v3 1/1] tape: introduce a tape backup job worker thread option

[Laurențiu Leahu-Vlăducu]

I tested your patches with a virtual tape library (thus did not notice 
any speed difference), but they worked as intended, without issues. LGTM 
otherwise, so please consider:

Tested-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>


On 20.03.25 17:30, Thomas Lamprecht wrote:
> Am 21.02.25 um 16:06 schrieb Dominik Csapak:
>> Using a single thread for reading is not optimal in some cases, e.g.
>> when the underlying storage can handle reads from multiple threads in
>> parallel.
>>
>> We use the ParallelHandler to handle the actual reads. Make the
>> sync_channel buffer size depend on the number of threads so we have
>> space for two chunks per thread. (But keep the minimum to 3 like
>> before).
>>
>> How this impacts the backup speed largely depends on the underlying
>> storage and how the backup is laid out on it.
> 
> And the amount of tasks going on at the same time, which can wildly
> influence the result and is not fully under the admin control as the
> duration of tasks is not exactly fixed.
> 
> FWIW, I think this is an OK stop-gap as it's really simple and if the
> admin is somewhat careful with configuring this and the tasks schedules
> it might help them already quite a bit as your benchmark shows, and
> again, it _really_ is simple, and the whole tape subsystem is a bit more
> specific and contained.
> 
> That said, in the long-term this would probably be better replaced with
> a global scheduling approach that respects this and other tasks workloads
> and resource usage, which is certainly not an easy thing to do as there
> are many aspects one needs to think through and schedulers are not really
> a done thing in academic research either, especially not general ones.
> I'm mostly mentioning this to avoid proliferation of such a mechanism to
> other tasks, as that would result in a configuration hell for admin where
> they hardly can tune for their workloads sanely anymore.
> 
> Code looks OK to me, one tiny nit about a comment inline though.
> 
>> diff --git a/src/tape/pool_writer/new_chunks_iterator.rs b/src/tape/pool_writer/new_chunks_iterator.rs
>> index 1454b33d2..de847b3c9 100644
>> --- a/src/tape/pool_writer/new_chunks_iterator.rs
>> +++ b/src/tape/pool_writer/new_chunks_iterator.rs
>> @@ -6,8 +6,9 @@ use anyhow::{format_err, Error};
>>   use pbs_datastore::{DataBlob, DataStore, SnapshotReader};
>>   
>>   use crate::tape::CatalogSet;
>> +use crate::tools::parallel_handler::ParallelHandler;
>>   
>> -/// Chunk iterator which use a separate thread to read chunks
>> +/// Chunk iterator which uses separate threads to read chunks
>>   ///
>>   /// The iterator skips duplicate chunks and chunks already in the
>>   /// catalog.
>> @@ -24,8 +25,11 @@ impl NewChunksIterator {
>>           datastore: Arc<DataStore>,
>>           snapshot_reader: Arc<Mutex<SnapshotReader>>,
>>           catalog_set: Arc<Mutex<CatalogSet>>,
>> +        read_threads: usize,
>>       ) -> Result<(std::thread::JoinHandle<()>, Self), Error> {
>> -        let (tx, rx) = std::sync::mpsc::sync_channel(3);
>> +        // use twice the threadcount for the channel, so the read thread can already send another
>> +        // one when the previous one was not consumed yet, but keep the minimum at 3
> 
> this reads a bit confusing like the channel could go up to 2 x thread
> count, while that does not make much sense if one is well acquainted
> with the matter at hand it'd be IMO still nicer to clarify that for
> others stumbling into this, maybe something like:
> 
> 
> // set the buffer size of the channel queues to twice the number of threads or 3, whichever
> // is greater, to reduce the chance of a reader thread (producer) being blocked.
> 
> Can be fixed up on applying though, if you agree (or propose something better).
> 
>> +        let (tx, rx) = std::sync::mpsc::sync_channel((read_threads * 2).max(3));
> 
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



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

[pbs-devel] applied: [PATCH proxmox v3 1/1] pbs api types: tape backup job: add worker threads option

[Thomas Lamprecht]

Am 21.02.25 um 16:06 schrieb Dominik Csapak:
> Sometimes it's useful to give the user control over how much
> parallelized the job is, so introduce a worker thread option.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-api-types/src/jobs.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
>

applied, thanks!


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

[pbs-devel] applied: [PATCH proxmox-backup v3 1/1] tape: introduce a tape backup job worker thread option

[Thomas Lamprecht]

Am 21.02.25 um 16:06 schrieb Dominik Csapak:
> Using a single thread for reading is not optimal in some cases, e.g.
> when the underlying storage can handle reads from multiple threads in
> parallel.
> 
> We use the ParallelHandler to handle the actual reads. Make the
> sync_channel buffer size depend on the number of threads so we have
> space for two chunks per thread. (But keep the minimum to 3 like
> before).
> 
> How this impacts the backup speed largely depends on the underlying
> storage and how the backup is laid out on it.
> 
> I benchmarked the following setups:
> 
> * Setup A: relatively spread out backup on a virtualized pbs on single HDDs
> * Setup B: mostly sequential chunks on a virtualized pbs on single HDDs
> * Setup C: backup on virtualized pbs on a fast NVME
> * Setup D: backup on bare metal pbs with ZFS in a RAID10 with 6 HDDs
>   and 2 fast special devices in a mirror
> 
> (values are reported in MB/s as seen in the task log, caches were
> cleared between runs, backups were bigger than the memory available)
> 
> setup  1 thread  2 threads  4 threads  8 threads
> A      55        70         80         95
> B      110       89         100        108
> C      294       294        294        294
> D      118       180        300        300
> 
> So there are cases where multiple read threads speed up the tape backup
> (dramatically). On the other hand there are situations where reading
> from a single thread is actually faster, probably because we can read
> from the HDD sequentially.
> 
> I left the default value of '1' to not change the default behavior.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v2:
> * move the ui config to the tape backup jobs and tape backup window
> * use pool writer to store the thread count, not datastore
> * keep minimum of channel size 3
> * keep default of one thread
> 
> i left the benchmark data intact, since the actual code that does
> the multithreading is the same as before, and i could not find a
> virtual setup to replicate the performance of a hdd very well
> (limiting virtual disks iops does not really do the trick due to
> disk caching, etc.)
> 
> If wanted i can of course setup a physical testbed with multiple hdds
> again.
> 
>  src/api2/config/tape_backup_job.rs          |  8 ++++
>  src/api2/tape/backup.rs                     |  4 ++
>  src/tape/pool_writer/mod.rs                 | 14 ++++++-
>  src/tape/pool_writer/new_chunks_iterator.rs | 44 +++++++++++++--------
>  www/tape/window/TapeBackup.js               | 12 ++++++
>  www/tape/window/TapeBackupJob.js            | 14 +++++++
>  6 files changed, 79 insertions(+), 17 deletions(-)
> 
>

applied, thanks! bumped the dependency for pbs-api-types upfront and adapted
the comment as discussed in this thread. I also dropped the commented-out
printf statement, feels always rather odd to read them to me, especially for
compiled code where one cannot uncomment them ad-hoc. debug/trace-logs might
be better suited if that can be useful.


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