[PATCH 0/7] fuse: Improve disconnect scheme and avoid taking fpq->lock on hot paths

[PATCH 0/7] fuse: Improve disconnect scheme and avoid taking fpq->lock on hot paths

[Kirill Tkhai]

There is no a reason to set individual FR_ABORTED state
for every request, since fuse_abort_conn() aborts all
unlocked requests at once. FR_ABORTED bit and fpq->io
list just allow fuse_copy_aborted() to end some of requests,
which are in the middle of fuse_dev_do_read() and
fuse_dev_do_write(). But this is not a big deal, since
these functions abort the requests themselves.

This patchset introduces a better scheme for fuse_abort_conn(),
which allows to remove excess flags and fpq->io, and optimizes
hot paths fuse_dev_do_read() and fuse_dev_do_write() by avoiding
taking fpq->lock there.

---

Kirill Tkhai (7):
      fuse: Check for fc->connected in fuse_dev_alloc()
      fuse: Move flush_bg_queue() up in fuse_abort_conn()
      fuse: Drop and reacquire fc->lock in middle of fuse_abort_conn()
      fuse: Add fud pointer to struct fuse_copy_state
      fuse: Introduce generic fuse_copy_aborted()
      fuse: Kill unused FR_ABORTED, FR_LOCKED and FR_PRIVATE flags
      fuse: Kill fuse_pqueue::io list and avoid taking fpq->lock on hot paths


 fs/fuse/dev.c    |  131 ++++++++++++++++--------------------------------------
 fs/fuse/fuse_i.h |   20 ++------
 fs/fuse/inode.c  |   10 ++++
 3 files changed, 52 insertions(+), 109 deletions(-)

--
Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>

[PATCH 1/7] fuse: Check for fc->connected in fuse_dev_alloc()

[Kirill Tkhai]

fuse_dev_alloc() may be called after fc->connected
is dropped (from ioctl), so here we add sanity check
for that case.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/inode.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 336844d0eb3a..0361a3d62356 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1054,10 +1054,19 @@ struct fuse_dev *fuse_dev_alloc(struct fuse_conn *fc)
 	fuse_pqueue_init(&fud->pq);
 
 	spin_lock(&fc->lock);
+	if (!fc->connected) {
+		spin_unlock(&fc->lock);
+		goto out_put;
+	}
 	list_add_tail(&fud->entry, &fc->devices);
 	spin_unlock(&fc->lock);
 
 	return fud;
+out_put:
+	fuse_conn_put(fc);
+	kfree(pq);
+	kfree(fud);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(fuse_dev_alloc);
 

[PATCH 2/7] fuse: Move flush_bg_queue() up in fuse_abort_conn()

[Kirill Tkhai]

Preparation for next patches.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/dev.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 8a63e52785e9..dd8f019447a9 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2191,6 +2191,9 @@ void fuse_abort_conn(struct fuse_conn *fc)
 		/* Background queuing checks fc->connected under bg_lock */
 		spin_lock(&fc->bg_lock);
 		fc->connected = 0;
+		fc->blocked = 0;
+		fc->max_background = UINT_MAX;
+		flush_bg_queue(fc);
 		spin_unlock(&fc->bg_lock);
 
 		fuse_set_initialized(fc);
@@ -2215,11 +2218,6 @@ void fuse_abort_conn(struct fuse_conn *fc)
 						      &to_end);
 			spin_unlock(&fpq->lock);
 		}
-		spin_lock(&fc->bg_lock);
-		fc->blocked = 0;
-		fc->max_background = UINT_MAX;
-		flush_bg_queue(fc);
-		spin_unlock(&fc->bg_lock);
 
 		spin_lock(&fiq->waitq.lock);
 		fiq->connected = 0;

[PATCH 3/7] fuse: Drop and reacquire fc->lock in middle of fuse_abort_conn()

[Kirill Tkhai]

Preparation for next patches.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/dev.c    |    7 +++++++
 fs/fuse/fuse_i.h |    3 +++
 2 files changed, 10 insertions(+)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index dd8f019447a9..b393fbedcc1f 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2231,11 +2231,18 @@ void fuse_abort_conn(struct fuse_conn *fc)
 		kill_fasync(&fiq->fasync, SIGIO, POLL_IN);
 		end_polls(fc);
 		wake_up_all(&fc->blocked_waitq);
