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

[Robert Obkircher <r.obkircher@proxmox.com>]

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.

>>> [...]
>>> ---
>>> 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.

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?


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

> [..]