afs: Return to userspace after AFS_NEW_BKG reqs 84/13984/4
authorAndrew Deason <adeason@sinenomine.net>
Fri, 13 Dec 2019 03:00:20 +0000 (21:00 -0600)
committerBenjamin Kaduk <kaduk@mit.edu>
Fri, 23 Oct 2020 15:16:47 +0000 (11:16 -0400)
Currently, for AFS_NEW_BKG, background daemons run in the context of a
normal user process (afsd), in order to return to run
userspace-handled background ops. For non-AFS_NEW_BKG when
AFS_DAEMONOP_ENV is defined, background daemons run as kernel threads
instead, and have no corresponding userspace process.

On LINUX, whether or not we run as a kernel thread has some odd
side-effects: at least one example of this is how open file handles
(struct file) are treated when closed. When the last reference to a
struct file is closed, the final free is deferred to an asynchronous
call to be executed "later", in order to avoid issues with lock
inversion. For kernel threads, "later" means the work is schedule on
the global system work queue (schedule_work()), but for userspace
processes, it is scheduled on the task work queue (task_work_add()),
which is run around when the thread returns to userspace. For
background daemons, we never return from the relevant syscall until we
get a userspace background request (or the client is shutting down),

Commit ca472e66 (LINUX: Turn on AFS_NEW_BKG) changed LINUX to use
AFS_NEW_BKG background daemons, so background requests now run as a
normal userspace process, and last-reference file closes are deferred.
Since we may never return to userspace, this means that our file
handles (used for accessing the disk cache) may never get freed,
leading to an unbounded number of file handles remaining open.

This can be seen by seeing the first value in /proc/sys/fs/file-nr
growing without bound (possibly slowly), as accessing /afs causes
background requests. Eventually the number of open files can exceed
the /proc/sys/fs/file-max limit, causing further file opens to fail,
causing various other problems and potentially panics.

To avoid this issue, define a new userspace background op, called
AFS_USPC_NOOP, which gets returned to the afsd background daemon
process. When afsd sees this, it just does nothing and calls the
AFSOP_BKG_HANDLER syscall again, to go into the background daemon loop
again. In afs_BackgroundDaemon, we return the AFS_USPC_NOOP op
whenever there are no pending background requests, or if we've run 100
background requests in a row without a break. This causes us to return
to userspace periodically, preventing any such task-queued work from
building up indefinitely.

Do this for all platforms (currently just LINUX and DARWIN), in order
to simplify the code, and possibly avoid other similar issues, since
staying inside a syscall for so long while doing real work is arguably
weird.

Add a documentation comment block for afs_BackgroundDaemon while we're
here.

Thanks to mvitale@sinenomine.net for discovering the file leak.

Change-Id: I1953d73b2142d4128b064f8c5f95a5858d7df131
Reviewed-on: https://gerrit.openafs.org/13984
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

src/afs/afs_daemons.c
src/afsd/afsd.c
src/config/afs_args.h

index 07139c4..8913958 100644 (file)
@@ -1006,6 +1006,44 @@ brequest_release(struct brequest *tb)
 }
 
 #ifdef AFS_NEW_BKG
+static_inline int
+should_do_noop(int foundAny, int n_processed)
+{
+    if (!foundAny && n_processed > 0) {
+       /* If there aren't any requests right now, and we've processed
+        * at least one request, do a noop. */
+       return 1;
+    } else if (foundAny && n_processed > 100) {
+       /* If we've processed over 100 requests in a row, do a noop. */
+       return 1;
+    }
+    return 0;
+}
+#endif
+
+/**
+ * Entry point for background daemon processes.
+ *
+ * For old-style background daemons (non-AFS_NEW_BKG), a background daemon afsd
+ * process will end up in this function, and it will stay in here forever
+ * processing in-kernel bkg requests until the client shuts down.
+ *
+ * For new-style background daemons (AFS_NEW_BKG), we can pass data back to
+ * afsd to perform some background operations in userspace, by populating
+ * 'uspc' with the operation to perform and then returning. When the afsd
+ * process enters this function again, the return code for that operation is
+ * also provided in 'uspc'.
+ *
+ * @param[inout] uspc   Userspace operation data. If uspc->ts is non-negative
+ *                      on entry, the related background request has finished,
+ *                      and we're providing the return code. On return,
+ *                      contains the userspace operation to perform.
+ * @param[inout] param1 Operation-specific pointer.
+ * @param[inout] param2 Operation-specific pointer.
+ *
+ * @return  Always returns 0.
+ */
+#ifdef AFS_NEW_BKG
 int
 afs_BackgroundDaemon(struct afs_uspc_param *uspc, void *param1, void *param2)
 #else
@@ -1015,6 +1053,7 @@ afs_BackgroundDaemon(void)
 {
     struct brequest *tb;
     int i, foundAny;
+    int n_processed = 0;
 
     AFS_STATCNT(afs_BackgroundDaemon);
     /* initialize subsystem */
@@ -1023,8 +1062,13 @@ afs_BackgroundDaemon(void)
        afs_BackgroundDaemon_once();
 
 #ifdef AFS_NEW_BKG
-    /* If it's a re-entering syscall, complete the request and release */
-    if (uspc->ts > -1) {
+    if (uspc->reqtype == AFS_USPC_NOOP) {
+       /* The daemon is re-entering from a noop, not actually returning data;
+        * don't look for an existing request. */
+       /* noop */
+
+    } else if (uspc->ts > -1) {
+       /* If it's a re-entering syscall, complete the request and release */
         tb = afs_brs;
         for (i = 0; i < NBRS; i++, tb++) {
             if (tb->ts == uspc->ts) {
@@ -1086,6 +1130,7 @@ afs_BackgroundDaemon(void)
            tb->flags |= BSTARTED;
            ReleaseWriteLock(&afs_xbrs);
            foundAny = 1;
+           n_processed++;
            afs_Trace1(afs_iclSetp, CM_TRACE_BKG1, ICL_TYPE_INT32,
                       tb->opcode);
            if (tb->opcode == BOP_FETCH)
@@ -1120,6 +1165,16 @@ afs_BackgroundDaemon(void)
            brequest_release(tb);
            ObtainWriteLock(&afs_xbrs, 305);
        }
+
+#ifdef AFS_NEW_BKG
+       if (should_do_noop(foundAny, n_processed)) {
+           ReleaseWriteLock(&afs_xbrs);
+           memset(uspc, 0, sizeof(*uspc));
+           uspc->reqtype = AFS_USPC_NOOP;
+           return 0;
+       }
+#endif
+
        if (!foundAny) {
            /* wait for new request */
            afs_brsDaemons++;
index 2d37aa1..4df6f7b 100644 (file)
@@ -1563,6 +1563,12 @@ BkgHandler(void)
            /* Client is shutting down */
            return;
 
+       case AFS_USPC_NOOP:
+           /* noop */
+           memset(srcName, 0, sizeof(srcName));
+           memset(dstName, 0, sizeof(dstName));
+           break;
+
 # ifdef AFS_DARWIN_ENV
        case AFS_USPC_UMV:
             {
index 3ea8af3..af09c81 100644 (file)
@@ -149,6 +149,7 @@ struct afs_umv_param {
 # define AFS_USPC_UMV 1
 #endif
 #define AFS_USPC_SHUTDOWN 2
+#define AFS_USPC_NOOP 3
 
 struct afs_uspc_param {
     afs_int32 retval;