Allow specifying integrity metadata reservation on pool creation #3733

mulkieran · 2024-12-16T04:50:34Z

Related stratis-storage/project#746

packit-as-a-service · 2024-12-16T05:07:21Z

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 · 2024-12-16T20:58:55Z

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.

jbaublitz

Just one comment.

jbaublitz · 2024-12-16T22:14:26Z

src/engine/strat_engine/backstore/blockdev/v2.rs

 ) -> 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)


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?

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.

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.

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 · 2024-12-20T19:37:14Z

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 · 2025-01-02T20:32:48Z

New metadata snippet:

    "data_tier": {
      "blockdev": {
        "allocs": [
          [
            {
              "length": 102946816,
              "parent": "fc9a33b3-6fc9-4610-9259-ba90cb1ba829",
              "start": 8192
            }
          ]
        ],
        "devs": [
          {
            "hardware_info": "0x5000c500640bdae3",
            "integrity_meta_allocs": [
              [
                102957048,
                1900552
              ]
            ],
            "uuid": "fc9a33b3-6fc9-4610-9259-ba90cb1ba829"
          }
        ]
      },
      "integrity_spec": {
        "allocate_superblock": true,
        "block_size": 4096,
        "journal_size": 262144,
        "tag_spec": "512b"
      }
    }
  },

mulkieran · 2025-01-02T20:39:00Z

If --integrity=no specified, pool metadata snippet looks like this:

    "data_tier": {
      "blockdev": {
        "allocs": [
          [
            {
              "length": 104847360,
              "parent": "07865bfc-e98d-4a2d-b2ed-05dd1ce3c5e0",
              "start": 8192
            }
          ]
        ],
        "devs": [
          {
            "hardware_info": "0x5000c500640bdae3",
            "integrity_meta_allocs": [
              [
                104857592,
                8
              ]
            ],
            "uuid": "07865bfc-e98d-4a2d-b2ed-05dd1ce3c5e0"
          }
        ]
      },
      "integrity_spec": {
        "allocate_superblock": false,
        "block_size": 4096,
        "journal_size": 0,
        "tag_spec": "0b"
      }
    }
  },

I think that is acceptable as regards the format of the integrity meta spec, but the integrity_meta_allocs length, 8 sectors, is wrong.

mulkieran · 2025-01-02T20:58:03Z

Better metadata snippet on --integrity=no:

"data_tier": {
      "blockdev": {
        "allocs": [
          [
            {
              "length": 104849408,
              "parent": "04b274b8-5815-4c09-9802-520f094fee2b",
              "start": 8192
            }
          ]
        ],
        "devs": [
          {
            "hardware_info": "0x5000c500640bdae3",
            "uuid": "04b274b8-5815-4c09-9802-520f094fee2b"
          }
        ]
      },
      "integrity_spec": {
        "allocate_superblock": false,
        "block_size": 4096,
        "journal_size": 0,
        "tag_spec": "0b"
      }
    }
  },

mulkieran · 2025-01-03T15:04:58Z

stratis-printmetadata snippet:

Integrity specification for data devices:
Allocate Superblock: true
Tag Specification: 512b
Journal Size: 262144 sectors
Block Size: 4096 bytes


Allocations from each data device:
Data device: 5cb0cd6f-c30b-455c-9367-c0620c7a4e44
0: Use: stratis_metadata                0 +         8192 =         8192 sectors
1: Use: allocated                    8192 +     32129024 =     32137216 sectors
2: Use: unused                   32137216 +         1784 =     32139000 sectors
3: Use: integrity_metadata       32139000 +       776456 =     32915456 sectors

mulkieran · 2025-01-03T17:17:48Z

Sample error:

Error: Device 5cb0cd6f-c30b-455c-9367-c0620c7a4e44: Integrity specification should resolve to 0 allocations for integrity, but data device has space allocated for integrity.

jbaublitz

Just one comment. Other than that, I'm okay with the changes.

jbaublitz · 2025-01-03T20:54:41Z

src/dbus_api/api/manager_3_8/api.rs

+        // b: true if the superblock reservation is supposed to be done
+        //
+        // Rust representation: (bool, bool)
+        .in_arg(("allocate_superblock", "(bb)"))


Can you explain a bit more why you chose to have an optional boolean data type? I feel like this would be better handled by just a boolean type.

Per our discussion in stand up, I'm okay with allowing an option to specify using a default for D-Bus users.

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. Have the string representation in the pool metadata match that for D-Bus API. 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]>

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]>

Signed-off-by: mulhern <[email protected]>

The error will print out its own description of itself as an error, so this extra error text is redundant. Signed-off-by: mulhern <[email protected]>

mulkieran · 2025-01-06T17:18:12Z

rebased, some minor squashing

mulkieran self-assigned this Dec 16, 2024

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

mulkieran requested a review from jbaublitz December 16, 2024 20:59

