Dynamically scale fuse threads #411

passaro · 2023-07-26T15:21:24Z

Description of change

Introduce a pool of fuse worker threads that will scale dynamically up to a max_workers limit when receiving kernel requests.

The implementation relies on the following changes in fuser/fork:

Session::run_with_callbacks(): version of Session::run() with callbacks before/after dispatch.
Request::is_forget(): we do not want to spawn new threads on spikes of forget/batch_forget requests.

Relevant issues: #7

Does this change impact existing behavior?

New --max-threads option replaces --thread-count and sets the maximum number of threads to spawn.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

jamesbornholt

If you want to think more about testing, you could factor out the Session::run part of the worker pool into a shared closure, and write a stress test with a different run function. Could even use shuttle for that if you were inclined.

jamesbornholt · 2023-07-26T19:37:14Z

vendor/fuser/src/session.rs

+    /// Run the session loop that receives kernel requests and dispatches them to method
+    /// calls into the filesystem.
+    /// Version with before/after_dispatch callbacks. TODO: review/refactor
+    pub fn run_with_callbacks<FA, FB>(&self, mut before_dispatch: FB, mut after_dispatch: FA) -> io::Result<()> 


Makes sense to me, but we should change run to just call this with empty callbacks.

jamesbornholt · 2023-07-26T19:41:50Z

mountpoint-s3/src/fuse/session.rs

+    state: Arc<WorkerPoolState<FS>>,
+    workers: Sender<WorkerHandle>,
+    max_workers: usize,
+    max_idle_workers: Option<usize>,


Kind of think we shouldn't bother with the scaling down idle workers stuff -- in practice threads are cheap (the CRT already spawns a bunch) and it's a tricky balance to decide when to scale down. The real goal here is just to make concurrent workloads work automatically, which only scaling upwards should achieve.

(libfuse encourages not setting a max idle workers)

Scaling down removed

jamesbornholt · 2023-07-26T19:51:30Z

mountpoint-s3/src/fuse/session.rs

+                if !req.is_forget() {
+                    self.state.idle_worker_count.fetch_sub(1, Ordering::SeqCst);
+                }
+
+                if self.state.idle_worker_count.load(Ordering::SeqCst) == 0 {
+                    if let Err(error) = self.try_add_worker() {
+                        warn!(?error, "unable to spawn fuse worker");
+                    }
+                }


The decrement and load need to happen atomically here, otherwise two racing requests could both see 0 and spawn two threads instead of one. fetch_sub returns the previous value, so probably just move the whole thing inside if !req.is_forget() and compare that to 1 to see if you should spawn something.

Also, a comment on why we exclude forget.

jamesbornholt · 2023-07-26T19:58:18Z

mountpoint-s3/src/fuse/session.rs

+        if !pool.try_add_worker()? {
+            return Err(anyhow::anyhow!("reached max worker threads"));
+        }


Should be impossible because we asserted max_workers > 0, right?

jamesbornholt · 2023-07-26T20:02:42Z

mountpoint-s3/src/main.rs

@@ -135,7 +135,7 @@ struct CliArgs {

    #[clap(
        long,
-        help = "Number of FUSE daemon threads",
+        help = "Maximum number of FUSE daemon threads",


should probably rename the actual flag to --max-threads too

passaro · 2023-07-27T10:56:25Z

Factored out the run part and added a test for WorkerPool, but I'm sure it could be improved.

jamesbornholt

Just a couple suggestions. The fuser changes look good to me, but we don't commit those directly to mainline because we've been trying to maintain fuser history separately. Instead, pull those into their own commit on the fuser/fork branch (you can push that without reviewing), and then run vendor-fuser.sh on the PR branch to pull the change in.

jamesbornholt · 2023-07-28T05:27:05Z

mountpoint-s3/src/fuse/session.rs

+    #[test_case(10, 10)]
+    #[test_case(10, 30)]
+    #[test_case(30, 10)]
+    fn test_worker_pool(max_worker_threads: usize, concurrent_messages: usize) {


It took me a long time to parse what this test is doing, so probably deserves a comment. Here's my understanding: it tests that the spawning logic never under-spawns threads, by assigning each worker thread a work item that only completes when a flag is flipped, and then arranges for the flag to flip only once max_worker_threads have been spawned. Neat!

I thought maybe you could use std::sync::Barrier to make this simpler (set the barrier to max_worker_threads + 1), but I guess it doesn't have a wait_timeout so you'd block forever if the test broke.

Also, I think we should write one other, simpler test: the work item is just incrementing a shared counter. We check that the counter got incremented exactly as many times as we expected, and that the number of spawned threads was no greater than max_worker_threads.

jamesbornholt · 2023-07-28T05:33:02Z

mountpoint-s3/src/fuse/session.rs

+        } else {
+            assert_eq!(workers.len(), min_expected_workers);
+        }
+    }


Wasn't too hard to run this to a Shuttle test as well:

#[cfg(feature = "shuttle")] mod shuttle_tests { use shuttle::rand::Rng; use shuttle::{check_pct, check_random}; fn test_worker_pool_helper() { let mut rng = shuttle::rand::thread_rng(); let num_worker_threads = rng.gen_range(1..=8); let num_concurrent_messages = rng.gen_range(1..=16); super::test_worker_pool(num_worker_threads, num_concurrent_messages); } #[test] fn test_worker_pool() { check_random(test_worker_pool_helper, 10000); check_pct(test_worker_pool_helper, 10000, 3); } }

