[pbs-devel] [PATCH proxmox-backup v2] datastore: remove datastore from internal cache based on maintenance mode
Thomas Lamprecht
t.lamprecht at proxmox.com
Mon Mar 4 12:27:08 CET 2024
Am 04/03/2024 um 12:12 schrieb Hannes Laimer:
> On Mon Mar 4, 2024 at 11:42 AM CET, Thomas Lamprecht wrote:
>> Am 01/03/2024 um 16:03 schrieb Hannes Laimer:
>>> @@ -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.
>>
>
> I agree, the name is somewhat misleading. The idea was to make it easy
> to potentially add more modes here in the future, so maybe something
> a little more general like `is_accessible` would make sense?
is_accessible is the negative of is_offline, can be fine, but if you're
explicitly check for != offline it could be better to avoid the double
negative for now, and whenever this is extended see if there's a better
fitting name for the existing and new uses cases.
in short, can be fine, but personally I'd prefer is_offline for this
use case.
>>> + self.ty == MaintenanceType::Offline
>>> + }
>>> +
>>> pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
>>> if self.ty == MaintenanceType::Delete {
>>> bail!("datastore is being deleted");
>>> + 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.
>
> I think that would entail having to open the file twice in the drop
> handler, once for updating it, and once for reading it. But just
> reading it here and explicitly clearing it from the cache seems
> reasonable, it makes it way clrearer what's happening. I'll change that
> in a v3.
Yeah, but not a must IMO.
You can also just add a comment here about reusing the drop handler for
its cache-dropping handling, it just should be conveyed rather explicit.
More information about the pbs-devel
mailing list