+		fc->aborting = true;
 		spin_unlock(&fc->lock);
 
 		end_requests(fc, &to_end);
+
+		spin_lock(&fc->lock);
+		fc->aborting = false;
+		spin_unlock(&fc->lock);
+		wake_up_all(&fc->blocked_waitq);
 	} else {
 		spin_unlock(&fc->lock);
+		wait_event(fc->blocked_waitq, !fc->aborting);
 	}
 }
 EXPORT_SYMBOL_GPL(fuse_abort_conn);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 033e30af519f..b5f2265c437c 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -587,6 +587,9 @@ struct fuse_conn {
 	    abort and device release */
 	unsigned connected;
 
+	/** Connection abort is in process */
+	bool aborting;
+
 	/** Connection aborted via sysfs */
 	bool aborted;
 

[PATCH 4/7] fuse: Add fud pointer to struct fuse_copy_state

[Kirill Tkhai]

... and propagate fud into fuse_copy_init().

This is preparation for next patches.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/dev.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index b393fbedcc1f..afadf462ec18 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -769,6 +769,7 @@ static int unlock_request(struct fuse_req *req)
 }
 
 struct fuse_copy_state {
+	struct fuse_dev *fud;
 	int write;
 	struct fuse_req *req;
 	struct iov_iter *iter;
@@ -782,10 +783,12 @@ struct fuse_copy_state {
 	unsigned move_pages:1;
 };
 
-static void fuse_copy_init(struct fuse_copy_state *cs, int write,
+static void fuse_copy_init(struct fuse_copy_state *cs,
+		           struct fuse_dev *fud, int write,
 			   struct iov_iter *iter)
 {
 	memset(cs, 0, sizeof(*cs));
+	cs->fud = fud;
 	cs->write = write;
 	cs->iter = iter;
 }
@@ -1436,7 +1439,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
 	if (!iter_is_iovec(to))
 		return -EINVAL;
 
-	fuse_copy_init(&cs, 1, to);
+	fuse_copy_init(&cs, fud, 1, to);
 
 	return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
 }
@@ -1459,7 +1462,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	if (!bufs)
 		return -ENOMEM;
 
-	fuse_copy_init(&cs, 1, NULL);
+	fuse_copy_init(&cs, fud, 1, NULL);
 	cs.pipebufs = bufs;
 	cs.pipe = pipe;
 	ret = fuse_dev_do_read(fud, in, &cs, len);
@@ -2020,7 +2023,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
 	if (!iter_is_iovec(from))
 		return -EINVAL;
 
-	fuse_copy_init(&cs, 0, from);
+	fuse_copy_init(&cs, fud, 0, from);
 
 	return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
 }
@@ -2089,7 +2092,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 	}
 	pipe_unlock(pipe);
 
-	fuse_copy_init(&cs, 0, NULL);
+	fuse_copy_init(&cs, fud, 0, NULL);
 	cs.pipebufs = bufs;
 	cs.nr_segs = nbuf;
 	cs.pipe = pipe;

[PATCH 5/7] fuse: Introduce generic fuse_copy_aborted()

[Kirill Tkhai]

There is no a reason to set individual FR_ABORTED state
for every request, since fuse_abort_conn() aborts all
unlocked requests at once. FR_ABORTED bit just makes
fuse_copy_aborted() to end some of requests, which are
in the middle of fuse_dev_do_read() and fuse_dev_do_write(),
but this is not a big deal. These functions may abort
the requests themselves.

The patch kills lock_request and unlock_request(),
and introduces generic fuse_copy_aborted() to use
instead of them. This allows next patches to kill
FR_ABORTED, FR_LOCKED and FR_PRIVATE, and simplify
fuse dev read/write function.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/dev.c |   82 +++++++++++++--------------------------------------------
 1 file changed, 18 insertions(+), 64 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index afadf462ec18..4905abfb279e 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -731,43 +731,6 @@ void fuse_force_forget(struct file *file, u64 nodeid)
 	fuse_put_request(fc, req);
 }
 