jbaublitz requested changes Dec 16, 2024

View reviewed changes

jbaublitz approved these changes Dec 17, 2024

View reviewed changes

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

mulkieran force-pushed the jbaublitz-metadata-rework-integrity branch 2 times, most recently from 4b876fb to 9437326 Compare December 23, 2024 23:12

mulkieran mentioned this pull request Jan 3, 2025

Add additional arguments for pool creation stratis-storage/testing#303

Merged

jbaublitz requested changes Jan 3, 2025

View reviewed changes

mulkieran force-pushed the jbaublitz-metadata-rework-integrity branch from f565c58 to c3b486c Compare January 6, 2025 11:55

jbaublitz approved these changes Jan 6, 2025

View reviewed changes

jbaublitz added 2 commits January 6, 2025 11:56

Record integrity parameters in metadata

f111f33

Add integrity tag and journal size parameters to D-Bus

1d033e2

jbaublitz and others added 9 commits January 6, 2025 11:56

Add tag and journal size parameters to prediction script

40e9e59

Return parser error on non-numeric value for integrity journal size

d4f0087

Specify default integrity journal size in parser as string value. Signed-off-by: mulhern <[email protected]>

Use IntegritySpec struct to manage spec

3226cdd

Combine both current specs in the struct. The default of both fields is None. Signed-off-by: mulhern <[email protected]>

Add a ValidatedIntegritySpec type

891df65

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

Add whether or not to allocate the integrity superblock

ec6106b

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

Print the integrity spec when printing the metadata

c9ceaa5

Signed-off-by: mulhern <[email protected]>

Do one check on integrity spec and integrity allocations

8364c5a

Signed-off-by: mulhern <[email protected]>

Do not print out "Error encountered"

eec882a

The error will print out its own description of itself as an error, so this extra error text is redundant. Signed-off-by: mulhern <[email protected]>

mulkieran force-pushed the jbaublitz-metadata-rework-integrity branch from c3b486c to eec882a Compare January 6, 2025 17:17

mulkieran marked this pull request as ready for review January 6, 2025 17:18

mulkieran changed the title ~~Jbaublitz metadata rework integrity~~ Allow specifying integrity metadata reservation on pool creation Jan 6, 2025

mulkieran merged commit 0706663 into stratis-storage:master Jan 6, 2025
44 of 49 checks passed

mulkieran deleted the jbaublitz-metadata-rework-integrity branch January 6, 2025 18:22

mulkieran mentioned this pull request Jan 7, 2025

Modifications to integrity metadata reservation #3716

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying integrity metadata reservation on pool creation #3733

Allow specifying integrity metadata reservation on pool creation #3733

mulkieran commented Dec 16, 2024 •

edited

Loading

packit-as-a-service bot commented Dec 16, 2024

mulkieran commented Dec 16, 2024

jbaublitz left a comment

jbaublitz Dec 16, 2024

mulkieran Dec 16, 2024

jbaublitz Dec 17, 2024 •

edited

Loading

mulkieran Dec 17, 2024

mulkieran commented Dec 20, 2024

mulkieran commented Jan 2, 2025 •

edited

Loading

mulkieran commented Jan 2, 2025 •

edited

Loading

mulkieran commented Jan 2, 2025

mulkieran commented Jan 3, 2025

mulkieran commented Jan 3, 2025

jbaublitz left a comment

jbaublitz Jan 3, 2025

jbaublitz Jan 6, 2025

mulkieran commented Jan 6, 2025

Allow specifying integrity metadata reservation on pool creation #3733

Allow specifying integrity metadata reservation on pool creation #3733

Conversation

mulkieran commented Dec 16, 2024 • edited Loading

packit-as-a-service bot commented Dec 16, 2024

mulkieran commented Dec 16, 2024

jbaublitz left a comment

Choose a reason for hiding this comment

jbaublitz Dec 16, 2024

Choose a reason for hiding this comment

mulkieran Dec 16, 2024

Choose a reason for hiding this comment

jbaublitz Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

mulkieran Dec 17, 2024

Choose a reason for hiding this comment

mulkieran commented Dec 20, 2024

mulkieran commented Jan 2, 2025 • edited Loading

mulkieran commented Jan 2, 2025 • edited Loading

mulkieran commented Jan 2, 2025

mulkieran commented Jan 3, 2025

mulkieran commented Jan 3, 2025

jbaublitz left a comment

Choose a reason for hiding this comment

jbaublitz Jan 3, 2025

Choose a reason for hiding this comment

jbaublitz Jan 6, 2025

Choose a reason for hiding this comment

mulkieran commented Jan 6, 2025

mulkieran commented Dec 16, 2024 •

edited

Loading

jbaublitz Dec 17, 2024 •

edited

Loading

mulkieran commented Jan 2, 2025 •

edited

Loading

mulkieran commented Jan 2, 2025 •

edited

Loading