Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Size of the DSTORAGE_REQUEST_OPTIONS #22

Open
hasenbanck opened this issue Jan 17, 2023 · 3 comments
Open

Size of the DSTORAGE_REQUEST_OPTIONS #22

hasenbanck opened this issue Jan 17, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@hasenbanck
Copy link

hasenbanck commented Jan 17, 2023

While porting the DirectStorage API to Rust, we found the following curriosity and would like to as if it's working as designed:

The DSTORAGE_REQUEST_OPTIONS is defines as a bitfield and it seems that the design goal was that all fields are packed into a single unsigned 64 bit integer field.

But the current implementation is not of byte size 8, but of size 16, since MSVC is only packing fields with the same type into the same backing field. Since DSTORAGE_COMPRESSION_FORMAT is an UINT8 and not an UINT64 like the other fields, it will be stored by MSVC into it's own backing field. Allignment and padding then will grow the size of DSTORAGE_REQUEST_SOURCE_TYPE to 16 bytes.

@damyanp
Copy link
Member

damyanp commented Jan 17, 2023

Yes, we noticed this recently as well. Unfortunately, it would be a breaking change to fix this to what was intended so we need to be very careful about how and when we do it - we'll probably try and line it up with some other breaking change.

One possible short-term fix we've considered is to update the struct definition so that it more obviously reflects the actual layout of the struct so it isn't so surprising.

@damyanp
Copy link
Member

damyanp commented Oct 5, 2023

Note: although version 1.2.0 of DirectStorage updated the struct definition to make it more obvious, keeping this issue open to track fixing the options field to what was originally intended.

@osor-io
Copy link

osor-io commented Oct 7, 2023

Also porting this to another language. I found it strange that the last member, Reserved, is set to occupy 48 bits, which leaves still one byte at the end of the struct unnacounted for as far as I can tell. Should that last member have been UINT64 Reserved : 56?

It doesn't effectively matter, since it'll be padded to 16 bytes. but if the definition update was intended to make the distribution of the member more obvious this might be worth changing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants