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 5:06 PM, Christian Ebner wrote:
> On 6/5/26 3:39 PM, Fabian Grünbichler wrote:
>> Quoting Christian Ebner (2026-06-04 16:09:19)
>>> 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.
>>>
>>> After each flock() call, which is performed as part of
>>> proxmox_sys::fs::open_file_locked(), stating the file and comparing
>>> locked files inode from the open file handle to the one currently
>>> present on the filesystem is performed. By this possible races are
>>> detected, resulting in missing or newly create lock files. In that
>>> case locking must be retried.
>>>
>>> Note that this locking mechanism is not fair, the first caller might
>>> end up being the last to actually acquire the lock. This is however
>>> not problematic for the intended use case of per-chunk file locking,
>>> with limited lock contention.
>>>
>>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7670
>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>> ---
>>> Sending this as RFC in case there are ideas for a different solution
>>> to the problem at hand which might be preferable.
>>
>> to recap - when we originally introduced this, we checked for memory 
>> usage by
>> querying tmpfs file system usage. the 1kb per file overhead in slab 
>> was missed.
>>
>> one solution to this would be to move the locks back to a regular 
>> filesystem,
>> and either
>>
>> - clean them up when removing chunks (with corresponding retry logic
>>    similar to this patch, but less frequent?)
> 
>  From the two options for this solutions, this one sounds preferable. 
> Ideally the locking would be performed on the chunk file itself... But 
> that has issues with non-local filesystems AFAIR, so not an option 
> unfortunately.
> 
> But probably we want to go with moving to a filesystem backed lock-files 
> AND reducing the granularity as suggested below.
> 
>> - clean them up by dropping the lock dir on reboot (e.g., via
>>    systemd-tmpfiles.d), which would be the equivalent of the current 
>> tmpfs
>>    approach but trading disk usage vs. memory usage, and probably 
>> slightly slower
>>    lock allocation
>>
>> for ext4, 100k empty lock files take up 2.2M (roughly 2% of the tmpfs 
>> slab
>> usage), 500k take up 11.2M, for ZFS it's 56M for 100k (roughly half 
>> the slab
>> usage), though in both cases we of course would need a hierarchy of 
>> prefix dirs
>> again to keep access times okayish? for 23M chunk locks we'd still end 
>> up with
>> (estimated) 5G of usage on ext4..
>>
>> another approach would be to switch to a different kind of locking 
>> mechanism
>> entirely (though with the cross-process and multi-threaded 
>> requirements we
>> have, this might not be easy either ;)) or to reduce the lock 
>> granularity.
> 
> Yeah, reducing the lock granularity and moving to something with 
> deterministic count could be a way out, and given the usage estimates 
> from above, using the 4 hex-digit prefix used also for chunk store 
> directory hierarchy might be a viable candidate for defining such lock- 
> files.
>> given that a mistake in the handling of retries below can cause 
>> dataloss, doing
>> it for every lock/unlock pair sounds a bit dangerous. there's also the
>> additional overhead if the lock is actually contended to account for - 
>> we need
>> at least two loop iterations if it is, potentially a lot more?
> 
> This is however not so frequently happening though? But yes, there is 
> overhead by the then unavoidable retry/retries.
> 
>> or we go back to square one, and revisit the whole interaction here to 
>> see if
>> we can get rid of the per-chunk locks again in favor of something that 
>> scales
>> with the number of pending uploads.. the last time we tried this 
>> turned out to
>> be quite tricky though.
> The major downside for all solutions is unfortunately that moving 
> forward can only happen with the current locking mechanism still in 
> place, so a transitional step for cleanup unavoidable. But at least it 
> is fine to remove the per-chunk lock files once they are being held in 
> the process instance implementing the new locking logic.
> 
> Thanks for your inputs!

Oh, and another thing to consider as well: The backup snapshot 
lock-files are also not cleaned up AFAIKT? While way less problematic, 
they will also accumulate and add unneeded memory overhead on systems 
with high frequency backups which are not rebooted frequently.

There removing the lock-file on prune and retry for vanished lock-files 
after flock calls might be acceptable? Also performance wise.