fuse-loop/fuse_do_work: Avoid lots of thread creations/destructions

On benchmarking metadata operations with a single threaded bonnie++
and "max_idle_threads" limited to 1, 'top' was showing suspicious
160% cpu usage.
Profiling the system with flame graphs showed that an astonishing
amount of CPU time was spent in thread creation and destruction.

After verifying the code it turned out that fuse_do_work() was
creating a new thread every time all existing idle threads
were already busy. And then just a few lines later after processing
the current request it noticed that it had created too many threads
and destructed the current thread. I.e. there was a thread
creation/destruction ping-pong.

Code is changed to only create new threads if the max number of
threads is not reached.

Furthermore, thread destruction is disabled, as creation/destruction
is expensive in general.

With this change cpu usage of passthrough_hp went from ~160% to
~80% (with different values of max_idle_threads). And bonnie
values got approximately faster by 90%. This is a with single
threaded bonnie++
bonnie++ -x 4 -q -s0  -d <path> -n 30:1:1:10 -r 0

Without this patch, using the default max_idle_threads=10 and just
a single bonnie++ the thread creation/destruction code path is not
triggered.  Just one libfuse and one application thread is just
a corner case - the requirement for the issue was just
n-application-threads >= max_idle_threads.


Signed-off-by: Bernd Schubert <bschubert@ddn.com>

7 files changed, 117 insertions, 14 deletions

diff --git a/example/passthrough_hp.cc b/example/passthrough_hp.cc
index 5c61928..e86da5d 100644
--- a/example/passthrough_hp.cc
+++ b/example/passthrough_hp.cc
@@ -43,7 +43,7 @@
  * \include passthrough_hp.cc
  */
 
-#define FUSE_USE_VERSION 35
+#define FUSE_USE_VERSION FUSE_MAKE_VERSION(3, 12)
 
 #ifdef HAVE_CONFIG_H
 #include "config.h"