-/*
- * Lock the request.  Up to the next unlock_request() there mustn't be
- * anything that could cause a page-fault.  If the request was already
- * aborted bail out.
- */
-static int lock_request(struct fuse_req *req)
-{
-	int err = 0;
-	if (req) {
-		spin_lock(&req->waitq.lock);
-		if (test_bit(FR_ABORTED, &req->flags))
-			err = -ENOENT;
-		else
-			set_bit(FR_LOCKED, &req->flags);
-		spin_unlock(&req->waitq.lock);
-	}
-	return err;
-}
-
-/*
- * Unlock request.  If it was aborted while locked, caller is responsible
- * for unlocking and ending the request.
- */
-static int unlock_request(struct fuse_req *req)
-{
-	int err = 0;
-	if (req) {
-		spin_lock(&req->waitq.lock);
-		if (test_bit(FR_ABORTED, &req->flags))
-			err = -ENOENT;
-		else
-			clear_bit(FR_LOCKED, &req->flags);
-		spin_unlock(&req->waitq.lock);
-	}
-	return err;
-}
-
 struct fuse_copy_state {
 	struct fuse_dev *fud;
 	int write;
@@ -812,6 +775,16 @@ static void fuse_copy_finish(struct fuse_copy_state *cs)
 	cs->pg = NULL;
 }
 
+static int fuse_copy_aborted(struct fuse_copy_state *cs)
+{
+	struct fuse_pqueue *pq = &cs->fud->pq;
+
+	if (READ_ONCE(pq->connected))
+		return 0;
+	else
+		return -ENOENT;
+}
+
 /*
  * Get another pagefull of userspace buffer, and map it to kernel
  * address space, and lock request
@@ -821,7 +794,7 @@ static int fuse_copy_fill(struct fuse_copy_state *cs)
 	struct page *page;
 	int err;
 
-	err = unlock_request(cs->req);
+	err = fuse_copy_aborted(cs);
 	if (err)
 		return err;
 
@@ -872,7 +845,7 @@ static int fuse_copy_fill(struct fuse_copy_state *cs)
 		iov_iter_advance(cs->iter, err);
 	}
 
-	return lock_request(cs->req);
+	return fuse_copy_aborted(cs);
 }
 
 /* Do as much copy to/from userspace buffer as we can */
@@ -923,7 +896,7 @@ static int fuse_try_move_page(struct fuse_copy_state *cs, struct page **pagep)
 	struct page *newpage;
 	struct pipe_buffer *buf = cs->pipebufs;
 
-	err = unlock_request(cs->req);
+	err = fuse_copy_aborted(cs);
 	if (err)
 		return err;
 
@@ -980,12 +953,10 @@ static int fuse_try_move_page(struct fuse_copy_state *cs, struct page **pagep)
 		lru_cache_add_file(newpage);
 
 	err = 0;
-	spin_lock(&cs->req->waitq.lock);
-	if (test_bit(FR_ABORTED, &cs->req->flags))
+	if (fuse_copy_aborted(cs))
 		err = -ENOENT;
 	else
 		*pagep = newpage;
