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

Return true from extend_thin_meta_device less #3648

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mulkieran
Copy link
Member

No description provided.

@mulkieran mulkieran self-assigned this Jul 10, 2024
@mulkieran mulkieran marked this pull request as ready for review July 10, 2024 14:32
@@ -1117,7 +1117,10 @@ impl ThinPool {
meta_growth,
);

(ext.is_ok(), ext)
match ext {
Ok(Sectors(0)) | Err(_) => (false, ext),
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this unreachable?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jbaublitz You might be misreading the diff.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, so this is an optimization to not save the metadata if it's not extended at all? I am failing to see a case where this will ever be reached. Unreachable might be the wrong term, because it could be reached, but meta_growth is guaranteed to be larger than 0. I don't see a case where this will be reached with the current implementation. I was originally thinking that ext could be 0 but I don't see a case where meta_growth would be non-zero but ext would be zero.

Copy link
Member

Choose a reason for hiding this comment

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

Let me dig into do_extend and see if maybe it's rounding down. That could potentially cause the behavior you're seeing.

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 understand your argument. I think we can leave this in pending for now and work on simplifying it later.

@mulkieran mulkieran force-pushed the avoid-changed-if-0-extension-meta branch from c75bd89 to b7aa992 Compare July 11, 2024 00:38
@mulkieran mulkieran requested a review from jbaublitz July 11, 2024 02:23
@jbaublitz
Copy link
Member

Is this still relevant or can it be closed?

@mulkieran
Copy link
Member Author

Is this still relevant or can it be closed?

It may be. I need to take another look.

@mulkieran mulkieran added this to the Metadata-related milestone Sep 18, 2024
@mulkieran mulkieran force-pushed the avoid-changed-if-0-extension-meta branch from d7c94f8 to 9cc1fbe Compare October 31, 2024 18:11
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-3648
  • And now you can install the packages.

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

2 similar comments
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-3648
  • And now you can install the packages.

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

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

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

@mulkieran
Copy link
Member Author

mulkieran commented Oct 31, 2024

To do this better we should just save the thinpool's metadata at the start of the check method and see if it has change when the check method is about to return, thereby eliminating complexity.

Necessary instead to do this in both the meta and data extend methods, rather than in the check method.

Return true only if the method returned without error AND the amount to
extend is non-zero.

This is the same condition already used in extend_thin_data_device.

Signed-off-by: mulhern <[email protected]>
alloc() return None if there is not enough space to satisfy the request.

Signed-off-by: mulhern <[email protected]>
@mulkieran mulkieran force-pushed the avoid-changed-if-0-extension-meta branch from 9cc1fbe to 4f81cc9 Compare November 15, 2024 03:40
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