* [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.