Modifications to integrity metadata reservation #3716

jbaublitz · 2024-11-12T15:37:59Z

Related to #3274
Related to #3637

packit-as-a-service · 2024-11-12T19:55:12Z

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-3716
And now you can install the packages.

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

jbaublitz · 2024-11-25T19:55:06Z

I'm going to add a few tests here for the parameters, but after that, this should be ready.

mulkieran · 2024-12-02T15:22:14Z

src/bin/utils/cmds.rs

+                         Arg::new("integrity_journal_size")
+                         .long("integrity-journal-size")
+                         .num_args(1)
+                         .help("Size of the integrity journal. Default is 128 MiB. Units are 4KiB blocks.")


Better just keep everything as bytes. I told Vojtech we would just do that, so we might as well stick with it.

mulkieran · 2024-12-02T15:24:51Z

src/dbus_api/api/manager_3_8/methods.rs

+        None => None,
+    };
+
+    let journal_size =


Better just to make units of journal size bytes everywhere.

mulkieran · 2024-12-03T20:05:21Z

@jbaublitz Please rebase when you get the chance.

mulkieran

One request so far...

mulkieran · 2024-12-04T01:51:30Z

src/engine/strat_engine/engine.rs

    ) -> StratisResult<CreateAction<PoolUuid>> {
        validate_name(name)?;
        let name = Name::new(name.to_owned());
+        let rounded_journal_size = match journal_size {


I think we will have to validate the tag size as well, matching our expectations to the dm-integrity expectations. I don't think there is any thing but trouble at this time that could result from allowing odd-valued tag sizes.

I don't think this is as simple as checking for odd numbers. I'm not opposed to doing some validating, but just checking odd numbers feels incomplete. All of the values I've seen for tag size are powers of two. I think the thing to remember with tag size, however, is that that is only used to calculate the tag area which will be rounded to 4k regardless of whether the number is odd or not so I think we should either leave it as is or only support valid values of tags that integrity supports.

I think we should only support the values that dm-integrity itself supports. We can relax that restriction at some time later if there ever appears to be any value to doing so, which seems very unlikely. It is the case that our design is to do external metadata, so integrity support in stratisd will only use this value to calculate the size of the external metadata device. But, at some time, we hope that we will be able to support integrity on NPo2 devices. If there is code that is shared for the implementation of these, then tag sizes that integrity does not support will be more of a problem. My other concern is that, at present, the D-Bus spec restricts the tag size to a byte. That means max is 128. Is that enough, or should the size be doubled?

The max is not 128 because as I understand it, it is unsigned. However the largest tag size I saw was 128 bytes anyway.

What I meant is: if we combine the powers of 2 restriction and the 8 digits restriction, the max possible value is 128. I also have not seen a larger tag size than 128 bytes. Is that fact enough to go on?

I think we'll have more information on that after I get a list of supported tag types.

mulkieran · 2024-12-05T15:11:35Z

@jbaublitz Speaking of tag size and also journal size, it is possible to make both 0. This still leave the, relatively small, superblock pre-allocated. It will legitimate for a user to specify 0 for both if they are using NPo2 devices and want to turn on integrity later and it is legitimate now if they never want to turn on integrity at all. So I believe that it is entirely acceptable to allow zero for both. I'll just add some more about that in the stratis-cli docs, unless you disagree.

jbaublitz · 2024-12-05T16:38:21Z

@jbaublitz Speaking of tag size and also journal size, it is possible to make both 0. This still leave the, relatively small, superblock pre-allocated. It will legitimate for a user to specify 0 for both if they are using NPo2 devices and want to turn on integrity later and it is legitimate now if they never want to turn on integrity at all. So I believe that it is entirely acceptable to allow zero for both. I'll just add some more about that in the stratis-cli docs, unless you disagree.

Do you mean we should leave things the way they are? If you believe this is best in order to support other features like NPo2 devices, I'm willing to leave it as it's a relatively small overhead for the device.

mulkieran · 2024-12-05T19:22:40Z

@jbaublitz Speaking of tag size and also journal size, it is possible to make both 0. This still leave the, relatively small, superblock pre-allocated. It will legitimate for a user to specify 0 for both if they are using NPo2 devices and want to turn on integrity later and it is legitimate now if they never want to turn on integrity at all. So I believe that it is entirely acceptable to allow zero for both. I'll just add some more about that in the stratis-cli docs, unless you disagree.

Do you mean we should leave things the way they are? If you believe this is best in order to support other features like NPo2 devices, I'm willing to leave it as it's a relatively small overhead for the device.

I think that we should continue to allow 0 for both values.

mulkieran · 2024-12-16T13:37:09Z

@jbaublitz Please rebase when you get the chance.

mulkieran · 2025-01-07T15:02:45Z

Superceded by #3733

jbaublitz self-assigned this Nov 12, 2024

jbaublitz force-pushed the metadata-rework-integrity branch from 59f77ae to ce3edf3 Compare November 12, 2024 19:37

jbaublitz force-pushed the metadata-rework-integrity branch from ce3edf3 to fffbbfd Compare November 20, 2024 14:19

mulkieran mentioned this pull request Nov 27, 2024

Allocate the same when adding as when creating #3721

Closed

mulkieran requested changes Dec 2, 2024

View reviewed changes

jbaublitz force-pushed the metadata-rework-integrity branch from 503aa58 to 56efb3a Compare December 2, 2024 20:55

jbaublitz force-pushed the metadata-rework-integrity branch from 56efb3a to a1238e0 Compare December 3, 2024 20:14

mulkieran requested changes Dec 4, 2024

View reviewed changes

jbaublitz force-pushed the metadata-rework-integrity branch from a1238e0 to f2112b3 Compare December 9, 2024 14:58

jbaublitz added 3 commits December 16, 2024 09:24

Record integrity parameters in metadata

134af09

Add integrity tag and journal size parameters to D-Bus

7ed77cf

Add tag and journal size parameters to prediction script

17f8bc3

jbaublitz force-pushed the metadata-rework-integrity branch from f2112b3 to 17f8bc3 Compare December 16, 2024 14:24

mulkieran closed this Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifications to integrity metadata reservation #3716

Modifications to integrity metadata reservation #3716

jbaublitz commented Nov 12, 2024

packit-as-a-service bot commented Nov 12, 2024

jbaublitz commented Nov 25, 2024

mulkieran Dec 2, 2024

mulkieran Dec 2, 2024

mulkieran commented Dec 3, 2024

mulkieran left a comment

mulkieran Dec 4, 2024

jbaublitz Dec 4, 2024

mulkieran Dec 4, 2024

jbaublitz Dec 4, 2024 •

edited

Loading

mulkieran Dec 4, 2024

jbaublitz Dec 4, 2024

mulkieran Dec 4, 2024

mulkieran commented Dec 5, 2024

jbaublitz commented Dec 5, 2024

mulkieran commented Dec 5, 2024

mulkieran commented Dec 16, 2024

mulkieran commented Jan 7, 2025

Modifications to integrity metadata reservation #3716

Modifications to integrity metadata reservation #3716

Conversation

jbaublitz commented Nov 12, 2024

packit-as-a-service bot commented Nov 12, 2024

jbaublitz commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mulkieran commented Dec 3, 2024

mulkieran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaublitz Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mulkieran commented Dec 5, 2024

jbaublitz commented Dec 5, 2024

mulkieran commented Dec 5, 2024

mulkieran commented Dec 16, 2024

mulkieran commented Jan 7, 2025

jbaublitz Dec 4, 2024 •

edited

Loading