We do several stages of merging in the block layer - the most likely one
to work is also the cheap one, merging direct in the per-task plug when
IO is submitted. Getting merges outside of that is a lot less likely,
but IO schedulers may still maintain internal data structures to
facilitate merge lookups outside of the plug.
Make BFQ skip expensive merge lookups if the queue lock or bfqd lock is
already contended. The likelihood of getting a merge here is not very
high, hence it should not be a problem skipping the attempt in the also
unlikely event that either the queue or bfqd are already contended.
Perf diff shows the difference between a random read/write workload
with 4 threads doing IO, with expensive merges turned on and off:
31.70% +54.80% [kernel.kallsyms] [k] queued_spin_lock_slowpath
where we almost tripple the lock contention (~32% -> ~87%) by attempting
these expensive merges, and performance drops from 1630K to 1050K IOPS.
At the same time, sys time drops from 37% to 14%.
Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
---
block/bfq-iosched.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 5ef4a4eba572..ea16a0c53082 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -467,6 +467,21 @@ static struct bfq_io_cq *bfq_bic_lookup(struct request_queue *q)
return icq;
}
+static struct bfq_io_cq *bfq_bic_try_lookup(struct request_queue *q)
+{
+ if (!current->io_context)
+ return NULL;
+ if (spin_trylock_irq(&q->queue_lock)) {
+ struct bfq_io_cq *icq;
+
+ icq = icq_to_bic(ioc_lookup_icq(q));
+ spin_unlock_irq(&q->queue_lock);
+ return icq;
+ }
+
+ return NULL;
+}
+
/*
* Scheduler run of queue, if there are requests pending and no one in the
* driver that will restart queueing.
@@ -2454,10 +2469,21 @@ static bool bfq_bio_merge(struct request_queue *q, struct bio *bio,
* returned by bfq_bic_lookup does not go away before
* bfqd->lock is taken.
*/
- struct bfq_io_cq *bic = bfq_bic_lookup(q);
+ struct bfq_io_cq *bic = bfq_bic_try_lookup(q);
bool ret;
- spin_lock_irq(&bfqd->lock);
+ /*
+ * bio merging is called for every bio queued, and it's very easy
+ * to run into contention because of that. If we fail getting
+ * the dd lock, just skip this merge attempt. For related IO, the
+ * plug will be the successful merging point. If we get here, we
+ * already failed doing the obvious merge. Chances of actually
+ * getting a merge off this path is a lot slimmer, so skipping an
+ * occassional lookup that will most likely not succeed anyway should
+ * not be a problem.
+ */
+ if (!spin_trylock_irq(&bfqd->lock))
+ return false;
if (bic) {
/*
--
2.43.0
