[pbs-devel] [PATCH proxmox-backup v2 0/3] fix GC atime update race window

[pbs-devel] [PATCH proxmox-backup v2 0/3] fix GC atime update race window

[Christian Ebner]

Sweeping of unused chunks during garbage collection checks their
atime to distinguish between chunks being in-use and chunks no
longer being used. While garbage collection does lock the chunk
store by guarding its mutex before reading file stats and deleting
unused chunks, the conditional touch did not do this before updating
the chunks atime (thereby also checking the presence).

Therefore there is a race window between the chunks metadata being
read and the chunk being removed, but the chunk being touched
in-between.

The race is however rare, as for this to happen the chunk must be
older than the cutoff time and not be referenced by any index file,
otherwise the atime would be updated during phase 1 already.

Fix by guarding the chunk store mutex before touching a chunk.

Lastly, also make sure that marker chunk inserts and atime updates
on bad chunks are performed in a locked context as well.

Changes since version 1 (thanks @Fabian for swiftly seeing the issues):
- Limit helpers scope for better encapsulation
- Make sure internal helpers do not try to lock the chunk store again
- Assure the chunk store is locked for s3 local store cache marker file
  insertion and atime updates on bad chunks.

Christian Ebner (3):
  chunk store: limit scope for atime update helper methods
  chunk store: fix race window between chunk stat and gc cleanup
  datastore: insert chunk marker and touch bad chunks in locked context

 pbs-datastore/src/chunk_store.rs | 48 +++++++++++++++++++++++++++-----
 pbs-datastore/src/datastore.rs   | 42 +++++++++++++++++-----------
 2 files changed, 66 insertions(+), 24 deletions(-)

-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v2 1/3] chunk store: limit scope for atime update helper methods

[Christian Ebner]

In preparation for fixing a race window due to missing chunk store
locking. These should never be called from outside the crate.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- not present in previous version

 pbs-datastore/src/chunk_store.rs | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index ba7618e40..1262377d5 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -204,7 +204,7 @@ impl ChunkStore {
         })
     }
 