@@ -1228,6 +1228,8 @@ static void maximize_fd_limit() {
 
 int main(int argc, char *argv[]) {
 
+    struct fuse_loop_config *loop_config = NULL;
+
     // Parse command line options
     auto options {parse_options(argc, argv)};
 
@@ -1274,15 +1276,15 @@ int main(int argc, char *argv[]) {
     umask(0);
 
     // Mount and run main loop
-    struct fuse_loop_config loop_config;
-    loop_config.clone_fd = 0;
-    loop_config.max_idle_threads = 10;
+    loop_config = fuse_loop_cfg_create();
+
     if (fuse_session_mount(se, argv[2]) != 0)
         goto err_out3;
     if (options.count("single"))
         ret = fuse_session_loop(se);
     else
-        ret = fuse_session_loop_mt(se, &loop_config);
+        ret = fuse_session_loop_mt(se, loop_config);
+
 
     fuse_session_unmount(se);
 
@@ -1291,6 +1293,8 @@ err_out3:
 err_out2:
     fuse_session_destroy(se);
 err_out1:
+
+    fuse_loop_cfg_destroy(loop_config);
     fuse_opt_free_args(&args);
 
     return ret ? 1 : 0;
diff --git a/include/fuse_common.h b/include/fuse_common.h
index b40814f..e9d8745 100644
--- a/include/fuse_common.h
+++ b/include/fuse_common.h
@@ -857,13 +857,19 @@ struct fuse_loop_config *fuse_loop_cfg_create(void);
 void fuse_loop_cfg_destroy(struct fuse_loop_config *config);
 
 /**
- * fuse_loop_config2 setter to set the number of max idle threads.
+ * fuse_loop_config setter to set the number of max idle threads.
  */
 void fuse_loop_cfg_set_idle_threads(struct fuse_loop_config *config,
 				    unsigned int value);
 
 /**
- * fuse_loop_config2 setter to enable the clone_fd feature
+ * fuse_loop_config setter to set the number of max threads.
+ */
+void fuse_loop_cfg_set_max_threads(struct fuse_loop_config *config,
+				   unsigned int value);
+
+/**
+ * fuse_loop_config setter to enable the clone_fd feature
  */
 void fuse_loop_cfg_set_clone_fd(struct fuse_loop_config *config,
 				unsigned int value);
diff --git a/include/fuse_lowlevel.h b/include/fuse_lowlevel.h
index 378742d..53f0fcf 100644
--- a/include/fuse_lowlevel.h
+++ b/include/fuse_lowlevel.h
@@ -1868,6 +1868,11 @@ void fuse_cmdline_help(void);
  * Filesystem setup & teardown                                 *
  * ----------------------------------------------------------- */
 
+/**
+ * Note: Any addition to this struct needs to create a compatibility symbol
+ *       for fuse_parse_cmdline(). For ABI compatibility reasons it is also
+ *       not possible to remove struct members.
+ */
 struct fuse_cmdline_opts {
 	int singlethread;
 	int foreground;
@@ -1877,7 +1882,11 @@ struct fuse_cmdline_opts {
 	int show_version;
 	int show_help;
 	int clone_fd;
-	unsigned int max_idle_threads;
+	unsigned int max_idle_threads; /* discouraged, due to thread
+	                                * destruct overhead */
+
+	/* Added in libfuse-3.12 */
+	unsigned int max_threads;
 };
 
 /**
@@ -1898,8 +1907,20 @@ struct fuse_cmdline_opts {
  * @param opts output argument for parsed options
  * @return 0 on success, -1 on failure
  */
+#if (!defined(__UCLIBC__) && !defined(__APPLE__))
 int fuse_parse_cmdline(struct fuse_args *args,
 		       struct fuse_cmdline_opts *opts);
+#else
+#if FUSE_USE_VERSION < FUSE_MAKE_VERSION(3, 12)
+int fuse_parse_cmdline_30(struct fuse_args *args,
+			   struct fuse_cmdline_opts *opts);
+#define fuse_parse_cmdline(args, opts) fuse_parse_cmdline_30(args, opts)
+#else
+int fuse_parse_cmdline_312(struct fuse_args *args,
+			   struct fuse_cmdline_opts *opts);
+#define fuse_parse_cmdline(args, opts) fuse_parse_cmdline_312(args, opts)
+#endif
+#endif
 
 /**
  * Create a low level session.
diff --git a/lib/fuse_i.h b/lib/fuse_i.h
index 6930a20..42f0e5f 100644
--- a/lib/fuse_i.h
+++ b/lib/fuse_i.h
@@ -128,6 +128,13 @@ struct fuse_loop_config
 	 * The special value of -1 means that this parameter is disabled.
 	 */
 	int max_idle_threads;
+
+	/**
+	 *  max number of threads taking and processing kernel requests
+	 *
+	 *  As of now threads are created dynamically
+	 */
+	unsigned int max_threads;
 };
 #endif
 
diff --git a/lib/fuse_loop_mt.c b/lib/fuse_loop_mt.c
index 2f0470b..9ec1fb2 100644
--- a/lib/fuse_loop_mt.c
+++ b/lib/fuse_loop_mt.c
@@ -31,6 +31,7 @@
 
 #define FUSE_LOOP_MT_V2_IDENTIFIER	 INT_MAX - 2
 #define FUSE_LOOP_MT_DEF_CLONE_FD	 0
+#define FUSE_LOOP_MT_DEF_MAX_THREADS 10
 #define FUSE_LOOP_MT_DEF_IDLE_THREADS -1 /* thread destruction is disabled
                                           * by default */
 
@@ -57,6 +58,7 @@ struct fuse_mt {
 	int error;
 	int clone_fd;
 	int max_idle;
+	int max_threads;
 };
 
 static struct fuse_chan *fuse_chan_new(int fd)
@@ -161,7 +163,7 @@ static void *fuse_do_work(void *data)
 
 		if (!isforget)
 			mt->numavail--;
-		if (mt->numavail == 0)
+		if (mt->numavail == 0 && mt->numworker < mt->max_threads)
 			fuse_loop_start_thread(mt);
 		pthread_mutex_unlock(&mt->lock);
 
@@ -170,7 +172,14 @@ static void *fuse_do_work(void *data)
 		pthread_mutex_lock(&mt->lock);
 		if (!isforget)
 			mt->numavail++;
-		if (mt->numavail > mt->max_idle) {
+
+		/* creating and destroying threads is rather expensive - and there is
+		 * not much gain from destroying existing threads. It is therefore
+		 * discouraged to set max_idle to anything else than -1. If there
+		 * is indeed a good reason to destruct threads it should be done
+		 * delayed, a moving average might be useful for that.
+		 */
+		if (mt->max_idle != -1 && mt->numavail > mt->max_idle) {
 			if (mt->exit) {
 				pthread_mutex_unlock(&mt->lock);
 				return NULL;
@@ -202,7 +211,10 @@ int fuse_start_thread(pthread_t *thread_id, void *(*func)(void *), void *arg)
 	pthread_attr_t attr;
 	char *stack_size;
 
-	/* Override default stack size */
+	/* Override default stack size
+	 * XXX: This should ideally be a parameter option. It is rather
+	 *      well hidden here.
+	 */
 	pthread_attr_init(&attr);
 	stack_size = getenv(ENVNAME_THREAD_STACK);
 	if (stack_size && pthread_attr_setstacksize(&attr, atoi(stack_size)))
@@ -328,6 +340,7 @@ int err;
 	mt.numworker = 0;
 	mt.numavail = 0;
 	mt.max_idle = config->max_idle_threads;
+	mt.max_threads = config->max_threads;
 	mt.main.thread_id = pthread_self();
 	mt.main.prev = mt.main.next = &mt.main;
 	sem_init(&mt.finish, 0, 0);
@@ -400,6 +413,7 @@ struct fuse_loop_config *fuse_loop_cfg_create(void)
 
 	config->version_id       = FUSE_LOOP_MT_V2_IDENTIFIER;
 	config->max_idle_threads = FUSE_LOOP_MT_DEF_IDLE_THREADS;
+	config->max_threads      = FUSE_LOOP_MT_DEF_MAX_THREADS;
 	config->clone_fd         = FUSE_LOOP_MT_DEF_CLONE_FD;
 
 	return config;
@@ -432,6 +446,12 @@ void fuse_loop_cfg_set_idle_threads(struct fuse_loop_config *config,
 	config->max_idle_threads = value;
 }
 
+void fuse_loop_cfg_set_max_threads(struct fuse_loop_config *config,
+				   unsigned int value)
+{
+	config->max_threads = value;
+}
+
 void fuse_loop_cfg_set_clone_fd(struct fuse_loop_config *config,
 				unsigned int value)
 {
diff --git a/lib/fuse_versionscript b/lib/fuse_versionscript
index aff7e84..7e50e75 100644
--- a/lib/fuse_versionscript
+++ b/lib/fuse_versionscript
@@ -178,8 +178,12 @@ FUSE_3.12 {
 		fuse_loop_cfg_create;
 		fuse_loop_cfg_destroy;
 		fuse_loop_cfg_set_idle_threads;
+		fuse_loop_cfg_set_max_threads;
 		fuse_loop_cfg_set_clone_fd;
 		fuse_loop_cfg_convert;
+		fuse_parse_cmdline;
+		fuse_parse_cmdline_30;
+		fuse_parse_cmdline_312;
 } FUSE_3.4;
 
 # Local Variables:
diff --git a/lib/helper.c b/lib/helper.c
index fc6a6ee..ea5f87d 100644
--- a/lib/helper.c
+++ b/lib/helper.c
@@ -50,6 +50,7 @@ static const struct fuse_opt fuse_helper_opts[] = {
 #endif
 	FUSE_HELPER_OPT("clone_fd",	clone_fd),
 	FUSE_HELPER_OPT("max_idle_threads=%u", max_idle_threads),
+	FUSE_HELPER_OPT("max_threads=%u", max_threads),
 	FUSE_OPT_END
 };
 
@@ -136,6 +137,8 @@ void fuse_cmdline_help(void)
 	       "    -o clone_fd            use separate fuse device fd for each thread\n"
 	       "                           (may improve performance)\n"
 	       "    -o max_idle_threads    the maximum number of idle worker threads\n"
+	       "                           allowed (default: -1)\n"
+	       "    -o max_threads         the maximum number of worker threads\n"
 	       "                           allowed (default: 10)\n");
 }
 
@@ -199,12 +202,16 @@ static int add_default_subtype(const char *progname, struct fuse_args *args)
 	return res;
 }
 
-int fuse_parse_cmdline(struct fuse_args *args,
-		       struct fuse_cmdline_opts *opts)
+int fuse_parse_cmdline_312(struct fuse_args *args,
+			   struct fuse_cmdline_opts *opts);
+FUSE_SYMVER("fuse_parse_cmdline_312", "fuse_parse_cmdline@@FUSE_3.12")
+int fuse_parse_cmdline_312(struct fuse_args *args,
+			   struct fuse_cmdline_opts *opts)
 {
 	memset(opts, 0, sizeof(struct fuse_cmdline_opts));
 
-	opts->max_idle_threads = 10;
+	opts->max_idle_threads = -1; /* new default in fuse version 3.12 */
+	opts->max_threads = 10;
 
 	if (fuse_opt_parse(args, opts, fuse_helper_opts,
 			   fuse_helper_opt_proc) == -1)
@@ -221,6 +228,40 @@ int fuse_parse_cmdline(struct fuse_args *args,
 	return 0;
 }
 
+/**
+ * struct fuse_cmdline_opts got extended in libfuse-3.12
+ */
+int fuse_parse_cmdline_30(struct fuse_args *args,
+		       struct fuse_cmdline_opts *opts);
+FUSE_SYMVER("fuse_parse_cmdline_37", "fuse_parse_cmdline@FUSE_3.0")
+int fuse_parse_cmdline_30(struct fuse_args *args,
+			  struct fuse_cmdline_opts *out_opts)
+{
+	struct fuse_cmdline_opts opts;
+
+
+	int rc = fuse_parse_cmdline_312(args, &opts);
+	if (rc == 0) {
+		/* copy up to the size of the old pre 3.12 struct */
+		memcpy(out_opts, &opts,
+		       offsetof(struct fuse_cmdline_opts, max_idle_threads) +
+		       sizeof(opts.max_idle_threads));
+	}
+
+	return rc;
+}
+
+/**
+ * Compatibility ABI symbol for systems that do not support version symboling
+ */
+#if (defined(__UCLIBC__) || defined(__APPLE__))
+int fuse_parse_cmdline(struct fuse_args *args,
+		       struct fuse_cmdline_opts *opts)
+{
+	return fuse_parse_cmdline_30(args, out_opts);
+}
+#endif
+
 
 int fuse_daemonize(int foreground)
 {