[PATCH 0/6] Extract bg queue logic out fuse_conn::lock

[PATCH 0/6] Extract bg queue logic out fuse_conn::lock

[Kirill Tkhai]

Hi, Miklos,

the patchset makes background queue and background-related
data be protected with new bg_lock. This allows async reads
not take fc->lock in fuse_request_send_background(), and
this gives performace-visible increase on out async read-write
test (1.7-5 % better). See [6/6] for the test results.

The new lock protects max_background, congestion_threshold,
num_background, active_background, bg_queue and blocked
fields. See [5/6] for the details.

Patches [1-3/6] are preparations. I've already sent you
two of them, and you said at least one is already applied,
but since they are yet no in linux-next, I add them to make
kbuild test robot happy.

[4/6] is a fix of existing issue. I don't know whether
stable needs this fix, so I don't send it in separate. Please,
say if it's needed resend it in this way.

The test results is for our proprietary vstorage fuse driver.
Feel free to ask me, in case of you want the patchset to be
tested on some other fuse driver and with some other test.

Thanks,
Kirill
---

Kirill Tkhai (6):
      fuse: Use list_first_entry() in flush_bg_queue()
      fuse: Move clear_bit() up in request_end()
      fuse: Underline congestion_threshold and max_background may be read w/o fc->lock
      fuse: Lock fc->lock during changing num_background and congestion_threshold
      fs: Introduce fuse_conn::bg_lock
      fuse: Do not take fuse_conn::lock on fuse_request_send_background()


 fs/fuse/control.c |   34 ++++++++++++++++++++++++++--------
 fs/fuse/dev.c     |   42 +++++++++++++++++++++++++++---------------
 fs/fuse/file.c    |    2 +-
 fs/fuse/fuse_i.h  |   11 +++++++++--
 fs/fuse/inode.c   |    4 ++++
 5 files changed, 67 insertions(+), 26 deletions(-)

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

[PATCH 1/6] fuse: Use list_first_entry() in flush_bg_queue()

[Kirill Tkhai]

This cleanup patch makes the function to use the primitive
instead of direct dereferencing.

Also, move fiq dereferencing out of cycle, since it's
always constant.


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

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 11ea2c4a38ab..214ce96b1d26 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -353,12 +353,13 @@ void fuse_queue_forget(struct fuse_conn *fc, struct fuse_forget_link *forget,
 
 static void flush_bg_queue(struct fuse_conn *fc)
 {
+	struct fuse_iqueue *fiq = &fc->iq;
+
 	while (fc->active_background < fc->max_background &&
 	       !list_empty(&fc->bg_queue)) {
 		struct fuse_req *req;
-		struct fuse_iqueue *fiq = &fc->iq;
 
-		req = list_entry(fc->bg_queue.next, struct fuse_req, list);
+		req = list_first_entry(&fc->bg_queue, struct fuse_req, list);
 		list_del(&req->list);
 		fc->active_background++;
 		spin_lock(&fiq->waitq.lock);

[PATCH 2/6] fuse: Move clear_bit() up in request_end()

[Kirill Tkhai]

Nobody can clear FR_BACKGROUND bit on processing
request in parallel, so it's possible to do it
out of fc->lock.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 214ce96b1d26..ce07a41ff8fe 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -390,8 +390,8 @@ static void request_end(struct fuse_conn *fc, struct fuse_req *req)
 	WARN_ON(test_bit(FR_PENDING, &req->flags));
 	WARN_ON(test_bit(FR_SENT, &req->flags));
 	if (test_bit(FR_BACKGROUND, &req->flags)) {
-		spin_lock(&fc->lock);
 		clear_bit(FR_BACKGROUND, &req->flags);
+		spin_lock(&fc->lock);
 		if (fc->num_background == fc->max_background)
 			fc->blocked = 0;
 

[PATCH 3/6] fuse: Underline congestion_threshold and max_background may be read w/o fc->lock

[Kirill Tkhai]

Since they are of unsigned int type, it's allowed to read them
unlocked during reporting to userspace. Let's underline this fact
with READ_ONCE() macroses.

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

diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index e09b9cd9c3fc..c7290625e171 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -107,7 +107,7 @@ static ssize_t fuse_conn_max_background_read(struct file *file,
 	if (!fc)
 		return 0;
 
-	val = fc->max_background;
+	val = READ_ONCE(fc->max_background);
 	fuse_conn_put(fc);
 
 	return fuse_conn_limit_read(file, buf, len, ppos, val);
@@ -144,7 +144,7 @@ static ssize_t fuse_conn_congestion_threshold_read(struct file *file,
 	if (!fc)
 		return 0;
 
-	val = fc->congestion_threshold;
+	val = READ_ONCE(fc->congestion_threshold);
 	fuse_conn_put(fc);
 
 	return fuse_conn_limit_read(file, buf, len, ppos, val);

[PATCH 4/6] fuse: Lock fc->lock during changing num_background and congestion_threshold

[Kirill Tkhai]

Functions sequences like request_end()->flush_bg_queue()
require max_background and congestion_threshold are constant
during their execution. Otherwise, checks like

	if (fc->num_background == fc->max_background)

made in different time may behave not like expected.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/control.c |   30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index c7290625e171..9206501bd94d 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -125,7 +125,12 @@ static ssize_t fuse_conn_max_background_write(struct file *file,
 	if (ret > 0) {
 		struct fuse_conn *fc = fuse_ctl_file_conn_get(file);
 		if (fc) {
+			spin_lock(&fc->lock);
 			fc->max_background = val;
+			fc->blocked = (fc->num_background >= fc->max_background);
+			if (!fc->blocked && waitqueue_active(&fc->blocked_waitq))
+				wake_up(&fc->blocked_waitq);
+			spin_unlock(&fc->lock);
 			fuse_conn_put(fc);
 		}
 	}
@@ -155,18 +160,31 @@ static ssize_t fuse_conn_congestion_threshold_write(struct file *file,
 						    size_t count, loff_t *ppos)
 {
 	unsigned uninitialized_var(val);
+	struct fuse_conn *fc;
 	ssize_t ret;
 
 	ret = fuse_conn_limit_write(file, buf, count, ppos, &val,
 				    max_user_congthresh);
-	if (ret > 0) {
-		struct fuse_conn *fc = fuse_ctl_file_conn_get(file);
-		if (fc) {
-			fc->congestion_threshold = val;
-			fuse_conn_put(fc);
+	if (ret <= 0)
+		goto out;
+	fc = fuse_ctl_file_conn_get(file);
+	if (!fc)
+		goto out;
+
+	spin_lock(&fc->lock);
+	fc->congestion_threshold = val;
+	if (fc->sb) {
+		if (fc->num_background < fc->congestion_threshold) {
+			clear_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
+			clear_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
+		} else {
+			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
+			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
 		}
 	}
-
+	spin_unlock(&fc->lock);
+	fuse_conn_put(fc);
+out:
 	return ret;
 }
 

[PATCH 5/6] fs: Introduce fuse_conn::bg_lock

[Kirill Tkhai]

To reduce contention of fuse_conn::lock, this patch
introduces bg_lock for protection of fields related
to background queue. These are: max_background,
congestion_threshold, num_background, active_background,
bg_queue and blocked.

This allows next patch to make async reads not requiring
fuse_conn::lock, so async reads and writes will have
better performance executed in parallel.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/control.c |    8 ++++----
 fs/fuse/dev.c     |   20 ++++++++++++--------
 fs/fuse/file.c    |    2 +-
 fs/fuse/fuse_i.h  |    8 ++++++--
 fs/fuse/inode.c   |    3 +++
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index 9206501bd94d..63d0e9552140 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -125,12 +125,12 @@ static ssize_t fuse_conn_max_background_write(struct file *file,
 	if (ret > 0) {
 		struct fuse_conn *fc = fuse_ctl_file_conn_get(file);
 		if (fc) {
-			spin_lock(&fc->lock);
+			spin_lock(&fc->bg_lock);
 			fc->max_background = val;
 			fc->blocked = (fc->num_background >= fc->max_background);
 			if (!fc->blocked && waitqueue_active(&fc->blocked_waitq))
 				wake_up(&fc->blocked_waitq);
-			spin_unlock(&fc->lock);
+			spin_unlock(&fc->bg_lock);
 			fuse_conn_put(fc);
 		}
 	}
@@ -171,7 +171,7 @@ static ssize_t fuse_conn_congestion_threshold_write(struct file *file,
 	if (!fc)
 		goto out;
 
-	spin_lock(&fc->lock);
+	spin_lock(&fc->bg_lock);
 	fc->congestion_threshold = val;
 	if (fc->sb) {
 		if (fc->num_background < fc->congestion_threshold) {
@@ -182,7 +182,7 @@ static ssize_t fuse_conn_congestion_threshold_write(struct file *file,
 			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
 		}
 	}
-	spin_unlock(&fc->lock);
+	spin_unlock(&fc->bg_lock);
 	fuse_conn_put(fc);
 out:
 	return ret;
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index ce07a41ff8fe..9690e7c79df7 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -287,10 +287,10 @@ void fuse_put_request(struct fuse_conn *fc, struct fuse_req *req)
 			 * We get here in the unlikely case that a background
 			 * request was allocated but not sent
 			 */
-			spin_lock(&fc->lock);
+			spin_lock(&fc->bg_lock);
 			if (!fc->blocked)
 				wake_up(&fc->blocked_waitq);
-			spin_unlock(&fc->lock);
+			spin_unlock(&fc->bg_lock);
 		}
 
 		if (test_bit(FR_WAITING, &req->flags)) {
@@ -391,7 +391,7 @@ static void request_end(struct fuse_conn *fc, struct fuse_req *req)
 	WARN_ON(test_bit(FR_SENT, &req->flags));
 	if (test_bit(FR_BACKGROUND, &req->flags)) {
 		clear_bit(FR_BACKGROUND, &req->flags);
-		spin_lock(&fc->lock);
+		spin_lock(&fc->bg_lock);
 		if (fc->num_background == fc->max_background)
 			fc->blocked = 0;
 
@@ -406,7 +406,7 @@ static void request_end(struct fuse_conn *fc, struct fuse_req *req)
 		fc->num_background--;
 		fc->active_background--;
 		flush_bg_queue(fc);
-		spin_unlock(&fc->lock);
+		spin_unlock(&fc->bg_lock);
 	}
 	wake_up(&req->waitq);
 	if (req->end)
@@ -579,8 +579,8 @@ ssize_t fuse_simple_request(struct fuse_conn *fc, struct fuse_args *args)
  *
  * fc->connected must have been checked previously
  */
-void fuse_request_send_background_locked(struct fuse_conn *fc,
-					 struct fuse_req *req)
+void fuse_request_send_background_nocheck(struct fuse_conn *fc,
+					  struct fuse_req *req)
 {
 	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
 	if (!test_bit(FR_WAITING, &req->flags)) {
@@ -588,6 +588,7 @@ void fuse_request_send_background_locked(struct fuse_conn *fc,
 		atomic_inc(&fc->num_waiting);
 	}
 	__set_bit(FR_ISREPLY, &req->flags);
+	spin_lock(&fc->bg_lock);
 	fc->num_background++;
 	if (fc->num_background == fc->max_background)
 		fc->blocked = 1;
@@ -597,6 +598,7 @@ void fuse_request_send_background_locked(struct fuse_conn *fc,
 	}
 	list_add_tail(&req->list, &fc->bg_queue);
 	flush_bg_queue(fc);
+	spin_unlock(&fc->bg_lock);
 }
 
 void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
@@ -604,7 +606,7 @@ void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
 	BUG_ON(!req->end);
 	spin_lock(&fc->lock);
 	if (fc->connected) {
-		fuse_request_send_background_locked(fc, req);
+		fuse_request_send_background_nocheck(fc, req);
 		spin_unlock(&fc->lock);
 	} else {
 		spin_unlock(&fc->lock);
@@ -2105,7 +2107,6 @@ void fuse_abort_conn(struct fuse_conn *fc, bool is_abort)
 		LIST_HEAD(to_end);
 
 		fc->connected = 0;
-		fc->blocked = 0;
 		fc->aborted = is_abort;
 		fuse_set_initialized(fc);
 		list_for_each_entry(fud, &fc->devices, entry) {
@@ -2127,8 +2128,11 @@ void fuse_abort_conn(struct fuse_conn *fc, bool is_abort)
 			list_splice_tail_init(&fpq->processing, &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;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 32d0b883e74f..a875959dd939 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1502,7 +1502,7 @@ __acquires(fc->lock)
 
 	req->in.args[1].size = inarg->size;
 	fi->writectr++;
-	fuse_request_send_background_locked(fc, req);
+	fuse_request_send_background_nocheck(fc, req);
 	return;
 
  out_free:
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f78e9614bb5f..b2c6a1cd1e2c 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -500,6 +500,10 @@ struct fuse_conn {
 	/** The list of background requests set aside for later queuing */
 	struct list_head bg_queue;
 
+	/** Protects: max_background, congestion_threshold, num_background,
+	 * active_background, bg_queue, blocked */
+	spinlock_t bg_lock;
+
 	/** Flag indicating that INIT reply has been received. Allocating
 	 * any fuse request will be suspended until the flag is set */
 	int initialized;
@@ -857,8 +861,8 @@ ssize_t fuse_simple_request(struct fuse_conn *fc, struct fuse_args *args);
  */
 void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
 
-void fuse_request_send_background_locked(struct fuse_conn *fc,
-					 struct fuse_req *req);
+void fuse_request_send_background_nocheck(struct fuse_conn *fc,
+					  struct fuse_req *req);
 
 /* Abort all requests */
 void fuse_abort_conn(struct fuse_conn *fc, bool is_abort);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 9db8c0c745a9..a4285ec7c248 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -606,6 +606,7 @@ void fuse_conn_init(struct fuse_conn *fc, struct user_namespace *user_ns)
 {
 	memset(fc, 0, sizeof(*fc));
 	spin_lock_init(&fc->lock);
+	spin_lock_init(&fc->bg_lock);
 	init_rwsem(&fc->killsb);
 	refcount_set(&fc->count, 1);
 	atomic_set(&fc->dev_count, 1);
@@ -853,6 +854,7 @@ static void process_init_limits(struct fuse_conn *fc, struct fuse_init_out *arg)
 	sanitize_global_limit(&max_user_bgreq);
 	sanitize_global_limit(&max_user_congthresh);
 
+	spin_lock(&fc->bg_lock);
 	if (arg->max_background) {
 		fc->max_background = arg->max_background;
 
@@ -866,6 +868,7 @@ static void process_init_limits(struct fuse_conn *fc, struct fuse_init_out *arg)
 		    fc->congestion_threshold > max_user_congthresh)
 			fc->congestion_threshold = max_user_congthresh;
 	}
+	spin_unlock(&fc->bg_lock);
 }
 
 static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)

[PATCH 6/6] fuse: Do not take fuse_conn::lock on fuse_request_send_background()

[Kirill Tkhai]

Currently, we take fc->lock there only to check for fc->connected.
But this flag is changed only on connection abort, which is very
rare operation. Good thing looks to make fuse_request_send_background()
faster, while fuse_abort_conn() slowler.

So, we make fuse_request_send_background() lockless and mark
(fc->connected == 1) region as RCU-protected. Abort function
just uses synchronize_sched() to wait till all pending background
requests is being queued, and then makes ordinary abort.

Note, that synchronize_sched() is used instead of synchronize_rcu(),
since we want to check for fc->connected without rcu_dereference()
in fuse_request_send_background() (i.e., not to add memory barriers
to this hot path).

Also, note fuse_conn::aborting field is introduced. It's aimed
to synchronize two sequential aborts executing in parallel:
we do not want the secondly called abort finishes before the first
one is done, after it sees fc->connected has already became 0.

Our storage test shows performance increase on parallel read and write
with the patchset (7 test runs, 300 seconds of execution of each, aio):

./io_iops --read --write --iops -u 48g -s 4k -p 24 -n 24 --aio -q 128 -t 300 -f /mnt/vstorage/file

Before (iops):
25721.58203 (worst)
26179.43359
26092.58594
25789.96484
26238.63477 (best)
25985.88867
25987.66406

After (iops):
26852.27539
27547.60742
26322.29688
27643.33398 (best)
26753.10547
26157.91016 (worst)
26694.63477

All *after* runs are better than any *before* run, except of one.
The best *after* run is 5% better than the best *before* run.
The worst *after* run is 1.7% better then the worst *before* run.

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

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 9690e7c79df7..064a65ca7283 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -575,8 +575,6 @@ ssize_t fuse_simple_request(struct fuse_conn *fc, struct fuse_args *args)
 }
 
 /*
- * Called under fc->lock
- *
  * fc->connected must have been checked previously
  */
 void fuse_request_send_background_nocheck(struct fuse_conn *fc,
@@ -604,12 +602,12 @@ void fuse_request_send_background_nocheck(struct fuse_conn *fc,
 void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
 {
 	BUG_ON(!req->end);
-	spin_lock(&fc->lock);
+	rcu_read_lock_sched();
 	if (fc->connected) {
 		fuse_request_send_background_nocheck(fc, req);
-		spin_unlock(&fc->lock);
+		rcu_read_unlock_sched();
 	} else {
-		spin_unlock(&fc->lock);
+		rcu_read_unlock_sched();
 		req->out.h.error = -ENOTCONN;
 		req->end(fc, req);
 		fuse_put_request(fc, req);
@@ -2107,6 +2105,13 @@ void fuse_abort_conn(struct fuse_conn *fc, bool is_abort)
 		LIST_HEAD(to_end);
 
 		fc->connected = 0;
+		fc->aborting = true;
+		spin_unlock(&fc->lock);
+
+		/* Propagate fc->connected */
+		synchronize_sched();
+
+		spin_lock(&fc->lock);
 		fc->aborted = is_abort;
 		fuse_set_initialized(fc);
 		list_for_each_entry(fud, &fc->devices, entry) {
@@ -2145,12 +2150,14 @@ void fuse_abort_conn(struct fuse_conn *fc, bool is_abort)
 		spin_unlock(&fiq->waitq.lock);
 		kill_fasync(&fiq->fasync, SIGIO, POLL_IN);
 		end_polls(fc);
+		fc->aborting = false;
 		wake_up_all(&fc->blocked_waitq);
 		spin_unlock(&fc->lock);
 
 		end_requests(fc, &to_end);
 	} 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 b2c6a1cd1e2c..b9a991b42c88 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -523,6 +523,9 @@ struct fuse_conn {
 	    abort and device release */
 	unsigned connected;
 
+	/** Connection is now aborting */
+	bool aborting;
+
 	/** Connection aborted via sysfs */
 	bool aborted;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index a4285ec7c248..9612eb4bc609 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -624,6 +624,7 @@ void fuse_conn_init(struct fuse_conn *fc, struct user_namespace *user_ns)
 	fc->blocked = 0;
 	fc->initialized = 0;
 	fc->connected = 1;
+	fc->aborting = false;
 	fc->attr_version = 1;
 	get_random_bytes(&fc->scramble_key, sizeof(fc->scramble_key));
 	fc->pid_ns = get_pid_ns(task_active_pid_ns(current));

Re: [PATCH 6/6] fuse: Do not take fuse_conn::lock on fuse_request_send_background()

[Miklos Szeredi]

On Mon, Aug 27, 2018 at 06:29:56PM +0300, Kirill Tkhai wrote:
> Currently, we take fc->lock there only to check for fc->connected.
> But this flag is changed only on connection abort, which is very
> rare operation. Good thing looks to make fuse_request_send_background()
> faster, while fuse_abort_conn() slowler.
> 
> So, we make fuse_request_send_background() lockless and mark
> (fc->connected == 1) region as RCU-protected. Abort function
> just uses synchronize_sched() to wait till all pending background
> requests is being queued, and then makes ordinary abort.
> 
> Note, that synchronize_sched() is used instead of synchronize_rcu(),
> since we want to check for fc->connected without rcu_dereference()
> in fuse_request_send_background() (i.e., not to add memory barriers
> to this hot path).

Apart from the inaccuracies in the above (_sched variant is for scheduling and
NMI taking code; _sched variant requires rcu_dereference() as well;
rcu_dereference() does not add barriers; rcu_dereference() is only for pointers,
so we can't use it for an integer), wouldn't it be simpler to just use bg_lock
for checking ->connected, and lock bg_lock (as well as fc->lock) when setting
->connected?

Updated patch below (untested).

Thanks,
Miklos

---
Subject: fuse: do not take fc->lock in fuse_request_send_background()
From: Kirill Tkhai <ktkhai@virtuozzo.com>
Date: Mon, 27 Aug 2018 18:29:56 +0300

Currently, we take fc->lock there only to check for fc->connected.
But this flag is changed only on connection abort, which is very
rare operation.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dev.c    |   46 +++++++++++++++++++++++-----------------------
 fs/fuse/file.c   |    4 +++-
 fs/fuse/fuse_i.h |    4 +---
 3 files changed, 27 insertions(+), 27 deletions(-)

--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -574,42 +574,38 @@ ssize_t fuse_simple_request(struct fuse_
 	return ret;
 }
 
-/*
- * Called under fc->lock
- *
- * fc->connected must have been checked previously
- */
-void fuse_request_send_background_nocheck(struct fuse_conn *fc,
-					  struct fuse_req *req)
+bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
 {
-	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
+	bool queued = false;
+
+	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
 	if (!test_bit(FR_WAITING, &req->flags)) {
 		__set_bit(FR_WAITING, &req->flags);
 		atomic_inc(&fc->num_waiting);
 	}
 	__set_bit(FR_ISREPLY, &req->flags);
 	spin_lock(&fc->bg_lock);
-	fc->num_background++;
-	if (fc->num_background == fc->max_background)
-		fc->blocked = 1;
-	if (fc->num_background == fc->congestion_threshold && fc->sb) {
-		set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
-		set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
+	if (likely(fc->connected)) {
+		fc->num_background++;
+		if (fc->num_background == fc->max_background)
+			fc->blocked = 1;
+		if (fc->num_background == fc->congestion_threshold && fc->sb) {
+			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
+			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
+		}
+		list_add_tail(&req->list, &fc->bg_queue);
+		flush_bg_queue(fc);
+		queued = true;
 	}
-	list_add_tail(&req->list, &fc->bg_queue);
-	flush_bg_queue(fc);
 	spin_unlock(&fc->bg_lock);
+
+	return queued;
 }
 
 void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
 {
-	BUG_ON(!req->end);
-	spin_lock(&fc->lock);
-	if (fc->connected) {
-		fuse_request_send_background_nocheck(fc, req);
-		spin_unlock(&fc->lock);
-	} else {
-		spin_unlock(&fc->lock);
+	WARN_ON(!req->end);
+	if (!fuse_request_queue_background(fc, req)) {
 		req->out.h.error = -ENOTCONN;
 		req->end(fc, req);
 		fuse_put_request(fc, req);
@@ -2112,7 +2108,11 @@ void fuse_abort_conn(struct fuse_conn *f
 		struct fuse_req *req, *next;
 		LIST_HEAD(to_end);
 
+		/* Background queuing checks fc->connected under bg_lock */
+		spin_lock(&fc->bg_lock);
 		fc->connected = 0;
+		spin_unlock(&fc->bg_lock);
+
 		fc->aborted = is_abort;
 		fuse_set_initialized(fc);
 		list_for_each_entry(fud, &fc->devices, entry) {
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -863,9 +863,7 @@ ssize_t fuse_simple_request(struct fuse_
  * Send a request in the background
  */
 void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
-
-void fuse_request_send_background_nocheck(struct fuse_conn *fc,
-					  struct fuse_req *req);
+bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
 
 /* Abort all requests */
 void fuse_abort_conn(struct fuse_conn *fc, bool is_abort);
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1487,6 +1487,7 @@ __acquires(fc->lock)
 	struct fuse_inode *fi = get_fuse_inode(req->inode);
 	struct fuse_write_in *inarg = &req->misc.write.in;
 	__u64 data_size = req->num_pages * PAGE_SIZE;
+	bool queued;
 
 	if (!fc->connected)
 		goto out_free;
@@ -1502,7 +1503,8 @@ __acquires(fc->lock)
 
 	req->in.args[1].size = inarg->size;
 	fi->writectr++;
-	fuse_request_send_background_nocheck(fc, req);
+	queued = fuse_request_queue_background(fc, req);
+	WARN_ON(!queued);
 	return;
 
  out_free:

Re: [PATCH 6/6] fuse: Do not take fuse_conn::lock on fuse_request_send_background()

[Kirill Tkhai]

On 26.09.2018 15:25, Miklos Szeredi wrote:
> On Mon, Aug 27, 2018 at 06:29:56PM +0300, Kirill Tkhai wrote:
>> Currently, we take fc->lock there only to check for fc->connected.
>> But this flag is changed only on connection abort, which is very
>> rare operation. Good thing looks to make fuse_request_send_background()
>> faster, while fuse_abort_conn() slowler.
>>
>> So, we make fuse_request_send_background() lockless and mark
>> (fc->connected == 1) region as RCU-protected. Abort function
>> just uses synchronize_sched() to wait till all pending background
>> requests is being queued, and then makes ordinary abort.
>>
>> Note, that synchronize_sched() is used instead of synchronize_rcu(),
>> since we want to check for fc->connected without rcu_dereference()
>> in fuse_request_send_background() (i.e., not to add memory barriers
>> to this hot path).
> 
> Apart from the inaccuracies in the above (_sched variant is for scheduling and
> NMI taking code; _sched variant requires rcu_dereference() as well;
> rcu_dereference() does not add barriers; rcu_dereference() is only for pointers,
> so we can't use it for an integer),

Writing this I was inspired by expand_fdtable(). Yes, the description confuses,
and we don't need rcu_dereference() since we do not touch memory pointed by __rcu
pointer, we have no pointer at all. synchronize_sched() guarantees:

  On systems with more than one CPU, when synchronize_sched() returns,
  each CPU is guaranteed to have executed a full memory barrier since the
  end of its last RCU-sched read-side critical section whose beginning
  preceded the call to synchronize_sched().

(and rcu_dereference() unfolds in smp_read_barrier_depends(), which I mean as
 added barriers)

But it does not so matter. I'm OK with the patch you updated.

>wouldn't it be simpler to just use bg_lock
> for checking ->connected, and lock bg_lock (as well as fc->lock) when setting
> ->connected?
> 
> Updated patch below (untested).

Tested it. Works for me.

Thanks,
Kirill

> 
> ---
> Subject: fuse: do not take fc->lock in fuse_request_send_background()
> From: Kirill Tkhai <ktkhai@virtuozzo.com>
> Date: Mon, 27 Aug 2018 18:29:56 +0300
> 
> Currently, we take fc->lock there only to check for fc->connected.
> But this flag is changed only on connection abort, which is very
> rare operation.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/fuse/dev.c    |   46 +++++++++++++++++++++++-----------------------
>  fs/fuse/file.c   |    4 +++-
>  fs/fuse/fuse_i.h |    4 +---
>  3 files changed, 27 insertions(+), 27 deletions(-)
> 
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -574,42 +574,38 @@ ssize_t fuse_simple_request(struct fuse_
>  	return ret;
>  }
>  
> -/*
> - * Called under fc->lock
> - *
> - * fc->connected must have been checked previously
> - */
> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
> -					  struct fuse_req *req)
> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
>  {
> -	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
> +	bool queued = false;
> +
> +	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
>  	if (!test_bit(FR_WAITING, &req->flags)) {
>  		__set_bit(FR_WAITING, &req->flags);
>  		atomic_inc(&fc->num_waiting);
>  	}
>  	__set_bit(FR_ISREPLY, &req->flags);
>  	spin_lock(&fc->bg_lock);
> -	fc->num_background++;
> -	if (fc->num_background == fc->max_background)
> -		fc->blocked = 1;
> -	if (fc->num_background == fc->congestion_threshold && fc->sb) {
> -		set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
> -		set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
> +	if (likely(fc->connected)) {
> +		fc->num_background++;
> +		if (fc->num_background == fc->max_background)
> +			fc->blocked = 1;
> +		if (fc->num_background == fc->congestion_threshold && fc->sb) {
> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
> +		}
> +		list_add_tail(&req->list, &fc->bg_queue);
> +		flush_bg_queue(fc);
> +		queued = true;
>  	}
> -	list_add_tail(&req->list, &fc->bg_queue);
> -	flush_bg_queue(fc);
>  	spin_unlock(&fc->bg_lock);
> +
> +	return queued;
>  }
>  
>  void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
>  {
> -	BUG_ON(!req->end);
> -	spin_lock(&fc->lock);
> -	if (fc->connected) {
> -		fuse_request_send_background_nocheck(fc, req);
> -		spin_unlock(&fc->lock);
> -	} else {
> -		spin_unlock(&fc->lock);
> +	WARN_ON(!req->end);
> +	if (!fuse_request_queue_background(fc, req)) {
>  		req->out.h.error = -ENOTCONN;
>  		req->end(fc, req);
>  		fuse_put_request(fc, req);
> @@ -2112,7 +2108,11 @@ void fuse_abort_conn(struct fuse_conn *f
>  		struct fuse_req *req, *next;
>  		LIST_HEAD(to_end);
>  
> +		/* Background queuing checks fc->connected under bg_lock */
> +		spin_lock(&fc->bg_lock);
>  		fc->connected = 0;
> +		spin_unlock(&fc->bg_lock);
> +
>  		fc->aborted = is_abort;
>  		fuse_set_initialized(fc);
>  		list_for_each_entry(fud, &fc->devices, entry) {
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -863,9 +863,7 @@ ssize_t fuse_simple_request(struct fuse_
>   * Send a request in the background
>   */
>  void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
> -
> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
> -					  struct fuse_req *req);
> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
>  
>  /* Abort all requests */
>  void fuse_abort_conn(struct fuse_conn *fc, bool is_abort);
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1487,6 +1487,7 @@ __acquires(fc->lock)
>  	struct fuse_inode *fi = get_fuse_inode(req->inode);
>  	struct fuse_write_in *inarg = &req->misc.write.in;
>  	__u64 data_size = req->num_pages * PAGE_SIZE;
> +	bool queued;
>  
>  	if (!fc->connected)
>  		goto out_free;
> @@ -1502,7 +1503,8 @@ __acquires(fc->lock)
>  
>  	req->in.args[1].size = inarg->size;
>  	fi->writectr++;
> -	fuse_request_send_background_nocheck(fc, req);
> +	queued = fuse_request_queue_background(fc, req);
> +	WARN_ON(!queued);
>  	return;
>  
>   out_free:
> 

Re: [PATCH 6/6] fuse: Do not take fuse_conn::lock on fuse_request_send_background()

[Kirill Tkhai]

Hi, Miklos,

should I resend the series with the patch you changed,
or you are already taken it since there is your SoB?

Thanks,
Kirill

On 26.09.2018 18:18, Kirill Tkhai wrote:
> On 26.09.2018 15:25, Miklos Szeredi wrote:
>> On Mon, Aug 27, 2018 at 06:29:56PM +0300, Kirill Tkhai wrote:
>>> Currently, we take fc->lock there only to check for fc->connected.
>>> But this flag is changed only on connection abort, which is very
>>> rare operation. Good thing looks to make fuse_request_send_background()
>>> faster, while fuse_abort_conn() slowler.
>>>
>>> So, we make fuse_request_send_background() lockless and mark
>>> (fc->connected == 1) region as RCU-protected. Abort function
>>> just uses synchronize_sched() to wait till all pending background
>>> requests is being queued, and then makes ordinary abort.
>>>
>>> Note, that synchronize_sched() is used instead of synchronize_rcu(),
>>> since we want to check for fc->connected without rcu_dereference()
>>> in fuse_request_send_background() (i.e., not to add memory barriers
>>> to this hot path).
>>
>> Apart from the inaccuracies in the above (_sched variant is for scheduling and
>> NMI taking code; _sched variant requires rcu_dereference() as well;
>> rcu_dereference() does not add barriers; rcu_dereference() is only for pointers,
>> so we can't use it for an integer),
> 
> Writing this I was inspired by expand_fdtable(). Yes, the description confuses,
> and we don't need rcu_dereference() since we do not touch memory pointed by __rcu
> pointer, we have no pointer at all. synchronize_sched() guarantees:
> 
>   On systems with more than one CPU, when synchronize_sched() returns,
>   each CPU is guaranteed to have executed a full memory barrier since the
>   end of its last RCU-sched read-side critical section whose beginning
>   preceded the call to synchronize_sched().
> 
> (and rcu_dereference() unfolds in smp_read_barrier_depends(), which I mean as
>  added barriers)
> 
> But it does not so matter. I'm OK with the patch you updated.
> 
>> wouldn't it be simpler to just use bg_lock
>> for checking ->connected, and lock bg_lock (as well as fc->lock) when setting
>> ->connected?
>>
>> Updated patch below (untested).
> 
> Tested it. Works for me.
> 
> Thanks,
> Kirill
> 
>>
>> ---
>> Subject: fuse: do not take fc->lock in fuse_request_send_background()
>> From: Kirill Tkhai <ktkhai@virtuozzo.com>
>> Date: Mon, 27 Aug 2018 18:29:56 +0300
>>
>> Currently, we take fc->lock there only to check for fc->connected.
>> But this flag is changed only on connection abort, which is very
>> rare operation.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>> ---
>>  fs/fuse/dev.c    |   46 +++++++++++++++++++++++-----------------------
>>  fs/fuse/file.c   |    4 +++-
>>  fs/fuse/fuse_i.h |    4 +---
>>  3 files changed, 27 insertions(+), 27 deletions(-)
>>
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -574,42 +574,38 @@ ssize_t fuse_simple_request(struct fuse_
>>  	return ret;
>>  }
>>  
>> -/*
>> - * Called under fc->lock
>> - *
>> - * fc->connected must have been checked previously
>> - */
>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>> -					  struct fuse_req *req)
>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
>>  {
>> -	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
>> +	bool queued = false;
>> +
>> +	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>  	if (!test_bit(FR_WAITING, &req->flags)) {
>>  		__set_bit(FR_WAITING, &req->flags);
>>  		atomic_inc(&fc->num_waiting);
>>  	}
>>  	__set_bit(FR_ISREPLY, &req->flags);
>>  	spin_lock(&fc->bg_lock);
>> -	fc->num_background++;
>> -	if (fc->num_background == fc->max_background)
>> -		fc->blocked = 1;
>> -	if (fc->num_background == fc->congestion_threshold && fc->sb) {
>> -		set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
>> -		set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
>> +	if (likely(fc->connected)) {
>> +		fc->num_background++;
>> +		if (fc->num_background == fc->max_background)
>> +			fc->blocked = 1;
>> +		if (fc->num_background == fc->congestion_threshold && fc->sb) {
>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
>> +		}
>> +		list_add_tail(&req->list, &fc->bg_queue);
>> +		flush_bg_queue(fc);
>> +		queued = true;
>>  	}
>> -	list_add_tail(&req->list, &fc->bg_queue);
>> -	flush_bg_queue(fc);
>>  	spin_unlock(&fc->bg_lock);
>> +
>> +	return queued;
>>  }
>>  
>>  void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
>>  {
>> -	BUG_ON(!req->end);
>> -	spin_lock(&fc->lock);
>> -	if (fc->connected) {
>> -		fuse_request_send_background_nocheck(fc, req);
>> -		spin_unlock(&fc->lock);
>> -	} else {
>> -		spin_unlock(&fc->lock);
>> +	WARN_ON(!req->end);
>> +	if (!fuse_request_queue_background(fc, req)) {
>>  		req->out.h.error = -ENOTCONN;
>>  		req->end(fc, req);
>>  		fuse_put_request(fc, req);
>> @@ -2112,7 +2108,11 @@ void fuse_abort_conn(struct fuse_conn *f
>>  		struct fuse_req *req, *next;
>>  		LIST_HEAD(to_end);
>>  
>> +		/* Background queuing checks fc->connected under bg_lock */
>> +		spin_lock(&fc->bg_lock);
>>  		fc->connected = 0;
>> +		spin_unlock(&fc->bg_lock);
>> +
>>  		fc->aborted = is_abort;
>>  		fuse_set_initialized(fc);
>>  		list_for_each_entry(fud, &fc->devices, entry) {
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -863,9 +863,7 @@ ssize_t fuse_simple_request(struct fuse_
>>   * Send a request in the background
>>   */
>>  void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
>> -
>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>> -					  struct fuse_req *req);
>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
>>  
>>  /* Abort all requests */
>>  void fuse_abort_conn(struct fuse_conn *fc, bool is_abort);
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -1487,6 +1487,7 @@ __acquires(fc->lock)
>>  	struct fuse_inode *fi = get_fuse_inode(req->inode);
>>  	struct fuse_write_in *inarg = &req->misc.write.in;
>>  	__u64 data_size = req->num_pages * PAGE_SIZE;
>> +	bool queued;
>>  
>>  	if (!fc->connected)
>>  		goto out_free;
>> @@ -1502,7 +1503,8 @@ __acquires(fc->lock)
>>  
>>  	req->in.args[1].size = inarg->size;
>>  	fi->writectr++;
>> -	fuse_request_send_background_nocheck(fc, req);
>> +	queued = fuse_request_queue_background(fc, req);
>> +	WARN_ON(!queued);
>>  	return;
>>  
>>   out_free:
>>

Re: [PATCH 6/6] fuse: Do not take fuse_conn::lock on fuse_request_send_background()

[Miklos Szeredi]

On Thu, Sep 27, 2018 at 10:37 AM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> Hi, Miklos,
>
> should I resend the series with the patch you changed,
> or you are already taken it since there is your SoB?

I already took that series.

Thanks,
Miklos

Re: [PATCH 0/6] Extract bg queue logic out fuse_conn::lock

[Kirill Tkhai]

Hi, Miklos,

Have you seen this? Some reaction on this?

Thanks,
Kirill

On 8/27/18 6:29 PM, Kirill Tkhai wrote:
> Hi, Miklos,
> 
> the patchset makes background queue and background-related
> data be protected with new bg_lock. This allows async reads
> not take fc->lock in fuse_request_send_background(), and
> this gives performace-visible increase on out async read-write
> test (1.7-5 % better). See [6/6] for the test results.
> 
> The new lock protects max_background, congestion_threshold,
> num_background, active_background, bg_queue and blocked
> fields. See [5/6] for the details.
> 
> Patches [1-3/6] are preparations. I've already sent you
> two of them, and you said at least one is already applied,
> but since they are yet no in linux-next, I add them to make
> kbuild test robot happy.
> 
> [4/6] is a fix of existing issue. I don't know whether
> stable needs this fix, so I don't send it in separate. Please,
> say if it's needed resend it in this way.
> 
> The test results is for our proprietary vstorage fuse driver.
> Feel free to ask me, in case of you want the patchset to be
> tested on some other fuse driver and with some other test.
> 
> Thanks,
> Kirill
> ---
> 
> Kirill Tkhai (6):
>       fuse: Use list_first_entry() in flush_bg_queue()
>       fuse: Move clear_bit() up in request_end()
>       fuse: Underline congestion_threshold and max_background may be read w/o fc->lock
>       fuse: Lock fc->lock during changing num_background and congestion_threshold
>       fs: Introduce fuse_conn::bg_lock
>       fuse: Do not take fuse_conn::lock on fuse_request_send_background()
> 
> 
>  fs/fuse/control.c |   34 ++++++++++++++++++++++++++--------
>  fs/fuse/dev.c     |   42 +++++++++++++++++++++++++++---------------
>  fs/fuse/file.c    |    2 +-
>  fs/fuse/fuse_i.h  |   11 +++++++++--
>  fs/fuse/inode.c   |    4 ++++
>  5 files changed, 67 insertions(+), 26 deletions(-)
> 
> --
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> 

Re: [PATCH 0/6] Extract bg queue logic out fuse_conn::lock

[Miklos Szeredi]

On Tue, Sep 11, 2018 at 12:14 PM, Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> Hi, Miklos,
>
> Have you seen this? Some reaction on this?

Applied and pushed to

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next

Thanks,
Miklos