From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 0BDCA1FF141 for ; Fri, 13 Feb 2026 14:37:03 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 008994131; Fri, 13 Feb 2026 14:37:50 +0100 (CET) Message-ID: Date: Fri, 13 Feb 2026 14:37:13 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1 2/6] s3-client: add persistent shared request counters for client To: Robert Obkircher , pbs-devel@lists.proxmox.com References: <20260209091533.156902-1-c.ebner@proxmox.com> <20260209091533.156902-3-c.ebner@proxmox.com> <2a88c886-9146-4448-9c5a-d27097f48eeb@proxmox.com> <441d3c9b-8987-46c1-8da9-e9cde4429aab@proxmox.com> <076ebe87-db27-47a0-b3fc-893254fac0db@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <076ebe87-db27-47a0-b3fc-893254fac0db@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770989830430 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 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 KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: D3NSB5WHQSZVVUYEORTGKA4ZCLGBGZ47 X-Message-ID-Hash: D3NSB5WHQSZVVUYEORTGKA4ZCLGBGZ47 X-MailFrom: c.ebner@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 2/12/26 10:54 AM, Robert Obkircher wrote: > On 2/11/26 13:40, Christian Ebner wrote: >> On 2/11/26 1:12 PM, Robert Obkircher wrote: >>> On 2/9/26 10:14, Christian Ebner wrote: [...] >>>> + >>>> +#[repr(C)] >>>> +struct MappableRequestCounters { >>>> +    magic: [u8; 8], >>>> +    counters: RequestCounters, >>>> +    _page_size_padding: [u8; MEMORY_PAGE_SIZE >>>> +        - PROXMOX_SHARED_REQUEST_COUNTERS_1_0.len() >>>> +        - std::mem::size_of::()], >>> Why do we need to hard-code a possibly incorrect assumption about the >>> page size? I know `SharedMemory` claims that the size needs to be a >>> multiple of 4096, but I don't understand why. >> >> This padding was chosen exactly because of that doc comment and >> since other users of `SharedMemory` do the same 4k page size padding. >> >> Will however have a look at the implementation details there, now >> that you sparked my interest, maybe that sheds some light. >> >> Or maybe you already have further details since you are working on >> the patches to make the code compatible with kernels with >> non-default page sizes? > That's why I was thinking about it, but I don't have further details yet. >>> I guess this implicitly prevents unused padding at the end of the >>> struct which would be problematic if we add a field and run two >>> processes with different versions at the same time, but that doesn't >>> explain the page size. >> >> Allowing to extend with further fields in the future has proven to >> be beneficial, yes. E.g. for the token shadow config version as >> mentioned in >> https://lore.proxmox.com/pbs-devel/20260121151408.731516-2-s.rufinatscha@proxmox.com/T/ >> So took a closer look at the implementation, the page size check for the length is actually enforced there, not just suggested. Also, CC'ing Dietmar as the author of the code, maybe he can tell us more about the reasons why. Here is what I found: proxmox_shared_memory::SharedMemory mmap's the file on init [0] using the implementation of proxmox_sys::mmap::Mmap::map_fd() [1], passing however a hard coded `offset` of 0. This internally calls nix::sys::mman::mmap(), passing no address (so the Kernel is free to choose it, would be page aligned by the Kernel if given), passing along the zero offset set before. The man-page [0] for mmap states that `offset` must be a multiple of the page size as returned by `sysconf(_SC_PAGE_SIZE)`, and that the contents of a file mapping are initialized for length bytes starting from given offset. So with the hard coded 0 this will always be fine. For the `length` the only constraint is that it must be greater than zero: "The length argument specifies the length of the mapping (which must be greater than 0)." It is however also specified [3], that: "The system shall always zero-fill any partial page at the end of an object. Further, the system shall never write out any modified portions of the last page of an object which are beyond its end. References within the address range starting at pa and continuing for len bytes to whole pages following the end of an object shall result in delivery of a SIGBUS signal." So I guess that the chosen constraint on the length is simply best practice to avoid writing to possibly invalid parts of the mappings. Given that, I would either opt to keep the PAGE_SIZE constant as implemented above or directly switch to read it from sysconf(_SC_PAGE_SIZE), in an effort to facilitate any transitions to Linux kernels using non-default page sizes. What are your thoughts on that? [0] https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-shared-memory/src/lib.rs;h=b067d1b96e73740e82a5b7be922edb7ba162c650;hb=HEAD#l58 [1] https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-sys/src/mmap.rs;h=8b8ad8a6ab7c102c156313e29e96816140912b44;hb=HEAD#l30 [2] https://man7.org/linux/man-pages/man2/mmap.2.html [3] https://pubs.opengroup.org/onlinepubs/9799919799/