Re: [PATCH proxmox-backup 3/4] datastore: move try_ensure_sync_level() to DataStoreImpl

[Christian Ebner <c.ebner@proxmox.com>]

On 5/11/26 11:21 AM, Robert Obkircher wrote:
> 
> On 08.05.26 14:28, Christian Ebner wrote:
>> Keep the public interface in place, but move the method to
>> DataStoreImpl so it can be reused from the inner context as well.
>>
>> This is in preparation for fixing the sync level updates not being
>> propagated to the chunk store.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   pbs-datastore/src/datastore.rs | 24 ++++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index bc218cb94..8f69dd7ac 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -234,6 +234,19 @@ impl DataStoreImpl {
>>               thresholds_exceeded_callback: None,
>>           })
>>       }
>> +
>> +    fn try_ensure_sync_level(&self) -> Result<(), Error> {
>> +        if self.sync_level != DatastoreFSyncLevel::Filesystem {
> If the level in the chunk store changes from File to Filesystem this
> still wouldn't sync the chunks that have been written afterwards.
> 
> It may be better to just move the entire method into the chunk store.

Good catch! Agreed that this should be fixed by moving the 
implementation to the chunk store.

>> +            return Ok(());
>> +        }
>> +        let file = std::fs::File::open(self.chunk_store.base_path())?;
>> +        let fd = file.as_raw_fd();
>> +        log::info!("syncing filesystem");
>> +        if unsafe { libc::syncfs(fd) } < 0 {
>> +            bail!("error during syncfs: {}", std::io::Error::last_os_error());
>> +        }
>> +        Ok(())
>> +    }
>>   }
>>   
>>   pub type ThresholdsExceededCallback = fn(&str, &str, u64, u64) -> Result<(), Error>;
>> @@ -3015,16 +3028,7 @@ impl DataStore {
>>       /// Syncs the filesystem of the datastore if 'sync_level' is set to
>>       /// [`DatastoreFSyncLevel::Filesystem`]. Uses syncfs(2).
>>       pub fn try_ensure_sync_level(&self) -> Result<(), Error> {
>> -        if self.inner.sync_level != DatastoreFSyncLevel::Filesystem {
>> -            return Ok(());
>> -        }
>> -        let file = std::fs::File::open(self.base_path())?;
>> -        let fd = file.as_raw_fd();
>> -        log::info!("syncing filesystem");
>> -        if unsafe { libc::syncfs(fd) } < 0 {
>> -            bail!("error during syncfs: {}", std::io::Error::last_os_error());
>> -        }
>> -        Ok(())
>> +        self.inner.try_ensure_sync_level()
>>       }
>>   
>>       /// Destroy a datastore. This requires that there are no active operations on the datastore.