Re: [PATCH v1 2/6] s3-client: add persistent shared request counters for client

[Christian Ebner <c.ebner@proxmox.com>]

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::<RequestCounters>()],
>>> 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/