From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 636DE61036 for ; Tue, 8 Feb 2022 11:13:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5A4B72CDE1 for ; Tue, 8 Feb 2022 11:13:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 99EAC2CDD8 for ; Tue, 8 Feb 2022 11:13:01 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 6A62B46109 for ; Tue, 8 Feb 2022 11:13:01 +0100 (CET) Date: Tue, 8 Feb 2022 11:13:00 +0100 From: Wolfgang Bumiller To: Hannes Laimer Cc: pbs-devel@lists.proxmox.com Message-ID: <20220208101300.23b2et6v3weq2y5e@olga.proxmox.com> References: <20220204111729.22107-1-h.laimer@proxmox.com> <20220204111729.22107-4-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220204111729.22107-4-h.laimer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.393 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs, maintenance.rs, datastore.rs, lib.rs, proxmox-backup-api.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup v7 3/6] pbs-datastore: add active operations tracking X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Feb 2022 10:13:02 -0000 On Fri, Feb 04, 2022 at 11:17:26AM +0000, Hannes Laimer wrote: > Saves the currently active read/write operation counts in a file. The > file is updated whenever a reference returned by lookup_datastore is > dropped and whenever a reference is returned by lookup_datastore. The > files are locked before every access, there is one file per datastore. > > Signed-off-by: Hannes Laimer > --- > pbs-api-types/src/maintenance.rs | 1 + > pbs-datastore/Cargo.toml | 1 + > pbs-datastore/src/datastore.rs | 114 +++++++++++++++++++---------- > pbs-datastore/src/lib.rs | 4 + > pbs-datastore/src/task_tracking.rs | 81 ++++++++++++++++++++ > src/bin/proxmox-backup-api.rs | 1 + > src/server/mod.rs | 16 +++- > 7 files changed, 180 insertions(+), 38 deletions(-) > create mode 100644 pbs-datastore/src/task_tracking.rs > > diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs > index 4d3ccb3b..1b5753d2 100644 > --- a/pbs-api-types/src/maintenance.rs > +++ b/pbs-api-types/src/maintenance.rs > @@ -25,6 +25,7 @@ pub enum MaintenanceType { > Offline(String), > } > > +#[derive(Copy ,Clone)] ^ wrong space > /// Operation requirments, used when checking for maintenance mode. > pub enum Operation { > Read, > diff --git a/pbs-datastore/Cargo.toml b/pbs-datastore/Cargo.toml > index d8cefa00..0c703d44 100644 > @@ -54,13 +56,42 @@ pub fn check_backup_owner( > /// > /// A Datastore can store severals backups, and provides the > /// management interface for backup. > -pub struct DataStore { > +pub struct DataStoreImpl { > chunk_store: Arc, > gc_mutex: Mutex<()>, > last_gc_status: Mutex, > verify_new: bool, > } > > +pub struct DataStore { > + inner: Arc, > + operation: Option, > +} > + > +impl Clone for DataStore { > + fn clone(&self) -> Self { > + if let Some(operation) = self.operation.clone() { > + if let Err(e) = update_active_operations(self.name(), operation, 1) { > + eprintln!("could not update active operations - {}", e); I think if this fails we should return a value with `operation: None` *or* have the count as a separate field, otherwise `Drop` will cause a miscount here, no? Also, please prefer `log::error!` here over `eprintln!`. > + } > + } > + DataStore { > + inner: self.inner.clone(), > + operation: self.operation.clone(), > + } > + } > +} > + > +impl Drop for DataStore { > + fn drop(&mut self) { > + if let Some(operation) = self.operation.clone() { > + if let Err(e) = update_active_operations(self.name(), operation, -1) { > + eprintln!("could not update active operations - {}", e); here too, `eprintln!` -> `log::error!` > + } > + } > + } > +} > + > impl DataStore { > pub fn lookup_datastore( > name: &str, > diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs > index d50a64a5..131040c6 100644 > diff --git a/pbs-datastore/src/task_tracking.rs b/pbs-datastore/src/task_tracking.rs > new file mode 100644 > index 00000000..a02d9a17 > --- /dev/null > +++ b/pbs-datastore/src/task_tracking.rs > @@ -0,0 +1,81 @@ > +use anyhow::Error; > +use libc::pid_t; > +use nix::unistd::Pid; > +use std::path::PathBuf; > + > +use pbs_api_types::Operation; > +use proxmox_sys::fs::{file_read_optional_string, open_file_locked, replace_file, CreateOptions}; > +use proxmox_sys::linux::procfs; > +use serde::{Deserialize, Serialize}; > + > +#[derive(Deserialize, Serialize, Clone)] > +struct TaskOperations { > + pid: u32, > + starttime: u64, > + reading_operations: i64, > + writing_operations: i64, > +} > + > +pub fn update_active_operations(name: &str, operation: Operation, count: i64) -> Result<(), Error> { I'm thinking there might be a way to use shared memory here, but dealing with dead processes becomes a bit more tricky. If you're motivated, you could take a look at `pbs-config/src/config_version_cache.rs` to see what I mean (DataStoreImpl could contain the mmapped region, but dealing with dead processes is still a bit of work, would need some atomics for the PID and registration state of a process...) > + let path = PathBuf::from(format!("{}/{}", crate::ACTIVE_OPERATIONS_DIR, name)); > + let lock_path = PathBuf::from(format!("{}/{}.lock", crate::ACTIVE_OPERATIONS_DIR, name)); > (...)