Filesystem limits

[jbaublitz]

Closes #3431

[jbaublitz]

This is just waiting on unit tests.

[jbaublitz]

Just completed the unit tests.

[jbaublitz]

/packit test

[mulkieran]

One minor request AND ought we to allow the user to specify the filesystem size limit on filesystem creation?

[mulkieran]

Tests revealed a typo.

[jbaublitz]

@mulkieran I'm fine with letting the user specify filesystem size limits on creation if you'd like. It's more a matter of preference.

[jbaublitz]

@mulkieran I agree that users might not know what the large value means. Some might believe that this is a supported size maximum or a value to test, these might not be reasons to no do it. I thought that Stratis might want to stay consistent with Key, where there is a set, unset, reset options, but the pool set-fs-limit doesn't have an unset or reset for good reasons. Another thought based on the maximum sizes is, does dm-thin and XFS share similar maximums?

The only reason we don't have an unset for set-fs-limit is because it's a metadata changing operation (as in we have to allocate more metadata room for each filesystem that we want to allow by the limit). I'm not convinced these two things are comparable because we don't store the filesystem size limit anywhere but the MDV so there is no additional space that needs to be allocated.

[jbaublitz]

My objection to these changes is that in general:

1. I think no filesystem limit is easy to understand. We can solicit feedback from @mvollmer on this if we'd like because I know he's had some opinions about usability in the past. I personally think it's less usable to display a very large number with no explanation and allow no way to remove it. That feels like leaking implementation details to the user in a way that's not helpful and will generate more questions than anything.
2. I think that maintaining the MAX_THINDEV_SIZE in our codebase at all may be a mistake, but an even bigger mistake to mix it in with the filesystem limit code where it will be relatively hard to remove in the future. I don't like duplicating constants that cannot be set and may change in the future. Sometimes it's necessary, but I don't see enough of a benefit here to support that. Here I would almost rather have filesystem creation or extension fail so that we don't have to support multiple values across kernels if it changes in the future.
3. I don't see the simplifications as large enough to justify the drawbacks.

I want to put up the following proposal for a simplification in how we deal with the size limit property; we can talk about it in standup:

In stratisd:

1. Since we treat `MAX_THIN_DEV_SIZE` like a limit, treat it thus in `StratFilesystem::extend_size`, i.e. explicitly cap the size at `MAX_THIN_DEV_SIZE`. I expect that other constraints will cause that size never to be reached, but you never know.

2. Change the value representing the size limit in the engine so that it is non-optional.

3. Change the D-Bus property so that it non-optional and if no other limit has been set, report `MAX_THIN_DEV_SIZE`.

In stratis-cli:

1. Always report the filesystem limit, as a number, in the filesystem listing under the heading: `Total / Used / Free / Limit`.

The advantages of this are:

1. Some real simplification in the engine internals, due to non-optional value.

2. Some real simplification in the D-Bus API due to non-optional property.

3. Some real simplification in stratis-cli. This idea was prompted because of a request for an `unset-size-limit` command in stratis-cli. I would much rather keep the just the `set-size-limit` subcommand. It already takes one special value, `stratis fs set-fs-limit <poolname> <fsname> current` which just means set the filesystem limit to the filesystems current size, which seems sensible. I would prefer to add another special value for that subcommand, like `stratis fs set-fs-limit <poolname> <fsname> largest`.  The way stratis would implement this request would be to have the constant `MAX_THIN_DEV_SIZE` defined; it would put that value in its property set call.

The disadvantages of this approach are:

1. User will see a very large limit in their `filesystem list` and may interpret it as meaning something different than it really does.

[mulkieran]

I'm willing to believe that dropping MAX_THIN_DEV_SIZE entirely might be the truly simplifying move. But then I think we should drop it from the checks we do for the size value as well in order to be consistent w/ ourselves. In theory, then xfs will return an error on filesystem creation that stratisd will report. We usually try to let xfs do its own work, and I'm not sure why we didn't in this case.

