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.

Sign up for GitHub

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

Jump to bottom

Ensured filesystem Used signal is sent if used value has changed on filesystem check #3639

Merged
merged 4 commits into from
Jul 1, 2024
Merged

Ensured filesystem Used signal is sent if used value has changed on filesystem check #3639

merged 4 commits into from
Jul 1, 2024

Conversation

mulkieran
Copy link
Member

@mulkieran mulkieran commented Jun 26, 2024 •
edited
Loading

Closes #3634

mulkieran added 3 commits June 26, 2024 15:51
@mulkieran
Remove early exit
05a024c 
Need to process filesystems with a size change, also.

Signed-off-by: mulhern <[email protected]>
@mulkieran
Add a clarifying comment
2ca531b 
Signed-off-by: mulhern <[email protected]>
@mulkieran
Change handle_extension to handle_fs_changes
03b0003 
It can function in the case that there is no need to actually extend the
filesystem, but only to communicate about the filesystem changes.

Signed-off-by: mulhern <[email protected]>
@mulkieran mulkieran self-assigned this Jun 26, 2024
@mulkieran
Copy link
Member Author

This approach is acceptable only if it is ok to spawn a thread for every filesystem.

@mulkieran
Copy link
Member Author

mulkieran commented Jun 27, 2024 •
edited
Loading

With simple test:

  1. Create a pool
  2. Create a filesystem
  3. Write 1 MiB to the filesystem

The filesystem Used signal is not emitted on HEAD (although pool TotalPhysicalUse signal is); it is emitted with this PR. It is not emitted on an fs that is not written to.

@mulkieran mulkieran requested a review from jbaublitz June 27, 2024 02:54
@mulkieran
Copy link
Member Author

@jbaublitz This draft PR contains all the essentials and is somewhat tested. Plz let me know what you think.

jbaublitz
jbaublitz reviewed Jun 27, 2024
View reviewed changes
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.

I looked at this for a while and the only change I wanted to potentially discuss is that I'm a bit unclear on what the benefit is of running a diff in the case of a filesystem that isn't mounted. Is that just for the sake of code simplicity? I see how it's necessary to run the diff code even when the extend size is zero, but I'm not entirely sure why we're doing that in the case where the filesystem isn't mounted.

@mulkieran
Copy link
Member Author

I looked at this for a while and the only change I wanted to potentially discuss is that I'm a bit unclear on what the benefit is of running a diff in the case of a filesystem that isn't mounted. Is that just for the sake of code simplicity? I see how it's necessary to run the diff code even when the extend size is zero, but I'm not entirely sure why we're doing that in the case where the filesystem isn't mounted.

I agree that's too much. I think to address that I should change should_extend() into should_update() and have it return an enum result with two values, Extend and Update.

@mulkieran
Copy link
Member Author

ubuntu test failures look like infrastructure

@jbaublitz
Copy link
Member

I looked at this for a while and the only change I wanted to potentially discuss is that I'm a bit unclear on what the benefit is of running a diff in the case of a filesystem that isn't mounted. Is that just for the sake of code simplicity? I see how it's necessary to run the diff code even when the extend size is zero, but I'm not entirely sure why we're doing that in the case where the filesystem isn't mounted.

I agree that's too much. I think to address that I should change should_extend() into should_update() and have it return an enum result with two values, Extend and Update.

Why not just still used .filter_map() but only return None in the case that the filesystem isn't mounted? I feel like that still would fix the bug while also maybe being a little bit conceptually simpler than adding an enum.

@mulkieran
Copy link
Member Author

I looked at this for a while and the only change I wanted to potentially discuss is that I'm a bit unclear on what the benefit is of running a diff in the case of a filesystem that isn't mounted. Is that just for the sake of code simplicity? I see how it's necessary to run the diff code even when the extend size is zero, but I'm not entirely sure why we're doing that in the case where the filesystem isn't mounted.

I agree that's too much. I think to address that I should change should_extend() into should_update() and have it return an enum result with two values, Extend and Update.

Why not just still used .filter_map() but only return None in the case that the filesystem isn't mounted? I feel like that still would fix the bug while also maybe being a little bit conceptually simpler than adding an enum.

We need two bits of info to know whether the filesystem should just be visited or whether it should also be extended. I'm trying out calling extend_size from should_visit only if the filesystem should be extended and returning 0 if the filesystem should not be extended.

@mulkieran
Copy link
Member Author

@jbaublitz Ok. This is ready again.

@mulkieran mulkieran requested a review from mvollmer June 28, 2024 18:46
@mulkieran mulkieran marked this pull request as ready for review June 28, 2024 18:47
@mulkieran
Copy link
Member Author

@mvollmer We believe this fixes the bug you reported.

jbaublitz
jbaublitz approved these changes Jul 1, 2024
View reviewed changes
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.

After our discussion, this looks good to me.

@mulkieran
Merge invocation of should_extend and extend_size
4c1dd79 
Change should_extend and have it transitively invoke extend_size.

Make extend_size a private method that takes self.
* private because it is not called from an external function
* takes self to calculate thindev size and remaining size in case it has
  a filesystem limit.

Since strictly more threads are started than were previously, insert a
post-check on the change that has occurred in order to decide whether
metadata should be written or D-Bus signals sent.

Signed-off-by: mulhern <[email protected]>
@mulkieran mulkieran force-pushed the issue_stratisd_3634 branch from 15ca7b4 to 4c1dd79 Compare July 1, 2024 16:46
@mulkieran mulkieran changed the title Issue stratisd 3634 Ensured filesystem Used signal is sent if used value has changed on filesystem check Jul 1, 2024
@mulkieran
Copy link
Member Author

Testing farm tests are hanging...presumably this is an infrastructure problem. Merging...

@mulkieran mulkieran merged commit e6064c2 into stratis-storage:master Jul 1, 2024
41 of 44 checks passed
@mulkieran mulkieran deleted the issue_stratisd_3634 branch July 1, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaublitz jbaublitz jbaublitz approved these changes

@mvollmer mvollmer Awaiting requested review from mvollmer

Assignees

@mulkieran mulkieran

Labels
None yet
Projects
No open projects
2024July
Status: Done (1)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

D-Bus notification for org.storage.stratis3.filesystem.r*.Used is missing
2 participants
@mulkieran @jbaublitz