* [PATCH] block: flush queued bios when the process blocks
@ 2014-05-27 15:03 Mikulas Patocka
2014-05-27 15:08 ` Jens Axboe
0 siblings, 1 reply; 11+ messages in thread
From: Mikulas Patocka @ 2014-05-27 15:03 UTC (permalink / raw
To: Jens Axboe, Kent Overstreet
Cc: linux-kernel, dm-devel, Alasdair G. Kergon, Mike Snitzer
The block layer uses per-process bio list to avoid recursion in
generic_make_request. When generic_make_request is called recursively, the
bio is added to current->bio_list and the function returns immediatelly.
The top-level instance of generic_make_requests takes bios from
current->bio_list and processes them.
This bio queuing can result in deadlocks. The following deadlock was
observed:
1) Process A sends one-page read bio to the dm-snapshot target. The bio
spans snapshot chunk boundary and so it is split to two bios by device
mapper.
2) Device mapper creates the first sub-bio and sends it to the snapshot
driver.
3) The function snapshot_map calls track_chunk (that allocates a structure
dm_snap_tracked_chunk and adds it to tracked_chunk_hash) and then it
remaps the bio to the underlying linear target and exits with
DM_MAPIO_REMAPPED.
4) The remapped bio is submitted with generic_make_request, but it isn't
processed - it is added to current->bio_list instead.
5) Meanwhile, process B executes pending_complete for the affected chunk,
it takes down_write(&s->lock) and then loops in
__check_for_conflicting_io, waiting for dm_snap_tracked_chunk created in
step 3) to be released.
6) Process A continues, it creates a new bio for the rest of the original
bio.
7) snapshot_map is called for this new bio, it waits on
down_write(&s->lock) that is held in step 5).
The resulting deadlock:
* bio added to current->bio_list at step 4) waits until the function in
step 7) finishes
* the function in step 7) waits until s->lock held in step 5) is released
* the process in step 5) waits until the bio queued in step 4) finishes
The general problem is that queuing bios on current->bio_list introduces
additional lock dependencies. If a device mapper target sends a bio to
some block device, it assumes that the bio only takes locks of the target
block device or devices that are below the target device. However, if the
bio is added to queue on current->bio_list, it creates artifical locking
dependency on locks taken by other bios that are on current->bio_list. In
the above scenario, this artifical locking dependency results in
deadlock.
Kent Overstreet already created a workqueue for every bio set and there is
a code that tries to resolve some low-memory deadlocks by redirecting bios
queued on current->bio_list to the workqueue if the system is low on
memory. However, other deadlocks (as described above) may happen without
any low memory condition.
This patch generalizes Kent's concept, it redirects bios on
current->bio_list to the bio_set's workqueue on every schedule call.
Consequently, when the process blocks on a mutex, the bios queued on
current->bio_list are dispatched to independent workqueus and they can
complete without waiting for the mutex to be available.
Bios allocated with bio_kmalloc do not have bio_set, so they are not
redirected, however bio_kmalloc shouldn't be used by stacking drivers (it
is currently used by raid1.c and raid10.c, we need to change it to
bio_set).
Note to stable kernel maintainers: before backporting this patch, you also
need to backport df2cb6daa4cbc34406bc4b1ac9b9335df1083a72.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org
---
fs/bio.c | 84 ++++++++++++++-----------------------------------
include/linux/blkdev.h | 7 +++-
kernel/sched/core.c | 7 ++++
3 files changed, 37 insertions(+), 61 deletions(-)
Index: linux-3.15-rc5/fs/bio.c
===================================================================
--- linux-3.15-rc5.orig/fs/bio.c 2014-05-26 19:02:47.000000000 +0200
+++ linux-3.15-rc5/fs/bio.c 2014-05-27 00:00:13.000000000 +0200
@@ -342,35 +342,34 @@ static void bio_alloc_rescue(struct work
}
}
-static void punt_bios_to_rescuer(struct bio_set *bs)
+/**
+ * blk_flush_bio_list
+ *
+ * Pop bios queued on current->bio_list and submit each of them to
+ * their rescue workqueue.
+ *
+ * If the bio doesn't have a bio_set, we leave it on current->bio_list.
+ * However, stacking drivers should use bio_set, so this shouldn't be
+ * an issue.
+ */
+void blk_flush_bio_list(void)
{
- struct bio_list punt, nopunt;
struct bio *bio;
+ struct bio_list list = *current->bio_list;
+ bio_list_init(current->bio_list);
- /*
- * In order to guarantee forward progress we must punt only bios that
- * were allocated from this bio_set; otherwise, if there was a bio on
- * there for a stacking driver higher up in the stack, processing it
- * could require allocating bios from this bio_set, and doing that from
- * our own rescuer would be bad.
- *
- * Since bio lists are singly linked, pop them all instead of trying to
- * remove from the middle of the list:
- */
-
- bio_list_init(&punt);
- bio_list_init(&nopunt);
-
- while ((bio = bio_list_pop(current->bio_list)))
- bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
-
- *current->bio_list = nopunt;
-
- spin_lock(&bs->rescue_lock);
- bio_list_merge(&bs->rescue_list, &punt);
- spin_unlock(&bs->rescue_lock);
+ while ((bio = bio_list_pop(&list))) {
+ struct bio_set *bs = bio->bi_pool;
+ if (unlikely(!bs)) {
+ bio_list_add(current->bio_list, bio);
+ } else {
+ spin_lock(&bs->rescue_lock);
+ bio_list_add(&bs->rescue_list, bio);
+ spin_unlock(&bs->rescue_lock);
- queue_work(bs->rescue_workqueue, &bs->rescue_work);
+ queue_work(bs->rescue_workqueue, &bs->rescue_work);
+ }
+ }
}
/**
@@ -410,7 +409,6 @@ static void punt_bios_to_rescuer(struct
*/
struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
{
- gfp_t saved_gfp = gfp_mask;
unsigned front_pad;
unsigned inline_vecs;
unsigned long idx = BIO_POOL_NONE;
@@ -428,36 +426,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_m
front_pad = 0;
inline_vecs = nr_iovecs;
} else {
- /*
- * generic_make_request() converts recursion to iteration; this
- * means if we're running beneath it, any bios we allocate and
- * submit will not be submitted (and thus freed) until after we
- * return.
- *
- * This exposes us to a potential deadlock if we allocate
- * multiple bios from the same bio_set() while running
- * underneath generic_make_request(). If we were to allocate
- * multiple bios (say a stacking block driver that was splitting
- * bios), we would deadlock if we exhausted the mempool's
- * reserve.
- *
- * We solve this, and guarantee forward progress, with a rescuer
- * workqueue per bio_set. If we go to allocate and there are
- * bios on current->bio_list, we first try the allocation
- * without __GFP_WAIT; if that fails, we punt those bios we
- * would be blocking to the rescuer workqueue before we retry
- * with the original gfp_flags.
- */
-
- if (current->bio_list && !bio_list_empty(current->bio_list))
- gfp_mask &= ~__GFP_WAIT;
-
p = mempool_alloc(bs->bio_pool, gfp_mask);
- if (!p && gfp_mask != saved_gfp) {
- punt_bios_to_rescuer(bs);
- gfp_mask = saved_gfp;
- p = mempool_alloc(bs->bio_pool, gfp_mask);
- }
front_pad = bs->front_pad;
inline_vecs = BIO_INLINE_VECS;
@@ -471,11 +440,6 @@ struct bio *bio_alloc_bioset(gfp_t gfp_m
if (nr_iovecs > inline_vecs) {
bvl = bvec_alloc(gfp_mask, nr_iovecs, &idx, bs->bvec_pool);
- if (!bvl && gfp_mask != saved_gfp) {
- punt_bios_to_rescuer(bs);
- gfp_mask = saved_gfp;
- bvl = bvec_alloc(gfp_mask, nr_iovecs, &idx, bs->bvec_pool);
- }
if (unlikely(!bvl))
goto err_free;
Index: linux-3.15-rc5/kernel/sched/core.c
===================================================================
--- linux-3.15-rc5.orig/kernel/sched/core.c 2014-05-26 19:30:51.000000000 +0200
+++ linux-3.15-rc5/kernel/sched/core.c 2014-05-27 00:23:00.000000000 +0200
@@ -2734,6 +2734,13 @@ static inline void sched_submit_work(str
if (!tsk->state || tsk_is_pi_blocked(tsk))
return;
/*
+ * If there are bios on the bio list, flush them to the appropriate
+ * rescue threads.
+ */
+ if (unlikely(current->bio_list != NULL) &&
+ !bio_list_empty(current->bio_list))
+ blk_flush_bio_list();
+ /*
* If we are going to sleep and we have plugged IO queued,
* make sure to submit it to avoid deadlocks.
*/
Index: linux-3.15-rc5/include/linux/blkdev.h
===================================================================
--- linux-3.15-rc5.orig/include/linux/blkdev.h 2014-05-26 23:54:48.000000000 +0200
+++ linux-3.15-rc5/include/linux/blkdev.h 2014-05-26 23:56:41.000000000 +0200
@@ -1103,6 +1103,8 @@ static inline bool blk_needs_flush_plug(
!list_empty(&plug->cb_list));
}
+extern void blk_flush_bio_list(void);
+
/*
* tag stuff
*/
@@ -1634,12 +1636,15 @@ static inline void blk_schedule_flush_pl
{
}
-
static inline bool blk_needs_flush_plug(struct task_struct *tsk)
{
return false;
}
+static inline void blk_flush_bio_list(void)
+{
+}
+
#endif /* CONFIG_BLOCK */
#endif
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 15:03 Mikulas Patocka
@ 2014-05-27 15:08 ` Jens Axboe
2014-05-27 15:23 ` Mikulas Patocka
2014-05-27 17:59 ` Kent Overstreet
0 siblings, 2 replies; 11+ messages in thread
From: Jens Axboe @ 2014-05-27 15:08 UTC (permalink / raw
To: Mikulas Patocka, Kent Overstreet
Cc: linux-kernel, dm-devel, Alasdair G. Kergon, Mike Snitzer
On 2014-05-27 09:03, Mikulas Patocka wrote:
> The block layer uses per-process bio list to avoid recursion in
> generic_make_request. When generic_make_request is called recursively, the
> bio is added to current->bio_list and the function returns immediatelly.
> The top-level instance of generic_make_requests takes bios from
> current->bio_list and processes them.
This really begs the question of why we just don't use the per-process
plugs for this. We already have scheduler hooks in place to flush those
at the appropriate time. Why are we reinventing something for
essentially the same thing?
--
Jens Axboe
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 15:08 ` Jens Axboe
@ 2014-05-27 15:23 ` Mikulas Patocka
2014-05-27 15:42 ` Jens Axboe
2014-05-27 17:59 ` Kent Overstreet
1 sibling, 1 reply; 11+ messages in thread
From: Mikulas Patocka @ 2014-05-27 15:23 UTC (permalink / raw
To: Jens Axboe
Cc: Kent Overstreet, linux-kernel, dm-devel, Alasdair G. Kergon,
Mike Snitzer
On Tue, 27 May 2014, Jens Axboe wrote:
> On 2014-05-27 09:03, Mikulas Patocka wrote:
> > The block layer uses per-process bio list to avoid recursion in
> > generic_make_request. When generic_make_request is called recursively, the
> > bio is added to current->bio_list and the function returns immediatelly.
> > The top-level instance of generic_make_requests takes bios from
> > current->bio_list and processes them.
>
> This really begs the question of why we just don't use the per-process plugs
> for this. We already have scheduler hooks in place to flush those at the
> appropriate time. Why are we reinventing something for essentially the same
> thing?
>
> --
> Jens Axboe
Plugs work with requests, this patch works with bios. They are different
structures, so you can't use one infrastructure to process them.
The patch adds bio list flushing to the scheduler just besides plug
flushsing.
Mikulas
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 15:23 ` Mikulas Patocka
@ 2014-05-27 15:42 ` Jens Axboe
2014-05-27 16:26 ` Mikulas Patocka
0 siblings, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2014-05-27 15:42 UTC (permalink / raw
To: Mikulas Patocka
Cc: Kent Overstreet, linux-kernel, dm-devel, Alasdair G. Kergon,
Mike Snitzer
On 2014-05-27 09:23, Mikulas Patocka wrote:
>
>
> On Tue, 27 May 2014, Jens Axboe wrote:
>
>> On 2014-05-27 09:03, Mikulas Patocka wrote:
>>> The block layer uses per-process bio list to avoid recursion in
>>> generic_make_request. When generic_make_request is called recursively, the
>>> bio is added to current->bio_list and the function returns immediatelly.
>>> The top-level instance of generic_make_requests takes bios from
>>> current->bio_list and processes them.
>>
>> This really begs the question of why we just don't use the per-process plugs
>> for this. We already have scheduler hooks in place to flush those at the
>> appropriate time. Why are we reinventing something for essentially the same
>> thing?
>>
>> --
>> Jens Axboe
>
> Plugs work with requests, this patch works with bios. They are different
> structures, so you can't use one infrastructure to process them.
Yes... I realize the list and plugs are for requests. But there's
nothing preventing a non-rq hook, we have uses like that too. And it
could easily be extended to handle bio lists, too.
> The patch adds bio list flushing to the scheduler just besides plug
> flushsing.
... which is exactly why I'm commenting. It'd be great to avoid yet one
more scheduler hook for this sort of thing.
--
Jens Axboe
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 15:42 ` Jens Axboe
@ 2014-05-27 16:26 ` Mikulas Patocka
2014-05-27 17:42 ` Jens Axboe
0 siblings, 1 reply; 11+ messages in thread
From: Mikulas Patocka @ 2014-05-27 16:26 UTC (permalink / raw
To: Jens Axboe
Cc: Kent Overstreet, linux-kernel, dm-devel, Alasdair G. Kergon,
Mike Snitzer
On Tue, 27 May 2014, Jens Axboe wrote:
> On 2014-05-27 09:23, Mikulas Patocka wrote:
>
> > The patch adds bio list flushing to the scheduler just besides plug
> > flushsing.
>
> ... which is exactly why I'm commenting. It'd be great to avoid yet one more
> scheduler hook for this sort of thing.
>
> --
> Jens Axboe
One could create something like schedule notifier chain, but I'm not sure
if it is worth the complexity because of just two users. If more users
come in the future, it could be generalized.
Mikulas
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 16:26 ` Mikulas Patocka
@ 2014-05-27 17:42 ` Jens Axboe
2014-05-27 19:56 ` Mikulas Patocka
2014-05-29 23:52 ` Mikulas Patocka
0 siblings, 2 replies; 11+ messages in thread
From: Jens Axboe @ 2014-05-27 17:42 UTC (permalink / raw
To: Mikulas Patocka
Cc: Kent Overstreet, linux-kernel, dm-devel, Alasdair G. Kergon,
Mike Snitzer
On 2014-05-27 10:26, Mikulas Patocka wrote:
> On Tue, 27 May 2014, Jens Axboe wrote:
>
>> On 2014-05-27 09:23, Mikulas Patocka wrote:
>>
>>> The patch adds bio list flushing to the scheduler just besides plug
>>> flushsing.
>>
>> ... which is exactly why I'm commenting. It'd be great to avoid yet one more
>> scheduler hook for this sort of thing.
>>
>> --
>> Jens Axboe
>
> One could create something like schedule notifier chain, but I'm not sure
> if it is worth the complexity because of just two users. If more users
> come in the future, it could be generalized.
Except such a thing already exists, there are unplug callback chains.
All I'm asking is that you look into how feasible it would be to use
something like that, instead of reinventing the wheel.
--
Jens Axboe
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 15:08 ` Jens Axboe
2014-05-27 15:23 ` Mikulas Patocka
@ 2014-05-27 17:59 ` Kent Overstreet
1 sibling, 0 replies; 11+ messages in thread
From: Kent Overstreet @ 2014-05-27 17:59 UTC (permalink / raw
To: Jens Axboe
Cc: Mikulas Patocka, linux-kernel@vger.kernel.org,
device-mapper development, Alasdair G. Kergon, Mike Snitzer
On Tue, May 27, 2014 at 8:08 AM, Jens Axboe <axboe@kernel.dk> wrote:
> This really begs the question of why we just don't use the per-process plugs
> for this. We already have scheduler hooks in place to flush those at the
> appropriate time. Why are we reinventing something for essentially the same
> thing?
Yes! Unifying the two plugging mechanisms has been on my todo/wishlist for ages.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 17:42 ` Jens Axboe
@ 2014-05-27 19:56 ` Mikulas Patocka
2014-05-27 20:06 ` Kent Overstreet
2014-05-29 23:52 ` Mikulas Patocka
1 sibling, 1 reply; 11+ messages in thread
From: Mikulas Patocka @ 2014-05-27 19:56 UTC (permalink / raw
To: Jens Axboe
Cc: Kent Overstreet, linux-kernel, dm-devel, Alasdair G. Kergon,
Mike Snitzer
On Tue, 27 May 2014, Jens Axboe wrote:
> On 2014-05-27 10:26, Mikulas Patocka wrote:
> > On Tue, 27 May 2014, Jens Axboe wrote:
> >
> > > On 2014-05-27 09:23, Mikulas Patocka wrote:
> > >
> > > > The patch adds bio list flushing to the scheduler just besides plug
> > > > flushsing.
> > >
> > > ... which is exactly why I'm commenting. It'd be great to avoid yet one
> > > more
> > > scheduler hook for this sort of thing.
> > >
> > > --
> > > Jens Axboe
> >
> > One could create something like schedule notifier chain, but I'm not sure
> > if it is worth the complexity because of just two users. If more users
> > come in the future, it could be generalized.
>
> Except such a thing already exists, there are unplug callback chains. All I'm
> asking is that you look into how feasible it would be to use something like
> that, instead of reinventing the wheel.
>
> --
> Jens Axboe
Do you mean moving current->bio_list to struct blk_plug and calling
blk_start_plug/blk_finish_plug around generic_make_request?
It would be possible on a condition that we can redirect all bios to a
workqueue (i.e. eliminate bio_kmalloc and always use bio_alloc_bioset).
What are performance implications of this - does it make sense to have
blk_start_plug/blk_finish_plug around every call to generic_make_request?
- that means that all i/o requests will be added to a plug and then
unplugged.
Mikulas
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 19:56 ` Mikulas Patocka
@ 2014-05-27 20:06 ` Kent Overstreet
0 siblings, 0 replies; 11+ messages in thread
From: Kent Overstreet @ 2014-05-27 20:06 UTC (permalink / raw
To: Mikulas Patocka
Cc: Jens Axboe, linux-kernel, dm-devel, Alasdair G. Kergon,
Mike Snitzer
On Tue, May 27, 2014 at 03:56:00PM -0400, Mikulas Patocka wrote:
>
>
> On Tue, 27 May 2014, Jens Axboe wrote:
>
> > On 2014-05-27 10:26, Mikulas Patocka wrote:
> > > On Tue, 27 May 2014, Jens Axboe wrote:
> > >
> > > > On 2014-05-27 09:23, Mikulas Patocka wrote:
> > > >
> > > > > The patch adds bio list flushing to the scheduler just besides plug
> > > > > flushsing.
> > > >
> > > > ... which is exactly why I'm commenting. It'd be great to avoid yet one
> > > > more
> > > > scheduler hook for this sort of thing.
> > > >
> > > > --
> > > > Jens Axboe
> > >
> > > One could create something like schedule notifier chain, but I'm not sure
> > > if it is worth the complexity because of just two users. If more users
> > > come in the future, it could be generalized.
> >
> > Except such a thing already exists, there are unplug callback chains. All I'm
> > asking is that you look into how feasible it would be to use something like
> > that, instead of reinventing the wheel.
> >
> > --
> > Jens Axboe
>
> Do you mean moving current->bio_list to struct blk_plug and calling
> blk_start_plug/blk_finish_plug around generic_make_request?
>
> It would be possible on a condition that we can redirect all bios to a
> workqueue (i.e. eliminate bio_kmalloc and always use bio_alloc_bioset).
>
> What are performance implications of this - does it make sense to have
> blk_start_plug/blk_finish_plug around every call to generic_make_request?
> - that means that all i/o requests will be added to a plug and then
> unplugged.
We've already got blk_start_plug() calls around IO submission at higher points
in the stack. (I actually have seen it show up in profiles though, it probably
would be worth inlining and slimming down a bit).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] block: flush queued bios when the process blocks
2014-05-27 17:42 ` Jens Axboe
2014-05-27 19:56 ` Mikulas Patocka
@ 2014-05-29 23:52 ` Mikulas Patocka
1 sibling, 0 replies; 11+ messages in thread
From: Mikulas Patocka @ 2014-05-29 23:52 UTC (permalink / raw
To: Jens Axboe
Cc: Kent Overstreet, linux-kernel, dm-devel, Alasdair G. Kergon,
Mike Snitzer
On Tue, 27 May 2014, Jens Axboe wrote:
> On 2014-05-27 10:26, Mikulas Patocka wrote:
> > On Tue, 27 May 2014, Jens Axboe wrote:
> >
> > > On 2014-05-27 09:23, Mikulas Patocka wrote:
> > >
> > > > The patch adds bio list flushing to the scheduler just besides plug
> > > > flushsing.
> > >
> > > ... which is exactly why I'm commenting. It'd be great to avoid yet one
> > > more
> > > scheduler hook for this sort of thing.
> > >
> > > --
> > > Jens Axboe
> >
> > One could create something like schedule notifier chain, but I'm not sure
> > if it is worth the complexity because of just two users. If more users
> > come in the future, it could be generalized.
>
> Except such a thing already exists, there are unplug callback chains. All I'm
> asking is that you look into how feasible it would be to use something like
> that, instead of reinventing the wheel.
>
> --
> Jens Axboe
You can use this patch as an example that moves current->bio_list to
struct plug, but I don't recommend to put it in the kernel - this patch
still has some issues (some lvm raid tests fail) - and at -rc7 stage we
should really be fixing bugs and not rearchitecting the code.
You should commit my original patch because it is small and it generated
no regressions for me. Think about stable kernels and enterprise
distributions - the smaller the patch is, the easier it is to backport.
Mikulas
---
block/blk-core.c | 19 ++++++++++++-------
drivers/md/dm-bufio.c | 2 +-
drivers/md/raid1.c | 6 +++---
drivers/md/raid10.c | 6 +++---
fs/bio.c | 21 +++++++++------------
include/linux/blkdev.h | 7 +++++--
include/linux/sched.h | 4 ----
kernel/sched/core.c | 7 -------
8 files changed, 33 insertions(+), 39 deletions(-)
Index: linux-3.15-rc5/block/blk-core.c
===================================================================
--- linux-3.15-rc5.orig/block/blk-core.c 2014-05-29 23:06:29.000000000 +0200
+++ linux-3.15-rc5/block/blk-core.c 2014-05-30 00:30:41.000000000 +0200
@@ -1828,7 +1828,7 @@ end_io:
*/
void generic_make_request(struct bio *bio)
{
- struct bio_list bio_list_on_stack;
+ struct blk_plug plug;
if (!generic_make_request_checks(bio))
return;
@@ -1858,8 +1858,8 @@ void generic_make_request(struct bio *bi
* it is non-NULL, then a make_request is active, and new requests
* should be added at the tail
*/
- if (current->bio_list) {
- bio_list_add(current->bio_list, bio);
+ if (current->plug) {
+ bio_list_add(¤t->plug->bio_list, bio);
return;
}
@@ -1877,17 +1877,18 @@ void generic_make_request(struct bio *bi
* of the top of the list (no pretending) and so remove it from
* bio_list, and call into ->make_request() again.
*/
+ blk_start_plug(&plug);
+ current->plug->in_generic_make_request = 1;
BUG_ON(bio->bi_next);
- bio_list_init(&bio_list_on_stack);
- current->bio_list = &bio_list_on_stack;
do {
struct request_queue *q = bdev_get_queue(bio->bi_bdev);
q->make_request_fn(q, bio);
- bio = bio_list_pop(current->bio_list);
+ bio = bio_list_pop(¤t->plug->bio_list);
} while (bio);
- current->bio_list = NULL; /* deactivate */
+ current->plug->in_generic_make_request = 0;
+ blk_finish_plug(&plug);
}
EXPORT_SYMBOL(generic_make_request);
@@ -2965,6 +2966,8 @@ void blk_start_plug(struct blk_plug *plu
INIT_LIST_HEAD(&plug->list);
INIT_LIST_HEAD(&plug->mq_list);
INIT_LIST_HEAD(&plug->cb_list);
+ bio_list_init(&plug->bio_list);
+ plug->in_generic_make_request = 0;
/*
* If this is a nested plug, don't actually assign it. It will be
@@ -3060,6 +3063,8 @@ void blk_flush_plug_list(struct blk_plug
BUG_ON(plug->magic != PLUG_MAGIC);
+ blk_flush_bio_list(plug);
+
flush_plug_callbacks(plug, from_schedule);
if (!list_empty(&plug->mq_list))
Index: linux-3.15-rc5/include/linux/blkdev.h
===================================================================
--- linux-3.15-rc5.orig/include/linux/blkdev.h 2014-05-29 23:05:46.000000000 +0200
+++ linux-3.15-rc5/include/linux/blkdev.h 2014-05-30 00:30:54.000000000 +0200
@@ -1061,6 +1061,8 @@ struct blk_plug {
struct list_head list; /* requests */
struct list_head mq_list; /* blk-mq requests */
struct list_head cb_list; /* md requires an unplug callback */
+ struct bio_list bio_list; /* list of queued bios */
+ int in_generic_make_request;
};
#define BLK_MAX_REQUEST_COUNT 16
@@ -1100,10 +1102,11 @@ static inline bool blk_needs_flush_plug(
return plug &&
(!list_empty(&plug->list) ||
!list_empty(&plug->mq_list) ||
- !list_empty(&plug->cb_list));
+ !list_empty(&plug->cb_list) ||
+ !bio_list_empty(&plug->bio_list));
}
-extern void blk_flush_bio_list(void);
+extern void blk_flush_bio_list(struct blk_plug *plug);
/*
* tag stuff
Index: linux-3.15-rc5/include/linux/sched.h
===================================================================
--- linux-3.15-rc5.orig/include/linux/sched.h 2014-05-29 23:07:01.000000000 +0200
+++ linux-3.15-rc5/include/linux/sched.h 2014-05-29 23:07:08.000000000 +0200
@@ -126,7 +126,6 @@ struct sched_attr {
struct exec_domain;
struct futex_pi_state;
struct robust_list_head;
-struct bio_list;
struct fs_struct;
struct perf_event_context;
struct blk_plug;
@@ -1427,9 +1426,6 @@ struct task_struct {
/* journalling filesystem info */
void *journal_info;
-/* stacked block device info */
- struct bio_list *bio_list;
-
#ifdef CONFIG_BLOCK
/* stack plugging */
struct blk_plug *plug;
Index: linux-3.15-rc5/fs/bio.c
===================================================================
--- linux-3.15-rc5.orig/fs/bio.c 2014-05-29 23:19:04.000000000 +0200
+++ linux-3.15-rc5/fs/bio.c 2014-05-29 23:36:40.000000000 +0200
@@ -352,23 +352,20 @@ static void bio_alloc_rescue(struct work
* However, stacking drivers should use bio_set, so this shouldn't be
* an issue.
*/
-void blk_flush_bio_list(void)
+void blk_flush_bio_list(struct blk_plug *plug)
{
struct bio *bio;
- struct bio_list list = *current->bio_list;
- bio_list_init(current->bio_list);
- while ((bio = bio_list_pop(&list))) {
+ while ((bio = bio_list_pop(&plug->bio_list))) {
struct bio_set *bs = bio->bi_pool;
- if (unlikely(!bs)) {
- bio_list_add(current->bio_list, bio);
- } else {
- spin_lock(&bs->rescue_lock);
- bio_list_add(&bs->rescue_list, bio);
- spin_unlock(&bs->rescue_lock);
+ if (!bs)
+ bs = fs_bio_set;
- queue_work(bs->rescue_workqueue, &bs->rescue_work);
- }
+ spin_lock(&bs->rescue_lock);
+ bio_list_add(&bs->rescue_list, bio);
+ spin_unlock(&bs->rescue_lock);
+
+ queue_work(bs->rescue_workqueue, &bs->rescue_work);
}
}
Index: linux-3.15-rc5/kernel/sched/core.c
===================================================================
--- linux-3.15-rc5.orig/kernel/sched/core.c 2014-05-29 23:17:04.000000000 +0200
+++ linux-3.15-rc5/kernel/sched/core.c 2014-05-29 23:18:28.000000000 +0200
@@ -2734,13 +2734,6 @@ static inline void sched_submit_work(str
if (!tsk->state || tsk_is_pi_blocked(tsk))
return;
/*
- * If there are bios on the bio list, flush them to the appropriate
- * rescue threads.
- */
- if (unlikely(current->bio_list != NULL) &&
- !bio_list_empty(current->bio_list))
- blk_flush_bio_list();
- /*
* If we are going to sleep and we have plugged IO queued,
* make sure to submit it to avoid deadlocks.
*/
Index: linux-3.15-rc5/drivers/md/dm-bufio.c
===================================================================
--- linux-3.15-rc5.orig/drivers/md/dm-bufio.c 2014-05-30 00:25:55.000000000 +0200
+++ linux-3.15-rc5/drivers/md/dm-bufio.c 2014-05-30 00:31:28.000000000 +0200
@@ -169,7 +169,7 @@ static inline int dm_bufio_cache_index(s
#define DM_BUFIO_CACHE(c) (dm_bufio_caches[dm_bufio_cache_index(c)])
#define DM_BUFIO_CACHE_NAME(c) (dm_bufio_cache_names[dm_bufio_cache_index(c)])
-#define dm_bufio_in_request() (!!current->bio_list)
+#define dm_bufio_in_request() (current->plug && current->plug->in_generic_make_request)
static void dm_bufio_lock(struct dm_bufio_client *c)
{
Index: linux-3.15-rc5/drivers/md/raid1.c
===================================================================
--- linux-3.15-rc5.orig/drivers/md/raid1.c 2014-05-30 00:19:28.000000000 +0200
+++ linux-3.15-rc5/drivers/md/raid1.c 2014-05-30 00:33:11.000000000 +0200
@@ -912,8 +912,8 @@ static sector_t wait_barrier(struct r1co
(!conf->barrier ||
((conf->start_next_window <
conf->next_resync + RESYNC_SECTORS) &&
- current->bio_list &&
- !bio_list_empty(current->bio_list))),
+ current->plug &&
+ !bio_list_empty(¤t->plug->bio_list))),
conf->resync_lock);
conf->nr_waiting--;
}
@@ -1052,7 +1052,7 @@ static void raid1_unplug(struct blk_plug
struct r1conf *conf = mddev->private;
struct bio *bio;
- if (from_schedule || current->bio_list) {
+ if (from_schedule || (current->plug && current->plug->in_generic_make_request)) {
spin_lock_irq(&conf->device_lock);
bio_list_merge(&conf->pending_bio_list, &plug->pending);
conf->pending_count += plug->pending_cnt;
Index: linux-3.15-rc5/drivers/md/raid10.c
===================================================================
--- linux-3.15-rc5.orig/drivers/md/raid10.c 2014-05-30 00:23:51.000000000 +0200
+++ linux-3.15-rc5/drivers/md/raid10.c 2014-05-30 00:32:50.000000000 +0200
@@ -1045,8 +1045,8 @@ static void wait_barrier(struct r10conf
wait_event_lock_irq(conf->wait_barrier,
!conf->barrier ||
(conf->nr_pending &&
- current->bio_list &&
- !bio_list_empty(current->bio_list)),
+ current->plug &&
+ !bio_list_empty(¤t->plug->bio_list)),
conf->resync_lock);
conf->nr_waiting--;
}
@@ -1122,7 +1122,7 @@ static void raid10_unplug(struct blk_plu
struct r10conf *conf = mddev->private;
struct bio *bio;
- if (from_schedule || current->bio_list) {
+ if (from_schedule || (current->plug && current->plug->in_generic_make_request)) {
spin_lock_irq(&conf->device_lock);
bio_list_merge(&conf->pending_bio_list, &plug->pending);
conf->pending_count += plug->pending_cnt;
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] block: flush queued bios when the process blocks
@ 2014-06-26 23:46 Mikulas Patocka
0 siblings, 0 replies; 11+ messages in thread
From: Mikulas Patocka @ 2014-06-26 23:46 UTC (permalink / raw
To: Jens Axboe, Kent Overstreet
Cc: linux-kernel, dm-devel, Alasdair G. Kergon, Mike Snitzer
Hi Jens
Would you please consider applying this patch?
Regarding your idea about merging bio and request plugging - I think this
could be done later when it is properly designed by Kent Overstreet or
someone else.
We need this patch to fix the deadlock in dm snapshot.
Mikulas
---------- Forwarded message ----------
Date: Tue, 27 May 2014 11:03:36 -0400 (EDT)
From: Mikulas Patocka <mpatocka@redhat.com>
To: Jens Axboe <axboe@kernel.dk>, Kent Overstreet <kmo@daterainc.com>
Cc: linux-kernel@vger.kernel.org, dm-devel@redhat.com,
Alasdair G. Kergon <agk@redhat.com>, Mike Snitzer <msnitzer@redhat.com>
Subject: [PATCH] block: flush queued bios when the process blocks
The block layer uses per-process bio list to avoid recursion in
generic_make_request. When generic_make_request is called recursively, the
bio is added to current->bio_list and the function returns immediatelly.
The top-level instance of generic_make_requests takes bios from
current->bio_list and processes them.
This bio queuing can result in deadlocks. The following deadlock was
observed:
1) Process A sends one-page read bio to the dm-snapshot target. The bio
spans snapshot chunk boundary and so it is split to two bios by device
mapper.
2) Device mapper creates the first sub-bio and sends it to the snapshot
driver.
3) The function snapshot_map calls track_chunk (that allocates a structure
dm_snap_tracked_chunk and adds it to tracked_chunk_hash) and then it
remaps the bio to the underlying linear target and exits with
DM_MAPIO_REMAPPED.
4) The remapped bio is submitted with generic_make_request, but it isn't
processed - it is added to current->bio_list instead.
5) Meanwhile, process B executes pending_complete for the affected chunk,
it takes down_write(&s->lock) and then loops in
__check_for_conflicting_io, waiting for dm_snap_tracked_chunk created in
step 3) to be released.
6) Process A continues, it creates a new bio for the rest of the original
bio.
7) snapshot_map is called for this new bio, it waits on
down_write(&s->lock) that is held in step 5).
The resulting deadlock:
* bio added to current->bio_list at step 4) waits until the function in
step 7) finishes
* the function in step 7) waits until s->lock held in step 5) is released
* the process in step 5) waits until the bio queued in step 4) finishes
The general problem is that queuing bios on current->bio_list introduces
additional lock dependencies. If a device mapper target sends a bio to
some block device, it assumes that the bio only takes locks of the target
block device or devices that are below the target device. However, if the
bio is added to queue on current->bio_list, it creates artifical locking
dependency on locks taken by other bios that are on current->bio_list. In
the above scenario, this artifical locking dependency results in
deadlock.
Kent Overstreet already created a workqueue for every bio set and there is
a code that tries to resolve some low-memory deadlocks by redirecting bios
queued on current->bio_list to the workqueue if the system is low on
memory. However, other deadlocks (as described above) may happen without
any low memory condition.
This patch generalizes Kent's concept, it redirects bios on
current->bio_list to the bio_set's workqueue on every schedule call.
Consequently, when the process blocks on a mutex, the bios queued on
current->bio_list are dispatched to independent workqueus and they can
complete without waiting for the mutex to be available.
Bios allocated with bio_kmalloc do not have bio_set, so they are not
redirected, however bio_kmalloc shouldn't be used by stacking drivers (it
is currently used by raid1.c and raid10.c, we need to change it to
bio_set).
Note to stable kernel maintainers: before backporting this patch, you also
need to backport df2cb6daa4cbc34406bc4b1ac9b9335df1083a72.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org
---
fs/bio.c | 84 ++++++++++++++-----------------------------------
include/linux/blkdev.h | 7 +++-
kernel/sched/core.c | 7 ++++
3 files changed, 37 insertions(+), 61 deletions(-)
Index: linux-3.15-rc5/fs/bio.c
===================================================================
--- linux-3.15-rc5.orig/fs/bio.c 2014-05-26 19:02:47.000000000 +0200
+++ linux-3.15-rc5/fs/bio.c 2014-05-27 00:00:13.000000000 +0200
@@ -342,35 +342,34 @@ static void bio_alloc_rescue(struct work
}
}
-static void punt_bios_to_rescuer(struct bio_set *bs)
+/**
+ * blk_flush_bio_list
+ *
+ * Pop bios queued on current->bio_list and submit each of them to
+ * their rescue workqueue.
+ *
+ * If the bio doesn't have a bio_set, we leave it on current->bio_list.
+ * However, stacking drivers should use bio_set, so this shouldn't be
+ * an issue.
+ */
+void blk_flush_bio_list(void)
{
- struct bio_list punt, nopunt;
struct bio *bio;
+ struct bio_list list = *current->bio_list;
+ bio_list_init(current->bio_list);
- /*
- * In order to guarantee forward progress we must punt only bios that
- * were allocated from this bio_set; otherwise, if there was a bio on
- * there for a stacking driver higher up in the stack, processing it
- * could require allocating bios from this bio_set, and doing that from
- * our own rescuer would be bad.
- *
- * Since bio lists are singly linked, pop them all instead of trying to
- * remove from the middle of the list:
- */
-
- bio_list_init(&punt);
- bio_list_init(&nopunt);
-
- while ((bio = bio_list_pop(current->bio_list)))
- bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
-
- *current->bio_list = nopunt;
-
- spin_lock(&bs->rescue_lock);
- bio_list_merge(&bs->rescue_list, &punt);
- spin_unlock(&bs->rescue_lock);
+ while ((bio = bio_list_pop(&list))) {
+ struct bio_set *bs = bio->bi_pool;
+ if (unlikely(!bs)) {
+ bio_list_add(current->bio_list, bio);
+ } else {
+ spin_lock(&bs->rescue_lock);
+ bio_list_add(&bs->rescue_list, bio);
+ spin_unlock(&bs->rescue_lock);
- queue_work(bs->rescue_workqueue, &bs->rescue_work);
+ queue_work(bs->rescue_workqueue, &bs->rescue_work);
+ }
+ }
}
/**
@@ -410,7 +409,6 @@ static void punt_bios_to_rescuer(struct
*/
struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
{
- gfp_t saved_gfp = gfp_mask;
unsigned front_pad;
unsigned inline_vecs;
unsigned long idx = BIO_POOL_NONE;
@@ -428,36 +426,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_m
front_pad = 0;
inline_vecs = nr_iovecs;
} else {
- /*
- * generic_make_request() converts recursion to iteration; this
- * means if we're running beneath it, any bios we allocate and
- * submit will not be submitted (and thus freed) until after we
- * return.
- *
- * This exposes us to a potential deadlock if we allocate
- * multiple bios from the same bio_set() while running
- * underneath generic_make_request(). If we were to allocate
- * multiple bios (say a stacking block driver that was splitting
- * bios), we would deadlock if we exhausted the mempool's
- * reserve.
- *
- * We solve this, and guarantee forward progress, with a rescuer
- * workqueue per bio_set. If we go to allocate and there are
- * bios on current->bio_list, we first try the allocation
- * without __GFP_WAIT; if that fails, we punt those bios we
- * would be blocking to the rescuer workqueue before we retry
- * with the original gfp_flags.
- */
-
- if (current->bio_list && !bio_list_empty(current->bio_list))
- gfp_mask &= ~__GFP_WAIT;
-
p = mempool_alloc(bs->bio_pool, gfp_mask);
- if (!p && gfp_mask != saved_gfp) {
- punt_bios_to_rescuer(bs);
- gfp_mask = saved_gfp;
- p = mempool_alloc(bs->bio_pool, gfp_mask);
- }
front_pad = bs->front_pad;
inline_vecs = BIO_INLINE_VECS;
@@ -471,11 +440,6 @@ struct bio *bio_alloc_bioset(gfp_t gfp_m
if (nr_iovecs > inline_vecs) {
bvl = bvec_alloc(gfp_mask, nr_iovecs, &idx, bs->bvec_pool);
- if (!bvl && gfp_mask != saved_gfp) {
- punt_bios_to_rescuer(bs);
- gfp_mask = saved_gfp;
- bvl = bvec_alloc(gfp_mask, nr_iovecs, &idx, bs->bvec_pool);
- }
if (unlikely(!bvl))
goto err_free;
Index: linux-3.15-rc5/kernel/sched/core.c
===================================================================
--- linux-3.15-rc5.orig/kernel/sched/core.c 2014-05-26 19:30:51.000000000 +0200
+++ linux-3.15-rc5/kernel/sched/core.c 2014-05-27 00:23:00.000000000 +0200
@@ -2734,6 +2734,13 @@ static inline void sched_submit_work(str
if (!tsk->state || tsk_is_pi_blocked(tsk))
return;
/*
+ * If there are bios on the bio list, flush them to the appropriate
+ * rescue threads.
+ */
+ if (unlikely(current->bio_list != NULL) &&
+ !bio_list_empty(current->bio_list))
+ blk_flush_bio_list();
+ /*
* If we are going to sleep and we have plugged IO queued,
* make sure to submit it to avoid deadlocks.
*/
Index: linux-3.15-rc5/include/linux/blkdev.h
===================================================================
--- linux-3.15-rc5.orig/include/linux/blkdev.h 2014-05-26 23:54:48.000000000 +0200
+++ linux-3.15-rc5/include/linux/blkdev.h 2014-05-26 23:56:41.000000000 +0200
@@ -1103,6 +1103,8 @@ static inline bool blk_needs_flush_plug(
!list_empty(&plug->cb_list));
}
+extern void blk_flush_bio_list(void);
+
/*
* tag stuff
*/
@@ -1634,12 +1636,15 @@ static inline void blk_schedule_flush_pl
{
}
-
static inline bool blk_needs_flush_plug(struct task_struct *tsk)
{
return false;
}
+static inline void blk_flush_bio_list(void)
+{
+}
+
#endif /* CONFIG_BLOCK */
#endif
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-06-26 23:47 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-26 23:46 [PATCH] block: flush queued bios when the process blocks Mikulas Patocka
-- strict thread matches above, loose matches on Subject: below --
2014-05-27 15:03 Mikulas Patocka
2014-05-27 15:08 ` Jens Axboe
2014-05-27 15:23 ` Mikulas Patocka
2014-05-27 15:42 ` Jens Axboe
2014-05-27 16:26 ` Mikulas Patocka
2014-05-27 17:42 ` Jens Axboe
2014-05-27 19:56 ` Mikulas Patocka
2014-05-27 20:06 ` Kent Overstreet
2014-05-29 23:52 ` Mikulas Patocka
2014-05-27 17:59 ` Kent Overstreet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.