Re: [RFC PATCH proxmox-backup] fix #7670: datastore: s3: allow for per-chunk file lock cleanup

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

On 6/5/26 6:20 PM, Robert Obkircher wrote:
> 
> On 05.06.26 14:31, Christian Ebner wrote:
>> On 6/5/26 2:11 PM, Robert Obkircher wrote:
>>>
>>> On 04.06.26 16:09, Christian Ebner wrote:
>>>> Per-chunk file locks are located on a tmpfs but never cleaned up to
>>>> avoid TOCTOU race conditions. Therefore lock files can accumulate
>>>> over time, the memory required to store the inodes finally lead to
>>>> OOM conditions if the system is not rebooted for a long time or a
>>>> high number of different chunks is written to the s3 backed
>>>> datastore.
>>>>
>>>> To fix this, use the double stating strategy already implemented by
>>>> pbs_datastore::backup_info::lock_helper(), but adapt it so that the
>>>> lock file is cleaned up before unlocking. Since after file removal
>>>> the lock can be acquired by a different thread/process, the file lock
>>>> must also be dropped immediately without performing any other
>>>> critical operation. To assure this, a ChunkLockGuard is implemented
>>>> which removes the file and drops the file descriptor by implementing
>>>> the Drop trait.
>>> One potential problem is that drop is not guaranteed to run if the
>>> process aborts, so this could still slowly leak files over time.
>>
>> Good catch, maybe such files could be cleaned up by spawning a
>> cleanup task triggered by chunk store instantiation. It could then
>> iterate all the lock direntries and perform the same lock -> cleanup
>> lock logic but without timeout, removing all lockfiles not currently
>> being held at the cost of some IO for that, unfortunately. OTOH the
>> number of lockfiles should be not to dramatic for that case, so IO
>> should not be that dramatic and could be controlled by delays.
>>
>> Other ideas?
>> er ideas?
> 
> With a regular cleanup task we could also get rid of the unlink on
> drop, which would probably make repeated lock acquisition a bit cheaper.
> 
> But I'm not sure if it's worth the complexity.

Cleanup could be done as part of GC phase 3 on s3 backed datastores 
though, by that it should not introduce to much complexity. But probably 
not the best way forwards, agreed.

> 
>>>> [...]
>>>> ---
>>>> Sending this as RFC in case there are ideas for a different solution
>>>> to the problem at hand which might be preferable.
> 
> Besides shared memory or a coordinator process, we could also just store a list of locked files in the active operations file.

I do not think that will scale very well neither? Not performance wise 
nor usage wise, after all this would need to keep the list for all 
active operations on each chunk file up-to-date?

> Flock on the actual chunk file should work as well. We only need to coordinate on the local machine, so flock should be fine even if it is a remote file system, no?

There are some caveats with that IIRC, especially with respect to 
latency and proper support from e.g. older NFS implementations. Further, 
the existence of a chunk file is currently used as marker for the chunk 
to being present on the S3 backend (at least one successful and 
acknowledge upload). For this to work that would need to be reworked as 
well and is probably not backwards compatible.

> Do we actually need the locks at all? Wouldn't it be sufficient if we
> prevent duplicate S3 uploads on a per-process basis?

The locking must protect against TOCTOU races, not only with upload but 
also chunk insertion in the local cache, GC, renaming of bad chunks for 
failed verifies, ...

The initial implementation did not perform the locking but relied on 
atomic operations for local marker files only, causing quite some churn 
due to races and resulting in the current implementation [1,2].

[1] 
https://lore.proxmox.com/pbs-devel/20251114131901.441650-1-c.ebner@proxmox.com/
[2] 
https://lore.proxmox.com/pbs-devel/20251120100523.12147-1-c.ebner@proxmox.com/