Remove partial locking of paths when using high-level API

As described in https://github.com/libfuse/libfuse/issues/695 and below, partial
locking of paths can cause a deadlock. Partial locking was added to prevent
starvation, but it's unclear what specific cases of starvation were of concern.
As far as I was able to determine, since we support reader locks that give
priority to writers (to prevent starvation), this means that to starve the queue
element, we'd need a constant stream of queued requests that lock the path for
write. Write locks are used when the element is being (potentially) removed, so
this stream of requests that starve the `rename` or `lock` operations seems
unlikely.

### Summarizing issue #695

The high-level API handles locking of the node structures it maintains to
prevent concurrent requests from deleting nodes that are in use by other
requests. This means that requests that might remove these structs (`rmdir`,
`rename`, `unlink`, `link`) need to acquire an (internally managed - not
pthread) exclusive lock before doing so. In the case where the lock is already
held (for read or for write), the operation is placed onto a queue of waiters.
On every unlock, the queue is reinspected for any element that might now be able
to make progress.

Since `rename` and `link` involve two paths, when added to the queue, a single
queue entry requires that we lock two different paths. There was, prior to this
change, support for partially locking the first queue element if it had two
paths to lock. This partial locking can cause a deadlock:

- set up a situation where the first element in the queue is partially locked
  (such as by holding a reader lock on one of the paths being renamed, but not
  the other). For example: `/rmthis/foo/foo.txt` [not-yet-locked] and
  `/rmthis/bar/bar.txt` [locked]
- add an `rmdir` for an ancestor directory of the not-yet-locked path to the
  queue. In this example: `/rmthis`

After getting into this situation, we have the following `treelock` values:

- `/rmthis`: 1 current reader (due to the locked `/rmthis/bar/bar.txt`), one
  waiting writer (`rmdir`): no new readers will acquire a read lock here.
- `/rmthis/bar`: 1 reader (the locked `/rmthis/bar/bar.txt`)
- `/rmthis/bar/bar.txt`: 1 writer (the locked `/rmthis/bar/bar.txt`)

This is deadlocked, because the partial lock will never be able to be completely
locked, as doing so would require adding a reader lock on `/rmthis`, and that
will be rejected due to write lock requests having priority -- until the writer
succeeds in locking it, no new readers can be added. However, the writer (the
`rmdir`) will never be able to acquire its write lock, as the reader lock will
never be dropped -- there's no support for downgrading a partially locked
element to be unlocked, the only state change that's allowed involves it
becoming completely locked.

diff --git a/lib/fuse.c b/lib/fuse.c
index 606c3dd54d8c2ae16fb69ce5f584b7ad3bd5935a..04b371f20676355ef90b82f2aacdf192cde62710 100644 (file)
--- a/lib/fuse.c
+++ b/lib/fuse.c
@@ -80,8 +80,6 @@ struct lock_queue_element {
        char **path2;
        struct node **wnode2;
        int err;
-       bool first_locked : 1;
-       bool second_locked : 1;
        bool done : 1;
 };
 
@@ -1075,22 +1073,6 @@ static int try_get_path(struct fuse *f, fuse_ino_t nodeid, const char *name,
        return err;
 }
 
-static void queue_element_unlock(struct fuse *f, struct lock_queue_element *qe)
-{
-       struct node *wnode;
-
-       if (qe->first_locked) {
-               wnode = qe->wnode1 ? *qe->wnode1 : NULL;
-               unlock_path(f, qe->nodeid1, wnode, NULL);
-               qe->first_locked = false;
-       }
-       if (qe->second_locked) {
-               wnode = qe->wnode2 ? *qe->wnode2 : NULL;
-               unlock_path(f, qe->nodeid2, wnode, NULL);
-               qe->second_locked = false;
-       }
-}
-
 static int try_get_path2(struct fuse *f, fuse_ino_t nodeid1, const char *name1,
                         fuse_ino_t nodeid2, const char *name2,
                         char **path1, char **path2,
@@ -1115,7 +1097,6 @@ static int try_get_path2(struct fuse *f, fuse_ino_t nodeid1, const char *name1,
 static void queue_element_wakeup(struct fuse *f, struct lock_queue_element *qe)
 {
        int err;
-       bool first = (qe == f->lockq);
 
        if (!qe->path1) {
                /* Just waiting for it to be unlocked */
@@ -1125,44 +1106,21 @@ static void queue_element_wakeup(struct fuse *f, struct lock_queue_element *qe)
                return;
        }
 
-       if (!qe->first_locked) {
+       if (qe->done)
+               return;  // Don't try to double-lock the element
+
+       if (!qe->path2) {
                err = try_get_path(f, qe->nodeid1, qe->name1, qe->path1,
                                   qe->wnode1, true);
-               if (!err)
-                       qe->first_locked = true;
-               else if (err != -EAGAIN)
-                       goto err_unlock;
-       }
-       if (!qe->second_locked && qe->path2) {
-               err = try_get_path(f, qe->nodeid2, qe->name2, qe->path2,
-                                  qe->wnode2, true);
-               if (!err)
-                       qe->second_locked = true;
-               else if (err != -EAGAIN)
-                       goto err_unlock;
-       }
-
-       if (qe->first_locked && (qe->second_locked || !qe->path2)) {
-               err = 0;
-               goto done;
+       } else {
+               err = try_get_path2(f, qe->nodeid1, qe->name1, qe->nodeid2,
+                                   qe->name2, qe->path1, qe->path2, qe->wnode1,
+                                   qe->wnode2);
        }
 
-       /*
-        * Only let the first element be partially locked otherwise there could
-        * be a deadlock.
-        *
-        * But do allow the first element to be partially locked to prevent
-        * starvation.
-        */
-       if (!first)
-               queue_element_unlock(f, qe);
-
-       /* keep trying */
-       return;
+       if (err == -EAGAIN)
+               return;  /* keep trying */
 
-err_unlock:
-       queue_element_unlock(f, qe);
-done:
        qe->err = err;
        qe->done = true;
        pthread_cond_signal(&qe->cond);
@@ -1200,8 +1158,6 @@ static void queue_path(struct fuse *f, struct lock_queue_element *qe)
        struct lock_queue_element **qp;
 
        qe->done = false;
-       qe->first_locked = false;
-       qe->second_locked = false;
        pthread_cond_init(&qe->cond, NULL);
        qe->next = NULL;
        for (qp = &f->lockq; *qp != NULL; qp = &(*qp)->next);