4.4-rc: 28 bioset threads on small notebook

Hi!

I know it is normal to spawn 8 threads for every single function,

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

Quoted 3 lines

Hi!

I know it is normal to spawn 8 threads for every single function,

...

Quoted 3 lines

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

...

How many physical block devices do you have?

DM is doing its part to not contribute to this:
dbba42d8a ("dm: eliminate unused "bioset" process for each bio-based DM device")

(but yeah, all these extra 'bioset' threads aren't ideal)

Quoted 3 lines · Pavel Machek

Hi!

I know it is normal to spawn 8 threads for every single function,

...

Quoted 3 lines

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

...

How many physical block devices do you have?

DM is doing its part to not contribute to this:
dbba42d8a ("dm: eliminate unused "bioset" process for each bio-based DM device")

(but yeah, all these extra 'bioset' threads aren't ideal)

Quoted 12 lines · Mike Snitzer

Quoted 3 lines · Pavel Machek

Hi!

I know it is normal to spawn 8 threads for every single function,

...

Quoted 3 lines

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

...

Quoted 6 lines

How many physical block devices do you have?

DM is doing its part to not contribute to this:
dbba42d8a ("dm: eliminate unused "bioset" process for each bio-based DM device")

(but yeah, all these extra 'bioset' threads aren't ideal)

Still there in 4.4-final.

Quoted 19 lines · Pavel Machek

Quoted 17 lines · Mike Snitzer

Quoted 3 lines · Pavel Machek

Hi!

I know it is normal to spawn 8 threads for every single function,

...

Quoted 3 lines

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

...

How many physical block devices do you have?

DM is doing its part to not contribute to this:
dbba42d8a ("dm: eliminate unused "bioset" process for each bio-based DM device")

Quoted 3 lines

Quoted 1 line

(but yeah, all these extra 'bioset' threads aren't ideal)

Still there in 4.4-final.

...and still there in 4.5-rc4 :-(.
Pavel

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 1 line

I know it is normal to spawn 8 threads for every single function,

...

Quoted 3 lines

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

...

How many physical block devices do you have?

DM is doing its part to not contribute to this:
dbba42d8a ("dm: eliminate unused "bioset" process for each bio-based DM device")

(but yeah, all these extra 'bioset' threads aren't ideal)

Quoted 3 lines

Quoted 1 line

Still there in 4.4-final.

...and still there in 4.5-rc4 :-(.

You're directing this concern to the wrong person.

I already told you DM is _not_ contributing any extra "bioset" threads (ever since commit dbba42d8a).

But in general, these "bioset" threads are a side-effect of the late-bio-splitting support. So is your position on it: "I don't like that feature if it comes at the expense of adding resources I can _see_ for something I (naively?) view as useless"?

Just seems... naive... but you could be trying to say something else entirely.

Anyway, if you don't like something: understand why it is there and then try to fix it to your liking (without compromising why it was there to begin with).

Hi!

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 1 line

I know it is normal to spawn 8 threads for every single function,

...

Quoted 3 lines

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

...

How many physical block devices do you have?

DM is doing its part to not contribute to this:
dbba42d8a ("dm: eliminate unused "bioset" process for each bio-based DM device")

(but yeah, all these extra 'bioset' threads aren't ideal)

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Still there in 4.4-final.

...and still there in 4.5-rc4 :-(.

You're directing this concern to the wrong person.

I already told you DM is _not_ contributing any extra "bioset" threads (ever since commit dbba42d8a).

Well, sorry about that. Note that l-k is on the cc list, so hopefully the right person sees it too.

Ok, let me check... it seems that 54efd50bfd873e2dbf784e0b21a8027ba4299a3e is responsible, thus Kent Overstreet <[email protected]> is to blame.

Um, and you acked the patch, so you are partly responsible.

Quoted 7 lines

But in general, these "bioset" threads are a side-effect of the late-bio-splitting support. So is your position on it: "I don't like that feature if it comes at the expense of adding resources I can _see_ for something I (naively?) view as useless"?

Just seems... naive... but you could be trying to say something else entirely.

Quoted 3 lines

Anyway, if you don't like something: understand why it is there and then try to fix it to your liking (without compromising why it was there to begin with).

Well, 28 kernel threads on a notebook is a bug, plain and simple. Do you argue it is not?

Quoted 8 lines

Quoted 4 lines

You're directing this concern to the wrong person.

I already told you DM is _not_ contributing any extra "bioset" threads (ever since commit dbba42d8a).

Well, sorry about that. Note that l-k is on the cc list, so hopefully the right person sees it too.

Quoted 5 lines

Ok, let me check... it seems that 54efd50bfd873e2dbf784e0b21a8027ba4299a3e is responsible, thus Kent Overstreet <[email protected]> is to blame.

Um, and you acked the patch, so you are partly responsible.

You still haven't shown you even understand the patch so don't try to blame me for one aspect you don't like.

Quoted 12 lines

Quoted 11 lines

But in general, these "bioset" threads are a side-effect of the late-bio-splitting support. So is your position on it: "I don't like that feature if it comes at the expense of adding resources I can _see_ for something I (naively?) view as useless"?

Just seems... naive... but you could be trying to say something else entirely.

Anyway, if you don't like something: understand why it is there and then try to fix it to your liking (without compromising why it was there to begin with).

Quoted 2 lines

Well, 28 kernel threads on a notebook is a bug, plain and simple. Do you argue it is not?

Just implies you have 28 request_queues right? You clearly have something else going on on your notebook than the average notebook user.

Hi!

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

You're directing this concern to the wrong person.

I already told you DM is _not_ contributing any extra "bioset" threads (ever since commit dbba42d8a).

Well, sorry about that. Note that l-k is on the cc list, so hopefully the right person sees it too.

Quoted 8 lines

Quoted 5 lines

Ok, let me check... it seems that 54efd50bfd873e2dbf784e0b21a8027ba4299a3e is responsible, thus Kent Overstreet <[email protected]> is to blame.

Um, and you acked the patch, so you are partly responsible.

You still haven't shown you even understand the patch so don't try to blame me for one aspect you don't like.

Well, I don't have to understand the patch to argue its wrong.

Quoted 15 lines

Quoted 14 lines

Quoted 11 lines

But in general, these "bioset" threads are a side-effect of the late-bio-splitting support. So is your position on it: "I don't like that feature if it comes at the expense of adding resources I can _see_ for something I (naively?) view as useless"?

Just seems... naive... but you could be trying to say something else entirely.

Anyway, if you don't like something: understand why it is there and then try to fix it to your liking (without compromising why it was there to begin with).

Well, 28 kernel threads on a notebook is a bug, plain and simple. Do you argue it is not?

Quoted 3 lines

Just implies you have 28 request_queues right? You clearly have something else going on on your notebook than the average notebook user.

I'm not using the modules, but otherwise I'm not doing anything special. How many request_queues should I expect? How many do you have on your notebook?

-----Original Message-----
From: Kent Overstreet [mailto:[email protected]]

Quoted 14 lines · Pavel Machek

Hi!

Quoted 12 lines

Quoted 12 lines

Quoted 4 lines

You're directing this concern to the wrong person.

I already told you DM is _not_ contributing any extra "bioset" threads (ever since commit dbba42d8a).

Well, sorry about that. Note that l-k is on the cc list, so hopefully the right person sees it too.

Ok, let me check... it seems that 54efd50bfd873e2dbf784e0b21a8027ba4299a3e is responsible, thus Kent Overstreet <[email protected]> is to blame.

Quoted 16 lines

Quoted 4 lines

Quoted 1 line

Um, and you acked the patch, so you are partly responsible.

You still haven't shown you even understand the patch so don't try to blame me for one aspect you don't like.

Well, I don't have to understand the patch to argue its wrong.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

But in general, these "bioset" threads are a side-effect of the late-bio-splitting support. So is your position on it: "I don't like that feature if it comes at the expense of adding resources I can _see_ for something I (naively?) view as useless"?

Just seems... naive... but you could be trying to say something else entirely.

Anyway, if you don't like something: understand why it is there and

then

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

try to fix it to your liking (without compromising why it was there to begin with).

Well, 28 kernel threads on a notebook is a bug, plain and simple. Do you argue it is not?

Quoted 6 lines

Quoted 3 lines

Just implies you have 28 request_queues right? You clearly have something else going on on your notebook than the average notebook user.

I'm not using the modules, but otherwise I'm not doing anything special. How many request_queues should I expect? How many do you

have

Quoted 1 line

on your notebook?

It's one rescuer thread per bio_set, not one per request queue, so 28 is more than I'd expect but there's lots of random bio_sets so it's not entirely unexpected.

It'd be better to have the rescuers be per request_queue, just someone is going
to have to write the code.

Quoted 1 line

-----Original Message-----

So it's almost already "per request_queue"

Quoted 3 lines · Ming Lin-SSI

Quoted 1 line

-----Original Message-----

So it's almost already "per request_queue"

Yes, that is because of the following line:

q->bio_split = bioset_create(BIO_POOL_SIZE, 0);

in blk_alloc_queue_node().

Looks like this bio_set doesn't need to be per-request_queue, and now it is only used for fast-cloning bio for splitting, and one global split bio_set should be enough.

Quoted 9 lines · Ming Lei

Quoted 3 lines · Ming Lin-SSI

Quoted 1 line

-----Original Message-----

So it's almost already "per request_queue"

Yes, that is because of the following line:

q->bio_split = bioset_create(BIO_POOL_SIZE, 0);

Quoted 5 lines

in blk_alloc_queue_node().

Looks like this bio_set doesn't need to be per-request_queue, and now it is only used for fast-cloning bio for splitting, and one global split bio_set should be enough.

It does have to be per request queue for stacking block devices (which includes loopback).

Quoted 16 lines · Kent Overstreet

Quoted 14 lines · Ming Lei

Quoted 3 lines · Ming Lin-SSI

Quoted 1 line

-----Original Message-----

So it's almost already "per request_queue"

Yes, that is because of the following line:

q->bio_split = bioset_create(BIO_POOL_SIZE, 0);

in blk_alloc_queue_node().

Looks like this bio_set doesn't need to be per-request_queue, and now it is only used for fast-cloning bio for splitting, and one global split bio_set should be enough.

Quoted 2 lines

It does have to be per request queue for stacking block devices (which includes loopback).

In commit df2cb6daa4(block: Avoid deadlocks with bio allocation by stacking drivers), deadlock in this situation has been avoided already. Or are there other issues with global bio_set? I appreciate if you may explain it a bit if there are.

Quoted 21 lines · Ming Lei

Quoted 18 lines · Kent Overstreet

Quoted 7 lines · Ming Lei

Quoted 3 lines · Ming Lin-SSI

Quoted 1 line

-----Original Message-----

So it's almost already "per request_queue"

Yes, that is because of the following line:

Quoted 7 lines

q->bio_split = bioset_create(BIO_POOL_SIZE, 0);

in blk_alloc_queue_node().

Looks like this bio_set doesn't need to be per-request_queue, and now it is only used for fast-cloning bio for splitting, and one global split bio_set should be enough.

It does have to be per request queue for stacking block devices (which includes loopback).

Quoted 4 lines

In commit df2cb6daa4(block: Avoid deadlocks with bio allocation by stacking drivers), deadlock in this situation has been avoided already. Or are there other issues with global bio_set? I appreciate if you may explain it a bit if there are.

Even with commit df2cb6daa4 there is still risk of deadlocks (even without low memory condition), see:
https://patchwork.kernel.org/patch/7398411/

(you may recall you blocked this patch with concerns about performance, context switches, plug merging being compromised, etc.. to which I never circled back to verify your concerns)

But it illustrates the type of problems that can occur when your rescue infrastructure is shared across devices (in the context of df2cb6daa4, current->bio_list contains bios from multiple devices).

If a single splitting bio_set were shared across devices there would be no guarantee of forward progress with complex stacked devices (one or more devices could exhaust the reserve and starve out other devices in the stack). So keeping the bio_set per request_queue isn't prone to failure like a shared bio_set might be.

Quoted 24 lines · Mike Snitzer

Quoted 21 lines · Ming Lei

Quoted 18 lines · Kent Overstreet

Quoted 7 lines · Ming Lei

Quoted 3 lines · Ming Lin-SSI

Quoted 1 line

-----Original Message-----

So it's almost already "per request_queue"

Yes, that is because of the following line:

Quoted 7 lines

q->bio_split = bioset_create(BIO_POOL_SIZE, 0);

in blk_alloc_queue_node().

Looks like this bio_set doesn't need to be per-request_queue, and now it is only used for fast-cloning bio for splitting, and one global split bio_set should be enough.

It does have to be per request queue for stacking block devices (which includes loopback).

Quoted 8 lines

Quoted 4 lines

In commit df2cb6daa4(block: Avoid deadlocks with bio allocation by stacking drivers), deadlock in this situation has been avoided already. Or are there other issues with global bio_set? I appreciate if you may explain it a bit if there are.

Even with commit df2cb6daa4 there is still risk of deadlocks (even without low memory condition), see:
https://patchwork.kernel.org/patch/7398411/

That is definitely another problem which isn't related with low memory, and I guess Kent means there might be deadlock risk in case of shared bio_set.

Quoted 4 lines

(you may recall you blocked this patch with concerns about performance, context switches, plug merging being compromised, etc.. to which I never circled back to verify your concerns)

I still remember that problem:

1) Process A
- two bio(a, b) are splitted in dm's make_request funtion
- bio(a) is submitted via generic_make_request(), so it is staged in current->bio_list
- time t1
- before bio(b) is submitted, down_write(&s->lock) is run and never return

2) Process B:
- just during time t1, wait completion of bio(a) by down_write(&s->lock)

Then Process A waits the lock which is acquired by B first, and the two bio(a, b)
can't reach to driver/device at all.

Looks that current->bio_list is fragile to locks from make_request function, and moving the lock into workqueue context should be helpful.

And I am happy to continue to discuss this issue further.

Quoted 10 lines

But it illustrates the type of problems that can occur when your rescue infrastructure is shared across devices (in the context of df2cb6daa4, current->bio_list contains bios from multiple devices).

If a single splitting bio_set were shared across devices there would be no guarantee of forward progress with complex stacked devices (one or more devices could exhaust the reserve and starve out other devices in the stack). So keeping the bio_set per request_queue isn't prone to failure like a shared bio_set might be.

Not consider the dm lock problem, from Kent's commit(df2cb6daa4) log and the patch, looks forward progress can be guaranteed for stacked devices with same bio_set, but better to get Kent's clarification.

If forward progress can be guaranteed, percpu mempool might avoid easy exhausting, because it is reasonable to assume that one CPU can only provide a certain amount of bandwidth wrt. block transfer.

Quoted 9 lines · Ming Lei

Quoted 3 lines · Ming Lin-SSI

Quoted 1 line

-----Original Message-----

So it's almost already "per request_queue"

Yes, that is because of the following line:

q->bio_split = bioset_create(BIO_POOL_SIZE, 0);

Quoted 5 lines

in blk_alloc_queue_node().

Looks like this bio_set doesn't need to be per-request_queue, and now it is only used for fast-cloning bio for splitting, and one global split bio_set should be enough.

It does have to be per request queue for stacking block devices (which includes loopback).

Hi!

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 1 line

I know it is normal to spawn 8 threads for every single function,

...

Quoted 3 lines

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

...

How many physical block devices do you have?

DM is doing its part to not contribute to this:
dbba42d8a ("dm: eliminate unused "bioset" process for each bio-based DM device")

(but yeah, all these extra 'bioset' threads aren't ideal)

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Still there in 4.4-final.

...and still there in 4.5-rc4 :-(.

You're directing this concern to the wrong person.

I already told you DM is _not_ contributing any extra "bioset" threads (ever since commit dbba42d8a).

Well, sorry about that. Note that l-k is on the cc list, so hopefully the right person sees it too.

Ok, let me check... it seems that 54efd50bfd873e2dbf784e0b21a8027ba4299a3e is responsible, thus Kent Overstreet <[email protected]> is to blame.

Um, and you acked the patch, so you are partly responsible.

Quoted 32 lines · Pavel Machek

Hi!

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 1 line

I know it is normal to spawn 8 threads for every single function,

...

Quoted 3 lines

but 28 threads?

root 974 0.0 0.0 0 0 ? S< Dec08 0:00 [bioset]

...

How many physical block devices do you have?

DM is doing its part to not contribute to this:
dbba42d8a ("dm: eliminate unused "bioset" process for each bio-based DM device")

(but yeah, all these extra 'bioset' threads aren't ideal)

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Still there in 4.4-final.

...and still there in 4.5-rc4 :-(.

You're directing this concern to the wrong person.

I already told you DM is _not_ contributing any extra "bioset" threads (ever since commit dbba42d8a).

Well, sorry about that. Note that l-k is on the cc list, so hopefully the right person sees it too.

Ok, let me check... it seems that 54efd50bfd873e2dbf784e0b21a8027ba4299a3e is responsible, thus Kent Overstreet <[email protected]> is to blame.

Quoted 1 line

Um, and you acked the patch, so you are partly responsible.

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Quoted 7 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

-- >8 --
Subject: [PATCH] block: Make rescuer threads per request_queue, not per bioset

Note: this patch is very lightly tested.

Also, trigger rescuing whenever with bios on current->bio_list, instead of only when we block in bio_alloc_bioset(). This is more correct, and should result in fewer rescuer threads.

XXX: The current->bio_list plugging needs to be unified with the blk_plug mechanism.

TODO: If we change normal request_queue drivers to handle arbitrary size bios by processing requests incrementally, instead of splitting bios, then we can get rid of rescuer threads from those devices.

Quoted 7 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

-- >8 --
Subject: [PATCH] block: Make rescuer threads per request_queue, not per bioset

Note: this patch is very lightly tested.

Also, trigger rescuing whenever with bios on current->bio_list, instead of only when we block in bio_alloc_bioset(). This is more correct, and should result in fewer rescuer threads.

XXX: The current->bio_list plugging needs to be unified with the blk_plug mechanism.

TODO: If we change normal request_queue drivers to handle arbitrary size bios by processing requests incrementally, instead of splitting bios, then we can get rid of rescuer threads from those devices.

---
 block/bio.c            | 107 ++++---------------------------------------------
 block/blk-core.c       |  58 ++++++++++++++++++++++++---
 block/blk-sysfs.c      |   2 +
 include/linux/bio.h    |  16 ++++----
 include/linux/blkdev.h |  10 +++++
 include/linux/sched.h  |   2 +-
 kernel/sched/core.c    |   4 ++
 7 files changed, 83 insertions(+), 116 deletions(-)

block/bio.c +7 -100

index f3b5786202..9ad54a9b12 100644
@@ -336,54 +336,6 @@ void bio_chain(struct bio *bio, struct bio *parent)
 }
 EXPORT_SYMBOL(bio_chain);
 
-static void bio_alloc_rescue(struct work_struct *work)
-{
-	struct bio_set *bs = container_of(work, struct bio_set, rescue_work);
-	struct bio *bio;
-
-	while (1) {
-		spin_lock(&bs->rescue_lock);
-		bio = bio_list_pop(&bs->rescue_list);
-		spin_unlock(&bs->rescue_lock);
-
-		if (!bio)
-			break;
-
-		generic_make_request(bio);
-	}
-}
-
-static void punt_bios_to_rescuer(struct bio_set *bs)
-{
-	struct bio_list punt, nopunt;
-	struct bio *bio;
-
-	/*
-	 * 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);
-
-	queue_work(bs->rescue_workqueue, &bs->rescue_work);
-}
-
 /**
  * bio_alloc_bioset - allocate a bio for I/O
  * @gfp_mask:   the GFP_ mask given to the slab allocator

@@ -421,54 +373,27 @@ static void punt_bios_to_rescuer(struct bio_set *bs)
  */
 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;
 	struct bio_vec *bvl = NULL;
 	struct bio *bio;
 	void *p;
 
-	if (!bs) {
-		if (nr_iovecs > UIO_MAXIOV)
-			return NULL;
+	WARN(current->bio_list &&
+	     !current->bio_list->q->rescue_workqueue,
+	     "allocating bio beneath generic_make_request() without rescuer");
 
+	if (nr_iovecs > UIO_MAXIOV)
+		return NULL;
+
+	if (!bs) {
 		p = kmalloc(sizeof(struct bio) +
 			    nr_iovecs * sizeof(struct bio_vec),
 			    gfp_mask);
 		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_DIRECT_RECLAIM; 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_DIRECT_RECLAIM;
-
 		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;
 	}

@@ -483,12 +408,6 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
 		unsigned long idx = 0;
 
 		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;
 

@@ -1938,10 +1857,6 @@ int biovec_init_pool(mempool_t *pool, int pool_entries)
 
 void bioset_exit(struct bio_set *bs)
 {
-	if (bs->rescue_workqueue)
-		destroy_workqueue(bs->rescue_workqueue);
-	bs->rescue_workqueue = NULL;
-
 	mempool_exit(&bs->bio_pool);
 	mempool_exit(&bs->bvec_pool);
 

@@ -1968,10 +1883,6 @@ static int __bioset_init(struct bio_set *bs,
 
 	bs->front_pad = front_pad;
 
-	spin_lock_init(&bs->rescue_lock);
-	bio_list_init(&bs->rescue_list);
-	INIT_WORK(&bs->rescue_work, bio_alloc_rescue);
-
 	bs->bio_slab = bio_find_or_create_slab(front_pad + back_pad);
 	if (!bs->bio_slab)
 		return -ENOMEM;

@@ -1983,10 +1894,6 @@ static int __bioset_init(struct bio_set *bs,
 	    biovec_init_pool(&bs->bvec_pool, pool_size))
 		goto bad;
 
-	bs->rescue_workqueue = alloc_workqueue("bioset", WQ_MEM_RECLAIM, 0);
-	if (!bs->rescue_workqueue)
-		goto bad;
-
 	return 0;
 bad:
 	bioset_exit(bs);

block/blk-core.c +52 -6

index 7e3cfa9c88..f716164cb3 100644
@@ -48,6 +48,8 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(block_unplug);
 
 DEFINE_IDA(blk_queue_ida);
 
+static void bio_rescue_work(struct work_struct *);
+
 /*
  * For the allocated request tables
  */

@@ -759,11 +761,21 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 				PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
 		goto fail_bdi;
 
-	if (blkcg_init_queue(q))
+	spin_lock_init(&q->rescue_lock);
+	bio_list_init(&q->rescue_list);
+	INIT_WORK(&q->rescue_work, bio_rescue_work);
+
+	q->rescue_workqueue = alloc_workqueue("bioset", WQ_MEM_RECLAIM, 0);
+	if (!q->rescue_workqueue)
 		goto fail_ref;
 
+	if (blkcg_init_queue(q))
+		goto fail_rescue;
+
 	return q;
 
+fail_rescue:
+	destroy_workqueue(q->rescue_workqueue);
 fail_ref:
 	percpu_ref_exit(&q->q_usage_counter);
 fail_bdi:

@@ -1994,7 +2006,7 @@ generic_make_request_checks(struct bio *bio)
  */
 blk_qc_t generic_make_request(struct bio *bio)
 {
-	struct bio_list bio_list_on_stack;
+	struct bio_plug_list bio_list_on_stack;
 	blk_qc_t ret = BLK_QC_T_NONE;
 
 	if (!generic_make_request_checks(bio))

@@ -2011,7 +2023,9 @@ blk_qc_t generic_make_request(struct bio *bio)
 	 * should be added at the tail
 	 */
 	if (current->bio_list) {
-		bio_list_add(current->bio_list, bio);
+		WARN(!current->bio_list->q->rescue_workqueue,
+		     "submitting bio beneath generic_make_request() without rescuer");
+		bio_list_add(&current->bio_list->bios, bio);
 		goto out;
 	}
 

@@ -2030,19 +2044,23 @@ blk_qc_t generic_make_request(struct bio *bio)
 	 * bio_list, and call into ->make_request() again.
 	 */
 	BUG_ON(bio->bi_next);
-	bio_list_init(&bio_list_on_stack);
+	bio_list_init(&bio_list_on_stack.bios);
 	current->bio_list = &bio_list_on_stack;
+
 	do {
 		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 
+		current->bio_list->q = q;
+
 		if (likely(blk_queue_enter(q, false) == 0)) {
 			ret = q->make_request_fn(q, bio);
 
 			blk_queue_exit(q);
 
-			bio = bio_list_pop(current->bio_list);
+			bio = bio_list_pop(&current->bio_list->bios);
 		} else {
-			struct bio *bio_next = bio_list_pop(current->bio_list);
+			struct bio *bio_next =
+				bio_list_pop(&current->bio_list->bios);
 
 			bio_io_error(bio);
 			bio = bio_next;

@@ -2055,6 +2073,34 @@ blk_qc_t generic_make_request(struct bio *bio)
 }
 EXPORT_SYMBOL(generic_make_request);
 
+static void bio_rescue_work(struct work_struct *work)
+{
+	struct request_queue *q =
+		container_of(work, struct request_queue, rescue_work);
+	struct bio *bio;
+
+	while (1) {
+		spin_lock(&q->rescue_lock);
+		bio = bio_list_pop(&q->rescue_list);
+		spin_unlock(&q->rescue_lock);
+
+		if (!bio)
+			break;
+
+		generic_make_request(bio);
+	}
+}
+
+void blk_punt_blocked_bios(struct bio_plug_list *list)
+{
+	spin_lock(&list->q->rescue_lock);
+	bio_list_merge(&list->q->rescue_list, &list->bios);
+	bio_list_init(&list->bios);
+	spin_unlock(&list->q->rescue_lock);
+
+	queue_work(list->q->rescue_workqueue, &list->q->rescue_work);
+}
+
 /**
  * submit_bio - submit a bio to the block device layer for I/O
  * @bio: The &struct bio which describes the I/O

block/blk-sysfs.c +2 -0

index 7f27a18cc4..77529238d1 100644
@@ -660,6 +660,8 @@ static void blk_release_queue(struct kobject *kobj)
 
 	blk_trace_shutdown(q);
 
+	if (q->rescue_workqueue)
+		destroy_workqueue(q->rescue_workqueue);
 	if (q->bio_split)
 		bioset_free(q->bio_split);
 

include/linux/bio.h +7 -9

index 1ffe8e37ae..87eeec7eda 100644
@@ -658,6 +658,13 @@ static inline struct bio *bio_list_get(struct bio_list *bl)
 	return bio;
 }
 
+struct bio_plug_list {
+	struct bio_list		bios;
+	struct request_queue	*q;
+};
+
+void blk_punt_blocked_bios(struct bio_plug_list *);
+
 /*
  * Increment chain count for the bio. Make sure the CHAIN flag update
  * is visible before the raised count.

@@ -687,15 +694,6 @@ struct bio_set {
 	mempool_t bio_integrity_pool;
 	mempool_t bvec_integrity_pool;
 #endif
-
-	/*
-	 * Deadlock avoidance for stacking block drivers: see comments in
-	 * bio_alloc_bioset() for details
-	 */
-	spinlock_t		rescue_lock;
-	struct bio_list		rescue_list;
-	struct work_struct	rescue_work;
-	struct workqueue_struct	*rescue_workqueue;
 };
 
 struct biovec_slab {

include/linux/blkdev.h +10 -0

index c47c358ba0..f64b886c65 100644
@@ -476,6 +476,16 @@ struct request_queue {
 	struct bio_set		*bio_split;
 
 	bool			mq_sysfs_init_done;
+
+	/*
+	 * Deadlock avoidance, to deal with the plugging in
+	 * generic_make_request() that converts recursion to iteration to avoid
+	 * stack overflow:
+	 */
+	spinlock_t		rescue_lock;
+	struct bio_list		rescue_list;
+	struct work_struct	rescue_work;
+	struct workqueue_struct	*rescue_workqueue;
 };
 
 #define QUEUE_FLAG_QUEUED	1	/* uses generic tag queueing */

include/linux/sched.h +1 -1

index 2865d10a28..59df7a1030 100644
@@ -1762,7 +1762,7 @@ struct task_struct {
 	void *journal_info;
 
 /* stacked block device info */
-	struct bio_list *bio_list;
+	struct bio_plug_list *bio_list;
 
 #ifdef CONFIG_BLOCK
 /* stack plugging */

kernel/sched/core.c +4 -0

index bd39d698cb..23b6290ba1 100644
@@ -3439,6 +3439,10 @@ static inline void sched_submit_work(struct task_struct *tsk)
 {
 	if (!tsk->state || tsk_is_pi_blocked(tsk))
 		return;
+
+	if (tsk->bio_list && !bio_list_empty(&tsk->bio_list->bios))
+		blk_punt_blocked_bios(tsk->bio_list);
+
 	/*
 	 * If we are going to sleep and we have plugged IO queued,
 	 * make sure to submit it to avoid deadlocks.

Quoted 6 lines · Kent Overstreet

Quoted 4 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

Quoted 4 lines

Quoted 2 lines

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

I guess it would be nice for me to test it... but what it is against? I tried after v4.10-rc5 and linux-next, but got rejects in both cases.

Quoted 6 lines · Kent Overstreet

Quoted 4 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

Quoted 4 lines

Quoted 2 lines

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

I guess it would be nice for me to test it... but what it is against? I tried after v4.10-rc5 and linux-next, but got rejects in both cases.

Quoted 7 lines · Pavel Machek

Quoted 6 lines · Kent Overstreet

Quoted 4 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

I guess it would be nice for me to test it... but what it is against? I tried after v4.10-rc5 and linux-next, but got rejects in both cases.

Sorry, I forgot I had a few other patches in my branch that touch mempool/biosets code.

Also, after thinking about it more and looking at the relevant code, I'm pretty sure we don't need rescuer threads for block devices that just split bios - i.e. most of them, so I changed my patch to do that.

Tested it by ripping out the current->bio_list checks/workarounds from the bcache code, appears to work:

Quoted 7 lines · Pavel Machek

Quoted 6 lines · Kent Overstreet

Quoted 4 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

I guess it would be nice for me to test it... but what it is against? I tried after v4.10-rc5 and linux-next, but got rejects in both cases.

Sorry, I forgot I had a few other patches in my branch that touch mempool/biosets code.

Also, after thinking about it more and looking at the relevant code, I'm pretty sure we don't need rescuer threads for block devices that just split bios - i.e. most of them, so I changed my patch to do that.

Tested it by ripping out the current->bio_list checks/workarounds from the bcache code, appears to work:

-- >8 --
Subject: [PATCH] block: Make rescuer threads per request_queue, not per bioset

Also, trigger rescuing whenever with bios on current->bio_list, instead of only when we block in bio_alloc_bioset(). This is more correct, and should result in fewer rescuer threads.

XXX: The current->bio_list plugging needs to be unified with the blk_plug mechanism.

Signed-off-by Kent Overstreet <[email protected]>

---

Quoted 19 lines · Kent Overstreet

Quoted 7 lines · Pavel Machek

Quoted 6 lines · Kent Overstreet

Quoted 4 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

I guess it would be nice for me to test it... but what it is against? I tried after v4.10-rc5 and linux-next, but got rejects in both cases.

Sorry, I forgot I had a few other patches in my branch that touch mempool/biosets code.

Quoted 6 lines

Also, after thinking about it more and looking at the relevant code, I'm pretty sure we don't need rescuer threads for block devices that just split bios - i.e. most of them, so I changed my patch to do that.

Tested it by ripping out the current->bio_list checks/workarounds from the bcache code, appears to work:

Feedback on this patch below, but first:

There are deeper issues with the current->bio_list and rescue workqueues than thread counts.

I cannot help but feel like you (and Jens) are repeatedly ignoring the issue that has been raised numerous times, most recently:
https://www.redhat.com/archives/dm-devel/2017-February/msg00059.html

FYI, this test (albeit ugly) can be used to check if the dm-snapshot deadlock is fixed:
https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html

This situation is the unfortunate pathological worst case for what happens when changes are merged and nobody wants to own fixing the
unforseen implications/regressions. Like everyone else in a position
of Linux maintenance I've tried to stay away from owning the responsibility of a fix -- it isn't working. Ok, I'll stop bitching now.. I do bear responsibility for not digging in myself. We're all busy and this issue is "hard".

Quoted 42 lines · Mike Snitzer

Quoted 19 lines · Kent Overstreet

Quoted 7 lines · Pavel Machek

Quoted 6 lines · Kent Overstreet

Quoted 4 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

I guess it would be nice for me to test it... but what it is against? I tried after v4.10-rc5 and linux-next, but got rejects in both cases.

Sorry, I forgot I had a few other patches in my branch that touch mempool/biosets code.

Quoted 6 lines

Also, after thinking about it more and looking at the relevant code, I'm pretty sure we don't need rescuer threads for block devices that just split bios - i.e. most of them, so I changed my patch to do that.

Tested it by ripping out the current->bio_list checks/workarounds from the bcache code, appears to work:

Feedback on this patch below, but first:

There are deeper issues with the current->bio_list and rescue workqueues than thread counts.

I cannot help but feel like you (and Jens) are repeatedly ignoring the issue that has been raised numerous times, most recently:
https://www.redhat.com/archives/dm-devel/2017-February/msg00059.html

FYI, this test (albeit ugly) can be used to check if the dm-snapshot deadlock is fixed:
https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html

Quoted 7 lines

This situation is the unfortunate pathological worst case for what happens when changes are merged and nobody wants to own fixing the
unforseen implications/regressions. Like everyone else in a position
of Linux maintenance I've tried to stay away from owning the responsibility of a fix -- it isn't working. Ok, I'll stop bitching now.. I do bear responsibility for not digging in myself. We're all busy and this issue is "hard".

Mike, it's not my job to debug DM code for you or sift through your bug reports. I don't read dm-devel, and I don't know why you think I that's my job.

If there's something you think the block layer should be doing differently, post patches - or at the very least, explain what you'd like to be done, with words. Don't get pissy because I'm not sifting through your bug reports.

Quoted 42 lines · Mike Snitzer

Quoted 19 lines · Kent Overstreet

Quoted 7 lines · Pavel Machek

Quoted 6 lines · Kent Overstreet

Quoted 4 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

I guess it would be nice for me to test it... but what it is against? I tried after v4.10-rc5 and linux-next, but got rejects in both cases.

Sorry, I forgot I had a few other patches in my branch that touch mempool/biosets code.

Quoted 6 lines

Also, after thinking about it more and looking at the relevant code, I'm pretty sure we don't need rescuer threads for block devices that just split bios - i.e. most of them, so I changed my patch to do that.

Tested it by ripping out the current->bio_list checks/workarounds from the bcache code, appears to work:

Feedback on this patch below, but first:

There are deeper issues with the current->bio_list and rescue workqueues than thread counts.

I cannot help but feel like you (and Jens) are repeatedly ignoring the issue that has been raised numerous times, most recently:
https://www.redhat.com/archives/dm-devel/2017-February/msg00059.html

FYI, this test (albeit ugly) can be used to check if the dm-snapshot deadlock is fixed:
https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html

Quoted 7 lines

This situation is the unfortunate pathological worst case for what happens when changes are merged and nobody wants to own fixing the
unforseen implications/regressions. Like everyone else in a position
of Linux maintenance I've tried to stay away from owning the responsibility of a fix -- it isn't working. Ok, I'll stop bitching now.. I do bear responsibility for not digging in myself. We're all busy and this issue is "hard".

Mike, it's not my job to debug DM code for you or sift through your bug reports. I don't read dm-devel, and I don't know why you think I that's my job.

If there's something you think the block layer should be doing differently, post patches - or at the very least, explain what you'd like to be done, with words. Don't get pissy because I'm not sifting through your bug reports.

Hell, I'm not getting paid to work on kernel code at all right now, and you trying to rope me into fixing device mapper sure makes me want to work on the block layer more.

DM developers have a long history of working siloed off from the rest of the block layer, building up their own crazy infrastructure (remember the old bio splitting code?) and going to extreme lengths to avoid having to work on or improve the core block layer infrastructure. It's ridiculous.

You know what would be nice? What'd really make my day is if just once I got a thank you or a bit of appreciation from DM developers for the bvec iterators/bio splitting work I did that cleaned up a _lot_ of crazy hairy messes. Or getting rid of merge_bvec_fn, or trying to come up with a better solution for deadlocks due to running under generic_make_request() now.

Quoted 7 lines · Kent Overstreet

Quoted 3 lines · Pavel Machek

Still there on v4.9, 36 threads on nokia n900 cellphone.

So.. what needs to be done there?

But, I just got an idea for how to handle this that might be halfway sane, maybe I'll try and come up with a patch...

Ok, here's such a patch, only lightly tested:

-- >8 --
Subject: [PATCH] block: Make rescuer threads per request_queue, not per bioset

Note: this patch is very lightly tested.

Also, trigger rescuing whenever with bios on current->bio_list, instead of only when we block in bio_alloc_bioset(). This is more correct, and should result in fewer rescuer threads.

XXX: The current->bio_list plugging needs to be unified with the blk_plug mechanism.

TODO: If we change normal request_queue drivers to handle arbitrary size bios by processing requests incrementally, instead of splitting bios, then we can get rid of rescuer threads from those devices.

---
 block/bio.c            | 107 ++++---------------------------------------------
 block/blk-core.c       |  58 ++++++++++++++++++++++++---
 block/blk-sysfs.c      |   2 +
 include/linux/bio.h    |  16 ++++----
 include/linux/blkdev.h |  10 +++++
 include/linux/sched.h  |   2 +-
 kernel/sched/core.c    |   4 ++
 7 files changed, 83 insertions(+), 116 deletions(-)

block/bio.c +7 -100

index f3b5786202..9ad54a9b12 100644
@@ -336,54 +336,6 @@ void bio_chain(struct bio *bio, struct bio *parent)
 }
 EXPORT_SYMBOL(bio_chain);
 
-static void bio_alloc_rescue(struct work_struct *work)
-{
-       struct bio_set *bs = container_of(work, struct bio_set, rescue_work);
-       struct bio *bio;
-
-       while (1) {
-               spin_lock(&bs->rescue_lock);
-               bio = bio_list_pop(&bs->rescue_list);
-               spin_unlock(&bs->rescue_lock);
-
-               if (!bio)
-                       break;
-
-               generic_make_request(bio);
-       }
-}
-
-static void punt_bios_to_rescuer(struct bio_set *bs)
-{
-       struct bio_list punt, nopunt;
-       struct bio *bio;
-
-       /*
-        * 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);
-
-       queue_work(bs->rescue_workqueue, &bs->rescue_work);
-}
-
 /**
  * bio_alloc_bioset - allocate a bio for I/O
  * @gfp_mask:   the GFP_ mask given to the slab allocator

@@ -421,54 +373,27 @@ static void punt_bios_to_rescuer(struct bio_set *bs)
  */
 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;
        struct bio_vec *bvl = NULL;
        struct bio *bio;
        void *p;
 
-       if (!bs) {
-               if (nr_iovecs > UIO_MAXIOV)
-                       return NULL;
+       WARN(current->bio_list &&
+            !current->bio_list->q->rescue_workqueue,
+            "allocating bio beneath generic_make_request() without rescuer");
 
+       if (nr_iovecs > UIO_MAXIOV)
+               return NULL;
+
+       if (!bs) {
                p = kmalloc(sizeof(struct bio) +
                            nr_iovecs * sizeof(struct bio_vec),
                            gfp_mask);
                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_DIRECT_RECLAIM; 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_DIRECT_RECLAIM;
-
                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;
        }

@@ -483,12 +408,6 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
                unsigned long idx = 0;
 
                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;
 

@@ -1938,10 +1857,6 @@ int biovec_init_pool(mempool_t *pool, int pool_entries)
 
 void bioset_exit(struct bio_set *bs)
 {
-       if (bs->rescue_workqueue)
-               destroy_workqueue(bs->rescue_workqueue);
-       bs->rescue_workqueue = NULL;
-
        mempool_exit(&bs->bio_pool);
        mempool_exit(&bs->bvec_pool);
 

@@ -1968,10 +1883,6 @@ static int __bioset_init(struct bio_set *bs,
 
        bs->front_pad = front_pad;
 
-       spin_lock_init(&bs->rescue_lock);
-       bio_list_init(&bs->rescue_list);
-       INIT_WORK(&bs->rescue_work, bio_alloc_rescue);
-
        bs->bio_slab = bio_find_or_create_slab(front_pad + back_pad);
        if (!bs->bio_slab)
                return -ENOMEM;

@@ -1983,10 +1894,6 @@ static int __bioset_init(struct bio_set *bs,
            biovec_init_pool(&bs->bvec_pool, pool_size))
                goto bad;
 
-       bs->rescue_workqueue = alloc_workqueue("bioset", WQ_MEM_RECLAIM, 0);
-       if (!bs->rescue_workqueue)
-               goto bad;
-
        return 0;
 bad:
        bioset_exit(bs);

block/blk-core.c +41 -6

index 7e3cfa9c88..f716164cb3 100644
@@ -48,6 +48,8 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(block_unplug);
 
 DEFINE_IDA(blk_queue_ida);
 
+static void bio_rescue_work(struct work_struct *);
+
 /*
  * For the allocated request tables
  */

@@ -759,11 +761,21 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
                                PERCPU_REF_INIT_ATOMIC, GFP_KERNEL))
                goto fail_bdi;
 
-       if (blkcg_init_queue(q))
+       spin_lock_init(&q->rescue_lock);
+       bio_list_init(&q->rescue_list);
+       INIT_WORK(&q->rescue_work, bio_rescue_work);
+
+       q->rescue_workqueue = alloc_workqueue("bioset", WQ_MEM_RECLAIM, 0);
+       if (!q->rescue_workqueue)
                goto fail_ref;
 
+       if (blkcg_init_queue(q))
+               goto fail_rescue;
+
        return q;
 
+fail_rescue:
+       destroy_workqueue(q->rescue_workqueue);
 fail_ref:
        percpu_ref_exit(&q->q_usage_counter);
 fail_bdi:

@@ -1994,7 +2006,7 @@ generic_make_request_checks(struct bio *bio)
  */
 blk_qc_t generic_make_request(struct bio *bio)
 {
-       struct bio_list bio_list_on_stack;
+       struct bio_plug_list bio_list_on_stack;
        blk_qc_t ret = BLK_QC_T_NONE;
 
        if (!generic_make_request_checks(bio))

@@ -2011,7 +2023,9 @@ blk_qc_t generic_make_request(struct bio *bio)
         * should be added at the tail
         */
        if (current->bio_list) {
-               bio_list_add(current->bio_list, bio);
+               WARN(!current->bio_list->q->rescue_workqueue,
+                    "submitting bio beneath generic_make_request() without rescuer");
+               bio_list_add(&current->bio_list->bios, bio);
                goto out;
        }
 

@@ -2030,19 +2044,23 @@ blk_qc_t generic_make_request(struct bio *bio)
         * bio_list, and call into ->make_request() again.
         */
        BUG_ON(bio->bi_next);
-       bio_list_init(&bio_list_on_stack);
+       bio_list_init(&bio_list_on_stack.bios);
        current->bio_list = &bio_list_on_stack;
+
        do {
                struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 
+               current->bio_list->q = q;
+
                if (likely(blk_queue_enter(q, false) == 0)) {
                        ret = q->make_request_fn(q, bio);
 
                        blk_queue_exit(q);
 
-                       bio = bio_list_pop(current->bio_list);
+                       bio = bio_list_pop(&current->bio_list->bios);
                } else {
-                       struct bio *bio_next = bio_list_pop(current->bio_list);
+                       struct bio *bio_next =
+                               bio_list_pop(&current->bio_list->bios);
 
                        bio_io_error(bio);
                        bio = bio_next;

@@ -2055,6 +2073,34 @@ blk_qc_t generic_make_request(struct bio *bio)
 }
 EXPORT_SYMBOL(generic_make_request);
 
+static void bio_rescue_work(struct work_struct *work)
+{
+       struct request_queue *q =
+               container_of(work, struct request_queue, rescue_work);
+       struct bio *bio;
+
+       while (1) {
+               spin_lock(&q->rescue_lock);
+               bio = bio_list_pop(&q->rescue_list);
+               spin_unlock(&q->rescue_lock);
+
+               if (!bio)
+                       break;
+
+               generic_make_request(bio);
+       }
+}

block/blk-core.c +1 -0

+
 /**
  * submit_bio - submit a bio to the block device layer for I/O
  * @bio: The &struct bio which describes the I/O

block/blk-sysfs.c +2 -0

index 7f27a18cc4..77529238d1 100644
@@ -660,6 +660,8 @@ static void blk_release_queue(struct kobject *kobj)
 
        blk_trace_shutdown(q);
 
+       if (q->rescue_workqueue)
+               destroy_workqueue(q->rescue_workqueue);
        if (q->bio_split)
                bioset_free(q->bio_split);
 

include/linux/bio.h +7 -9

index 1ffe8e37ae..87eeec7eda 100644
@@ -658,6 +658,13 @@ static inline struct bio *bio_list_get(struct bio_list *bl)
        return bio;
 }
 
+struct bio_plug_list {
+       struct bio_list         bios;
+       struct request_queue    *q;
+};
+
+void blk_punt_blocked_bios(struct bio_plug_list *);
+
 /*
  * Increment chain count for the bio. Make sure the CHAIN flag update
  * is visible before the raised count.

@@ -687,15 +694,6 @@ struct bio_set {
        mempool_t bio_integrity_pool;
        mempool_t bvec_integrity_pool;
 #endif
-
-       /*
-        * Deadlock avoidance for stacking block drivers: see comments in
-        * bio_alloc_bioset() for details
-        */
-       spinlock_t              rescue_lock;
-       struct bio_list         rescue_list;
-       struct work_struct      rescue_work;
-       struct workqueue_struct *rescue_workqueue;
 };
 
 struct biovec_slab {

include/linux/blkdev.h +10 -0

index c47c358ba0..f64b886c65 100644
@@ -476,6 +476,16 @@ struct request_queue {
        struct bio_set          *bio_split;
 
        bool                    mq_sysfs_init_done;
+
+       /*
+        * Deadlock avoidance, to deal with the plugging in
+        * generic_make_request() that converts recursion to iteration to avoid
+        * stack overflow:
+        */
+       spinlock_t              rescue_lock;
+       struct bio_list         rescue_list;
+       struct work_struct      rescue_work;
+       struct workqueue_struct *rescue_workqueue;
 };
 
 #define QUEUE_FLAG_QUEUED      1       /* uses generic tag queueing */

include/linux/sched.h +1 -1

index 2865d10a28..59df7a1030 100644
@@ -1762,7 +1762,7 @@ struct task_struct {
        void *journal_info;
 
 /* stacked block device info */
-       struct bio_list *bio_list;
+       struct bio_plug_list *bio_list;
 
 #ifdef CONFIG_BLOCK
 /* stack plugging */

kernel/sched/core.c +4 -1

index bd39d698cb..23b6290ba1 100644
@@ -3439,6 +3439,10 @@ static inline void sched_submit_work(struct task_struct *tsk)
 {
        if (!tsk->state || tsk_is_pi_blocked(tsk))
                return;
+
+       if (tsk->bio_list && !bio_list_empty(&tsk->bio_list->bios))
+               blk_punt_blocked_bios(tsk->bio_list);
+
        /*
         * If we are going to sleep and we have plugged IO queued,
         * make sure to submit it to avoid deadlocks.
--

2.11.0