Thanks! Added together with the new test.

jamesbornholt · 2023-07-28T05:36:27Z

mountpoint-s3/src/fuse/session.rs

+trait Work: Send + Sync + 'static {
+    type Result: Send;
+
+    fn run<FB, FA>(&self, before: FB, after: FA) -> Self::Result


Add a brief comment about the semantics of this function.

jamesbornholt · 2023-07-28T05:38:43Z

vendor/fuser/src/session.rs

+    /// calls into the filesystem.
+    /// This version also notifies callers of kernel requests before and after they
+    /// are dispatched to the filesystem.
+    pub fn run_and_notify<FA, FB>(&self, mut before_dispatch: FB, mut after_dispatch: FA) -> io::Result<()> 


naming nit: I'd probably call it run_with_callbacks or something.

edit: lol, that's exactly what you called it the first time around

jamesbornholt · 2023-07-28T05:44:38Z

mountpoint-s3/src/main.rs

@@ -135,13 +135,13 @@ struct CliArgs {

    #[clap(
        long,
-        help = "Number of FUSE daemon threads",
+        help = "Maximum number of FUSE daemon threads",
        value_name = "N",
        default_value = "1",


Let's make the default here 16 (just a number I made up, feel free to make up your own).

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

passaro temporarily deployed to PR integration tests July 26, 2023 15:21 — with GitHub Actions Inactive

passaro added the performance PRs to run benchmarks on label Jul 26, 2023

passaro temporarily deployed to PR benchmarks July 26, 2023 15:21 — with GitHub Actions Inactive

jamesbornholt reviewed Jul 26, 2023

View reviewed changes

passaro had a problem deploying to PR benchmarks July 27, 2023 10:53 — with GitHub Actions Failure

passaro temporarily deployed to PR integration tests July 27, 2023 10:53 — with GitHub Actions Inactive

passaro had a problem deploying to PR benchmarks July 27, 2023 16:36 — with GitHub Actions Failure

passaro temporarily deployed to PR integration tests July 27, 2023 16:36 — with GitHub Actions Inactive

passaro temporarily deployed to PR benchmarks July 27, 2023 16:36 — with GitHub Actions Inactive

passaro temporarily deployed to PR integration tests July 27, 2023 16:36 — with GitHub Actions Inactive

jamesbornholt reviewed Jul 28, 2023

View reviewed changes

passaro added 4 commits July 28, 2023 11:09

Update vendored fuser to 07f1987

61c17d4

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

Dynamically scale fuse threads

c9aa620

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

Replace --thread-count with --max-threads

5479a33

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

Update benchmark scripts

5089ae4

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

passaro force-pushed the dynamic-fuse-threads branch from 72a85ae to 5089ae4 Compare July 28, 2023 10:40

passaro temporarily deployed to PR integration tests July 28, 2023 10:40 — with GitHub Actions Inactive

passaro temporarily deployed to PR benchmarks July 28, 2023 10:40 — with GitHub Actions Inactive

passaro temporarily deployed to PR integration tests July 28, 2023 10:40 — with GitHub Actions Inactive

passaro temporarily deployed to PR benchmarks July 28, 2023 10:40 — with GitHub Actions Inactive

passaro temporarily deployed to PR integration tests July 28, 2023 10:40 — with GitHub Actions Inactive

passaro marked this pull request as ready for review July 28, 2023 10:57

jamesbornholt approved these changes Jul 28, 2023

View reviewed changes

jamesbornholt added this pull request to the merge queue Jul 28, 2023

Merged via the queue into awslabs:main with commit d6b530f Jul 28, 2023

passaro deleted the dynamic-fuse-threads branch July 29, 2023 03:59

jamesbornholt mentioned this pull request Aug 30, 2023

Readahead reordering causes prefetcher resets #488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically scale fuse threads #411

Dynamically scale fuse threads #411

passaro commented Jul 26, 2023 •

edited

Loading

jamesbornholt left a comment

jamesbornholt Jul 26, 2023

jamesbornholt Jul 26, 2023

passaro Jul 27, 2023

jamesbornholt Jul 26, 2023

jamesbornholt Jul 26, 2023

jamesbornholt Jul 26, 2023

jamesbornholt Jul 26, 2023

passaro commented Jul 27, 2023 •

edited

Loading

jamesbornholt left a comment

jamesbornholt Jul 28, 2023

jamesbornholt Jul 28, 2023

jamesbornholt Jul 28, 2023

passaro Jul 28, 2023

jamesbornholt Jul 28, 2023

jamesbornholt Jul 28, 2023 •

edited

Loading

passaro Jul 28, 2023

jamesbornholt Jul 28, 2023 •

edited

Loading

Dynamically scale fuse threads #411

Dynamically scale fuse threads #411

Conversation

passaro commented Jul 26, 2023 • edited Loading

Description of change

Does this change impact existing behavior?

jamesbornholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

passaro commented Jul 27, 2023 • edited Loading

jamesbornholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesbornholt Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesbornholt Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

passaro commented Jul 26, 2023 •

edited

Loading

passaro commented Jul 27, 2023 •

edited

Loading

jamesbornholt Jul 28, 2023 •

edited

Loading

jamesbornholt Jul 28, 2023 •

edited

Loading