Re: [pbs-devel] [PATCH proxmox-backup] api2/tape/backup: wait indefinitely for lock in scheduled backup jobs

[Dominik Csapak <d.csapak@proxmox.com>]

On 3/18/21 06:53, Dietmar Maurer wrote:
>> -    // early check/lock before starting worker
>> -    let drive_lock = lock_tape_device(&drive_config, &setup.drive)?;
>> +    // for scheduled jobs we acquire the lock later in the worker
>> +    let drive_lock = if schedule.is_some() {
>> +        None
>> +    } else {
>> +        Some(lock_tape_device(&drive_config, &setup.drive)?)
>> +    };
> 
> What is the reason for the different locking times? Can't we always lock
> later in the worker?
> 

yes we can, but for the non-scheduled backup, we only have one try
to lock and i did not want to start a task since that would generate
a task entry that was preventable by checking beforehand

On 3/18/21 07:04, Dietmar Maurer wrote:
 >
 >> +            let (job_result, summary) = match try_block!({
 >> +                if schedule.is_some() {
 >> +                    // for scheduled tape backup jobs, we wait 
indefinitely for the lock
 >> +                    task_log!(worker, "waiting for drive lock...");
 >> +                    loop {
 >> +                        if let Ok(lock) = 
lock_tape_device(&drive_config, &setup.drive) {
 >> +                            drive_lock = Some(lock);
 >> +                            break;
 >> +                        } // ignore errors
 >
 > I would prefer to add a timeout parameter to lock_tape_device() call.
 > The question is if the lock call gets interrupted by task abort?

i did it this way, because an abort would not trigger if we are just 
waiting on a lock, so i decided to try the lock in a loop with
the check_abort()? call below (so one can abort this)

we could ofc add a timeout parameter, but would not change
the code here (and currently has no real upside, since
we probably would not use different timeouts anyway?)

 >
 >> +
 >> +                        worker.check_abort()?;
 >> +                    }
 >> +                }
 >> +                set_tape_device_state(&setup.drive, 
&worker.upid().to_string())?;