-	spin_unlock(&cs->req->waitq.lock);
 
 	if (err) {
 		unlock_page(newpage);
@@ -1005,7 +976,7 @@ static int fuse_try_move_page(struct fuse_copy_state *cs, struct page **pagep)
 	cs->pg = buf->page;
 	cs->offset = buf->offset;
 
-	err = lock_request(cs->req);
+	err = fuse_copy_aborted(cs);
 	if (err)
 		return err;
 
@@ -1021,7 +992,7 @@ static int fuse_ref_page(struct fuse_copy_state *cs, struct page *page,
 	if (cs->nr_segs == cs->pipe->buffers)
 		return -EIO;
 
-	err = unlock_request(cs->req);
+	err = fuse_copy_aborted(cs);
 	if (err)
 		return err;
 
@@ -2172,13 +2143,6 @@ static void end_polls(struct fuse_conn *fc)
  * and all users of the filesystem.  The exception is the combination of an
  * asynchronous request and the tricky deadlock (see
  * Documentation/filesystems/fuse.txt).
- *
- * Aborting requests under I/O goes as follows: 1: Separate out unlocked
- * requests, they should be finished off immediately.  Locked requests will be
- * finished after unlock; see unlock_request(). 2: Finish off the unlocked
- * requests.  It is possible that some request will finish before we can.  This
- * is OK, the request will in that case be removed from the list before we touch
- * it.
  */
 void fuse_abort_conn(struct fuse_conn *fc)
 {
@@ -2187,7 +2151,7 @@ void fuse_abort_conn(struct fuse_conn *fc)
 	spin_lock(&fc->lock);
 	if (fc->connected) {
 		struct fuse_dev *fud;
-		struct fuse_req *req, *next;
+		struct fuse_req *req;
 		LIST_HEAD(to_end);
 		unsigned int i;
 
@@ -2205,17 +2169,6 @@ void fuse_abort_conn(struct fuse_conn *fc)
 
 			spin_lock(&fpq->lock);
 			fpq->connected = 0;
-			list_for_each_entry_safe(req, next, &fpq->io, list) {
-				req->out.h.error = -ECONNABORTED;
-				spin_lock(&req->waitq.lock);
-				set_bit(FR_ABORTED, &req->flags);
-				if (!test_bit(FR_LOCKED, &req->flags)) {
-					set_bit(FR_PRIVATE, &req->flags);
-					__fuse_get_request(req);
-					list_move(&req->list, &to_end);
-				}
-				spin_unlock(&req->waitq.lock);
-			}
 			for (i = 0; i < FUSE_PQ_HASH_SIZE; i++)
 				list_splice_tail_init(&fpq->processing[i],
 						      &to_end);
@@ -2238,6 +2191,7 @@ void fuse_abort_conn(struct fuse_conn *fc)
 		spin_unlock(&fc->lock);
 
 		end_requests(fc, &to_end);
+		fuse_wait_aborted(fc);
 
 		spin_lock(&fc->lock);
 		fc->aborting = false;

[PATCH 6/7] fuse: Kill unused FR_ABORTED, FR_LOCKED and FR_PRIVATE flags

[Kirill Tkhai]

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/dev.c    |    9 ++-------
 fs/fuse/fuse_i.h |   14 +-------------
 2 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 4905abfb279e..c3bacf9191a6 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1349,7 +1349,6 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 				     (struct fuse_arg *) in->args, 0);
 	fuse_copy_finish(cs);
 	spin_lock(&fpq->lock);
-	clear_bit(FR_LOCKED, &req->flags);
 	if (!fpq->connected) {
 		err = fc->aborted ? -ECONNABORTED : -ENODEV;
 		goto out_end;
@@ -1376,8 +1375,7 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 	return reqsize;
 
 out_end:
-	if (!test_bit(FR_PRIVATE, &req->flags))
-		list_del_init(&req->list);
+	list_del_init(&req->list);
 	spin_unlock(&fpq->lock);
 	request_end(fc, req);
 	return err;
@@ -1955,7 +1953,6 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
 	clear_bit(FR_SENT, &req->flags);
 	list_move(&req->list, &fpq->io);
 	req->out.h = oh;
-	set_bit(FR_LOCKED, &req->flags);
 	spin_unlock(&fpq->lock);
 	cs->req = req;
 	if (!req->out.page_replace)
@@ -1965,13 +1962,11 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
 	fuse_copy_finish(cs);
 
 	spin_lock(&fpq->lock);
-	clear_bit(FR_LOCKED, &req->flags);
 	if (!fpq->connected)
 		err = -ENOENT;
 	else if (err)
 		req->out.h.error = -EIO;
-	if (!test_bit(FR_PRIVATE, &req->flags))
-		list_del_init(&req->list);
+	list_del_init(&req->list);
 	spin_unlock(&fpq->lock);
 
 	request_end(fc, req);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index b5f2265c437c..09ea5773ad81 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -332,35 +332,23 @@ struct fuse_io_priv {
  * FR_FORCE:		force sending of the request even if interrupted
  * FR_BACKGROUND:	request is sent in the background
  * FR_WAITING:		request is counted as "waiting"
- * FR_ABORTED:		the request was aborted
  * FR_INTERRUPTED:	the request has been interrupted
- * FR_LOCKED:		data is being copied to/from the request
  * FR_PENDING:		request is not yet in userspace
  * FR_SENT:		request is in userspace, waiting for an answer
  * FR_FINISHED:		request is finished
- * FR_PRIVATE:		request is on private list
  */
 enum fuse_req_flag {
 	FR_ISREPLY,
 	FR_FORCE,
 	FR_BACKGROUND,
 	FR_WAITING,
-	FR_ABORTED,
 	FR_INTERRUPTED,
-	FR_LOCKED,
 	FR_PENDING,
 	FR_SENT,
 	FR_FINISHED,
-	FR_PRIVATE,
 };
 
-/**
- * A request to the client
- *
- * .waitq.lock protects the following fields:
- *   - FR_ABORTED
- *   - FR_LOCKED (may also be modified under fc->lock, tested under both)
- */
+/* A request to the client */
 struct fuse_req {
 	/** This can be on either pending processing or io lists in
 	    fuse_conn */

[PATCH 7/7] fuse: Kill fuse_pqueue::io list and avoid taking fpq->lock on hot paths

[Kirill Tkhai]

This list was used to make fuse_abort_conn() end some
of requests under io. But it is not used anymore.
This allows to avoid taking unneeded fpq->lock in two
hot paths: fuse_dev_do_write() and fuse_dev_do_read().

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/dev.c    |   16 +++-------------
 fs/fuse/fuse_i.h |    3 ---
 fs/fuse/inode.c  |    1 -
 3 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index c3bacf9191a6..b4ce37dda353 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1339,9 +1339,6 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 		request_end(fc, req);
 		goto restart;
 	}
-	spin_lock(&fpq->lock);
-	list_add(&req->list, &fpq->io);
-	spin_unlock(&fpq->lock);
 	cs->req = req;
 	err = fuse_copy_one(cs, &in->h, sizeof(in->h));
 	if (!err)
@@ -1362,7 +1359,7 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 		goto out_end;
 	}
 	hash = fuse_req_hash(req->in.h.unique);
-	list_move_tail(&req->list, &fpq->processing[hash]);
+	list_add_tail(&req->list, &fpq->processing[hash]);
 	__fuse_get_request(req);
 	set_bit(FR_SENT, &req->flags);
 	spin_unlock(&fpq->lock);
@@ -1375,7 +1372,6 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
 	return reqsize;
 
 out_end:
-	list_del_init(&req->list);
 	spin_unlock(&fpq->lock);
 	request_end(fc, req);
 	return err;
@@ -1951,7 +1947,7 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
 	}
 
 	clear_bit(FR_SENT, &req->flags);
-	list_move(&req->list, &fpq->io);
+	list_del_init(&req->list);
 	req->out.h = oh;
 	spin_unlock(&fpq->lock);
 	cs->req = req;
@@ -1961,13 +1957,8 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
 	err = copy_out_args(cs, &req->out, nbytes);
 	fuse_copy_finish(cs);
 
-	spin_lock(&fpq->lock);
-	if (!fpq->connected)
-		err = -ENOENT;
-	else if (err)
+	if (err)
 		req->out.h.error = -EIO;
-	list_del_init(&req->list);
-	spin_unlock(&fpq->lock);
 
 	request_end(fc, req);
 out:
@@ -2217,7 +2208,6 @@ int fuse_dev_release(struct inode *inode, struct file *file)
 		unsigned int i;
 
 		spin_lock(&fpq->lock);
-		WARN_ON(!list_empty(&fpq->io));
 		for (i = 0; i < FUSE_PQ_HASH_SIZE; i++)
 			list_splice_init(&fpq->processing[i], &to_end);
 		spin_unlock(&fpq->lock);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 09ea5773ad81..e6b7087fd6b1 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -469,9 +469,6 @@ struct fuse_pqueue {
 
 	/** Hash table of requests being processed */
 	struct list_head *processing;
-
-	/** The list of requests under I/O */
-	struct list_head io;
 };
 
 /**
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 0361a3d62356..a36d2675471f 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -601,7 +601,6 @@ static void fuse_pqueue_init(struct fuse_pqueue *fpq)
 	spin_lock_init(&fpq->lock);
 	for (i = 0; i < FUSE_PQ_HASH_SIZE; i++)
 		INIT_LIST_HEAD(&fpq->processing[i]);
-	INIT_LIST_HEAD(&fpq->io);
 	fpq->connected = 1;
 }
 

Re: [PATCH 5/7] fuse: Introduce generic fuse_copy_aborted()

[Miklos Szeredi]

On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> There is no a reason to set individual FR_ABORTED state
> for every request, since fuse_abort_conn() aborts all
> unlocked requests at once. FR_ABORTED bit just makes
> fuse_copy_aborted() to end some of requests, which are
> in the middle of fuse_dev_do_read() and fuse_dev_do_write(),
> but this is not a big deal. These functions may abort
> the requests themselves.
>
> The patch kills lock_request and unlock_request(),
> and introduces generic fuse_copy_aborted() to use
> instead of them. This allows next patches to kill
> FR_ABORTED, FR_LOCKED and FR_PRIVATE, and simplify
> fuse dev read/write function.

Patch is no good: you assume that fuse_copy_args() called from
fuse_dev_do_*() will finish in finite time.  That's not the case if
page fault on userspace buffer is hung (see e.g.
Documentation/filesystems/fuse.txt #Tricky deadlock).

You *are* right that this is a big wart in the current implementation,
and would be really nice to get rid of.  Certainly doable, just need
to make sure all buffers stored in the request are properly refcounted
and have lifetime bound to that of the request, i.e. all synchronous
requests (not background) can be aborted asynchronously, the caller of
the request can return in this case with an abort, yet the request
processing (copy to/from userspace) can finish without encountering
already freed memory.

The fuse_simple_request() thing was a first step in this direction, so
that one's pretty easy to deal with: copy arguments to a temporary
buffer that is freed only when the request itself is freed...

Requests which have pages need special attention.   A ref is taken for
those pages, so they may be OK, but in some cases they may not be.

Thanks,
Miklos

Re: [PATCH 1/7] fuse: Check for fc->connected in fuse_dev_alloc()

[Miklos Szeredi]

On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> fuse_dev_alloc() may be called after fc->connected
> is dropped (from ioctl), so here we add sanity check
> for that case.

AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to
the fuse_conn's list after disconnection there would be no leak.

In other words, it's irrelevant whether the connection reset comes
just before the ioctl completes or just after.   Or am I missing
something?

Thanks,
Miklos

>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  fs/fuse/inode.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 336844d0eb3a..0361a3d62356 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1054,10 +1054,19 @@ struct fuse_dev *fuse_dev_alloc(struct fuse_conn *fc)
>         fuse_pqueue_init(&fud->pq);
>
>         spin_lock(&fc->lock);
> +       if (!fc->connected) {
> +               spin_unlock(&fc->lock);
> +               goto out_put;
> +       }
>         list_add_tail(&fud->entry, &fc->devices);
>         spin_unlock(&fc->lock);
>
>         return fud;
> +out_put:
> +       fuse_conn_put(fc);
> +       kfree(pq);
> +       kfree(fud);
> +       return NULL;
>  }
>  EXPORT_SYMBOL_GPL(fuse_dev_alloc);
>
>

Re: [PATCH 1/7] fuse: Check for fc->connected in fuse_dev_alloc()

[Kirill Tkhai]

On 18.01.2019 15:07, Miklos Szeredi wrote:
> On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>
>> fuse_dev_alloc() may be called after fc->connected
>> is dropped (from ioctl), so here we add sanity check
>> for that case.
> 
> AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to
> the fuse_conn's list after disconnection there would be no leak.
> 
> In other words, it's irrelevant whether the connection reset comes
> just before the ioctl completes or just after.   Or am I missing
> something?

Yeah, there won't be a leak. The only problem I see, userspace daemon
may become waiting in fuse_dev_do_read() after abort is finished.
This means fc->count won't be put, and manual killing signal will be
needed.

I.e., umount -f will wait till the daemon is killed manually.
Not so big a problem, but not very pleasant...

Kirill

>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  fs/fuse/inode.c |    9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index 336844d0eb3a..0361a3d62356 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -1054,10 +1054,19 @@ struct fuse_dev *fuse_dev_alloc(struct fuse_conn *fc)
>>         fuse_pqueue_init(&fud->pq);
>>
>>         spin_lock(&fc->lock);
>> +       if (!fc->connected) {
>> +               spin_unlock(&fc->lock);
>> +               goto out_put;
>> +       }
>>         list_add_tail(&fud->entry, &fc->devices);
>>         spin_unlock(&fc->lock);
>>
>>         return fud;
>> +out_put:
>> +       fuse_conn_put(fc);
>> +       kfree(pq);
>> +       kfree(fud);
>> +       return NULL;
>>  }
>>  EXPORT_SYMBOL_GPL(fuse_dev_alloc);
>>
>>

Re: [PATCH 1/7] fuse: Check for fc->connected in fuse_dev_alloc()

[Miklos Szeredi]

On Fri, Jan 18, 2019 at 1:28 PM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> On 18.01.2019 15:07, Miklos Szeredi wrote:
> > On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> >>
> >> fuse_dev_alloc() may be called after fc->connected
> >> is dropped (from ioctl), so here we add sanity check
> >> for that case.
> >
> > AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to
> > the fuse_conn's list after disconnection there would be no leak.
> >
> > In other words, it's irrelevant whether the connection reset comes
> > just before the ioctl completes or just after.   Or am I missing
> > something?
>
> Yeah, there won't be a leak. The only problem I see, userspace daemon
> may become waiting in fuse_dev_do_read() after abort is finished.

By that time fiq->connected will be reset, so fuse_dev_do_read() will
return ENODEV/ECONNABORTED.

Am I missing something?

Thanks,
Miklos

Re: [PATCH 1/7] fuse: Check for fc->connected in fuse_dev_alloc()

[Kirill Tkhai]

On 23.01.2019 12:45, Miklos Szeredi wrote:
> On Fri, Jan 18, 2019 at 1:28 PM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>
>> On 18.01.2019 15:07, Miklos Szeredi wrote:
>>> On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>>
>>>> fuse_dev_alloc() may be called after fc->connected
>>>> is dropped (from ioctl), so here we add sanity check
>>>> for that case.
>>>
>>> AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to
>>> the fuse_conn's list after disconnection there would be no leak.
>>>
>>> In other words, it's irrelevant whether the connection reset comes
>>> just before the ioctl completes or just after.   Or am I missing
>>> something?
>>
>> Yeah, there won't be a leak. The only problem I see, userspace daemon
>> may become waiting in fuse_dev_do_read() after abort is finished.
> 
> By that time fiq->connected will be reset, so fuse_dev_do_read() will
> return ENODEV/ECONNABORTED.
> 
> Am I missing something?

I mean:

[cpu0]                                          [cpu1]
fuse_device_clone()              
  fuse_dev_alloc()                              fuse_abort_conn()
                                                  spin_lock(&fc->lock)
                                                  list_for_each_entry(fud, &fc->devices, entry)
                                                    fpq->connected = 0
                                                  spin_unlock(&fc->lock)
  spin_lock(&fc->lock);
  list_add_tail(&fud->entry, &fc->devices);
  spin_unlock(&fc->lock);
...
fuse_dev_do_read()
  wait_event_interruptible_exclusive_locked() <-- wait forever

If you have manager-thread on cpu1, and it expects all worker-threads (like on cpu0)
will finish after abort, this won't be true.

Kirill



    

Re: [PATCH 1/7] fuse: Check for fc->connected in fuse_dev_alloc()

[Miklos Szeredi]

On Wed, Jan 23, 2019 at 10:55 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> On 23.01.2019 12:45, Miklos Szeredi wrote:
> > On Fri, Jan 18, 2019 at 1:28 PM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> >>
> >> On 18.01.2019 15:07, Miklos Szeredi wrote:
> >>> On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> >>>>
> >>>> fuse_dev_alloc() may be called after fc->connected
> >>>> is dropped (from ioctl), so here we add sanity check
> >>>> for that case.
> >>>
> >>> AFAICS this is not fixing a bug; i.e. even if the fuse_dev is added to
> >>> the fuse_conn's list after disconnection there would be no leak.
> >>>
> >>> In other words, it's irrelevant whether the connection reset comes
> >>> just before the ioctl completes or just after.   Or am I missing
> >>> something?
> >>
> >> Yeah, there won't be a leak. The only problem I see, userspace daemon
> >> may become waiting in fuse_dev_do_read() after abort is finished.
> >
> > By that time fiq->connected will be reset, so fuse_dev_do_read() will
> > return ENODEV/ECONNABORTED.
> >
> > Am I missing something?
>
> I mean:
>
> [cpu0]                                          [cpu1]
> fuse_device_clone()
>   fuse_dev_alloc()                              fuse_abort_conn()
>                                                   spin_lock(&fc->lock)
>                                                   list_for_each_entry(fud, &fc->devices, entry)
>                                                     fpq->connected = 0
>                                                   spin_unlock(&fc->lock)
>   spin_lock(&fc->lock);
>   list_add_tail(&fud->entry, &fc->devices);

Okay, so there's missing

     fpq->connected = fc->connected;

here.

>   spin_unlock(&fc->lock);
> ...
> fuse_dev_do_read()
>   wait_event_interruptible_exclusive_locked() <-- wait forever

That will exit immediately, because it checks fiq->connected (not
fpq->connected) which is already zero.

Thanks,
Miklos