[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));
> 
>