From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 5041E1FF142 for ; Fri, 05 Jun 2026 18:21:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B74B231A75; Fri, 5 Jun 2026 18:21:52 +0200 (CEST) Message-ID: <3863ce57-42b9-4e43-a6ab-30e297732054@proxmox.com> Date: Fri, 5 Jun 2026 18:21:18 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH proxmox-backup] fix #7670: datastore: s3: allow for per-chunk file lock cleanup To: Christian Ebner , pbs-devel@lists.proxmox.com References: <20260604140919.97686-1-c.ebner@proxmox.com> <706c6feb-8f7c-4f07-af09-c0df91f95469@proxmox.com> Content-Language: en-US, de-AT From: Robert Obkircher In-Reply-To: <706c6feb-8f7c-4f07-af09-c0df91f95469@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1780676439301 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: BZFEDKCQ32RVN6ZF5G2A5XTXALUWSPJR X-Message-ID-Hash: BZFEDKCQ32RVN6ZF5G2A5XTXALUWSPJR X-MailFrom: r.obkircher@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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? > [..]