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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B042C9707D for ; Mon, 4 Mar 2024 11:43:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8E5E115C2B for ; Mon, 4 Mar 2024 11:42:30 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 4 Mar 2024 11:42:29 +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 92B8C42695 for ; Mon, 4 Mar 2024 11:42:29 +0100 (CET) Message-ID: <07b5578e-52c4-4c41-83e2-20f5f73fce93@proxmox.com> Date: Mon, 4 Mar 2024 11:42:28 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox Backup Server development discussion , Hannes Laimer References: <20240301150315.12253-1-h.laimer@proxmox.com> Content-Language: en-GB, de-AT From: Thomas Lamprecht Autocrypt: addr=t.lamprecht@proxmox.com; keydata= xsFNBFsLjcYBEACsaQP6uTtw/xHTUCKF4VD4/Wfg7gGn47+OfCKJQAD+Oyb3HSBkjclopC5J uXsB1vVOfqVYE6PO8FlD2L5nxgT3SWkc6Ka634G/yGDU3ZC3C/7NcDVKhSBI5E0ww4Qj8s9w OQRloemb5LOBkJNEUshkWRTHHOmk6QqFB/qBPW2COpAx6oyxVUvBCgm/1S0dAZ9gfkvpqFSD 90B5j3bL6i9FIv3YGUCgz6Ue3f7u+HsEAew6TMtlt90XV3vT4M2IOuECG/pXwTy7NtmHaBQ7 UJBcwSOpDEweNob50+9B4KbnVn1ydx+K6UnEcGDvUWBkREccvuExvupYYYQ5dIhRFf3fkS4+ wMlyAFh8PQUgauod+vqs45FJaSgTqIALSBsEHKEs6IoTXtnnpbhu3p6XBin4hunwoBFiyYt6 YHLAM1yLfCyX510DFzX/Ze2hLqatqzY5Wa7NIXqYYelz7tXiuCLHP84+sV6JtEkeSUCuOiUY virj6nT/nJK8m0BzdR6FgGtNxp7RVXFRz/+mwijJVLpFsyG1i0Hmv2zTn3h2nyGK/I6yhFNt dX69y5hbo6LAsRjLUvZeHXpTU4TrpN/WiCjJblbj5um5eEr4yhcwhVmG102puTtuCECsDucZ jpKpUqzXlpLbzG/dp9dXFH3MivvfuaHrg3MtjXY1i+/Oxyp5iwARAQABzTNUaG9tYXMgTGFt cHJlY2h0IChBdXRoLTQpIDx0LmxhbXByZWNodEBwcm94bW94LmNvbT7CwY4EEwEIADgWIQQO R4qbEl/pah9K6VrTZCM6gDZWBgUCWwuNxgIbAwULCQgHAgYVCAkKCwIEFgIDAQIeAQIXgAAK CRDTZCM6gDZWBm/jD/4+6JB2s67eaqoP6x9VGaXNGJPCscwzLuxDTCG90G9FYu29VcXtubH/ bPwsyBbNUQpqTm/s4XboU2qpS5ykCuTjqavrcP33tdkYfGcItj2xMipJ1i3TWvpikQVsX42R G64wovLs/dvpTYphRZkg5DwhgTmy3mRkmofFCTa+//MOcNOORltemp984tWjpR3bUJETNWpF sKGZHa3N4kCNxb7A+VMsJZ/1gN3jbQbQG7GkJtnHlWkw9rKCYqBtWrnrHa4UAvSa9M/XCIAB FThFGqZI1ojdVlv5gd6b/nWxfOPrLlSxbUo5FZ1i/ycj7/24nznW1V4ykG9iUld4uYUY86bB UGSjew1KYp9FmvKiwEoB+zxNnuEQfS7/Bj1X9nxizgweiHIyFsRqgogTvLh403QMSGNSoArk tqkorf1U+VhEncIn4H3KksJF0njZKfilrieOO7Vuot1xKr9QnYrZzJ7m7ZxJ/JfKGaRHXkE1 feMmrvZD1AtdUATZkoeQtTOpMu4r6IQRfSdwm/CkppZXfDe50DJxAMDWwfK2rr2bVkNg/yZI tKLBS0YgRTIynkvv0h8d9dIjiicw3RMeYXyqOnSWVva2r+tl+JBaenr8YTQw0zARrhC0mttu cIZGnVEvQuDwib57QLqMjQaC1gazKHvhA15H5MNxUhwm229UmdH3KM7BTQRbC43GARAAyTkR D6KRJ9Xa2fVMh+6f186q0M3ni+5tsaVhUiykxjsPgkuWXWW9MbLpYXkzX6h/RIEKlo2BGA95 QwG5+Ya2Bo3g7FGJHAkXY6loq7DgMp5/TVQ8phsSv3WxPTJLCBq6vNBamp5hda4cfXFUymsy HsJy4dtgkrPQ/bnsdFDCRUuhJHopnAzKHN8APXpKU6xV5e3GE4LwFsDhNHfH/m9+2yO/trcD txSFpyftbK2gaMERHgA8SKkzRhiwRTt9w5idOfpJVkYRsgvuSGZ0pcD4kLCOIFrer5xXudk6 NgJc36XkFRMnwqrL/bB4k6Pi2u5leyqcXSLyBgeHsZJxg6Lcr2LZ35+8RQGPOw9C0ItmRjtY ZpGKPlSxjxA1WHT2YlF9CEt3nx7c4C3thHHtqBra6BGPyW8rvtq4zRqZRLPmZ0kt/kiMPhTM 8wZAlObbATVrUMcZ/uNjRv2vU9O5aTAD9E5r1B0dlqKgxyoImUWB0JgpILADaT3VybDd3C8X s6Jt8MytUP+1cEWt9VKo4vY4Jh5vwrJUDLJvzpN+TsYCZPNVj18+jf9uGRaoK6W++DdMAr5l gQiwsNgf9372dbMI7pt2gnT5/YdG+ZHnIIlXC6OUonA1Ro/Itg90Q7iQySnKKkqqnWVc+qO9 GJbzcGykxD6EQtCSlurt3/5IXTA7t6sAEQEAAcLBdgQYAQgAIBYhBA5HipsSX+lqH0rpWtNk IzqANlYGBQJbC43GAhsMAAoJENNkIzqANlYGD1sP/ikKgHgcspEKqDED9gQrTBvipH85si0j /Jwu/tBtnYjLgKLh2cjv1JkgYYjb3DyZa1pLsIv6rGnPX9bH9IN03nqirC/Q1Y1lnbNTynPk IflgvsJjoTNZjgu1wUdQlBgL/JhUp1sIYID11jZphgzfDgp/E6ve/8xE2HMAnf4zAfJaKgD0 F+fL1DlcdYUditAiYEuN40Ns/abKs8I1MYx7Yglu3RzJfBzV4t86DAR+OvuF9v188WrFwXCS RSf4DmJ8tntyNej+DVGUnmKHupLQJO7uqCKB/1HLlMKc5G3GLoGqJliHjUHUAXNzinlpE2Vj C78pxpwxRNg2ilE3AhPoAXrY5qED5PLE9sLnmQ9AzRcMMJUXjTNEDxEYbF55SdGBHHOAcZtA kEQKub86e+GHA+Z8oXQSGeSGOkqHi7zfgW1UexddTvaRwE6AyZ6FxTApm8wq8NT2cryWPWTF BDSGB3ujWHMM8ERRYJPcBSjTvt0GcEqnd+OSGgxTkGOdufn51oz82zfpVo1t+J/FNz6MRMcg 8nEC+uKvgzH1nujxJ5pRCBOquFZaGn/p71Yr0oVitkttLKblFsqwa+10Lt6HBxm+2+VLp4Ja 0WZNncZciz3V3cuArpan/ZhhyiWYV5FD0pOXPCJIx7WS9PTtxiv0AOS4ScWEUmBxyhFeOpYa DrEx In-Reply-To: <20240301150315.12253-1-h.laimer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.055 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy 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 - Subject: Re: [pbs-devel] [PATCH proxmox-backup v2] datastore: remove datastore from internal cache based on maintenance mode 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: Mon, 04 Mar 2024 10:43:00 -0000 Am 01/03/2024 um 16:03 schrieb Hannes Laimer: > We keep a DataStore cache, so ChunkStore's and lock files are kept by > the proxy process and don't have to be reopened every time. However, for > specific maintenance modes, e.g. 'offline', our process should not keep > file in that datastore open. This clears the cache entry of a datastore > if it is in a specific maintanance mode and the last task finished, which > also drops any files still open by the process. One always asks themselves if command sockets are the right approach, but for this it seems alright. Some code style comments inline. > Signed-off-by: Hannes Laimer > Tested-by: Gabriel Goller > Reviewed-by: Gabriel Goller > --- > > v2, thanks @Gabriel: > - improve comments > - remove not needed &'s and .clone()'s > > pbs-api-types/src/maintenance.rs | 6 +++++ > pbs-datastore/src/datastore.rs | 41 ++++++++++++++++++++++++++++-- > pbs-datastore/src/task_tracking.rs | 23 ++++++++++------- > src/api2/config/datastore.rs | 18 +++++++++++++ > src/bin/proxmox-backup-proxy.rs | 8 ++++++ > 5 files changed, 85 insertions(+), 11 deletions(-) > > diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs > index 1b03ca94..a1564031 100644 > --- a/pbs-api-types/src/maintenance.rs > +++ b/pbs-api-types/src/maintenance.rs > @@ -77,6 +77,12 @@ pub struct MaintenanceMode { > } > > impl MaintenanceMode { > + /// Used for deciding whether the datastore is cleared from the internal cache after the last > + /// task finishes, so all open files are closed. > + pub fn clear_from_cache(&self) -> bool { that function name makes it sound like calling it does actively clears it, but this is only for checking if a required condition for clearing is met. So maybe use a name that better convey that and maybe even avoid coupling this to an action that a user of ours executes, as this might have some use for other call sites too. >From top of my head one could use `is_offline` as name, adding a note to the doc-comment that this is e.g. used to check if a datastore can be removed from the cache would still be fine though. > + self.ty == MaintenanceType::Offline > + } > + > pub fn check(&self, operation: Option) -> Result<(), Error> { > if self.ty == MaintenanceType::Delete { > bail!("datastore is being deleted"); > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs > index 2f0e5279..f26dff83 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -104,8 +104,27 @@ impl Clone for DataStore { > impl Drop for DataStore { > fn drop(&mut self) { > if let Some(operation) = self.operation { > - if let Err(e) = update_active_operations(self.name(), operation, -1) { > - log::error!("could not update active operations - {}", e); > + let mut last_task = false; > + match update_active_operations(self.name(), operation, -1) { > + Err(e) => log::error!("could not update active operations - {}", e), > + Ok(updated_operations) => { > + last_task = updated_operations.read + updated_operations.write == 0; > + } > + } > + > + // remove datastore from cache iff > + // - last task finished, and > + // - datastore is in a maintenance mode that mandates it > + let remove_from_cache = last_task > + && pbs_config::datastore::config() > + .and_then(|(s, _)| s.lookup::("datastore", self.name())) > + .map_or(false, |c| { > + c.get_maintenance_mode() > + .map_or(false, |m| m.clear_from_cache()) > + }); > + > + if remove_from_cache { > + DATASTORE_MAP.lock().unwrap().remove(self.name()); > } > } > } > @@ -193,6 +212,24 @@ impl DataStore { > Ok(()) > } > > + /// trigger clearing cache entries based on maintenance mode. Entries will only > + /// be cleared iff there is no other task running, if there is, the end of the > + /// last running task will trigger the clearing of the cache entry. > + pub fn update_datastore_cache() -> Result<(), Error> { why does this work on all but not a single datastore, after all we always want to remove a specific one? > + let (config, _digest) = pbs_config::datastore::config()?; > + for (store, (_, _)) in &config.sections { > + let datastore: DataStoreConfig = config.lookup("datastore", store)?; > + if datastore > + .get_maintenance_mode() > + .map_or(false, |m| m.clear_from_cache()) > + { > + let _ = DataStore::lookup_datastore(store, Some(Operation::Lookup)); A comment that the actual removal from the cache happens through the drop handler would be good, as this is a bit to subtle for my taste, if one stumbles over this in a few months down the line it might cause a bit to much easily to avoid head scratching... Alternatively, factor the actual check-maintenance-mode-and-remove-from-cache out of the drop handler and call that explicit here, all you need of outside info is the name there anyway.