[pbs-devel] [RFC v2 proxmox-backup 12/21] datastore: clear trashed snapshot dir if re-creation requested

[Christian Ebner]

On 5/9/25 14:27, Fabian Grünbichler wrote:
> On May 8, 2025 3:05 pm, Christian Ebner wrote:
>> If a previously trashed snapshot has been requested for re-creation
>> (e.g. by a sync job in push direction), drop the contents of the
>> currently trashed snapshot.
>> The snapshot directory itself is already locked at that point, either
>> by the old locking mechanism acting on the directory itself or by the
>> new locking mechanism. Therefore, concurrent operations can be
>> excluded.
>>
>> For the call site this acts as if the snapshot directory has been
>> newly created.
>>
>> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
>> ---
>>   pbs-datastore/src/datastore.rs | 29 ++++++++++++++++++++++++++++-
>>   1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index ffc6a7039..4f7766c36 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -951,8 +951,9 @@ impl DataStore {
>>       ) -> Result<(PathBuf, bool, BackupLockGuard), Error> {
>>           let backup_dir = self.backup_dir(ns.clone(), backup_dir.clone())?;
>>           let relative_path = backup_dir.relative_path();
>> +        let full_path = backup_dir.full_path();
>>   
>> -        match std::fs::create_dir(backup_dir.full_path()) {
>> +        match std::fs::create_dir(&full_path) {
>>               Ok(_) => {
>>                   let guard = backup_dir.lock().with_context(|| {
>>                       format!("while creating new locked snapshot '{backup_dir:?}'")
>> @@ -963,6 +964,32 @@ impl DataStore {
>>                   let guard = backup_dir
>>                       .lock()
>>                       .with_context(|| format!("while creating locked snapshot '{backup_dir:?}'"))?;
>> +
>> +                if backup_dir.is_trashed() {
>> +                    info!("clear trashed backup snapshot {full_path:?}");
>> +                    let dir_entries = std::fs::read_dir(&full_path).context(
>> +                        "failed to read directory contents during cleanup of trashed snapshot",
>> +                    )?;
>> +                    for entry in dir_entries {
>> +                        let entry = entry.context(
>> +                            "failed to read directory entry during clenup of trashed snapshot",
>> +                        )?;
>> +                        // Only expect regular file entries
>> +                        std::fs::remove_file(entry.path()).context(
>> +                            "failed to remove directory entry during clenup of trashed snapshot",
>> +                        )?;
>> +                    }
>> +                    let group = BackupGroup::from(backup_dir);
>> +                    let group_trash_file = group.full_group_path().join(TRASH_MARKER_FILENAME);
>> +                    if let Err(err) = std::fs::remove_file(&group_trash_file) {
>> +                        if err.kind() != std::io::ErrorKind::NotFound {
>> +                            bail!("failed to remove group trash file of trashed snapshot");
>> +                        }
>> +                    }
> 
> this shouldn't be possible to hit, right? as creating a backup dir
> entails first creating the backup group (guarded by the group lock), and
> that would already clear the group's trash marker..

Yes, you are right: The whole group and namespace un-trashing logic is 
already performed by `create_locked_backup_group` and redundant at this 
point. So I will drop this and add a comment mentioning this fact instead.