Re: [pbs-devel] [PATCH proxmox-backup] task tracking: improve pruning of reused-PID stale entries

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 17, 2025 10:47 am, Hannes Laimer wrote:
> On 11/17/25 10:30, Fabian Grünbichler wrote:
>> On November 17, 2025 9:58 am, Hannes Laimer wrote:
>>> Keep entries only when check_process_running(pid) reports a
>>> starttime equal to the stored one. This improves pruning of stale
>>> entries for all PIDs (not just the current one) and aligns update
>>> with the read path. Counting behavior and semantics are unchanged.
>> 
>> I don't think this is correct? the starttime comparison is only valid
>> for the current process, an old process will almost certainly have a
>> different starting time and we still want to keep its entry if it is
>> still running..
>> 
> 
> But not the same PID. This drops an entry only if the starttime for a 
> pid in `/proc/<pid>/stat` and the tracking file don't match.
> we can't have a reused pid with the old process not dead
> 
> hope I'm not missing something :P
> 

ah right, the starttime comparison there is between the running process'
pidstat and the task entry, not between the pidstat we did for our
current PID, sorry..

then we could simply re-order things like this to make this more
readable:

    let mut updated_tasks: Vec<TaskOperations> = match file_read_optional_string(&path)? {
        Some(data) => serde_json::from_str::<Vec<TaskOperations>>(&data)?
            .into_iter()
            .filter_map(
                |mut task| match procfs::check_process_running(task.pid as pid_t) {
                    // Drop entries for recycled PIDs
                    Some(stat) if stat.starttime != task.starttime => None,
                    // Update entry for current PID
                    Some(_stat) if pid == task.pid => {
                        found_entry = true;
                        match operation {
                            Operation::Read => task.active_operations.read += count,
                            Operation::Write => task.active_operations.write += count,
                            Operation::Lookup => (), // no IO must happen there
                        };
                        updated_active_operations = task.active_operations;
                        Some(task)
                    }
                    // Keep other entries
                    Some(_stat) => Some(task),
                    // Drop entries for PIDs which are not running..
                    None => None,
                },
            )
            .collect(),
        None => Vec::new(),
    };

or, since we already have a helper implementing these semantics:

    let mut updated_tasks: Vec<TaskOperations> = match file_read_optional_string(&path)? {
        Some(data) => serde_json::from_str::<Vec<TaskOperations>>(&data)?
            .into_iter()
            .filter_map(|mut task| {
                match procfs::check_process_running_pstart(task.pid as pid_t, task.starttime) {
                    // Update entry for current PID
                    Some(_stat) if pid == task.pid => {
                        found_entry = true;
                        match operation {
                            Operation::Read => task.active_operations.read += count,
                            Operation::Write => task.active_operations.write += count,
                            Operation::Lookup => (), // no IO must happen there
                        };
                        updated_active_operations = task.active_operations;
                        Some(task)
                    }
                    // Keep other entries
                    Some(_stat) => Some(task),
                    // Drop entries for PIDs which are not running or have been recycled
                    None => None,
                }
            })
            .collect(),
        None => Vec::new(),
    };

we also only ever call update_active_operations with a count of 1 or -1,
which might be worth fixing as well ;) right now, if for some reason we
call it with -1 but there is no entry, we'd end up with 1 active
operation instead of 0 or an error..

> 
>> if we want to improve this, we would need to query the process starttime
>> for all entries, and then compare, but that would make this more
>> expensive..
>> 
>>>
>>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>>> ---
>>> noticed while looking through recent proposed changes to the tracking
>>> logic. this isn't a problem, but should keep the tracking file cleaner
>>> and remove a match arm in the code
>>>
>>>   pbs-datastore/src/task_tracking.rs | 3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/pbs-datastore/src/task_tracking.rs b/pbs-datastore/src/task_tracking.rs
>>> index 44a4522d..4fcbbaa4 100644
>>> --- a/pbs-datastore/src/task_tracking.rs
>>> +++ b/pbs-datastore/src/task_tracking.rs
>>> @@ -114,8 +114,7 @@ pub fn update_active_operations(
>>>               .iter_mut()
>>>               .filter_map(
>>>                   |task| match procfs::check_process_running(task.pid as pid_t) {
>>> -                    Some(stat) if pid == task.pid && stat.starttime != task.starttime => None,
>>> -                    Some(_) => {
>>> +                    Some(stat) if stat.starttime == task.starttime => {
>>>                           if pid == task.pid {
>>>                               found_entry = true;
>>>                               match operation {
>>> -- 
>>> 2.47.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 mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel