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

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

On 06.06.26 10:41, Christian Ebner wrote:
> On 6/5/26 6:20 PM, Robert Obkircher wrote:
>> [..]
>>
>> 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. 
GC might not reach phase 3 once you are out of memory.
>>
>>>>> [...]
>>>>> ---
>>>>> 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? 
Yeah, active operations probably wouldn't work.
>
>> 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. 
Right, I somehow thought they use local locks, but that is not the case.
>
>> 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/ 
Thank you for the links.