Re: [pbs-devel] [PATCH proxmox-backup v2] datastore: remove datastore from internal cache based on maintenance mode

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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.