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

Jbaublitz metadata rework integrity #3733

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

mulkieran
Copy link
Member

No description provided.

@mulkieran mulkieran self-assigned this Dec 16, 2024
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratisd-3733
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

@mulkieran mulkieran force-pushed the jbaublitz-metadata-rework-integrity branch 4 times, most recently from cb519dd to 18b1495 Compare December 16, 2024 20:55
@mulkieran
Copy link
Member Author

Help text for integrity size spec for file size predictor is:

      --integrity-tag-spec <integrity_tag_spec>
          Integrity tag specification defining the size of the tag to store a checksum or other value for each block on a device.
          
          [default: 512b]
          [possible values: 0b, 32b, 512b]

stratis-cli would do the same.

@mulkieran mulkieran requested a review from jbaublitz December 16, 2024 20:59
Copy link
Member

@jbaublitz jbaublitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment.

) -> Sectors {
Bytes(4096).sectors()
+ journal_size
+ Bytes::from(((*total_space.bytes() / *block_size) * *tag_size + 4095) & !4095).sectors()
+ Bytes::from(
(((((total_space.bytes() / block_size) * u128::from(tag_size.as_bits())) / 8 + 7)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to understand the logic of rounding up to the nearest byte if we're representing this as bits. This seems to defeat the purpose of representing it as bits in the first place. Could you explain a little bit more what you're trying to do here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 4095 in the expression '+4095 & !4095' is implicitly in units of bytes. If I don't round up, then the value for the tag space may underestimate by 7 the number of bits required. If, by a weird chance, that underestimate is also divisible by 4096, then the total allocation for integrity metadata is nicely aligned but up to 7 bits short of what might be required.

Copy link
Member

@jbaublitz jbaublitz Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is another reason why the units would make more sense in bytes. The best case scenario is that we end up overestimating in a way that rounds to Mo8 hash sizes no matter what so this does not seem like we are getting anything from representing it as bits.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be happy to change the order of operations. Instead of multiplying the number of blocks by the bits value and then rounding up to the nearest byte, rounding the bits value up to the nearest byte and multiplying it by the number of blocks. Of course, that has no effect at this point on the computation's result.

What I think would be shortsighted at this point is limiting our D-Bus API and our pool-level metadata to allow only units of bytes.

I'll make the proposed change.

@mulkieran mulkieran force-pushed the jbaublitz-metadata-rework-integrity branch 6 times, most recently from 743c737 to 2714a96 Compare December 20, 2024 18:22
@mulkieran
Copy link
Member Author

Pool metadata snippet, accepting all defaults:

 "data_tier": {
            "blockdev": {
                "allocs": [
                    [
                        {
                            "length": 16242688,
                            "parent": "ec6d3a58-4b5b-4878-9d11-c8f42b12ec8a",
                            "start": 8192
                        }
                    ]
                ],
                "devs": [
                    {
                        "integrity_meta_allocs": [
                            [
                                16250904,
                                524264
                            ]
                        ],
                        "uuid": "ec6d3a58-4b5b-4878-9d11-c8f42b12ec8a"
                    }
                ]
            },
            "integrity_spec": {
                "allocate_superblock": true,
                "block_size": 4096,
                "journal_size": 262144,
                "tag_spec": "B512"
            }
        }
    },

@mulkieran mulkieran force-pushed the jbaublitz-metadata-rework-integrity branch 2 times, most recently from 4b876fb to 9437326 Compare December 23, 2024 23:12
jbaublitz and others added 10 commits December 23, 2024 18:13
Specify default integrity journal size in parser as string value.

Signed-off-by: mulhern <[email protected]>
Define an enum to represent the tag size.

Store the IntegrityTagSpec value in the pool-level metadata.
Nothing is gained by turning the enum into bits before storing it in the
pool-level metadata.

Compute using the bytes value that is at least as large as the number of
bits the tag spec represents.

Use tag_spec instead of tag_size for the name of the field in the
CreatePool method.

Signed-off-by: mulhern <[email protected]>
Signed-off-by: mulhern <[email protected]>
Combine both current specs in the struct.

The default of both fields is None.

Signed-off-by: mulhern <[email protected]>
Use ValidatedIntegritySpec for serialization and to pass to methods
invoked by engine's create_pool method.

Signed-off-by: mulhern <[email protected]>
This is an option in the D-Bus, the predict script and the pool-level
metadata.

Signed-off-by: mulhern <[email protected]>
Have the string representation match that for D-Bus API.

Signed-off-by: mulhern <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants