* Re: [PATCH v1 3/3] blk-mq: start to freeze queue just after setting dying
From: Bart Van Assche @ 2017-03-17 23:26 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, tom.leiming@gmail.com, axboe@fb.com
Cc: yizhan@redhat.com, tj@kernel.org
In-Reply-To: <20170317095711.5819-4-tom.leiming@gmail.com>
On Fri, 2017-03-17 at 17:57 +0800, Ming Lei wrote:
> Given blk_set_queue_dying() is always called in remove path
> of block device, and queue will be cleaned up later, we don't
> need to worry about undoing the counter.
>=20
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d772c221cc17..62d4967c369f 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -500,9 +500,12 @@ void blk_set_queue_dying(struct request_queue *q)
> queue_flag_set(QUEUE_FLAG_DYING, q);
> spin_unlock_irq(q->queue_lock);
> =20
> - if (q->mq_ops)
> + if (q->mq_ops) {
> blk_mq_wake_waiters(q);
> - else {
> +
> + /* block new I/O coming */
> + blk_mq_freeze_queue_start(q);
> + } else {
> struct request_list *rl;
> =20
> spin_lock_irq(q->queue_lock);
Hello Ming,
The blk_freeze_queue() call in blk_cleanup_queue() waits until q_usage_coun=
ter
drops to zero. Since the above blk_mq_freeze_queue_start() call increases t=
hat
counter by one, how is blk_freeze_queue() expected to finish ever?
Bart.=
^ permalink raw reply
* [RFC PATCH 0/4] blk-mq: multiqueue I/O scheduler
From: Omar Sandoval @ 2017-03-17 22:03 UTC (permalink / raw)
To: linux-block; +Cc: kernel-team
From: Omar Sandoval <osandov@fb.com>
This patch series implements an I/O scheduler for multiqueue devices
combining several techniques: the scalable bitmap library, the new
blk-stats API, and queue depth throttling similar to blk-wbt.
These patches are on top of my earlier blk-stats series [1]. They also
need a fix in Jens' for-linus branch in order to work properly [2].
Patches 1 and 2 implement a new sbitmap operation and patch 3 exports a
required function. Patch 4 implements the new scheduler, named Kyber.
The commit message in patch 4 describes Kyber in detail. The scheduler
employs some heuristics that I've experimented with, but that's probably
the area that needs the most work. I'll be at LSF/MM next week, and
there's currently a lightning talk on the schedule to discuss
improvements and extensions.
A quick test case that demonstrates the scheduler in action:
---
[global]
filename=/dev/nvme0n1
direct=1
runtime=10s
time_based
group_reporting
[writers]
numjobs=40
ioengine=libaio
iodepth=1024
rw=randwrite
[reader]
new_group
rate_iops=1000
ioengine=sync
rw=randread
---
With Kyber set to a target latency of 1 ms, the reader sees latencies of 8 ms
on average. Kyber brings this down to just over 1 ms.
Thanks!
1: http://marc.info/?l=linux-block&m=148952547205774&w=2
2: http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=efd4b81abbe1ac753717f2f10cd3dab8bed6c103
Omar Sandoval (4):
sbitmap: add sbitmap_get_shallow() operation
blk-mq: add shallow depth option for blk_mq_get_tag()
blk-mq: export blk_mq_finish_request()
blk-mq: introduce Kyber multiqueue I/O scheduler
block/Kconfig.iosched | 8 +
block/Makefile | 1 +
block/blk-mq-tag.c | 5 +-
block/blk-mq.c | 1 +
block/blk-mq.h | 1 +
block/elevator.c | 9 +-
block/kyber-iosched.c | 586 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/sbitmap.h | 55 +++++
lib/sbitmap.c | 75 ++++++-
9 files changed, 729 insertions(+), 12 deletions(-)
create mode 100644 block/kyber-iosched.c
--
2.12.0
^ permalink raw reply
* [RFC PATCH 3/4] blk-mq: export blk_mq_finish_request()
From: Omar Sandoval @ 2017-03-17 22:03 UTC (permalink / raw)
To: linux-block; +Cc: kernel-team
In-Reply-To: <cover.1489787289.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
This is required for schedulers that define their own put_request().
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-mq.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 559f9b0f24a1..94973103e809 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -370,6 +370,7 @@ void blk_mq_finish_request(struct request *rq)
{
blk_mq_finish_hctx_request(blk_mq_map_queue(rq->q, rq->mq_ctx->cpu), rq);
}
+EXPORT_SYMBOL_GPL(blk_mq_finish_request);
void blk_mq_free_request(struct request *rq)
{
--
2.12.0
^ permalink raw reply related
* [RFC PATCH 1/4] sbitmap: add sbitmap_get_shallow() operation
From: Omar Sandoval @ 2017-03-17 22:03 UTC (permalink / raw)
To: linux-block; +Cc: kernel-team
In-Reply-To: <cover.1489787289.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
This operation supports the use case of limiting the number of bits that
can be allocated for a given operation. Rather than setting aside some
bits at the end of the bitmap, we can set aside bits in each word of the
bitmap. This means we can keep the allocation hints spread out and
support sbitmap_resize() nicely at the cost of lower granularity for the
allowed depth.
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
include/linux/sbitmap.h | 55 ++++++++++++++++++++++++++++++++++++
lib/sbitmap.c | 75 ++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 123 insertions(+), 7 deletions(-)
diff --git a/include/linux/sbitmap.h b/include/linux/sbitmap.h
index d4e0a204c118..a1904aadbc45 100644
--- a/include/linux/sbitmap.h
+++ b/include/linux/sbitmap.h
@@ -176,6 +176,25 @@ void sbitmap_resize(struct sbitmap *sb, unsigned int depth);
int sbitmap_get(struct sbitmap *sb, unsigned int alloc_hint, bool round_robin);
/**
+ * sbitmap_get_shallow() - Try to allocate a free bit from a &struct sbitmap,
+ * limiting the depth used from each word.
+ * @sb: Bitmap to allocate from.
+ * @alloc_hint: Hint for where to start searching for a free bit.
+ * @shallow_depth: The maximum number of bits to allocate from a single word.
+ *
+ * This rather specific operation allows for having multiple users with
+ * different allocation limits. E.g., there can be a high-priority class that
+ * uses sbitmap_get() and a low-priority class that uses sbitmap_get_shallow()
+ * with a @shallow_depth of (1 << (@sb->shift - 1)). Then, the low-priority
+ * class can only allocate half of the total bits in the bitmap, preventing it
+ * from starving out the high-priority class.
+ *
+ * Return: Non-negative allocated bit number if successful, -1 otherwise.
+ */
+int sbitmap_get_shallow(struct sbitmap *sb, unsigned int alloc_hint,
+ unsigned long shallow_depth);
+
+/**
* sbitmap_any_bit_set() - Check for a set bit in a &struct sbitmap.
* @sb: Bitmap to check.
*
@@ -326,6 +345,19 @@ void sbitmap_queue_resize(struct sbitmap_queue *sbq, unsigned int depth);
int __sbitmap_queue_get(struct sbitmap_queue *sbq);
/**
+ * __sbitmap_queue_get_shallow() - Try to allocate a free bit from a &struct
+ * sbitmap_queue, limiting the depth used from each word, with preemption
+ * already disabled.
+ * @sbq: Bitmap queue to allocate from.
+ * @shallow_depth: The maximum number of bits to allocate from a single word.
+ * See sbitmap_get_shallow().
+ *
+ * Return: Non-negative allocated bit number if successful, -1 otherwise.
+ */
+int __sbitmap_queue_get_shallow(struct sbitmap_queue *sbq,
+ unsigned int shallow_depth);
+
+/**
* sbitmap_queue_get() - Try to allocate a free bit from a &struct
* sbitmap_queue.
* @sbq: Bitmap queue to allocate from.
@@ -346,6 +378,29 @@ static inline int sbitmap_queue_get(struct sbitmap_queue *sbq,
}
/**
+ * sbitmap_queue_get_shallow() - Try to allocate a free bit from a &struct
+ * sbitmap_queue, limiting the depth used from each word.
+ * @sbq: Bitmap queue to allocate from.
+ * @cpu: Output parameter; will contain the CPU we ran on (e.g., to be passed to
+ * sbitmap_queue_clear()).
+ * @shallow_depth: The maximum number of bits to allocate from a single word.
+ * See sbitmap_get_shallow().
+ *
+ * Return: Non-negative allocated bit number if successful, -1 otherwise.
+ */
+static inline int sbitmap_queue_get_shallow(struct sbitmap_queue *sbq,
+ unsigned int *cpu,
+ unsigned int shallow_depth)
+{
+ int nr;
+
+ *cpu = get_cpu();
+ nr = __sbitmap_queue_get_shallow(sbq, shallow_depth);
+ put_cpu();
+ return nr;
+}
+
+/**
* sbitmap_queue_clear() - Free an allocated bit and wake up waiters on a
* &struct sbitmap_queue.
* @sbq: Bitmap to free from.
diff --git a/lib/sbitmap.c b/lib/sbitmap.c
index 60e800e0b5a0..80aa8d5463fa 100644
--- a/lib/sbitmap.c
+++ b/lib/sbitmap.c
@@ -79,15 +79,15 @@ void sbitmap_resize(struct sbitmap *sb, unsigned int depth)
}
EXPORT_SYMBOL_GPL(sbitmap_resize);
-static int __sbitmap_get_word(struct sbitmap_word *word, unsigned int hint,
- bool wrap)
+static int __sbitmap_get_word(unsigned long *word, unsigned long depth,
+ unsigned int hint, bool wrap)
{
unsigned int orig_hint = hint;
int nr;
while (1) {
- nr = find_next_zero_bit(&word->word, word->depth, hint);
- if (unlikely(nr >= word->depth)) {
+ nr = find_next_zero_bit(word, depth, hint);
+ if (unlikely(nr >= depth)) {
/*
* We started with an offset, and we didn't reset the
* offset to 0 in a failure case, so start from 0 to
@@ -100,11 +100,11 @@ static int __sbitmap_get_word(struct sbitmap_word *word, unsigned int hint,
return -1;
}
- if (!test_and_set_bit(nr, &word->word))
+ if (!test_and_set_bit(nr, word))
break;
hint = nr + 1;
- if (hint >= word->depth - 1)
+ if (hint >= depth - 1)
hint = 0;
}
@@ -119,7 +119,8 @@ int sbitmap_get(struct sbitmap *sb, unsigned int alloc_hint, bool round_robin)
index = SB_NR_TO_INDEX(sb, alloc_hint);
for (i = 0; i < sb->map_nr; i++) {
- nr = __sbitmap_get_word(&sb->map[index],
+ nr = __sbitmap_get_word(&sb->map[index].word,
+ sb->map[index].depth,
SB_NR_TO_BIT(sb, alloc_hint),
!round_robin);
if (nr != -1) {
@@ -141,6 +142,37 @@ int sbitmap_get(struct sbitmap *sb, unsigned int alloc_hint, bool round_robin)
}
EXPORT_SYMBOL_GPL(sbitmap_get);
+int sbitmap_get_shallow(struct sbitmap *sb, unsigned int alloc_hint,
+ unsigned long shallow_depth)
+{
+ unsigned int i, index;
+ int nr = -1;
+
+ index = SB_NR_TO_INDEX(sb, alloc_hint);
+
+ for (i = 0; i < sb->map_nr; i++) {
+ nr = __sbitmap_get_word(&sb->map[index].word,
+ min(sb->map[index].depth, shallow_depth),
+ SB_NR_TO_BIT(sb, alloc_hint), true);
+ if (nr != -1) {
+ nr += index << sb->shift;
+ break;
+ }
+
+ /* Jump to next index. */
+ index++;
+ alloc_hint = index << sb->shift;
+
+ if (index >= sb->map_nr) {
+ index = 0;
+ alloc_hint = 0;
+ }
+ }
+
+ return nr;
+}
+EXPORT_SYMBOL_GPL(sbitmap_get_shallow);
+
bool sbitmap_any_bit_set(const struct sbitmap *sb)
{
unsigned int i;
@@ -342,6 +374,35 @@ int __sbitmap_queue_get(struct sbitmap_queue *sbq)
}
EXPORT_SYMBOL_GPL(__sbitmap_queue_get);
+int __sbitmap_queue_get_shallow(struct sbitmap_queue *sbq,
+ unsigned int shallow_depth)
+{
+ unsigned int hint, depth;
+ int nr;
+
+ hint = this_cpu_read(*sbq->alloc_hint);
+ depth = READ_ONCE(sbq->sb.depth);
+ if (unlikely(hint >= depth)) {
+ hint = depth ? prandom_u32() % depth : 0;
+ this_cpu_write(*sbq->alloc_hint, hint);
+ }
+ nr = sbitmap_get_shallow(&sbq->sb, hint, shallow_depth);
+
+ if (nr == -1) {
+ /* If the map is full, a hint won't do us much good. */
+ this_cpu_write(*sbq->alloc_hint, 0);
+ } else if (nr == hint || unlikely(sbq->round_robin)) {
+ /* Only update the hint if we used it. */
+ hint = nr + 1;
+ if (hint >= depth - 1)
+ hint = 0;
+ this_cpu_write(*sbq->alloc_hint, hint);
+ }
+
+ return nr;
+}
+EXPORT_SYMBOL_GPL(__sbitmap_queue_get_shallow);
+
static struct sbq_wait_state *sbq_wake_ptr(struct sbitmap_queue *sbq)
{
int i, wake_index;
--
2.12.0
^ permalink raw reply related
* Re: [PATCH v1 3/3] blk-mq: start to freeze queue just after setting dying
From: Bart Van Assche @ 2017-03-17 20:36 UTC (permalink / raw)
To: tom.leiming@gmail.com
Cc: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, yizhan@redhat.com, axboe@fb.com,
tj@kernel.org
In-Reply-To: <CACVXFVPF0Sx5HyU+xw0MyghYMWifZ0SDH0YCtM-jrhieOJOJGg@mail.gmail.com>
On Sat, 2017-03-18 at 02:32 +0800, Ming Lei wrote:
> On Sat, Mar 18, 2017 at 1:32 AM, Bart Van Assche wrote:
> > + /*
> > + * Avoid that the updates of the queue flags and q_usage_counte=
r
> > + * are reordered.
> > + */
> > + smp_wmb();
>=20
> atomic_inc_return() in blk_mq_freeze_queue_start() does imply a
> barrier(smp_mb()).
Hello Ming,
It's probably a good idea to mention that in a comment. The implementation
of blk_mq_freeze_queue_start() namely could be changed in the future such
that it uses another atomic operation that doesn't implicitly perform smp_m=
b().
Thanks,
Bart.=
^ permalink raw reply
* Re: [PATCH v1 3/3] blk-mq: start to freeze queue just after setting dying
From: Ming Lei @ 2017-03-17 18:32 UTC (permalink / raw)
To: Bart Van Assche
Cc: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, axboe@fb.com, yizhan@redhat.com,
tj@kernel.org
In-Reply-To: <1489771915.2826.4.camel@sandisk.com>
On Sat, Mar 18, 2017 at 1:32 AM, Bart Van Assche
<Bart.VanAssche@sandisk.com> wrote:
> On Fri, 2017-03-17 at 17:57 +0800, Ming Lei wrote:
>> Before commit 780db2071a(blk-mq: decouble blk-mq freezing
>> from generic bypassing), the dying flag is checked before
>> entering queue, and Tejun converts the checking into .mq_freeze_depth,
>> and assumes the counter is increased just after dying flag
>> is set. Unfortunately we doesn't do that in blk_set_queue_dying().
>>
>> This patch calls blk_mq_freeze_queue_start() for blk-mq in
>> blk_set_queue_dying(), so that we can block new I/O coming
>> once the queue is set as dying.
>>
>> Given blk_set_queue_dying() is always called in remove path
>> of block device, and queue will be cleaned up later, we don't
>> need to worry about undoing the counter.
>>
>> Cc: Bart Van Assche <bart.vanassche@sandisk.com>
>> Cc: Tejun Heo <tj@kernel.org>
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> ---
>> block/blk-core.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index d772c221cc17..62d4967c369f 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -500,9 +500,12 @@ void blk_set_queue_dying(struct request_queue *q)
>> queue_flag_set(QUEUE_FLAG_DYING, q);
>> spin_unlock_irq(q->queue_lock);
>>
>> - if (q->mq_ops)
>> + if (q->mq_ops) {
>> blk_mq_wake_waiters(q);
>> - else {
>> +
>> + /* block new I/O coming */
>> + blk_mq_freeze_queue_start(q);
>> + } else {
>> struct request_list *rl;
>>
>> spin_lock_irq(q->queue_lock);
>
> Hello Ming,
>
> I think we need the queue freezing not only for blk-mq but also for blk-sq.
Yes, we can, but it may not be a big deal for blk-sq, since get_request() does
check the dying flag.
> Since the queue flags and the mq_freeze_depth are stored in different
> variables we need to prevent that the CPU reorders the stores to these
Not needed, see below.
> variables. The comment about blk_mq_freeze_queue_start() should be more
> clear. How about something like the patch below?
>
>
> [PATCH] blk-mq: Force block layer users to check the "dying" flag after it has been set
>
> Commit 780db2071ac4 removed the blk_queue_dying() check from the
> hot path of blk_mq_queue_enter() although that check is necessary
> when cleaning up a queue. Hence make sure that blk_queue_enter()
> and blk_mq_queue_enter() check the dying flag after it has been set.
>
> Because blk_set_queue_dying() is only called from the remove path
> of a block device we don't need to worry about unfreezing the queue.
>
> Fixes: commit 780db2071ac4 ("blk-mq: decouble blk-mq freezing from generic bypassing")
> ---
> block/blk-core.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d772c221cc17..730f715b72ff 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -500,6 +500,19 @@ void blk_set_queue_dying(struct request_queue *q)
> queue_flag_set(QUEUE_FLAG_DYING, q);
> spin_unlock_irq(q->queue_lock);
>
> + /*
> + * Avoid that the updates of the queue flags and q_usage_counter
> + * are reordered.
> + */
> + smp_wmb();
atomic_inc_return() in blk_mq_freeze_queue_start() does imply a
barrier(smp_mb()).
> +
> + /*
> + * Force blk_queue_enter() and blk_mq_queue_enter() to check the
> + * "dying" flag. Despite its name, blk_mq_freeze_queue_start()
> + * affects blk-sq and blk-mq queues.
> + */
> + blk_mq_freeze_queue_start(q);
We need to change the name into blk_freeze_queue_start(), since it is quite
confusing to call a _mq function for both mq and legacy.
I will make it for both in v2, if no one objects.
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH v1 2/3] blk-mq: comment on races related with timeout handler
From: Ming Lei @ 2017-03-17 18:23 UTC (permalink / raw)
To: Bart Van Assche
Cc: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, axboe@fb.com, yizhan@redhat.com
In-Reply-To: <1489772332.2826.6.camel@sandisk.com>
On Sat, Mar 18, 2017 at 1:39 AM, Bart Van Assche
<Bart.VanAssche@sandisk.com> wrote:
> On Fri, 2017-03-17 at 17:57 +0800, Ming Lei wrote:
>> +/*
>> + * When we reach here because queue is busy, REQ_ATOM_COMPLETE
>> + * flag isn't set yet, so there may be race with timeout hanlder,
>> + * but given rq->deadline is just set in .queue_rq() under
>> + * this sitation, the race won't be possible in reality because
>> + * rq->timeout should be set as big enough to cover the window
>> + * between blk_mq_start_request() called from .queue_rq() and
>> + * clearing REQ_ATOM_STARTED here.
>> + */
>> static void __blk_mq_requeue_request(struct request *rq)
>> {
>> struct request_queue *q = rq->q;
>> @@ -700,6 +709,19 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
>> if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
>> return;
>>
>> + /*
>> + * The rq being checked may have been freed and reallocated
>> + * out already here, we avoid this race by checking rq->deadline
>> + * and REQ_ATOM_COMPLETE flag together:
>> + *
>> + * - if rq->deadline is observed as new value because of
>> + * reusing, the rq won't be timed out because of timing.
>> + * - if rq->deadline is observed as previous value,
>> + * REQ_ATOM_COMPLETE flag won't be cleared in reuse path
>> + * because we put a barrier between setting rq->deadline
>> + * and clearing the flag in blk_mq_start_request(), so
>> + * this rq won't be timed out too.
>> + */
>> if (time_after_eq(jiffies, rq->deadline)) {
>> if (!blk_mark_rq_complete(rq))
>> blk_mq_rq_timed_out(rq, reserved);
>
> Since this explanation applies to the same race addressed by patch 1/3,
First, this explains how we deal with the race of reuse vs. timeout, and 1/3
fixes another race or rq corruption. Did you see anywhere I mentioned STARTED
flag in above comment?
In case of 1/3, the rq to be dispatched can be destroyed simply by the
blk_mq_end_request() from timeout. Or even it can survive, the same rq
can be allocated into another I/O path, and this situation is different with
reuse vs. timeout. And I can't see any help from the comment for explaining
1/3's issue, can you? Maybe I need to mention rq corruption in 1/3 explicitly.
Secondly introducing this comment to 1/3 just causes unnecessary
backporting burden, as we have to make it into -stable.
> please consider squashing this patch into patch 1/3.
So please do not consider that.
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH v1 2/3] blk-mq: comment on races related with timeout handler
From: Bart Van Assche @ 2017-03-17 17:39 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, tom.leiming@gmail.com, axboe@fb.com
Cc: yizhan@redhat.com
In-Reply-To: <20170317095711.5819-3-tom.leiming@gmail.com>
On Fri, 2017-03-17 at 17:57 +0800, Ming Lei wrote:
> +/*
> + * When we reach here because queue is busy, REQ_ATOM_COMPLETE
> + * flag isn't set yet, so there may be race with timeout hanlder,
> + * but given rq->deadline is just set in .queue_rq() under
> + * this sitation, the race won't be possible in reality because
> + * rq->timeout should be set as big enough to cover the window
> + * between blk_mq_start_request() called from .queue_rq() and
> + * clearing REQ_ATOM_STARTED here.
> + */
> static void __blk_mq_requeue_request(struct request *rq)
> {
> struct request_queue *q =3D rq->q;
> @@ -700,6 +709,19 @@ static void blk_mq_check_expired(struct blk_mq_hw_ct=
x *hctx,
> if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> return;
> =20
> + /*
> + * The rq being checked may have been freed and reallocated
> + * out already here, we avoid this race by checking rq->deadline
> + * and REQ_ATOM_COMPLETE flag together:
> + *
> + * - if rq->deadline is observed as new value because of
> + * reusing, the rq won't be timed out because of timing.
> + * - if rq->deadline is observed as previous value,
> + * REQ_ATOM_COMPLETE flag won't be cleared in reuse path
> + * because we put a barrier between setting rq->deadline
> + * and clearing the flag in blk_mq_start_request(), so
> + * this rq won't be timed out too.
> + */
> if (time_after_eq(jiffies, rq->deadline)) {
> if (!blk_mark_rq_complete(rq))
> blk_mq_rq_timed_out(rq, reserved);
Since this explanation applies to the same race addressed by patch 1/3,
please consider squashing this patch into patch 1/3.
Thanks,
Bart.=
^ permalink raw reply
* Re: [PATCH v1 3/3] blk-mq: start to freeze queue just after setting dying
From: Bart Van Assche @ 2017-03-17 17:32 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, tom.leiming@gmail.com, axboe@fb.com
Cc: yizhan@redhat.com, tj@kernel.org
In-Reply-To: <20170317095711.5819-4-tom.leiming@gmail.com>
On Fri, 2017-03-17 at 17:57 +0800, Ming Lei wrote:
> Before commit 780db2071a(blk-mq: decouble blk-mq freezing
> from generic bypassing), the dying flag is checked before
> entering queue, and Tejun converts the checking into .mq_freeze_depth,
> and assumes the counter is increased just after dying flag
> is set. Unfortunately we doesn't do that in blk_set_queue_dying().
>=20
> This patch calls blk_mq_freeze_queue_start() for blk-mq in
> blk_set_queue_dying(), so that we can block new I/O coming
> once the queue is set as dying.
>=20
> Given blk_set_queue_dying() is always called in remove path
> of block device, and queue will be cleaned up later, we don't
> need to worry about undoing the counter.
>=20
> Cc: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
> block/blk-core.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>=20
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d772c221cc17..62d4967c369f 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -500,9 +500,12 @@ void blk_set_queue_dying(struct request_queue *q)
> queue_flag_set(QUEUE_FLAG_DYING, q);
> spin_unlock_irq(q->queue_lock);
> =20
> - if (q->mq_ops)
> + if (q->mq_ops) {
> blk_mq_wake_waiters(q);
> - else {
> +
> + /* block new I/O coming */
> + blk_mq_freeze_queue_start(q);
> + } else {
> struct request_list *rl;
> =20
> spin_lock_irq(q->queue_lock);
Hello Ming,
I think we need the queue freezing not only for blk-mq but also for blk-sq.
Since the queue flags and the mq_freeze_depth are stored in different
variables we need to prevent that the CPU reorders the stores to these
variables. The comment about=A0blk_mq_freeze_queue_start() should be more
clear. How about something like the patch below?
[PATCH] blk-mq: Force block layer users to check the "dying" flag=A0after i=
t has been set
Commit 780db2071ac4 removed the blk_queue_dying() check from the
hot path of blk_mq_queue_enter() although that check is necessary
when cleaning up a queue. Hence make sure that blk_queue_enter()
and blk_mq_queue_enter() check the dying flag after it has been set.
Because blk_set_queue_dying() is only called from the remove path
of a block device we don't need to worry about unfreezing the queue.
Fixes: commit 780db2071ac4 ("blk-mq: decouble blk-mq freezing from generic =
bypassing")
---
=A0block/blk-core.c | 13 +++++++++++++
=A01 file changed, 13 insertions(+)
diff --git a/block/blk-core.c b/block/blk-core.c
index d772c221cc17..730f715b72ff 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -500,6 +500,19 @@ void blk_set_queue_dying(struct request_queue *q)
=A0 queue_flag_set(QUEUE_FLAG_DYING, q);
=A0 spin_unlock_irq(q->queue_lock);
=A0
+ /*
+ =A0* Avoid that the updates of the queue flags and q_usage_counter
+ =A0* are reordered.
+ =A0*/
+ smp_wmb();
+
+ /*
+ =A0* Force blk_queue_enter() and blk_mq_queue_enter() to check the
+ =A0* "dying" flag. Despite its name, blk_mq_freeze_queue_start()
+ =A0* affects blk-sq and blk-mq queues.
+ =A0*/
+ blk_mq_freeze_queue_start(q);
+
=A0 if (q->mq_ops)
=A0 blk_mq_wake_waiters(q);
=A0 else {
Thanks,
Bart.
^ permalink raw reply related
* Re: [PATCH 5/8] nowait aio: return on congested block device
From: Goldwyn Rodrigues @ 2017-03-17 12:23 UTC (permalink / raw)
To: Jens Axboe, Goldwyn Rodrigues, linux-fsdevel
Cc: jack, hch, linux-block, linux-btrfs, linux-ext4, linux-xfs, sagi,
avi, linux-api, willy
In-Reply-To: <eee4683d-9f44-434f-b97f-b0b24c7b3dab@kernel.dk>
On 03/16/2017 09:33 AM, Jens Axboe wrote:
> On 03/15/2017 03:51 PM, Goldwyn Rodrigues wrote:
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 0eeb99e..2e5cba2 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2014,7 +2019,7 @@ blk_qc_t generic_make_request(struct bio *bio)
>> do {
>> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>>
>> - if (likely(blk_queue_enter(q, false) == 0)) {
>> + if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT)) == 0)) {
>> struct bio_list hold;
>> struct bio_list lower, same;
>>
>> @@ -2040,7 +2045,10 @@ blk_qc_t generic_make_request(struct bio *bio)
>> bio_list_merge(&bio_list_on_stack, &same);
>> bio_list_merge(&bio_list_on_stack, &hold);
>> } else {
>> - bio_io_error(bio);
>> + if (unlikely(bio_flagged(bio, BIO_NOWAIT)))
>> + bio_wouldblock_error(bio);
>> + else
>> + bio_io_error(bio);
>
> This doesn't look right. What if the queue is dying, and BIO_NOWAIT just
> happened to be set?
Yes, need a check for that.
>
> And you're missing wbt_wait() as well as a blocking point. Ditto in
> blk-mq.
Agree.
>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 159187a..942ce8c 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -1518,6 +1518,8 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
>> rq = blk_mq_sched_get_request(q, bio, bio->bi_opf, &data);
>> if (unlikely(!rq)) {
>> __wbt_done(q->rq_wb, wb_acct);
>> + if (bio && bio_flagged(bio, BIO_NOWAIT))
>> + bio_wouldblock_error(bio);
>> return BLK_QC_T_NONE;
>> }
>>
>
> This seems a little fragile now, since not both paths free the bio.
>
bio_put() is handled in dio_bio_complete(). However, I am not sure why
there is no bio_endio() call for !rq. IIRC, it was not meant to be.
--
Goldwyn
^ permalink raw reply
* Re: [PATCH 5/8] nowait aio: return on congested block device
From: Goldwyn Rodrigues @ 2017-03-17 12:23 UTC (permalink / raw)
To: Dave Chinner, Goldwyn Rodrigues
Cc: linux-fsdevel, jack, hch, linux-block, linux-btrfs, linux-ext4,
linux-xfs, sagi, avi, axboe, linux-api, willy
In-Reply-To: <20170316213134.GV17542@dastard>
On 03/16/2017 04:31 PM, Dave Chinner wrote:
> On Wed, Mar 15, 2017 at 04:51:04PM -0500, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> A new flag BIO_NOWAIT is introduced to identify bio's
>> orignating from iocb with IOCB_NOWAIT. This flag indicates
>> to return immediately if a request cannot be made instead
>> of retrying.
>
> So this makes a congested block device run the bio IO completion
> callback with an -EAGAIN error present? Are all the filesystem
> direct IO submission and completion routines OK with that? i.e. does
> such a congestion case cause filesystems to temporarily expose stale
> data to unprivileged users when the IO is requeued in this way?
>
> e.g. filesystem does allocation without blocking, submits bio,
> device is congested, runs IO completion with error, so nothing
> written to allocated blocks, write gets queued, so other read
> comes in while the write is queued, reads data from uninitialised
> blocks that were allocated during the write....
>
> Seems kinda problematic to me to have a undocumented design
> constraint (i.e a landmine) where we submit the AIO only to have it
> error out and then expect the filesystem to do something special and
> different /without blocking/ on EAGAIN.
If the filesystems has to perform block allocation, we would return
-EAGAIN early enough. However, I agree there is a problem, since not all
filesystems know this. I worked on only three of them.
>
> Why isn't the congestion check at a higher layer like we do for page
> cache readahead? i.e. using the bdi*congested() API at the time we
> are doing all the other filesystem blocking checks.
>
Yes, that may work better. We will have to call bdi_read_congested() on
a write path. (will have to comment that part of the code). Would it
encompass all possible waits in the block layer?
--
Goldwyn
^ permalink raw reply
* [PATCH v1 1/3] blk-mq: don't complete un-started request in timeout handler
From: Ming Lei @ 2017-03-17 9:57 UTC (permalink / raw)
To: Jens Axboe, linux-kernel, linux-block, Christoph Hellwig
Cc: Yi Zhang, Bart Van Assche, Ming Lei, stable
In-Reply-To: <20170317095711.5819-1-tom.leiming@gmail.com>
When iterating busy requests in timeout handler,
if the STARTED flag of one request isn't set, that means
the request is being processed in block layer or driver, and
isn't dispatch to hardware yet.
In current implementation of blk_mq_check_expired(),
if the request queue becomes dying, un-started requests are
handled as being completed/freed immediately. This way is
wrong, and can cause use-after-free issue easily[1][2], when
doing I/O and removing&resetting NVMe device at the sametime.
This patch fixes several issues reported by Yi Zhang.
[1]. oops log 1
[ 581.789754] ------------[ cut here ]------------
[ 581.789758] kernel BUG at block/blk-mq.c:374!
[ 581.789760] invalid opcode: 0000 [#1] SMP
[ 581.789761] Modules linked in: vfat fat ipmi_ssif intel_rapl sb_edac
edac_core x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm nvme
irqbypass crct10dif_pclmul nvme_core crc32_pclmul ghash_clmulni_intel
intel_cstate ipmi_si mei_me ipmi_devintf intel_uncore sg ipmi_msghandler
intel_rapl_perf iTCO_wdt mei iTCO_vendor_support mxm_wmi lpc_ich dcdbas shpchp
pcspkr acpi_power_meter wmi nfsd auth_rpcgss nfs_acl lockd dm_multipath grace
sunrpc ip_tables xfs libcrc32c sd_mod mgag200 i2c_algo_bit drm_kms_helper
syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm ahci libahci
crc32c_intel tg3 libata megaraid_sas i2c_core ptp fjes pps_core dm_mirror
dm_region_hash dm_log dm_mod
[ 581.789796] CPU: 1 PID: 1617 Comm: kworker/1:1H Not tainted 4.10.0.bz1420297+ #4
[ 581.789797] Hardware name: Dell Inc. PowerEdge R730xd/072T6D, BIOS 2.2.5 09/06/2016
[ 581.789804] Workqueue: kblockd blk_mq_timeout_work
[ 581.789806] task: ffff8804721c8000 task.stack: ffffc90006ee4000
[ 581.789809] RIP: 0010:blk_mq_end_request+0x58/0x70
[ 581.789810] RSP: 0018:ffffc90006ee7d50 EFLAGS: 00010202
[ 581.789811] RAX: 0000000000000001 RBX: ffff8802e4195340 RCX: ffff88028e2f4b88
[ 581.789812] RDX: 0000000000001000 RSI: 0000000000001000 RDI: 0000000000000000
[ 581.789813] RBP: ffffc90006ee7d60 R08: 0000000000000003 R09: ffff88028e2f4b00
[ 581.789814] R10: 0000000000001000 R11: 0000000000000001 R12: 00000000fffffffb
[ 581.789815] R13: ffff88042abe5780 R14: 000000000000002d R15: ffff88046fbdff80
[ 581.789817] FS: 0000000000000000(0000) GS:ffff88047fc00000(0000) knlGS:0000000000000000
[ 581.789818] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 581.789819] CR2: 00007f64f403a008 CR3: 000000014d078000 CR4: 00000000001406e0
[ 581.789820] Call Trace:
[ 581.789825] blk_mq_check_expired+0x76/0x80
[ 581.789828] bt_iter+0x45/0x50
[ 581.789830] blk_mq_queue_tag_busy_iter+0xdd/0x1f0
[ 581.789832] ? blk_mq_rq_timed_out+0x70/0x70
[ 581.789833] ? blk_mq_rq_timed_out+0x70/0x70
[ 581.789840] ? __switch_to+0x140/0x450
[ 581.789841] blk_mq_timeout_work+0x88/0x170
[ 581.789845] process_one_work+0x165/0x410
[ 581.789847] worker_thread+0x137/0x4c0
[ 581.789851] kthread+0x101/0x140
[ 581.789853] ? rescuer_thread+0x3b0/0x3b0
[ 581.789855] ? kthread_park+0x90/0x90
[ 581.789860] ret_from_fork+0x2c/0x40
[ 581.789861] Code: 48 85 c0 74 0d 44 89 e6 48 89 df ff d0 5b 41 5c 5d c3 48
8b bb 70 01 00 00 48 85 ff 75 0f 48 89 df e8 7d f0 ff ff 5b 41 5c 5d c3 <0f>
0b e8 71 f0 ff ff 90 eb e9 0f 1f 40 00 66 2e 0f 1f 84 00 00
[ 581.789882] RIP: blk_mq_end_request+0x58/0x70 RSP: ffffc90006ee7d50
[ 581.789889] ---[ end trace bcaf03d9a14a0a70 ]---
[2]. oops log2
[ 6984.857362] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
[ 6984.857372] IP: nvme_queue_rq+0x6e6/0x8cd [nvme]
[ 6984.857373] PGD 0
[ 6984.857374]
[ 6984.857376] Oops: 0000 [#1] SMP
[ 6984.857379] Modules linked in: ipmi_ssif vfat fat intel_rapl sb_edac
edac_core x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm
irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel ipmi_si iTCO_wdt
iTCO_vendor_support mxm_wmi ipmi_devintf intel_cstate sg dcdbas intel_uncore
mei_me intel_rapl_perf mei pcspkr lpc_ich ipmi_msghandler shpchp
acpi_power_meter wmi nfsd auth_rpcgss dm_multipath nfs_acl lockd grace sunrpc
ip_tables xfs libcrc32c sd_mod mgag200 i2c_algo_bit drm_kms_helper syscopyarea
sysfillrect crc32c_intel sysimgblt fb_sys_fops ttm nvme drm nvme_core ahci
libahci i2c_core tg3 libata ptp megaraid_sas pps_core fjes dm_mirror
dm_region_hash dm_log dm_mod
[ 6984.857416] CPU: 7 PID: 1635 Comm: kworker/7:1H Not tainted
4.10.0-2.el7.bz1420297.x86_64 #1
[ 6984.857417] Hardware name: Dell Inc. PowerEdge R730xd/072T6D, BIOS 2.2.5 09/06/2016
[ 6984.857427] Workqueue: kblockd blk_mq_run_work_fn
[ 6984.857429] task: ffff880476e3da00 task.stack: ffffc90002e90000
[ 6984.857432] RIP: 0010:nvme_queue_rq+0x6e6/0x8cd [nvme]
[ 6984.857433] RSP: 0018:ffffc90002e93c50 EFLAGS: 00010246
[ 6984.857434] RAX: 0000000000000000 RBX: ffff880275646600 RCX: 0000000000001000
[ 6984.857435] RDX: 0000000000000fff RSI: 00000002fba2a000 RDI: ffff8804734e6950
[ 6984.857436] RBP: ffffc90002e93d30 R08: 0000000000002000 R09: 0000000000001000
[ 6984.857437] R10: 0000000000001000 R11: 0000000000000000 R12: ffff8804741d8000
[ 6984.857438] R13: 0000000000000040 R14: ffff880475649f80 R15: ffff8804734e6780
[ 6984.857439] FS: 0000000000000000(0000) GS:ffff88047fcc0000(0000) knlGS:0000000000000000
[ 6984.857440] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6984.857442] CR2: 0000000000000010 CR3: 0000000001c09000 CR4: 00000000001406e0
[ 6984.857443] Call Trace:
[ 6984.857451] ? mempool_free+0x2b/0x80
[ 6984.857455] ? bio_free+0x4e/0x60
[ 6984.857459] blk_mq_dispatch_rq_list+0xf5/0x230
[ 6984.857462] blk_mq_process_rq_list+0x133/0x170
[ 6984.857465] __blk_mq_run_hw_queue+0x8c/0xa0
[ 6984.857467] blk_mq_run_work_fn+0x12/0x20
[ 6984.857473] process_one_work+0x165/0x410
[ 6984.857475] worker_thread+0x137/0x4c0
[ 6984.857478] kthread+0x101/0x140
[ 6984.857480] ? rescuer_thread+0x3b0/0x3b0
[ 6984.857481] ? kthread_park+0x90/0x90
[ 6984.857489] ret_from_fork+0x2c/0x40
[ 6984.857490] Code: 8b bd 70 ff ff ff 89 95 50 ff ff ff 89 8d 58 ff ff ff 44
89 95 60 ff ff ff e8 b7 dd 12 e1 8b 95 50 ff ff ff 48 89 85 68 ff ff ff <4c>
8b 48 10 44 8b 58 18 8b 8d 58 ff ff ff 44 8b 95 60 ff ff ff
[ 6984.857511] RIP: nvme_queue_rq+0x6e6/0x8cd [nvme] RSP: ffffc90002e93c50
[ 6984.857512] CR2: 0000000000000010
[ 6984.895359] ---[ end trace 2d7ceb528432bf83 ]---
Cc: stable@vger.kernel.org
Reported-by: Yi Zhang <yizhan@redhat.com>
Tested-by: Yi Zhang <yizhan@redhat.com>
Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
block/blk-mq.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index a4546f060e80..08a49c69738b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -697,17 +697,8 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
{
struct blk_mq_timeout_data *data = priv;
- if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags)) {
- /*
- * If a request wasn't started before the queue was
- * marked dying, kill it here or it'll go unnoticed.
- */
- if (unlikely(blk_queue_dying(rq->q))) {
- rq->errors = -EIO;
- blk_mq_end_request(rq, rq->errors);
- }
+ if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
return;
- }
if (time_after_eq(jiffies, rq->deadline)) {
if (!blk_mark_rq_complete(rq))
--
2.9.3
^ permalink raw reply related
* [PATCH v1 0/3] blk-mq: dying queue fix & improvement
From: Ming Lei @ 2017-03-17 9:57 UTC (permalink / raw)
To: Jens Axboe, linux-kernel, linux-block, Christoph Hellwig
Cc: Yi Zhang, Bart Van Assche, Ming Lei
From: Ming Lei <minlei@redhat.com>
Hi,
The 1st patch fixes one race between timeout and dying queue.
The 2nd patch add comments on races with timeout handler.
The 3rd patch improves handling for dying queue.
V1:
- add comments on races related with timeout handler
- add Tested-by & Reviewed-by tag
thanks,
Ming
Ming Lei (3):
blk-mq: don't complete un-started request in timeout handler
blk-mq: comment on races related timeout handler
blk-mq: start to freeze queue just after setting dying
block/blk-core.c | 7 +++++--
block/blk-mq.c | 33 +++++++++++++++++++++++----------
2 files changed, 28 insertions(+), 12 deletions(-)
--
2.9.3
^ permalink raw reply
* [PATCH v1 3/3] blk-mq: start to freeze queue just after setting dying
From: Ming Lei @ 2017-03-17 9:57 UTC (permalink / raw)
To: Jens Axboe, linux-kernel, linux-block, Christoph Hellwig
Cc: Yi Zhang, Bart Van Assche, Ming Lei, Tejun Heo
In-Reply-To: <20170317095711.5819-1-tom.leiming@gmail.com>
Before commit 780db2071a(blk-mq: decouble blk-mq freezing
from generic bypassing), the dying flag is checked before
entering queue, and Tejun converts the checking into .mq_freeze_depth,
and assumes the counter is increased just after dying flag
is set. Unfortunately we doesn't do that in blk_set_queue_dying().
This patch calls blk_mq_freeze_queue_start() for blk-mq in
blk_set_queue_dying(), so that we can block new I/O coming
once the queue is set as dying.
Given blk_set_queue_dying() is always called in remove path
of block device, and queue will be cleaned up later, we don't
need to worry about undoing the counter.
Cc: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
block/blk-core.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index d772c221cc17..62d4967c369f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -500,9 +500,12 @@ void blk_set_queue_dying(struct request_queue *q)
queue_flag_set(QUEUE_FLAG_DYING, q);
spin_unlock_irq(q->queue_lock);
- if (q->mq_ops)
+ if (q->mq_ops) {
blk_mq_wake_waiters(q);
- else {
+
+ /* block new I/O coming */
+ blk_mq_freeze_queue_start(q);
+ } else {
struct request_list *rl;
spin_lock_irq(q->queue_lock);
--
2.9.3
^ permalink raw reply related
* [PATCH v1 2/3] blk-mq: comment on races related with timeout handler
From: Ming Lei @ 2017-03-17 9:57 UTC (permalink / raw)
To: Jens Axboe, linux-kernel, linux-block, Christoph Hellwig
Cc: Yi Zhang, Bart Van Assche, Ming Lei
In-Reply-To: <20170317095711.5819-1-tom.leiming@gmail.com>
This patch adds comment on two races related with
timeout handler:
- requeue from queue busy vs. timeout
- rq free & reallocation vs. timeout
Both the races themselves and current solution aren't
explicit enough, so add comments on them.
Cc: Bart Van Assche <bart.vanassche@sandisk.com>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
block/blk-mq.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 08a49c69738b..7068779d3bac 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -527,6 +527,15 @@ void blk_mq_start_request(struct request *rq)
}
EXPORT_SYMBOL(blk_mq_start_request);
+/*
+ * When we reach here because queue is busy, REQ_ATOM_COMPLETE
+ * flag isn't set yet, so there may be race with timeout hanlder,
+ * but given rq->deadline is just set in .queue_rq() under
+ * this sitation, the race won't be possible in reality because
+ * rq->timeout should be set as big enough to cover the window
+ * between blk_mq_start_request() called from .queue_rq() and
+ * clearing REQ_ATOM_STARTED here.
+ */
static void __blk_mq_requeue_request(struct request *rq)
{
struct request_queue *q = rq->q;
@@ -700,6 +709,19 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
return;
+ /*
+ * The rq being checked may have been freed and reallocated
+ * out already here, we avoid this race by checking rq->deadline
+ * and REQ_ATOM_COMPLETE flag together:
+ *
+ * - if rq->deadline is observed as new value because of
+ * reusing, the rq won't be timed out because of timing.
+ * - if rq->deadline is observed as previous value,
+ * REQ_ATOM_COMPLETE flag won't be cleared in reuse path
+ * because we put a barrier between setting rq->deadline
+ * and clearing the flag in blk_mq_start_request(), so
+ * this rq won't be timed out too.
+ */
if (time_after_eq(jiffies, rq->deadline)) {
if (!blk_mark_rq_complete(rq))
blk_mq_rq_timed_out(rq, reserved);
--
2.9.3
^ permalink raw reply related
* Re: [PATCH 5/8] nowait aio: return on congested block device
From: Ming Lei @ 2017-03-17 2:03 UTC (permalink / raw)
To: Jens Axboe
Cc: Goldwyn Rodrigues, Linux FS Devel, Jan Kara, Christoph Hellwig,
linux-block, Btrfs BTRFS, open list:EXT4 FILE SYSTEM,
open list:XFS FILESYSTEM, Sagi Grimberg, avi, linux-api,
Matthew Wilcox, Goldwyn Rodrigues
In-Reply-To: <eee4683d-9f44-434f-b97f-b0b24c7b3dab@kernel.dk>
On Thu, Mar 16, 2017 at 10:33 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 03/15/2017 03:51 PM, Goldwyn Rodrigues wrote:
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 0eeb99e..2e5cba2 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2014,7 +2019,7 @@ blk_qc_t generic_make_request(struct bio *bio)
>> do {
>> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>>
>> - if (likely(blk_queue_enter(q, false) == 0)) {
>> + if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT)) == 0)) {
>> struct bio_list hold;
>> struct bio_list lower, same;
>>
>> @@ -2040,7 +2045,10 @@ blk_qc_t generic_make_request(struct bio *bio)
>> bio_list_merge(&bio_list_on_stack, &same);
>> bio_list_merge(&bio_list_on_stack, &hold);
>> } else {
>> - bio_io_error(bio);
>> + if (unlikely(bio_flagged(bio, BIO_NOWAIT)))
>> + bio_wouldblock_error(bio);
>> + else
>> + bio_io_error(bio);
>
> This doesn't look right. What if the queue is dying, and BIO_NOWAIT just
> happened to be set?
>
> And you're missing wbt_wait() as well as a blocking point. Ditto in
> blk-mq.
There are also tons of block points in dm/md/bcache, which need to be
considered or be documented at least, :-)
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq: don't complete un-started request in timeout handler
From: Ming Lei @ 2017-03-17 0:07 UTC (permalink / raw)
To: Bart Van Assche
Cc: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, yizhan@redhat.com, axboe@fb.com,
stable@vger.kernel.org
In-Reply-To: <1489700141.2574.16.camel@sandisk.com>
On Fri, Mar 17, 2017 at 5:35 AM, Bart Van Assche
<Bart.VanAssche@sandisk.com> wrote:
> On Thu, 2017-03-16 at 08:07 +0800, Ming Lei wrote:
>> > * Check whether REQ_ATOM_STARTED has been set.
>> > * Check whether REQ_ATOM_COMPLETE has not yet been set.
>> > * If both conditions have been met, set REQ_ATOM_COMPLETE.
>> >
>> > I don't think there is another solution than using a single state variable to
>> > represent the REQ_ATOM_STARTED and REQ_ATOM_COMPLETE states instead of two
>> > independent bits. How about the patch below?
>>
>> I would review it if you can confirm me that it is a real issue, :-)
>
> Hello Ming,
>
> I was chasing a v4.11 regression in the SCSI stack. Since my tests of today
> revealed that it's probably not a block layer issue, let's proceed with your
> patch.
Yeah, it shouldn't have been related with blk-mq timeout handler which
isn't changed much
recently, and thanks for your review!
Thanks,
Ming Lei
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq: don't complete un-started request in timeout handler
From: Bart Van Assche @ 2017-03-16 21:37 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, tom.leiming@gmail.com, axboe@fb.com
Cc: yizhan@redhat.com, stable@vger.kernel.org
In-Reply-To: <1489064578-17305-3-git-send-email-tom.leiming@gmail.com>
On Thu, 2017-03-09 at 21:02 +0800, Ming Lei wrote:
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 159187a28d66..0aff380099d5 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -697,17 +697,8 @@ static void blk_mq_check_expired(struct blk_mq_hw_ct=
x *hctx,
> {
> struct blk_mq_timeout_data *data =3D priv;
> =20
> - if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags)) {
> - /*
> - * If a request wasn't started before the queue was
> - * marked dying, kill it here or it'll go unnoticed.
> - */
> - if (unlikely(blk_queue_dying(rq->q))) {
> - rq->errors =3D -EIO;
> - blk_mq_end_request(rq, rq->errors);
> - }
> + if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags))
> return;
> - }
> =20
> if (time_after_eq(jiffies, rq->deadline)) {
> if (!blk_mark_rq_complete(rq))
Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>=
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq: don't complete un-started request in timeout handler
From: Bart Van Assche @ 2017-03-16 21:35 UTC (permalink / raw)
To: tom.leiming@gmail.com
Cc: linux-kernel@vger.kernel.org, hch@infradead.org,
linux-block@vger.kernel.org, yizhan@redhat.com, axboe@fb.com,
stable@vger.kernel.org
In-Reply-To: <20170316000747.GA19948@ming.t460p>
On Thu, 2017-03-16 at 08:07 +0800, Ming Lei wrote:
> > * Check whether REQ_ATOM_STARTED has been set.
> > * Check whether REQ_ATOM_COMPLETE has not yet been set.
> > * If both conditions have been met, set REQ_ATOM_COMPLETE.
> >=20
> > I don't think there is another solution than using a single state varia=
ble to
> > represent the REQ_ATOM_STARTED and REQ_ATOM_COMPLETE states instead of =
two
> > independent bits. How about the patch below?
>=20
> I would review it if you can confirm me that it is a real issue, :-)
Hello Ming,
I was chasing a v4.11 regression in the SCSI stack. Since my tests of today
revealed that it's probably not a block layer issue, let's proceed with you=
r
patch.
Bart.=
^ permalink raw reply
* Re: [PATCH 5/8] nowait aio: return on congested block device
From: Dave Chinner @ 2017-03-16 21:31 UTC (permalink / raw)
To: Goldwyn Rodrigues
Cc: linux-fsdevel, jack, hch, linux-block, linux-btrfs, linux-ext4,
linux-xfs, sagi, avi, axboe, linux-api, willy, Goldwyn Rodrigues
In-Reply-To: <20170315215107.5628-6-rgoldwyn@suse.de>
On Wed, Mar 15, 2017 at 04:51:04PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> A new flag BIO_NOWAIT is introduced to identify bio's
> orignating from iocb with IOCB_NOWAIT. This flag indicates
> to return immediately if a request cannot be made instead
> of retrying.
So this makes a congested block device run the bio IO completion
callback with an -EAGAIN error present? Are all the filesystem
direct IO submission and completion routines OK with that? i.e. does
such a congestion case cause filesystems to temporarily expose stale
data to unprivileged users when the IO is requeued in this way?
e.g. filesystem does allocation without blocking, submits bio,
device is congested, runs IO completion with error, so nothing
written to allocated blocks, write gets queued, so other read
comes in while the write is queued, reads data from uninitialised
blocks that were allocated during the write....
Seems kinda problematic to me to have a undocumented design
constraint (i.e a landmine) where we submit the AIO only to have it
error out and then expect the filesystem to do something special and
different /without blocking/ on EAGAIN.
Why isn't the congestion check at a higher layer like we do for page
cache readahead? i.e. using the bdi*congested() API at the time we
are doing all the other filesystem blocking checks.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* [PATCH v3 14/14] md: raid10: avoid direct access to bvec table in handle_reshape_read_error
From: Ming Lei @ 2017-03-16 16:12 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-raid, linux-block,
Christoph Hellwig; +Cc: Ming Lei
In-Reply-To: <20170316161235.27110-1-tom.leiming@gmail.com>
All reshape I/O share pages from 1st copy device, so just use that pages
for avoiding direct access to bvec table in handle_reshape_read_error.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid10.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 8db3b0869be4..67cf09c0c649 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4681,7 +4681,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
struct r10bio *r10b = &on_stack.r10_bio;
int slot = 0;
int idx = 0;
- struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
+ struct page **pages;
+
+ /* reshape IOs share pages from .devs[0].bio */
+ pages = get_resync_pages(r10_bio->devs[0].bio)->pages;
r10b->sector = r10_bio->sector;
__raid10_find_phys(&conf->prev, r10b);
@@ -4710,7 +4713,7 @@ static int handle_reshape_read_error(struct mddev *mddev,
success = sync_page_io(rdev,
addr,
s << 9,
- bvec[idx].bv_page,
+ pages[idx],
REQ_OP_READ, 0, false);
rdev_dec_pending(rdev, mddev);
rcu_read_lock();
--
2.9.3
^ permalink raw reply related
* [PATCH v3 13/14] md: raid10: retrieve page from preallocated resync page array
From: Ming Lei @ 2017-03-16 16:12 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-raid, linux-block,
Christoph Hellwig; +Cc: Ming Lei
In-Reply-To: <20170316161235.27110-1-tom.leiming@gmail.com>
Now one page array is allocated for each resync bio, and we can
retrieve page from this table directly.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid10.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index d2cb68971486..8db3b0869be4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -2075,6 +2075,7 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
int i, first;
struct bio *tbio, *fbio;
int vcnt;
+ struct page **tpages, **fpages;
atomic_set(&r10_bio->remaining, 1);
@@ -2090,6 +2091,7 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
fbio = r10_bio->devs[i].bio;
fbio->bi_iter.bi_size = r10_bio->sectors << 9;
fbio->bi_iter.bi_idx = 0;
+ fpages = get_resync_pages(fbio)->pages;
vcnt = (r10_bio->sectors + (PAGE_SIZE >> 9) - 1) >> (PAGE_SHIFT - 9);
/* now find blocks with errors */
@@ -2104,6 +2106,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
continue;
if (i == first)
continue;
+
+ tpages = get_resync_pages(tbio)->pages;
d = r10_bio->devs[i].devnum;
rdev = conf->mirrors[d].rdev;
if (!r10_bio->devs[i].bio->bi_error) {
@@ -2116,8 +2120,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
int len = PAGE_SIZE;
if (sectors < (len / 512))
len = sectors * 512;
- if (memcmp(page_address(fbio->bi_io_vec[j].bv_page),
- page_address(tbio->bi_io_vec[j].bv_page),
+ if (memcmp(page_address(fpages[j]),
+ page_address(tpages[j]),
len))
break;
sectors -= len/512;
@@ -2215,6 +2219,7 @@ static void fix_recovery_read_error(struct r10bio *r10_bio)
int idx = 0;
int dr = r10_bio->devs[0].devnum;
int dw = r10_bio->devs[1].devnum;
+ struct page **pages = get_resync_pages(bio)->pages;
while (sectors) {
int s = sectors;
@@ -2230,7 +2235,7 @@ static void fix_recovery_read_error(struct r10bio *r10_bio)
ok = sync_page_io(rdev,
addr,
s << 9,
- bio->bi_io_vec[idx].bv_page,
+ pages[idx],
REQ_OP_READ, 0, false);
if (ok) {
rdev = conf->mirrors[dw].rdev;
@@ -2238,7 +2243,7 @@ static void fix_recovery_read_error(struct r10bio *r10_bio)
ok = sync_page_io(rdev,
addr,
s << 9,
- bio->bi_io_vec[idx].bv_page,
+ pages[idx],
REQ_OP_WRITE, 0, false);
if (!ok) {
set_bit(WriteErrorSeen, &rdev->flags);
--
2.9.3
^ permalink raw reply related
* [PATCH v3 11/14] md: raid10: refactor code of read reshape's .bi_end_io
From: Ming Lei @ 2017-03-16 16:12 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-raid, linux-block,
Christoph Hellwig; +Cc: Ming Lei
In-Reply-To: <20170316161235.27110-1-tom.leiming@gmail.com>
reshape read request is a bit special and requires one extra
bio which isn't allocated from r10buf_pool.
Refactor the .bi_end_io for read reshape, so that we can use
raid10's resync page mangement approach easily in the following
patches.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid10.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 2b40420299e3..98e53f39b66e 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1909,17 +1909,9 @@ static int raid10_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
return err;
}
-static void end_sync_read(struct bio *bio)
+static void __end_sync_read(struct r10bio *r10_bio, struct bio *bio, int d)
{
- struct r10bio *r10_bio = bio->bi_private;
struct r10conf *conf = r10_bio->mddev->private;
- int d;
-
- if (bio == r10_bio->master_bio) {
- /* this is a reshape read */
- d = r10_bio->read_slot; /* really the read dev */
- } else
- d = find_bio_disk(conf, r10_bio, bio, NULL, NULL);
if (!bio->bi_error)
set_bit(R10BIO_Uptodate, &r10_bio->state);
@@ -1943,6 +1935,22 @@ static void end_sync_read(struct bio *bio)
}
}
+static void end_sync_read(struct bio *bio)
+{
+ struct r10bio *r10_bio = bio->bi_private;
+ struct r10conf *conf = r10_bio->mddev->private;
+ int d = find_bio_disk(conf, r10_bio, bio, NULL, NULL);
+
+ __end_sync_read(r10_bio, bio, d);
+}
+
+static void end_reshape_read(struct bio *bio)
+{
+ struct r10bio *r10_bio = bio->bi_private;
+
+ __end_sync_read(r10_bio, bio, r10_bio->read_slot);
+}
+
static void end_sync_request(struct r10bio *r10_bio)
{
struct mddev *mddev = r10_bio->mddev;
@@ -4466,7 +4474,7 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
read_bio->bi_iter.bi_sector = (r10_bio->devs[r10_bio->read_slot].addr
+ rdev->data_offset);
read_bio->bi_private = r10_bio;
- read_bio->bi_end_io = end_sync_read;
+ read_bio->bi_end_io = end_reshape_read;
bio_set_op_attrs(read_bio, REQ_OP_READ, 0);
read_bio->bi_flags &= (~0UL << BIO_RESET_BITS);
read_bio->bi_error = 0;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 12/14] md: raid10: don't use bio's vec table to manage resync pages
From: Ming Lei @ 2017-03-16 16:12 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-raid, linux-block,
Christoph Hellwig; +Cc: Ming Lei
In-Reply-To: <20170316161235.27110-1-tom.leiming@gmail.com>
Now we allocate one page array for managing resync pages, instead
of using bio's vec table to do that, and the old way is very hacky
and won't work any more if multipage bvec is enabled.
The introduced cost is that we need to allocate (128 + 16) * copies
bytes per r10_bio, and it is fine because the inflight r10_bio for
resync shouldn't be much, as pointed by Shaohua.
Also bio_reset() in raid10_sync_request() and reshape_request()
are removed because all bios are freshly new now in these functions
and not necessary to reset any more.
This patch can be thought as cleanup too.
Suggested-by: Shaohua Li <shli@kernel.org>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid10.c | 134 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 82 insertions(+), 52 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 98e53f39b66e..d2cb68971486 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -110,6 +110,24 @@ static void end_reshape(struct r10conf *conf);
#define raid10_log(md, fmt, args...) \
do { if ((md)->queue) blk_add_trace_msg((md)->queue, "raid10 " fmt, ##args); } while (0)
+/*
+ * 'strct resync_pages' stores actual pages used for doing the resync
+ * IO, and it is per-bio, so make .bi_private points to it.
+ */
+static inline struct resync_pages *get_resync_pages(struct bio *bio)
+{
+ return bio->bi_private;
+}
+
+/*
+ * for resync bio, r10bio pointer can be retrieved from the per-bio
+ * 'struct resync_pages'.
+ */
+static inline struct r10bio *get_resync_r10bio(struct bio *bio)
+{
+ return get_resync_pages(bio)->raid_bio;
+}
+
static void * r10bio_pool_alloc(gfp_t gfp_flags, void *data)
{
struct r10conf *conf = data;
@@ -140,11 +158,11 @@ static void r10bio_pool_free(void *r10_bio, void *data)
static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
{
struct r10conf *conf = data;
- struct page *page;
struct r10bio *r10_bio;
struct bio *bio;
- int i, j;
- int nalloc;
+ int j;
+ int nalloc, nalloc_rp;
+ struct resync_pages *rps;
r10_bio = r10bio_pool_alloc(gfp_flags, conf);
if (!r10_bio)
@@ -156,6 +174,15 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
else
nalloc = 2; /* recovery */
+ /* allocate once for all bios */
+ if (!conf->have_replacement)
+ nalloc_rp = nalloc;
+ else
+ nalloc_rp = nalloc * 2;
+ rps = kmalloc(sizeof(struct resync_pages) * nalloc_rp, gfp_flags);
+ if (!rps)
+ goto out_free_r10bio;
+
/*
* Allocate bios.
*/
@@ -175,36 +202,40 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
* Allocate RESYNC_PAGES data pages and attach them
* where needed.
*/
- for (j = 0 ; j < nalloc; j++) {
+ for (j = 0; j < nalloc; j++) {
struct bio *rbio = r10_bio->devs[j].repl_bio;
+ struct resync_pages *rp, *rp_repl;
+
+ rp = &rps[j];
+ if (rbio)
+ rp_repl = &rps[nalloc + j];
+
bio = r10_bio->devs[j].bio;
- for (i = 0; i < RESYNC_PAGES; i++) {
- if (j > 0 && !test_bit(MD_RECOVERY_SYNC,
- &conf->mddev->recovery)) {
- /* we can share bv_page's during recovery
- * and reshape */
- struct bio *rbio = r10_bio->devs[0].bio;
- page = rbio->bi_io_vec[i].bv_page;
- get_page(page);
- } else
- page = alloc_page(gfp_flags);
- if (unlikely(!page))
+
+ if (!j || test_bit(MD_RECOVERY_SYNC,
+ &conf->mddev->recovery)) {
+ if (resync_alloc_pages(rp, gfp_flags))
goto out_free_pages;
+ } else {
+ memcpy(rp, &rps[0], sizeof(*rp));
+ resync_get_all_pages(rp);
+ }
- bio->bi_io_vec[i].bv_page = page;
- if (rbio)
- rbio->bi_io_vec[i].bv_page = page;
+ rp->idx = 0;
+ rp->raid_bio = r10_bio;
+ bio->bi_private = rp;
+ if (rbio) {
+ memcpy(rp_repl, rp, sizeof(*rp));
+ rbio->bi_private = rp_repl;
}
}
return r10_bio;
out_free_pages:
- for ( ; i > 0 ; i--)
- safe_put_page(bio->bi_io_vec[i-1].bv_page);
- while (j--)
- for (i = 0; i < RESYNC_PAGES ; i++)
- safe_put_page(r10_bio->devs[j].bio->bi_io_vec[i].bv_page);
+ while (--j >= 0)
+ resync_free_pages(&rps[j * 2]);
+
j = 0;
out_free_bio:
for ( ; j < nalloc; j++) {
@@ -213,30 +244,34 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
if (r10_bio->devs[j].repl_bio)
bio_put(r10_bio->devs[j].repl_bio);
}
+ kfree(rps);
+out_free_r10bio:
r10bio_pool_free(r10_bio, conf);
return NULL;
}
static void r10buf_pool_free(void *__r10_bio, void *data)
{
- int i;
struct r10conf *conf = data;
struct r10bio *r10bio = __r10_bio;
int j;
+ struct resync_pages *rp = NULL;
- for (j=0; j < conf->copies; j++) {
+ for (j = conf->copies; j--; ) {
struct bio *bio = r10bio->devs[j].bio;
- if (bio) {
- for (i = 0; i < RESYNC_PAGES; i++) {
- safe_put_page(bio->bi_io_vec[i].bv_page);
- bio->bi_io_vec[i].bv_page = NULL;
- }
- bio_put(bio);
- }
+
+ rp = get_resync_pages(bio);
+ resync_free_pages(rp);
+ bio_put(bio);
+
bio = r10bio->devs[j].repl_bio;
if (bio)
bio_put(bio);
}
+
+ /* resync pages array stored in the 1st bio's .bi_private */
+ kfree(rp);
+
r10bio_pool_free(r10bio, conf);
}
@@ -1937,7 +1972,7 @@ static void __end_sync_read(struct r10bio *r10_bio, struct bio *bio, int d)
static void end_sync_read(struct bio *bio)
{
- struct r10bio *r10_bio = bio->bi_private;
+ struct r10bio *r10_bio = get_resync_r10bio(bio);
struct r10conf *conf = r10_bio->mddev->private;
int d = find_bio_disk(conf, r10_bio, bio, NULL, NULL);
@@ -1946,6 +1981,7 @@ static void end_sync_read(struct bio *bio)
static void end_reshape_read(struct bio *bio)
{
+ /* reshape read bio isn't allocated from r10buf_pool */
struct r10bio *r10_bio = bio->bi_private;
__end_sync_read(r10_bio, bio, r10_bio->read_slot);
@@ -1980,7 +2016,7 @@ static void end_sync_request(struct r10bio *r10_bio)
static void end_sync_write(struct bio *bio)
{
- struct r10bio *r10_bio = bio->bi_private;
+ struct r10bio *r10_bio = get_resync_r10bio(bio);
struct mddev *mddev = r10_bio->mddev;
struct r10conf *conf = mddev->private;
int d;
@@ -2060,6 +2096,7 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
for (i=0 ; i < conf->copies ; i++) {
int j, d;
struct md_rdev *rdev;
+ struct resync_pages *rp;
tbio = r10_bio->devs[i].bio;
@@ -2101,11 +2138,13 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
* First we need to fixup bv_offset, bv_len and
* bi_vecs, as the read request might have corrupted these
*/
+ rp = get_resync_pages(tbio);
bio_reset(tbio);
tbio->bi_vcnt = vcnt;
tbio->bi_iter.bi_size = fbio->bi_iter.bi_size;
- tbio->bi_private = r10_bio;
+ rp->raid_bio = r10_bio;
+ tbio->bi_private = rp;
tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;
tbio->bi_end_io = end_sync_write;
bio_set_op_attrs(tbio, REQ_OP_WRITE, 0);
@@ -3173,10 +3212,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
}
}
bio = r10_bio->devs[0].bio;
- bio_reset(bio);
bio->bi_next = biolist;
biolist = bio;
- bio->bi_private = r10_bio;
bio->bi_end_io = end_sync_read;
bio_set_op_attrs(bio, REQ_OP_READ, 0);
if (test_bit(FailFast, &rdev->flags))
@@ -3200,10 +3237,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (!test_bit(In_sync, &mrdev->flags)) {
bio = r10_bio->devs[1].bio;
- bio_reset(bio);
bio->bi_next = biolist;
biolist = bio;
- bio->bi_private = r10_bio;
bio->bi_end_io = end_sync_write;
bio_set_op_attrs(bio, REQ_OP_WRITE, 0);
bio->bi_iter.bi_sector = to_addr
@@ -3228,10 +3263,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (mreplace == NULL || bio == NULL ||
test_bit(Faulty, &mreplace->flags))
break;
- bio_reset(bio);
bio->bi_next = biolist;
biolist = bio;
- bio->bi_private = r10_bio;
bio->bi_end_io = end_sync_write;
bio_set_op_attrs(bio, REQ_OP_WRITE, 0);
bio->bi_iter.bi_sector = to_addr +
@@ -3353,7 +3386,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
r10_bio->devs[i].repl_bio->bi_end_io = NULL;
bio = r10_bio->devs[i].bio;
- bio_reset(bio);
bio->bi_error = -EIO;
rcu_read_lock();
rdev = rcu_dereference(conf->mirrors[d].rdev);
@@ -3378,7 +3410,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
atomic_inc(&r10_bio->remaining);
bio->bi_next = biolist;
biolist = bio;
- bio->bi_private = r10_bio;
bio->bi_end_io = end_sync_read;
bio_set_op_attrs(bio, REQ_OP_READ, 0);
if (test_bit(FailFast, &conf->mirrors[d].rdev->flags))
@@ -3397,13 +3428,11 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
/* Need to set up for writing to the replacement */
bio = r10_bio->devs[i].repl_bio;
- bio_reset(bio);
bio->bi_error = -EIO;
sector = r10_bio->devs[i].addr;
bio->bi_next = biolist;
biolist = bio;
- bio->bi_private = r10_bio;
bio->bi_end_io = end_sync_write;
bio_set_op_attrs(bio, REQ_OP_WRITE, 0);
if (test_bit(FailFast, &conf->mirrors[d].rdev->flags))
@@ -3442,7 +3471,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
if (len == 0)
break;
for (bio= biolist ; bio ; bio=bio->bi_next) {
- page = bio->bi_io_vec[bio->bi_vcnt].bv_page;
+ struct resync_pages *rp = get_resync_pages(bio);
+ page = resync_fetch_page(rp, rp->idx++);
/*
* won't fail because the vec table is big enough
* to hold all these pages
@@ -3451,7 +3481,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
}
nr_sectors += len>>9;
sector_nr += len>>9;
- } while (biolist->bi_vcnt < RESYNC_PAGES);
+ } while (get_resync_pages(biolist)->idx < RESYNC_PAGES);
r10_bio->sectors = nr_sectors;
while (biolist) {
@@ -3459,7 +3489,7 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
biolist = biolist->bi_next;
bio->bi_next = NULL;
- r10_bio = bio->bi_private;
+ r10_bio = get_resync_r10bio(bio);
r10_bio->sectors = nr_sectors;
if (bio->bi_end_io == end_sync_read) {
@@ -4354,6 +4384,7 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
struct bio *blist;
struct bio *bio, *read_bio;
int sectors_done = 0;
+ struct page **pages;
if (sector_nr == 0) {
/* If restarting in the middle, skip the initial sectors */
@@ -4504,11 +4535,9 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
if (!rdev2 || test_bit(Faulty, &rdev2->flags))
continue;
- bio_reset(b);
b->bi_bdev = rdev2->bdev;
b->bi_iter.bi_sector = r10_bio->devs[s/2].addr +
rdev2->new_data_offset;
- b->bi_private = r10_bio;
b->bi_end_io = end_reshape_write;
bio_set_op_attrs(b, REQ_OP_WRITE, 0);
b->bi_next = blist;
@@ -4518,8 +4547,9 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
/* Now add as many pages as possible to all of these bios. */
nr_sectors = 0;
+ pages = get_resync_pages(r10_bio->devs[0].bio)->pages;
for (s = 0 ; s < max_sectors; s += PAGE_SIZE >> 9) {
- struct page *page = r10_bio->devs[0].bio->bi_io_vec[s/(PAGE_SIZE>>9)].bv_page;
+ struct page *page = pages[s / (PAGE_SIZE >> 9)];
int len = (max_sectors - s) << 9;
if (len > PAGE_SIZE)
len = PAGE_SIZE;
@@ -4703,7 +4733,7 @@ static int handle_reshape_read_error(struct mddev *mddev,
static void end_reshape_write(struct bio *bio)
{
- struct r10bio *r10_bio = bio->bi_private;
+ struct r10bio *r10_bio = get_resync_r10bio(bio);
struct mddev *mddev = r10_bio->mddev;
struct r10conf *conf = mddev->private;
int d;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 10/14] md: raid1: improve write behind
From: Ming Lei @ 2017-03-16 16:12 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-raid, linux-block,
Christoph Hellwig; +Cc: Ming Lei
In-Reply-To: <20170316161235.27110-1-tom.leiming@gmail.com>
This patch improve handling of write behind in the following ways:
- introduce behind master bio to hold all write behind pages
- fast clone bios from behind master bio
- avoid to change bvec table directly
- use bio_copy_data() and make code more clean
Suggested-by: Shaohua Li <shli@fb.com>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid1.c | 118 ++++++++++++++++++++++++-----------------------------
drivers/md/raid1.h | 10 +++--
2 files changed, 61 insertions(+), 67 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 2f3622c695ce..3c13286190c1 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -405,12 +405,9 @@ static void close_write(struct r1bio *r1_bio)
{
/* it really is the end of this request */
if (test_bit(R1BIO_BehindIO, &r1_bio->state)) {
- /* free extra copy of the data pages */
- int i = r1_bio->behind_page_count;
- while (i--)
- safe_put_page(r1_bio->behind_bvecs[i].bv_page);
- kfree(r1_bio->behind_bvecs);
- r1_bio->behind_bvecs = NULL;
+ bio_free_pages(r1_bio->behind_master_bio);
+ bio_put(r1_bio->behind_master_bio);
+ r1_bio->behind_master_bio = NULL;
}
/* clear the bitmap if all writes complete successfully */
bitmap_endwrite(r1_bio->mddev->bitmap, r1_bio->sector,
@@ -512,6 +509,10 @@ static void raid1_end_write_request(struct bio *bio)
}
if (behind) {
+ /* we release behind master bio when all write are done */
+ if (r1_bio->behind_master_bio == bio)
+ to_put = NULL;
+
if (test_bit(WriteMostly, &rdev->flags))
atomic_dec(&r1_bio->behind_remaining);
@@ -1096,39 +1097,46 @@ static void unfreeze_array(struct r1conf *conf)
wake_up(&conf->wait_barrier);
}
-/* duplicate the data pages for behind I/O
- */
-static void alloc_behind_pages(struct bio *bio, struct r1bio *r1_bio)
+static struct bio *alloc_behind_master_bio(struct r1bio *r1_bio,
+ struct bio *bio,
+ int offset, int size)
{
- int i;
- struct bio_vec *bvec;
- struct bio_vec *bvecs = kzalloc(bio->bi_vcnt * sizeof(struct bio_vec),
- GFP_NOIO);
- if (unlikely(!bvecs))
- return;
+ unsigned vcnt = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
+ int i = 0;
+ struct bio *behind_bio = NULL;
+
+ behind_bio = bio_alloc_mddev(GFP_NOIO, vcnt, r1_bio->mddev);
+ if (!behind_bio)
+ goto fail;
+
+ while (i < vcnt && size) {
+ struct page *page;
+ int len = min_t(int, PAGE_SIZE, size);
+
+ page = alloc_page(GFP_NOIO);
+ if (unlikely(!page))
+ goto free_pages;
+
+ bio_add_page(behind_bio, page, len, 0);
+
+ size -= len;
+ i++;
+ }
- bio_for_each_segment_all(bvec, bio, i) {
- bvecs[i] = *bvec;
- bvecs[i].bv_page = alloc_page(GFP_NOIO);
- if (unlikely(!bvecs[i].bv_page))
- goto do_sync_io;
- memcpy(kmap(bvecs[i].bv_page) + bvec->bv_offset,
- kmap(bvec->bv_page) + bvec->bv_offset, bvec->bv_len);
- kunmap(bvecs[i].bv_page);
- kunmap(bvec->bv_page);
- }
- r1_bio->behind_bvecs = bvecs;
- r1_bio->behind_page_count = bio->bi_vcnt;
+ bio_copy_data_partial(behind_bio, bio, offset,
+ behind_bio->bi_iter.bi_size);
+
+ r1_bio->behind_master_bio = behind_bio;;
set_bit(R1BIO_BehindIO, &r1_bio->state);
- return;
-do_sync_io:
- for (i = 0; i < bio->bi_vcnt; i++)
- if (bvecs[i].bv_page)
- put_page(bvecs[i].bv_page);
- kfree(bvecs);
+ return behind_bio;
+
+ free_pages:
pr_debug("%dB behind alloc failed, doing sync I/O\n",
bio->bi_iter.bi_size);
+ bio_free_pages(behind_bio);
+ fail:
+ return behind_bio;
}
struct raid1_plug_cb {
@@ -1499,11 +1507,9 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio)
(atomic_read(&bitmap->behind_writes)
< mddev->bitmap_info.max_write_behind) &&
!waitqueue_active(&bitmap->behind_wait)) {
- mbio = bio_clone_bioset_partial(bio, GFP_NOIO,
- mddev->bio_set,
- offset << 9,
- max_sectors << 9);
- alloc_behind_pages(mbio, r1_bio);
+ mbio = alloc_behind_master_bio(r1_bio, bio,
+ offset << 9,
+ max_sectors << 9);
}
bitmap_startwrite(bitmap, r1_bio->sector,
@@ -1514,26 +1520,17 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio)
}
if (!mbio) {
- if (r1_bio->behind_bvecs)
- mbio = bio_clone_bioset_partial(bio, GFP_NOIO,
- mddev->bio_set,
- offset << 9,
- max_sectors << 9);
+ if (r1_bio->behind_master_bio)
+ mbio = bio_clone_fast(r1_bio->behind_master_bio,
+ GFP_NOIO,
+ mddev->bio_set);
else {
mbio = bio_clone_fast(bio, GFP_NOIO, mddev->bio_set);
bio_trim(mbio, offset, max_sectors);
}
}
- if (r1_bio->behind_bvecs) {
- struct bio_vec *bvec;
- int j;
-
- /*
- * We trimmed the bio, so _all is legit
- */
- bio_for_each_segment_all(bvec, mbio, j)
- bvec->bv_page = r1_bio->behind_bvecs[j].bv_page;
+ if (r1_bio->behind_master_bio) {
if (test_bit(WriteMostly, &conf->mirrors[i].rdev->flags))
atomic_inc(&r1_bio->behind_remaining);
}
@@ -2405,18 +2402,11 @@ static int narrow_write_error(struct r1bio *r1_bio, int i)
/* Write at 'sector' for 'sectors'*/
if (test_bit(R1BIO_BehindIO, &r1_bio->state)) {
- unsigned vcnt = r1_bio->behind_page_count;
- struct bio_vec *vec = r1_bio->behind_bvecs;
-
- while (!vec->bv_page) {
- vec++;
- vcnt--;
- }
-
- wbio = bio_alloc_mddev(GFP_NOIO, vcnt, mddev);
- memcpy(wbio->bi_io_vec, vec, vcnt * sizeof(struct bio_vec));
-
- wbio->bi_vcnt = vcnt;
+ wbio = bio_clone_fast(r1_bio->behind_master_bio,
+ GFP_NOIO,
+ mddev->bio_set);
+ /* We really need a _all clone */
+ wbio->bi_iter = (struct bvec_iter){ 0 };
} else {
wbio = bio_clone_fast(r1_bio->master_bio, GFP_NOIO,
mddev->bio_set);
diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
index dd22a37d0d83..4271cd7ac2de 100644
--- a/drivers/md/raid1.h
+++ b/drivers/md/raid1.h
@@ -153,9 +153,13 @@ struct r1bio {
int read_disk;
struct list_head retry_list;
- /* Next two are only valid when R1BIO_BehindIO is set */
- struct bio_vec *behind_bvecs;
- int behind_page_count;
+
+ /*
+ * When R1BIO_BehindIO is set, we store pages for write behind
+ * in behind_master_bio.
+ */
+ struct bio *behind_master_bio;
+
/*
* if the IO is in WRITE direction, then multiple bios are used.
* We choose the number when they are allocated.
--
2.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox