v2: Changes in [1-2,5] patches. The biggest change is in [5],
where repeater FR_INTERRUPTED assignment is removed.
This patchset consists of several parts. Patches [1-2] optimize
likely case of request_end(), and make this function to take
fiq->waitq.lock only, when it is really needed. This improves
performance of this function completes.
Patch 3 makes request_end() to call wake_up() only for not
background requests (background requests never wait answer),
and this optimizes this function a bit more
Patches [4-5] makes code to check userspace requests correct
interrupt id to requeue (whether we've sent this interrupt).
---
Kirill Tkhai (5):
fuse: Kill fasync only if interrupt is queued in queue_interrupt()
fuse: Optimize request_end() by not taking fiq->waitq.lock
fuse: Wake up req->waitq of only not background requests in request_end()
fuse: Do some refactoring in fuse_dev_do_write()
fuse: Verify userspace asks to requeue interrupt that we really sent
fs/fuse/dev.c | 57 ++++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 38 insertions(+), 19 deletions(-)
--
Signed-off-by: Kirill Tkhai <[email protected]>
We take global fiq->waitq.lock every time, when we are
in this function, but interrupted requests are just small
subset of all requests. This patch optimizes request_end()
and makes it to take the lock when it's really needed.
queue_interrupt() needs small change for that. After req
is linked to interrupt list, we do smp_mb() and check
for FR_FINISHED again. In case of FR_FINISHED bit has
appeared, we remove req and leave the function:
CPU 0 CPU 1
queue_interrupt() request_end()
spin_lock(&fiq->waitq.lock)
list_add_tail(&req->intr_entry, &fiq->interrupts) test_and_set_bit(FR_FINISHED, &req->flags)
smp_mb() <memory barrier implied test_and_set_bit()>
if (test_bit(FR_FINISHED, &req->flags)) if (!list_empty(&req->intr_entry))
list_del_init(&req->intr_entry) spin_lock(&fiq->waitq.lock)
list_del_init(&req->intr_entry)
Check the change is visible in perf report:
1)Firstly mount fusexmp_fh:
$fuse/example/.libs/fusexmp_fh mnt
2)Run test doing
futimes(fd, tv1);
futimes(fd, tv2);
in many threads endlessly.
3)perf record -g (all the system load)
Without the patch in request_end() we spend 62.58% of do_write() time:
(= 12.58 / 20.10 * 100%)
55,22% entry_SYSCALL_64
20,10% do_writev
...
18,08% fuse_dev_do_write
12,58% request_end
10,08% __wake_up_common_lock
1,97% queued_spin_lock_slowpath
1,31% fuse_copy_args
1,04% fuse_copy_one
0,85% queued_spin_lock_slowpath
With the patch, the perf report becomes better, and only 58.16%
of do_write() time we spend in request_end():
54,15% entry_SYSCALL_64
18,24% do_writev
...
16,25% fuse_dev_do_write
10,61% request_end
10,25% __wake_up_common_lock
1,34% fuse_copy_args
1,06% fuse_copy_one
0,88% queued_spin_lock_slowpath
v2: Remove unlocked test of FR_FINISHED from queue_interrupt()
Signed-off-by: Kirill Tkhai <[email protected]>
---
fs/fuse/dev.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 1ecec7fcb841..7374a23b1bc8 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -427,10 +427,16 @@ static void request_end(struct fuse_conn *fc, struct fuse_req *req)
if (test_and_set_bit(FR_FINISHED, &req->flags))
goto put_request;
-
- spin_lock(&fiq->waitq.lock);
- list_del_init(&req->intr_entry);
- spin_unlock(&fiq->waitq.lock);
+ /*
+ * test_and_set_bit() implies smp_mb() between bit
+ * changing and below intr_entry check. Pairs with
+ * smp_mb() from queue_interrupt().
+ */
+ if (!list_empty(&req->intr_entry)) {
+ spin_lock(&fiq->waitq.lock);
+ list_del_init(&req->intr_entry);
+ spin_unlock(&fiq->waitq.lock);
+ }
WARN_ON(test_bit(FR_PENDING, &req->flags));
WARN_ON(test_bit(FR_SENT, &req->flags));
if (test_bit(FR_BACKGROUND, &req->flags)) {
@@ -469,12 +475,18 @@ static void request_end(struct fuse_conn *fc, struct fuse_req *req)
static void queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req)
{
spin_lock(&fiq->waitq.lock);
- if (test_bit(FR_FINISHED, &req->flags)) {
- spin_unlock(&fiq->waitq.lock);
- return;
- }
if (list_empty(&req->intr_entry)) {
list_add_tail(&req->intr_entry, &fiq->interrupts);
+ /*
+ * Pairs with smp_mb() implied by test_and_set_bit()
+ * from request_end().
+ */
+ smp_mb();
+ if (test_bit(FR_FINISHED, &req->flags)) {
+ list_del_init(&req->intr_entry);
+ spin_unlock(&fiq->waitq.lock);
+ return;
+ }
wake_up_locked(&fiq->waitq);
kill_fasync(&fiq->fasync, SIGIO, POLL_IN);
}
This is needed for next patch.
Signed-off-by: Kirill Tkhai <[email protected]>
---
fs/fuse/dev.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index cc9e5a9bb147..7684fb7dc680 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1947,18 +1947,14 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
__fuse_get_request(req);
spin_unlock(&fpq->lock);
- err = -EINVAL;
- if (nbytes != sizeof(struct fuse_out_header)) {
- fuse_put_request(fc, req);
- goto err_finish;
- }
-
- if (oh.error == -ENOSYS)
+ if (nbytes != sizeof(struct fuse_out_header))
+ nbytes = -EINVAL;
+ else if (oh.error == -ENOSYS)
fc->no_interrupt = 1;
else if (oh.error == -EAGAIN)
queue_interrupt(&fc->iq, req);
- fuse_put_request(fc, req);
+ fuse_put_request(fc, req);
fuse_copy_finish(cs);
return nbytes;
}
When queue_interrupt() is called from fuse_dev_do_write(),
it came from userspace directly. Userspace may pass any
request id, even the request's we have not interrupted
(or even background's request). This patch adds sanity
check to make kernel safe against that.
v2: Keep in mind FR_INTERRUPTED is visible under fiq->waitq.lock
in requeuer.
Signed-off-by: Kirill Tkhai <[email protected]>
---
fs/fuse/dev.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 7684fb7dc680..403a2ebad468 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -475,9 +475,15 @@ static void request_end(struct fuse_conn *fc, struct fuse_req *req)
fuse_put_request(fc, req);
}
-static void queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req)
+static int queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req)
{
spin_lock(&fiq->waitq.lock);
+ /* Check for we've sent request to interrupt this req */
+ if (unlikely(!test_bit(FR_INTERRUPTED, &req->flags))) {
+ spin_unlock(&fiq->waitq.lock);
+ return -EINVAL;
+ }
+
if (list_empty(&req->intr_entry)) {
list_add_tail(&req->intr_entry, &fiq->interrupts);
/*
@@ -488,12 +494,13 @@ static void queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req)
if (test_bit(FR_FINISHED, &req->flags)) {
list_del_init(&req->intr_entry);
spin_unlock(&fiq->waitq.lock);
- return;
+ return 0;
}
wake_up_locked(&fiq->waitq);
kill_fasync(&fiq->fasync, SIGIO, POLL_IN);
}
spin_unlock(&fiq->waitq.lock);
+ return 0;
}
static void request_wait_answer(struct fuse_conn *fc, struct fuse_req *req)
@@ -1951,8 +1958,9 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
nbytes = -EINVAL;
else if (oh.error == -ENOSYS)
fc->no_interrupt = 1;
- else if (oh.error == -EAGAIN)
- queue_interrupt(&fc->iq, req);
+ else if (oh.error == -EAGAIN &&
+ queue_interrupt(&fc->iq, req) < 0)
+ nbytes = -EINVAL;
fuse_put_request(fc, req);
fuse_copy_finish(cs);
We should sent signal only in case of interrupt is really queued.
Not a real problem, but this makes the code clearer and intuitive.
v2: Move kill_fasync() under fiq->waitq.lock
Signed-off-by: Kirill Tkhai <[email protected]>
---
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 ae813e609932..1ecec7fcb841 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -476,9 +476,9 @@ static void queue_interrupt(struct fuse_iqueue *fiq, struct fuse_req *req)
if (list_empty(&req->intr_entry)) {
list_add_tail(&req->intr_entry, &fiq->interrupts);
wake_up_locked(&fiq->waitq);
+ kill_fasync(&fiq->fasync, SIGIO, POLL_IN);
}
spin_unlock(&fiq->waitq.lock);
- kill_fasync(&fiq->fasync, SIGIO, POLL_IN);
}
static void request_wait_answer(struct fuse_conn *fc, struct fuse_req *req)
Currently, we wait on req->waitq in request_wait_answer()
function only, and it's never used for background requests.
Since wake_up() is not a light-weight macros, instead of this,
it unfolds in really called function, which makes locking
operations taking some cpu cycles, let's avoid its call
for the case we definitely know it's completely useless.
Signed-off-by: Kirill Tkhai <[email protected]>
---
fs/fuse/dev.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 7374a23b1bc8..cc9e5a9bb147 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -464,8 +464,11 @@ static void request_end(struct fuse_conn *fc, struct fuse_req *req)
fc->active_background--;
flush_bg_queue(fc);
spin_unlock(&fc->bg_lock);
+ } else {
+ /* Wake up waiter sleeping in request_wait_answer() */
+ wake_up(&req->waitq);
}
- wake_up(&req->waitq);
+
if (req->end)
req->end(fc, req);
put_request:
ping
On 08.11.2018 12:05, Kirill Tkhai wrote:
> v2: Changes in [1-2,5] patches. The biggest change is in [5],
> where repeater FR_INTERRUPTED assignment is removed.
>
> This patchset consists of several parts. Patches [1-2] optimize
> likely case of request_end(), and make this function to take
> fiq->waitq.lock only, when it is really needed. This improves
> performance of this function completes.
>
> Patch 3 makes request_end() to call wake_up() only for not
> background requests (background requests never wait answer),
> and this optimizes this function a bit more
>
> Patches [4-5] makes code to check userspace requests correct
> interrupt id to requeue (whether we've sent this interrupt).
>
> ---
>
> Kirill Tkhai (5):
> fuse: Kill fasync only if interrupt is queued in queue_interrupt()
> fuse: Optimize request_end() by not taking fiq->waitq.lock
> fuse: Wake up req->waitq of only not background requests in request_end()
> fuse: Do some refactoring in fuse_dev_do_write()
> fuse: Verify userspace asks to requeue interrupt that we really sent
>
>
> fs/fuse/dev.c | 57 ++++++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 19 deletions(-)
>
> --
> Signed-off-by: Kirill Tkhai <[email protected]>
>
On Wed, Dec 26, 2018 at 2:25 PM Kirill Tkhai <[email protected]> wrote:
>
> ping
>
> On 08.11.2018 12:05, Kirill Tkhai wrote:
> > v2: Changes in [1-2,5] patches. The biggest change is in [5],
> > where repeater FR_INTERRUPTED assignment is removed.
> >
> > This patchset consists of several parts. Patches [1-2] optimize
> > likely case of request_end(), and make this function to take
> > fiq->waitq.lock only, when it is really needed. This improves
> > performance of this function completes.
> >
> > Patch 3 makes request_end() to call wake_up() only for not
> > background requests (background requests never wait answer),
> > and this optimizes this function a bit more
> >
> > Patches [4-5] makes code to check userspace requests correct
> > interrupt id to requeue (whether we've sent this interrupt).
Pushed to fuse.git#for-next.
Thanks,
Miklos