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

[mulkieran]

Closes #3634

[mulkieran]

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

[mulkieran]

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]

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

[mulkieran]

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]

ubuntu test failures look like infrastructure

[jbaublitz]

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]

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]

@jbaublitz Ok. This is ready again.

[mulkieran]

@mvollmer We believe this fixes the bug you reported.

[mulkieran]

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