* [pbs-devel] [PATCH proxmox-backup] task tracking: improve pruning of reused-PID stale entries
@ 2025-11-17 8:58 Hannes Laimer
2025-11-17 9:30 ` Fabian Grünbichler
0 siblings, 1 reply; 4+ messages in thread
From: Hannes Laimer @ 2025-11-17 8:58 UTC (permalink / raw)
To: pbs-devel
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.
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] task tracking: improve pruning of reused-PID stale entries
2025-11-17 8:58 [pbs-devel] [PATCH proxmox-backup] task tracking: improve pruning of reused-PID stale entries Hannes Laimer
@ 2025-11-17 9:30 ` Fabian Grünbichler
2025-11-17 9:47 ` Hannes Laimer
0 siblings, 1 reply; 4+ messages in thread
From: Fabian Grünbichler @ 2025-11-17 9:30 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
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..
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] task tracking: improve pruning of reused-PID stale entries
2025-11-17 9:30 ` Fabian Grünbichler
@ 2025-11-17 9:47 ` Hannes Laimer
2025-11-17 12:07 ` Fabian Grünbichler
0 siblings, 1 reply; 4+ messages in thread
From: Hannes Laimer @ 2025-11-17 9:47 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Fabian Grünbichler
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
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] task tracking: improve pruning of reused-PID stale entries
2025-11-17 9:47 ` Hannes Laimer
@ 2025-11-17 12:07 ` Fabian Grünbichler
0 siblings, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2025-11-17 12:07 UTC (permalink / raw)
To: Hannes Laimer, Proxmox Backup Server development discussion
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-11-17 12:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-17 8:58 [pbs-devel] [PATCH proxmox-backup] task tracking: improve pruning of reused-PID stale entries Hannes Laimer
2025-11-17 9:30 ` Fabian Grünbichler
2025-11-17 9:47 ` Hannes Laimer
2025-11-17 12:07 ` Fabian Grünbichler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.