My objection to these changes is that in general:

1. I think no filesystem limit is easy to understand. We can solicit feedback from @mvollmer on this if we'd like because I know he's had some opinions about usability in the past. I personally think it's less usable to display a very large number with no explanation and allow no way to remove it. That feels like leaking implementation details to the user in a way that's not helpful and will generate more questions than anything.

2. I think that maintaining the `MAX_THINDEV_SIZE` in our codebase at all may be a mistake, but an even bigger mistake to mix it in with the filesystem limit code where it will be relatively hard to remove in the future. I don't like duplicating constants that cannot be set and may change in the future. Sometimes it's necessary, but I don't see enough of a benefit here to support that. Here I would almost rather have filesystem creation or extension fail so that we don't have to support multiple values across kernels if it changes in the future.

3. I don't see the simplifications as large enough to justify the drawbacks.

I want to put up the following proposal for a simplification in how we deal with the size limit property; we can talk about it in standup:
In stratisd:

1. Since we treat `MAX_THIN_DEV_SIZE` like a limit, treat it thus in `StratFilesystem::extend_size`, i.e. explicitly cap the size at `MAX_THIN_DEV_SIZE`. I expect that other constraints will cause that size never to be reached, but you never know.

2. Change the value representing the size limit in the engine so that it is non-optional.

3. Change the D-Bus property so that it non-optional and if no other limit has been set, report `MAX_THIN_DEV_SIZE`.

In stratis-cli:

1. Always report the filesystem limit, as a number, in the filesystem listing under the heading: `Total / Used / Free / Limit`.

The advantages of this are:

1. Some real simplification in the engine internals, due to non-optional value.

2. Some real simplification in the D-Bus API due to non-optional property.

3. Some real simplification in stratis-cli. This idea was prompted because of a request for an `unset-size-limit` command in stratis-cli. I would much rather keep the just the `set-size-limit` subcommand. It already takes one special value, `stratis fs set-fs-limit <poolname> <fsname> current` which just means set the filesystem limit to the filesystems current size, which seems sensible. I would prefer to add another special value for that subcommand, like `stratis fs set-fs-limit <poolname> <fsname> largest`.  The way stratis would implement this request would be to have the constant `MAX_THIN_DEV_SIZE` defined; it would put that value in its property set call.

The disadvantages of this approach are:

1. User will see a very large limit in their `filesystem list` and may interpret it as meaning something different than it really does.

[jbaublitz]

Okay great! That's exactly what I wanted when I wrote the PR so I'll remove the MAX_THINDEV_SIZE altogether.

[jbaublitz]

@mulkieran Should we also remove MIN_THINDEV_SIZE?

[mulkieran]

@mulkieran Should we also remove MIN_THINDEV_SIZE?

@jbaublitz The version of xfs in f38+ now enforces its lower bound so I think that we might be able to do this. Let's discuss in standup.

[mulkieran]

whoops

[mulkieran]

In the sim engine only, it looks like setting the size-limit on creation somehow doesn't cause the sim filesystem's size limit to be set. This is confirmed by stratis report which shows size limit as None for the sim engine and the set value is not displayed in the filesystem list view. For the strat engine, everything seems to be working correctly. For the sim engine and for the strat engine, setting and unsetting the size property seems to work correctly.

[jbaublitz]

In the sim engine only, it looks like setting the size-limit on creation somehow doesn't cause the sim filesystem's size limit to be set. This is confirmed by stratis report which shows size limit as None for the sim engine and the set value is not displayed in the filesystem list view. For the strat engine, everything seems to be working correctly. For the sim engine and for the strat engine, setting and unsetting the size property seems to work correctly.

Resolved. This was caught by the unit tests for the strat_engine but isn't tested in the sim_engine so it wasn't caught.

[jbaublitz]

/packit test

[mulkieran]

@jbaublitz Plz rebase when you get the chance.

[mulkieran]

/packit test