-    pub fn touch_chunk(&self, digest: &[u8; 32]) -> Result<(), Error> {
+    fn touch_chunk(&self, digest: &[u8; 32]) -> Result<(), Error> {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
 
@@ -212,7 +212,11 @@ impl ChunkStore {
         Ok(())
     }
 
-    pub fn cond_touch_chunk(&self, digest: &[u8; 32], assert_exists: bool) -> Result<bool, Error> {
+    pub(super) fn cond_touch_chunk(
+        &self,
+        digest: &[u8; 32],
+        assert_exists: bool,
+    ) -> Result<bool, Error> {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
 
@@ -220,7 +224,7 @@ impl ChunkStore {
         self.cond_touch_path(&chunk_path, assert_exists)
     }
 
-    pub fn cond_touch_path(&self, path: &Path, assert_exists: bool) -> Result<bool, Error> {
+    pub(super) fn cond_touch_path(&self, path: &Path, assert_exists: bool) -> Result<bool, Error> {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
 
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v2 2/3] chunk store: fix race window between chunk stat and gc cleanup

[Christian Ebner]

Sweeping of unused chunks during garbage collection checks their
atime to distinguish between chunks being in-use and chunks no
longer being used. While garbage collection does lock the chunk
store by guarding its mutex before reading file stats and deleting
unused chunks, the conditional touch did not do this before updating
the chunks atime (thereby also checking the presence).

Therefore there is a race window between the chunks metadata being
read and the chunk being removed, but the chunk being touched
in-between.

The race is however rare, as for this to happen the chunk must be
older than the cutoff time and not be referenced by any index file,
otherwise the atime would be updated during phase 1 already.

Fix by guarding the chunk store mutex before touching a chunk.

To achieve this, rename and splitoff internal touch chunk helpers to
reflect that the internal helpers do not acquire the chunk store
lock, while the one exposed to be accessed from outside the chunk
store module does.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- make sure internal helpers already holding the mutex guard try to lock
  it again

 pbs-datastore/src/chunk_store.rs | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 1262377d5..b88a0a096 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -204,18 +204,31 @@ impl ChunkStore {
         })
     }
 
-    fn touch_chunk(&self, digest: &[u8; 32]) -> Result<(), Error> {
+    fn touch_chunk_no_lock(&self, digest: &[u8; 32]) -> Result<(), Error> {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
 
-        self.cond_touch_chunk(digest, true)?;
+        self.cond_touch_chunk_no_lock(digest, true)?;
         Ok(())
     }
 
+    /// Update the chunk files atime if it exists.
+    ///
+    /// If the chunk file does not exist, return with error if assert_exists is true, with
+    /// Ok(false) otherwise.
     pub(super) fn cond_touch_chunk(
         &self,
         digest: &[u8; 32],
         assert_exists: bool,
+    ) -> Result<bool, Error> {
+        let _lock = self.mutex.lock();
+        self.cond_touch_chunk_no_lock(digest, assert_exists)
+    }
+
+    fn cond_touch_chunk_no_lock(
+        &self,
+        digest: &[u8; 32],
+        assert_exists: bool,
     ) -> Result<bool, Error> {
         // unwrap: only `None` in unit tests
         assert!(self.locker.is_some());
@@ -587,7 +600,7 @@ impl ChunkStore {
             }
             let old_size = metadata.len();
             if encoded_size == old_size {
-                self.touch_chunk(digest)?;
+                self.touch_chunk_no_lock(digest)?;
                 return Ok((true, old_size));
             } else if old_size == 0 {
                 log::warn!("found empty chunk '{digest_str}' in store {name}, overwriting");
@@ -612,11 +625,11 @@ impl ChunkStore {
                 // compressed, the size mismatch could be caused by different zstd versions
                 // so let's keep the one that was uploaded first, bit-rot is hopefully detected by
                 // verification at some point..
-                self.touch_chunk(digest)?;
+                self.touch_chunk_no_lock(digest)?;
                 return Ok((true, old_size));
             } else if old_size < encoded_size {
                 log::debug!("Got another copy of chunk with digest '{digest_str}', existing chunk is smaller, discarding uploaded one.");
-                self.touch_chunk(digest)?;
+                self.touch_chunk_no_lock(digest)?;
                 return Ok((true, old_size));
             } else {
                 log::debug!("Got another copy of chunk with digest '{digest_str}', existing chunk is bigger, replacing with uploaded one.");
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v2 3/3] datastore: insert chunk marker and touch bad chunks in locked context

[Christian Ebner]

Assures that both, the touching of bad chunks as well as the
insertion of missing chunk marker files are done while the chunk
store mutex is guarded, other operations therefore get a consistent
state.

To achieve this, introduces a helper method which allows to run a
callback in a locked context if the chunk file is missing.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 1:
- not present in previous version

 pbs-datastore/src/chunk_store.rs | 17 +++++++++++++
 pbs-datastore/src/datastore.rs   | 42 +++++++++++++++++++-------------
 2 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index b88a0a096..063bc55f6 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -212,6 +212,23 @@ impl ChunkStore {
         Ok(())
     }
 
+    /// Update the chunk files atime if it exists, call the provided callback inside a chunk store
+    /// locked scope otherwise.
+    pub(super) fn cond_touch_chunk_or_locked<T>(
+        &self,
+        digest: &[u8; 32],
+        callback: T,
+    ) -> Result<(), Error>
+    where
+        T: FnOnce() -> Result<(), Error>,
+    {
+        let _lock = self.mutex.lock();
+        if !self.cond_touch_chunk_no_lock(digest, false)? {
+            callback()?;
+        }
+        Ok(())
+    }
+
     /// Update the chunk files atime if it exists.
     ///
     /// If the chunk file does not exist, return with error if assert_exists is true, with
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 4527b40f4..b2f414ce1 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1302,15 +1302,16 @@ impl DataStore {
             match s3_client {
                 None => {
                     // Filesystem backend
-                    if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
+                    self.inner.chunk_store.cond_touch_chunk_or_locked(digest, || {
                         let hex = hex::encode(digest);
                         warn!(
                             "warning: unable to access non-existent chunk {hex}, required by {file_name:?}"
                         );
 
-                        // touch any corresponding .bad files to keep them around, meaning if a chunk is
-                        // rewritten correctly they will be removed automatically, as well as if no index
-                        // file requires the chunk anymore (won't get to this loop then)
+                        // touch any corresponding .bad files to keep them around, meaning if a
+                        // chunk is rewritten correctly they will be removed automatically, as well
+                        // as if no index file requires the chunk anymore (won't get to this loop
+                        // then)
                         for i in 0..=9 {
                             let bad_ext = format!("{i}.bad");
                             let mut bad_path = PathBuf::new();
@@ -1318,22 +1319,29 @@ impl DataStore {
                             bad_path.set_extension(bad_ext);
                             self.inner.chunk_store.cond_touch_path(&bad_path, false)?;
                         }
-                    }
+                        Ok(())
+                    })?;
                 }
                 Some(ref _s3_client) => {
                     // Update atime on local cache marker files.
-                    if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
-                        let (chunk_path, _digest) = self.chunk_path(digest);
-                        // Insert empty file as marker to tell GC phase2 that this is
-                        // a chunk still in-use, so to keep in the S3 object store.
-                        std::fs::File::options()
-                            .write(true)
-                            .create_new(true)
-                            .open(&chunk_path)
-                            .with_context(|| {
-                                format!("failed to create marker for chunk {}", hex::encode(digest))
-                            })?;
-                    }
+                    self.inner
+                        .chunk_store
+                        .cond_touch_chunk_or_locked(digest, || {
+                            let (chunk_path, _digest) = self.chunk_path(digest);
+                            // Insert empty file as marker to tell GC phase2 that this is
+                            // a chunk still in-use, so to keep in the S3 object store.
+                            std::fs::File::options()
+                                .write(true)
+                                .create_new(true)
+                                .open(&chunk_path)
+                                .with_context(|| {
+                                    format!(
+                                        "failed to create marker for chunk {}",
+                                        hex::encode(digest)
+                                    )
+                                })?;
+                            Ok(())
+                        })?;
                 }
             }
         }
-- 
2.47.3



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] partially-applied: [PATCH proxmox-backup v2 0/3] fix GC atime update race window

[Thomas Lamprecht]

On Thu, 06 Nov 2025 18:13:55 +0100, Christian Ebner wrote:
> Sweeping of unused chunks during garbage collection checks their
> atime to distinguish between chunks being in-use and chunks no
> longer being used. While garbage collection does lock the chunk
> store by guarding its mutex before reading file stats and deleting
> unused chunks, the conditional touch did not do this before updating
> the chunks atime (thereby also checking the presence).
> 
> [...]

Applied the first two patches, thanks!

For the third one I would rather like an actual review from Fabian, especially
w.r.t. adding the callback.

[1/3] chunk store: limit scope for atime update helper methods
      commit: 5799bcda53220c78ff45a44e5b58582a0fac2e83
[2/3] chunk store: fix race window between chunk stat and gc cleanup
      commit: d45077366778c20293901b5e3ef36725cf123f88
[3/3] datastore: insert chunk marker and touch bad chunks in locked context
      SKIPPED


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v2 3/3] datastore: insert chunk marker and touch bad chunks in locked context

[Fabian Grünbichler]

On November 6, 2025 6:13 pm, Christian Ebner wrote:
> Assures that both, the touching of bad chunks as well as the
> insertion of missing chunk marker files are done while the chunk
> store mutex is guarded, other operations therefore get a consistent
> state.
> 
> To achieve this, introduces a helper method which allows to run a
> callback in a locked context if the chunk file is missing.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 1:
> - not present in previous version
> 
>  pbs-datastore/src/chunk_store.rs | 17 +++++++++++++
>  pbs-datastore/src/datastore.rs   | 42 +++++++++++++++++++-------------
>  2 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
> index b88a0a096..063bc55f6 100644
> --- a/pbs-datastore/src/chunk_store.rs
> +++ b/pbs-datastore/src/chunk_store.rs
> @@ -212,6 +212,23 @@ impl ChunkStore {
>          Ok(())
>      }
>  
> +    /// Update the chunk files atime if it exists, call the provided callback inside a chunk store
> +    /// locked scope otherwise.
> +    pub(super) fn cond_touch_chunk_or_locked<T>(
> +        &self,
> +        digest: &[u8; 32],
> +        callback: T,
> +    ) -> Result<(), Error>
> +    where
> +        T: FnOnce() -> Result<(), Error>,
> +    {
> +        let _lock = self.mutex.lock();
> +        if !self.cond_touch_chunk_no_lock(digest, false)? {
> +            callback()?;
> +        }
> +        Ok(())
> +    }
> +
>      /// Update the chunk files atime if it exists.
>      ///
>      /// If the chunk file does not exist, return with error if assert_exists is true, with
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 4527b40f4..b2f414ce1 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1302,15 +1302,16 @@ impl DataStore {
>              match s3_client {
>                  None => {
>                      // Filesystem backend
> -                    if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
> +                    self.inner.chunk_store.cond_touch_chunk_or_locked(digest, || {
>                          let hex = hex::encode(digest);
>                          warn!(
>                              "warning: unable to access non-existent chunk {hex}, required by {file_name:?}"
>                          );
>  
> -                        // touch any corresponding .bad files to keep them around, meaning if a chunk is
> -                        // rewritten correctly they will be removed automatically, as well as if no index
> -                        // file requires the chunk anymore (won't get to this loop then)
> +                        // touch any corresponding .bad files to keep them around, meaning if a
> +                        // chunk is rewritten correctly they will be removed automatically, as well
> +                        // as if no index file requires the chunk anymore (won't get to this loop
> +                        // then)
>                          for i in 0..=9 {
>                              let bad_ext = format!("{i}.bad");
>                              let mut bad_path = PathBuf::new();
> @@ -1318,22 +1319,29 @@ impl DataStore {
>                              bad_path.set_extension(bad_ext);
>                              self.inner.chunk_store.cond_touch_path(&bad_path, false)?;
>                          }
> -                    }
> +                        Ok(())
> +                    })?;

do we need to hold the mutex for touching the bad chunks? we don't when
creating them during verification (should we?), we do when removing them
during GC..

if we do, then we should probably have a 

pub(super) fn cond_touch_bad_chunk(s?) in the chunk store

because each touch here is independent, and there is no overarching need
to hold the mutex across all of them, and this would allow us to make
cond_touch_path private..

should we also touch bad chunks for the S3 case?

>                  }
>                  Some(ref _s3_client) => {
>                      // Update atime on local cache marker files.
> -                    if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
> -                        let (chunk_path, _digest) = self.chunk_path(digest);
> -                        // Insert empty file as marker to tell GC phase2 that this is
> -                        // a chunk still in-use, so to keep in the S3 object store.
> -                        std::fs::File::options()
> -                            .write(true)
> -                            .create_new(true)
> -                            .open(&chunk_path)
> -                            .with_context(|| {
> -                                format!("failed to create marker for chunk {}", hex::encode(digest))
> -                            })?;
> -                    }
> +                    self.inner
> +                        .chunk_store
> +                        .cond_touch_chunk_or_locked(digest, || {
> +                            let (chunk_path, _digest) = self.chunk_path(digest);
> +                            // Insert empty file as marker to tell GC phase2 that this is
> +                            // a chunk still in-use, so to keep in the S3 object store.
> +                            std::fs::File::options()
> +                                .write(true)
> +                                .create_new(true)
> +                                .open(&chunk_path)
> +                                .with_context(|| {
> +                                    format!(
> +                                        "failed to create marker for chunk {}",
> +                                        hex::encode(digest)
> +                                    )
> +                                })?;
> +                            Ok(())
> +                        })?;

AFAICT, we can fix this together with the other S3-races by obtaining
the flock on the chunk here?

i.e.,

// without flock first, since a chunk missing is the unlikely path
// (corruption detected by verify, or manual damage to the chunk store)
if !cond_touch_chunk {
  // now with flock, to protect the next two calls against concurrent
  // insert+uploads/renames/..
  flock {
    // somebody else could have inserted it since we checked without
    // locking above
    if !cond_touch_chunk {
      // insert empty marker only if chunk is not there
      store.clear_chunk
    }
  }
}

if (not-)caching were a property of the datastore, instead of the backup
session, we could even just fetch the missing chunk into the cache here
if caching is enabled

>                  }
>              }
>          }
> -- 
> 2.47.3
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v2 3/3] datastore: insert chunk marker and touch bad chunks in locked context

[Christian Ebner]

On 11/10/25 9:31 AM, Fabian Grünbichler wrote:
> On November 6, 2025 6:13 pm, Christian Ebner wrote:
>> Assures that both, the touching of bad chunks as well as the
>> insertion of missing chunk marker files are done while the chunk
>> store mutex is guarded, other operations therefore get a consistent
>> state.
>>
>> To achieve this, introduces a helper method which allows to run a
>> callback in a locked context if the chunk file is missing.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> changes since version 1:
>> - not present in previous version
>>
>>   pbs-datastore/src/chunk_store.rs | 17 +++++++++++++
>>   pbs-datastore/src/datastore.rs   | 42 +++++++++++++++++++-------------
>>   2 files changed, 42 insertions(+), 17 deletions(-)
>>
>> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
>> index b88a0a096..063bc55f6 100644
>> --- a/pbs-datastore/src/chunk_store.rs
>> +++ b/pbs-datastore/src/chunk_store.rs
>> @@ -212,6 +212,23 @@ impl ChunkStore {
>>           Ok(())
>>       }
>>   
>> +    /// Update the chunk files atime if it exists, call the provided callback inside a chunk store
>> +    /// locked scope otherwise.
>> +    pub(super) fn cond_touch_chunk_or_locked<T>(
>> +        &self,
>> +        digest: &[u8; 32],
>> +        callback: T,
>> +    ) -> Result<(), Error>
>> +    where
>> +        T: FnOnce() -> Result<(), Error>,
>> +    {
>> +        let _lock = self.mutex.lock();
>> +        if !self.cond_touch_chunk_no_lock(digest, false)? {
>> +            callback()?;
>> +        }
>> +        Ok(())
>> +    }
>> +
>>       /// Update the chunk files atime if it exists.
>>       ///
>>       /// If the chunk file does not exist, return with error if assert_exists is true, with
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 4527b40f4..b2f414ce1 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -1302,15 +1302,16 @@ impl DataStore {
>>               match s3_client {
>>                   None => {
>>                       // Filesystem backend
>> -                    if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
>> +                    self.inner.chunk_store.cond_touch_chunk_or_locked(digest, || {
>>                           let hex = hex::encode(digest);
>>                           warn!(
>>                               "warning: unable to access non-existent chunk {hex}, required by {file_name:?}"
>>                           );
>>   
>> -                        // touch any corresponding .bad files to keep them around, meaning if a chunk is
>> -                        // rewritten correctly they will be removed automatically, as well as if no index
>> -                        // file requires the chunk anymore (won't get to this loop then)
>> +                        // touch any corresponding .bad files to keep them around, meaning if a
>> +                        // chunk is rewritten correctly they will be removed automatically, as well
>> +                        // as if no index file requires the chunk anymore (won't get to this loop
>> +                        // then)
>>                           for i in 0..=9 {
>>                               let bad_ext = format!("{i}.bad");
>>                               let mut bad_path = PathBuf::new();
>> @@ -1318,22 +1319,29 @@ impl DataStore {
>>                               bad_path.set_extension(bad_ext);
>>                               self.inner.chunk_store.cond_touch_path(&bad_path, false)?;
>>                           }
>> -                    }
>> +                        Ok(())
>> +                    })?;
> 
> do we need to hold the mutex for touching the bad chunks? we don't when
> creating them during verification (should we?), we do when removing them
> during GC..
> 
> if we do, then we should probably have a
> 
> pub(super) fn cond_touch_bad_chunk(s?) in the chunk store
> 
> because each touch here is independent, and there is no overarching need
> to hold the mutex across all of them, and this would allow us to make
> cond_touch_path private..

No, these can indeed be independent since cleanup in GC only happens if 
the good chunk is not present, and the rename will then simply use the 
next free bad filename slot.

> should we also touch bad chunks for the S3 case?

True, so they need to be present to reflect the state for these as well. 
Otherwise they will get incorrectly cleaned up in phase 2.
  >>                   }
>>                   Some(ref _s3_client) => {
>>                       // Update atime on local cache marker files.
>> -                    if !self.inner.chunk_store.cond_touch_chunk(digest, false)? {
>> -                        let (chunk_path, _digest) = self.chunk_path(digest);
>> -                        // Insert empty file as marker to tell GC phase2 that this is
>> -                        // a chunk still in-use, so to keep in the S3 object store.
>> -                        std::fs::File::options()
>> -                            .write(true)
>> -                            .create_new(true)
>> -                            .open(&chunk_path)
>> -                            .with_context(|| {
>> -                                format!("failed to create marker for chunk {}", hex::encode(digest))
>> -                            })?;
>> -                    }
>> +                    self.inner
>> +                        .chunk_store
>> +                        .cond_touch_chunk_or_locked(digest, || {
>> +                            let (chunk_path, _digest) = self.chunk_path(digest);
>> +                            // Insert empty file as marker to tell GC phase2 that this is
>> +                            // a chunk still in-use, so to keep in the S3 object store.
>> +                            std::fs::File::options()
>> +                                .write(true)
>> +                                .create_new(true)
>> +                                .open(&chunk_path)
>> +                                .with_context(|| {
>> +                                    format!(
>> +                                        "failed to create marker for chunk {}",
>> +                                        hex::encode(digest)
>> +                                    )
>> +                                })?;
>> +                            Ok(())
>> +                        })?;
> 
> AFAICT, we can fix this together with the other S3-races by obtaining
> the flock on the chunk here?
> 
> i.e.,
> 
> // without flock first, since a chunk missing is the unlikely path
> // (corruption detected by verify, or manual damage to the chunk store)
> if !cond_touch_chunk {
>    // now with flock, to protect the next two calls against concurrent
>    // insert+uploads/renames/..
>    flock {
>      // somebody else could have inserted it since we checked without
>      // locking above
>      if !cond_touch_chunk {
>        // insert empty marker only if chunk is not there
>        store.clear_chunk
>      }
>    }
> }


Okay, will do by moving the s3 specific logic into the regular one, 
which covers then both and send a v2, now however as followup patches to 
be applied on top of [0] since that is required for the per-chunk file 
locking.

[0] 
https://lore.proxmox.com/pbs-devel/20251110115627.280318-1-c.ebner@proxmox.com/T/


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel