Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1591666 > unrolled thread

[PATCH] blk: improve order of bio handling in generic_make_request()

Back to article view | Back to linux.kernel

Contents

  [PATCH] blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-03 06:20 +0100
    Re: [PATCH] blk: improve order of bio handling in  generic_make_request() Jack Wang <jinpu.wang@profitbricks.com> - 2017-03-03 10:40 +0100
      Re: [PATCH] blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-06 05:50 +0100
        Re: [PATCH] blk: improve order of bio handling in  generic_make_request() Jack Wang <jinpu.wang@profitbricks.com> - 2017-03-06 10:50 +0100
          Re: [PATCH] blk: improve order of bio handling in  generic_make_request() Pavel Machek <pavel@ucw.cz> - 2017-03-07 16:50 +0100
            Re: [PATCH] blk: improve order of bio handling in  generic_make_request() Jens Axboe <axboe@kernel.dk> - 2017-03-07 17:30 +0100
            Re: [PATCH] blk: improve order of bio handling in  generic_make_request() Jack Wang <jinpu.wang@profitbricks.com> - 2017-03-07 20:00 +0100
        Re: [PATCH] blk: improve order of bio handling in  generic_make_request() Jens Axboe <axboe@kernel.dk> - 2017-03-07 03:10 +0100
          Re: [PATCH] blk: improve order of bio handling in  generic_make_request() Jack Wang <jinpu.wang@profitbricks.com> - 2017-03-07 10:00 +0100
            Re: blk: improve order of bio handling in generic_make_request() Jens Axboe <axboe@kernel.dk> - 2017-03-07 18:20 +0100
              Re: blk: improve order of bio handling in generic_make_request() Mike Snitzer <snitzer@redhat.com> - 2017-03-07 18:40 +0100
                Re: blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-07 22:20 +0100
                  Re: blk: improve order of bio handling in generic_make_request() Mike Snitzer <snitzer@redhat.com> - 2017-03-08 02:00 +0100
                  Re: blk: improve order of bio handling in generic_make_request() Mikulas Patocka <mpatocka@redhat.com> - 2017-03-08 18:00 +0100
                    Re: blk: improve order of bio handling in generic_make_request() Lars Ellenberg <lars.ellenberg@linbit.com> - 2017-03-08 18:40 +0100
                    Re: blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-09 07:10 +0100
            Re: blk: improve order of bio handling in generic_make_request() Mike Snitzer <snitzer@redhat.com> - 2017-03-07 18:50 +0100
              Re: blk: improve order of bio handling in generic_make_request() Lars Ellenberg <lars.ellenberg@linbit.com> - 2017-03-08 14:00 +0100
          [PATCH v2] blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-07 21:50 +0100
            Re: [PATCH v2] blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-10 05:40 +0100
              [PATCH 5/5] block_dev: make blkdev_dio_pool a non-rescuing bioset NeilBrown <neilb@suse.com> - 2017-03-10 05:40 +0100
              Re: [PATCH v2] blk: improve order of bio handling in  generic_make_request() Jens Axboe <axboe@kernel.dk> - 2017-03-10 05:40 +0100
                Re: [PATCH v2] blk: improve order of bio handling in  generic_make_request() Jens Axboe <axboe@kernel.dk> - 2017-03-10 05:50 +0100
                Re: [PATCH v2] blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-10 06:20 +0100
                  Re: [PATCH v2] blk: improve order of bio handling in generic_make_request() Lars Ellenberg <lars.ellenberg@linbit.com> - 2017-03-10 13:40 +0100
                    Re: [PATCH v2] blk: improve order of bio handling in  generic_make_request() Mike Snitzer <snitzer@redhat.com> - 2017-03-10 15:40 +0100
                      Re: [PATCH v2] blk: improve order of bio handling in  generic_make_request() Mikulas Patocka <mpatocka@redhat.com> - 2017-03-10 16:00 +0100
                        Re: [PATCH v2] blk: improve order of bio handling in  generic_make_request() Jack Wang <jinpu.wang@profitbricks.com> - 2017-03-10 16:20 +0100
                          Re: [PATCH v2] blk: improve order of bio handling in  generic_make_request() Mike Snitzer <snitzer@redhat.com> - 2017-03-10 16:40 +0100
                          Re: [PATCH v2] blk: improve order of bio handling in  generic_make_request() Lars Ellenberg <lars.ellenberg@linbit.com> - 2017-03-10 20:00 +0100
                    Re: [PATCH v2] blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-11 01:50 +0100
              [PATCH 1/5 v3] blk: improve order of bio handling in generic_make_request() NeilBrown <neilb@suse.com> - 2017-03-10 05:40 +0100
              [PATCH 3/5] blk: make the bioset rescue_workqueue optional. NeilBrown <neilb@suse.com> - 2017-03-10 05:40 +0100
              [PATCH 4/5] blk: use non-rescuing bioset for q->bio_split. NeilBrown <neilb@suse.com> - 2017-03-10 05:40 +0100
              [PATCH 2/5] blk: remove bio_set arg from blk_queue_split() NeilBrown <neilb@suse.com> - 2017-03-10 05:40 +0100

Page 1 of 2  [1] 2  Next page →

#1591666 — [PATCH] blk: improve order of bio handling in generic_make_request()

[Multipart message — attachments visible in raw view] — view raw

[ Hi Jens,
  you might have seen assorted email threads recently about
  deadlocks, particular in dm-snap or md/raid1/10.  Also about
  the excess of rescuer threads.
  I think a big part of the problem is my ancient improvement
  to generic_make_request to queue bios and handle them in
  a strict FIFO order.  As described below, that can cause
  problems which individual block devices cannot fix themselves
  without punting to various threads.
  This patch does not fix everything, but provides a basis that
  drives can build on to create dead-lock free solutions without
  excess threads.
  If you accept this, I will look into improving at least md
  and bio_alloc_set() to be less dependant on rescuer threads.
  Thanks,
  NeilBrown
 ]


To avoid recursion on the kernel stack when stacked block devices
are in use, generic_make_request() will, when called recursively,
queue new requests for later handling.  They will be handled when the
make_request_fn for the current bio completes.

If any bios are submitted by a make_request_fn, these will ultimately
handled seqeuntially.  If the handling of one of those generates
further requests, they will be added to the end of the queue.

This strict first-in-first-out behaviour can lead to deadlocks in
various ways, normally because a request might need to wait for a
previous request to the same device to complete.  This can happen when
they share a mempool, and can happen due to interdependencies
particular to the device.  Both md and dm have examples where this happens.

These deadlocks can be erradicated by more selective ordering of bios.
Specifically by handling them in depth-first order.  That is: when the
handling of one bio generates one or more further bios, they are
handled immediately after the parent, before any siblings of the
parent.  That way, when generic_make_request() calls make_request_fn
for some particular device, it we can be certain that all previously
submited request for that device have been completely handled and are
not waiting for anything in the queue of requests maintained in
generic_make_request().

An easy way to achieve this would be to use a last-in-first-out stack
instead of a queue.  However this will change the order of consecutive
bios submitted by a make_request_fn, which could have unexpected consequences.
Instead we take a slightly more complex approach.
A fresh queue is created for each call to a make_request_fn.  After it completes,
any bios for a different device are placed on the front of the main queue, followed
by any bios for the same device, followed by all bios that were already on
the queue before the make_request_fn was called.
This provides the depth-first approach without reordering bios on the same level.

This, by itself, it not enough to remove the deadlocks.  It just makes
it possible for drivers to take the extra step required themselves.

To avoid deadlocks, drivers must never risk waiting for a request
after submitting one to generic_make_request.  This includes never
allocing from a mempool twice in the one call to a make_request_fn.

A common pattern in drivers is to call bio_split() in a loop, handling
the first part and then looping around to possibly split the next part.
Instead, a driver that finds it needs to split a bio should queue
(with generic_make_request) the second part, handle the first part,
and then return.  The new code in generic_make_request will ensure the
requests to underlying bios are processed first, then the second bio
that was split off.  If it splits again, the same process happens.  In
each case one bio will be completely handled before the next one is attempted.

With this is place, it should be possible to disable the
punt_bios_to_recover() recovery thread for many block devices, and
eventually it may be possible to remove it completely.

Tested-by: Jinpu Wang <jinpu.wang@profitbricks.com>
Inspired-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 block/blk-core.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index b9e857f4afe8..ef55f210dd7c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2018,10 +2018,32 @@ blk_qc_t generic_make_request(struct bio *bio)
 		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 
 		if (likely(blk_queue_enter(q, false) == 0)) {
+			struct bio_list hold;
+			struct bio_list lower, same;
+
+			/* Create a fresh bio_list for all subordinate requests */
+			bio_list_init(&hold);
+			bio_list_merge(&hold, &bio_list_on_stack);
+			bio_list_init(&bio_list_on_stack);
 			ret = q->make_request_fn(q, bio);
 
 			blk_queue_exit(q);
 
+			/* sort new bios into those for a lower level
+			 * and those for the same level
+			 */
+			bio_list_init(&lower);
+			bio_list_init(&same);
+			while ((bio = bio_list_pop(&bio_list_on_stack)) != NULL)
+				if (q == bdev_get_queue(bio->bi_bdev))
+					bio_list_add(&same, bio);
+				else
+					bio_list_add(&lower, bio);
+			/* now assemble so we handle the lowest level first */
+			bio_list_merge(&bio_list_on_stack, &lower);
+			bio_list_merge(&bio_list_on_stack, &same);
+			bio_list_merge(&bio_list_on_stack, &hold);
+
 			bio = bio_list_pop(current->bio_list);
 		} else {
 			struct bio *bio_next = bio_list_pop(current->bio_list);
-- 
2.11.0

[toc] | [next] | [standalone]

#1591802 — Re: [PATCH] blk: improve order of bio handling in generic_make_request()

On 03.03.2017 06:14, NeilBrown wrote:
> 
> [ Hi Jens,
>   you might have seen assorted email threads recently about
>   deadlocks, particular in dm-snap or md/raid1/10.  Also about
>   the excess of rescuer threads.
>   I think a big part of the problem is my ancient improvement
>   to generic_make_request to queue bios and handle them in
>   a strict FIFO order.  As described below, that can cause
>   problems which individual block devices cannot fix themselves
>   without punting to various threads.
>   This patch does not fix everything, but provides a basis that
>   drives can build on to create dead-lock free solutions without
>   excess threads.
>   If you accept this, I will look into improving at least md
>   and bio_alloc_set() to be less dependant on rescuer threads.
>   Thanks,
>   NeilBrown
>  ]
> 
> 
> To avoid recursion on the kernel stack when stacked block devices
> are in use, generic_make_request() will, when called recursively,
> queue new requests for later handling.  They will be handled when the
> make_request_fn for the current bio completes.
> 
> If any bios are submitted by a make_request_fn, these will ultimately
> handled seqeuntially.  If the handling of one of those generates
> further requests, they will be added to the end of the queue.
> 
> This strict first-in-first-out behaviour can lead to deadlocks in
> various ways, normally because a request might need to wait for a
> previous request to the same device to complete.  This can happen when
> they share a mempool, and can happen due to interdependencies
> particular to the device.  Both md and dm have examples where this happens.
> 
> These deadlocks can be erradicated by more selective ordering of bios.
> Specifically by handling them in depth-first order.  That is: when the
> handling of one bio generates one or more further bios, they are
> handled immediately after the parent, before any siblings of the
> parent.  That way, when generic_make_request() calls make_request_fn
> for some particular device, it we can be certain that all previously
> submited request for that device have been completely handled and are
> not waiting for anything in the queue of requests maintained in
> generic_make_request().
> 
> An easy way to achieve this would be to use a last-in-first-out stack
> instead of a queue.  However this will change the order of consecutive
> bios submitted by a make_request_fn, which could have unexpected consequences.
> Instead we take a slightly more complex approach.
> A fresh queue is created for each call to a make_request_fn.  After it completes,
> any bios for a different device are placed on the front of the main queue, followed
> by any bios for the same device, followed by all bios that were already on
> the queue before the make_request_fn was called.
> This provides the depth-first approach without reordering bios on the same level.
> 
> This, by itself, it not enough to remove the deadlocks.  It just makes
> it possible for drivers to take the extra step required themselves.
> 
> To avoid deadlocks, drivers must never risk waiting for a request
> after submitting one to generic_make_request.  This includes never
> allocing from a mempool twice in the one call to a make_request_fn.
> 
> A common pattern in drivers is to call bio_split() in a loop, handling
> the first part and then looping around to possibly split the next part.
> Instead, a driver that finds it needs to split a bio should queue
> (with generic_make_request) the second part, handle the first part,
> and then return.  The new code in generic_make_request will ensure the
> requests to underlying bios are processed first, then the second bio
> that was split off.  If it splits again, the same process happens.  In
> each case one bio will be completely handled before the next one is attempted.
> 
> With this is place, it should be possible to disable the
> punt_bios_to_recover() recovery thread for many block devices, and
> eventually it may be possible to remove it completely.
> 
> Tested-by: Jinpu Wang <jinpu.wang@profitbricks.com>
> Inspired-by: Lars Ellenberg <lars.ellenberg@linbit.com>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  block/blk-core.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index b9e857f4afe8..ef55f210dd7c 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2018,10 +2018,32 @@ blk_qc_t generic_make_request(struct bio *bio)
>  		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>  
>  		if (likely(blk_queue_enter(q, false) == 0)) {
> +			struct bio_list hold;
> +			struct bio_list lower, same;
> +
> +			/* Create a fresh bio_list for all subordinate requests */
> +			bio_list_init(&hold);
> +			bio_list_merge(&hold, &bio_list_on_stack);
> +			bio_list_init(&bio_list_on_stack);
>  			ret = q->make_request_fn(q, bio);
>  
>  			blk_queue_exit(q);
>  
> +			/* sort new bios into those for a lower level
> +			 * and those for the same level
> +			 */
> +			bio_list_init(&lower);
> +			bio_list_init(&same);
> +			while ((bio = bio_list_pop(&bio_list_on_stack)) != NULL)
> +				if (q == bdev_get_queue(bio->bi_bdev))
> +					bio_list_add(&same, bio);
> +				else
> +					bio_list_add(&lower, bio);
> +			/* now assemble so we handle the lowest level first */
> +			bio_list_merge(&bio_list_on_stack, &lower);
> +			bio_list_merge(&bio_list_on_stack, &same);
> +			bio_list_merge(&bio_list_on_stack, &hold);
> +
>  			bio = bio_list_pop(current->bio_list);
>  		} else {
>  			struct bio *bio_next = bio_list_pop(current->bio_list);
> 

Thanks Neil for pushing the fix.

We can optimize generic_make_request a little bit:
- assign bio_list struct hold directly instead init and merge
- remove duplicate code

I think better to squash into your fix.
---
 block/blk-core.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3bc7202..b29b7e5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2147,8 +2147,7 @@ blk_qc_t generic_make_request(struct bio *bio)
 			struct bio_list lower, same, hold;
 
 			/* Create a fresh bio_list for all subordinate requests */
-			bio_list_init(&hold);
-			bio_list_merge(&hold, &bio_list_on_stack);
+			hold = bio_list_on_stack;
 			bio_list_init(&bio_list_on_stack);
 
 			ret = q->make_request_fn(q, bio);
@@ -2168,14 +2167,10 @@ blk_qc_t generic_make_request(struct bio *bio)
 			bio_list_merge(&bio_list_on_stack, &lower);
 			bio_list_merge(&bio_list_on_stack, &same);
 			bio_list_merge(&bio_list_on_stack, &hold);
-
-			bio = bio_list_pop(current->bio_list);
 		} else {
-			struct bio *bio_next = bio_list_pop(current->bio_list);
-
 			bio_io_error(bio);
-			bio = bio_next;
 		}
+		bio = bio_list_pop(current->bio_list);
 	} while (bio);
 	current->bio_list = NULL; /* deactivate */
 
-- 

Regards,
Jack 

[toc] | [prev] | [next] | [standalone]

#1592923

[Multipart message — attachments visible in raw view] — view raw

On Fri, Mar 03 2017, Jack Wang wrote:
>
> Thanks Neil for pushing the fix.
>
> We can optimize generic_make_request a little bit:
> - assign bio_list struct hold directly instead init and merge
> - remove duplicate code
>
> I think better to squash into your fix.

Hi Jack,
 I don't object to your changes, but I'd like to see a response from
 Jens first.
 My preference would be to get the original patch in, then other changes
 that build on it, such as this one, can be added.  Until the core
 changes lands, any other work is pointless.

 Of course if Jens wants a this merged before he'll apply it, I'll
 happily do that.

Thanks,
NeilBrown



> ---
>  block/blk-core.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3bc7202..b29b7e5 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2147,8 +2147,7 @@ blk_qc_t generic_make_request(struct bio *bio)
>  			struct bio_list lower, same, hold;
>  
>  			/* Create a fresh bio_list for all subordinate requests */
> -			bio_list_init(&hold);
> -			bio_list_merge(&hold, &bio_list_on_stack);
> +			hold = bio_list_on_stack;
>  			bio_list_init(&bio_list_on_stack);
>  
>  			ret = q->make_request_fn(q, bio);
> @@ -2168,14 +2167,10 @@ blk_qc_t generic_make_request(struct bio *bio)
>  			bio_list_merge(&bio_list_on_stack, &lower);
>  			bio_list_merge(&bio_list_on_stack, &same);
>  			bio_list_merge(&bio_list_on_stack, &hold);
> -
> -			bio = bio_list_pop(current->bio_list);
>  		} else {
> -			struct bio *bio_next = bio_list_pop(current->bio_list);
> -
>  			bio_io_error(bio);
> -			bio = bio_next;
>  		}
> +		bio = bio_list_pop(current->bio_list);
>  	} while (bio);
>  	current->bio_list = NULL; /* deactivate */
>  
> -- 
>
> Regards,
> Jack 

[toc] | [prev] | [next] | [standalone]

#1593127 — Re: [PATCH] blk: improve order of bio handling in generic_make_request()

On 06.03.2017 05:40, NeilBrown wrote:
> On Fri, Mar 03 2017, Jack Wang wrote:
>>
>> Thanks Neil for pushing the fix.
>>
>> We can optimize generic_make_request a little bit:
>> - assign bio_list struct hold directly instead init and merge
>> - remove duplicate code
>>
>> I think better to squash into your fix.
> 
> Hi Jack,
>  I don't object to your changes, but I'd like to see a response from
>  Jens first.
>  My preference would be to get the original patch in, then other changes
>  that build on it, such as this one, can be added.  Until the core
>  changes lands, any other work is pointless.
> 
>  Of course if Jens wants a this merged before he'll apply it, I'll
>  happily do that.
> 
> Thanks,
> NeilBrown

Hi Neil,

Totally agree, let's wait for Jens's decision.

Hi Jens,

Please consider this fix also for stable 4.3+ 

Thanks,
Jack Wang

> 
> 
> 
>> ---
>>  block/blk-core.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 3bc7202..b29b7e5 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2147,8 +2147,7 @@ blk_qc_t generic_make_request(struct bio *bio)
>>  			struct bio_list lower, same, hold;
>>  
>>  			/* Create a fresh bio_list for all subordinate requests */
>> -			bio_list_init(&hold);
>> -			bio_list_merge(&hold, &bio_list_on_stack);
>> +			hold = bio_list_on_stack;
>>  			bio_list_init(&bio_list_on_stack);
>>  
>>  			ret = q->make_request_fn(q, bio);
>> @@ -2168,14 +2167,10 @@ blk_qc_t generic_make_request(struct bio *bio)
>>  			bio_list_merge(&bio_list_on_stack, &lower);
>>  			bio_list_merge(&bio_list_on_stack, &same);
>>  			bio_list_merge(&bio_list_on_stack, &hold);
>> -
>> -			bio = bio_list_pop(current->bio_list);
>>  		} else {
>> -			struct bio *bio_next = bio_list_pop(current->bio_list);
>> -
>>  			bio_io_error(bio);
>> -			bio = bio_next;
>>  		}
>> +		bio = bio_list_pop(current->bio_list);
>>  	} while (bio);
>>  	current->bio_list = NULL; /* deactivate */
>>  
>> -- 
>>
>> Regards,
>> Jack 

[toc] | [prev] | [next] | [standalone]

#1594369 — Re: [PATCH] blk: improve order of bio handling in generic_make_request()

[Multipart message — attachments visible in raw view] — view raw

On Mon 2017-03-06 10:43:59, Jack Wang wrote:
> 
> 
> On 06.03.2017 05:40, NeilBrown wrote:
> > On Fri, Mar 03 2017, Jack Wang wrote:
> >>
> >> Thanks Neil for pushing the fix.
> >>
> >> We can optimize generic_make_request a little bit:
> >> - assign bio_list struct hold directly instead init and merge
> >> - remove duplicate code
> >>
> >> I think better to squash into your fix.
> > 
> > Hi Jack,
> >  I don't object to your changes, but I'd like to see a response from
> >  Jens first.
> >  My preference would be to get the original patch in, then other changes
> >  that build on it, such as this one, can be added.  Until the core
> >  changes lands, any other work is pointless.
> > 
> >  Of course if Jens wants a this merged before he'll apply it, I'll
> >  happily do that.
> > 
> > Thanks,
> > NeilBrown
> 
> Hi Neil,
> 
> Totally agree, let's wait for Jens's decision.
> 
> Hi Jens,
> 
> Please consider this fix also for stable 4.3+ 

Stable? We don't put this into stable, with exception of minimal fixes
for real bugs...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1594407 — Re: [PATCH] blk: improve order of bio handling in generic_make_request()

On 03/07/2017 08:46 AM, Pavel Machek wrote:
> On Mon 2017-03-06 10:43:59, Jack Wang wrote:
>>
>>
>> On 06.03.2017 05:40, NeilBrown wrote:
>>> On Fri, Mar 03 2017, Jack Wang wrote:
>>>>
>>>> Thanks Neil for pushing the fix.
>>>>
>>>> We can optimize generic_make_request a little bit:
>>>> - assign bio_list struct hold directly instead init and merge
>>>> - remove duplicate code
>>>>
>>>> I think better to squash into your fix.
>>>
>>> Hi Jack,
>>>  I don't object to your changes, but I'd like to see a response from
>>>  Jens first.
>>>  My preference would be to get the original patch in, then other changes
>>>  that build on it, such as this one, can be added.  Until the core
>>>  changes lands, any other work is pointless.
>>>
>>>  Of course if Jens wants a this merged before he'll apply it, I'll
>>>  happily do that.
>>>
>>> Thanks,
>>> NeilBrown
>>
>> Hi Neil,
>>
>> Totally agree, let's wait for Jens's decision.
>>
>> Hi Jens,
>>
>> Please consider this fix also for stable 4.3+ 
> 
> Stable? We don't put this into stable, with exception of minimal fixes
> for real bugs...

What are you smoking? How is this not a real bug?

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1594545 — Re: [PATCH] blk: improve order of bio handling in generic_make_request()

On 07.03.2017 16:46, Pavel Machek wrote:
> On Mon 2017-03-06 10:43:59, Jack Wang wrote:
>>
>>
>> On 06.03.2017 05:40, NeilBrown wrote:
>>> On Fri, Mar 03 2017, Jack Wang wrote:
>>>>
>>>> Thanks Neil for pushing the fix.
>>>>
>>>> We can optimize generic_make_request a little bit:
>>>> - assign bio_list struct hold directly instead init and merge
>>>> - remove duplicate code
>>>>
>>>> I think better to squash into your fix.
>>>
>>> Hi Jack,
>>>  I don't object to your changes, but I'd like to see a response from
>>>  Jens first.
>>>  My preference would be to get the original patch in, then other changes
>>>  that build on it, such as this one, can be added.  Until the core
>>>  changes lands, any other work is pointless.
>>>
>>>  Of course if Jens wants a this merged before he'll apply it, I'll
>>>  happily do that.
>>>
>>> Thanks,
>>> NeilBrown
>>
>> Hi Neil,
>>
>> Totally agree, let's wait for Jens's decision.
>>
>> Hi Jens,
>>
>> Please consider this fix also for stable 4.3+ 
> 
> Stable? We don't put this into stable, with exception of minimal fixes
> for real bugs...
> 									Pavel
> 
It indeed fixes deadlock in RAID1 case.
Please follow the link I replied to Jens regarding the test.
It did hit us in our production.

Thanks,
Jack

[toc] | [prev] | [next] | [standalone]

#1593855 — Re: [PATCH] blk: improve order of bio handling in generic_make_request()

On 03/05/2017 09:40 PM, NeilBrown wrote:
> On Fri, Mar 03 2017, Jack Wang wrote:
>>
>> Thanks Neil for pushing the fix.
>>
>> We can optimize generic_make_request a little bit:
>> - assign bio_list struct hold directly instead init and merge
>> - remove duplicate code
>>
>> I think better to squash into your fix.
> 
> Hi Jack,
>  I don't object to your changes, but I'd like to see a response from
>  Jens first.
>  My preference would be to get the original patch in, then other changes
>  that build on it, such as this one, can be added.  Until the core
>  changes lands, any other work is pointless.
> 
>  Of course if Jens wants a this merged before he'll apply it, I'll
>  happily do that.

I like the change, and thanks for tackling this. It's been a pending
issue for way too long. I do think we should squash Jack's patch
into the original, as it does clean up the code nicely.

Do we have a proper test case for this, so we can verify that it
does indeed also work in practice?

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1593998 — Re: [PATCH] blk: improve order of bio handling in generic_make_request()

On 06.03.2017 21:18, Jens Axboe wrote:
> On 03/05/2017 09:40 PM, NeilBrown wrote:
>> On Fri, Mar 03 2017, Jack Wang wrote:
>>>
>>> Thanks Neil for pushing the fix.
>>>
>>> We can optimize generic_make_request a little bit:
>>> - assign bio_list struct hold directly instead init and merge
>>> - remove duplicate code
>>>
>>> I think better to squash into your fix.
>>
>> Hi Jack,
>>  I don't object to your changes, but I'd like to see a response from
>>  Jens first.
>>  My preference would be to get the original patch in, then other changes
>>  that build on it, such as this one, can be added.  Until the core
>>  changes lands, any other work is pointless.
>>
>>  Of course if Jens wants a this merged before he'll apply it, I'll
>>  happily do that.
> 
> I like the change, and thanks for tackling this. It's been a pending
> issue for way too long. I do think we should squash Jack's patch
> into the original, as it does clean up the code nicely.
> 
> Do we have a proper test case for this, so we can verify that it
> does indeed also work in practice?
> 
Hi Jens,

I can trigger deadlock with in RAID1 with test below:

I create one md with one local loop device and one remote scsi
exported by SRP. running fio with mix rw on top of md, force_close
session on storage side. mdx_raid1 is wait on free_array in D state,
and a lot of fio also in D state in wait_barrier.

With the patch from Neil above, I can no longer trigger it anymore.

The discussion was in link below:
http://www.spinics.net/lists/raid/msg54680.html

Thanks,
Jack Wang

[toc] | [prev] | [next] | [standalone]

#1594449 — Re: blk: improve order of bio handling in generic_make_request()

On 03/07/2017 09:52 AM, Mike Snitzer wrote:
> On Tue, Mar 07 2017 at  3:49am -0500,
> Jack Wang <jinpu.wang@profitbricks.com> wrote:
> 
>>
>>
>> On 06.03.2017 21:18, Jens Axboe wrote:
>>> On 03/05/2017 09:40 PM, NeilBrown wrote:
>>>> On Fri, Mar 03 2017, Jack Wang wrote:
>>>>>
>>>>> Thanks Neil for pushing the fix.
>>>>>
>>>>> We can optimize generic_make_request a little bit:
>>>>> - assign bio_list struct hold directly instead init and merge
>>>>> - remove duplicate code
>>>>>
>>>>> I think better to squash into your fix.
>>>>
>>>> Hi Jack,
>>>>  I don't object to your changes, but I'd like to see a response from
>>>>  Jens first.
>>>>  My preference would be to get the original patch in, then other changes
>>>>  that build on it, such as this one, can be added.  Until the core
>>>>  changes lands, any other work is pointless.
>>>>
>>>>  Of course if Jens wants a this merged before he'll apply it, I'll
>>>>  happily do that.
>>>
>>> I like the change, and thanks for tackling this. It's been a pending
>>> issue for way too long. I do think we should squash Jack's patch
>>> into the original, as it does clean up the code nicely.
>>>
>>> Do we have a proper test case for this, so we can verify that it
>>> does indeed also work in practice?
>>>
>> Hi Jens,
>>
>> I can trigger deadlock with in RAID1 with test below:
>>
>> I create one md with one local loop device and one remote scsi
>> exported by SRP. running fio with mix rw on top of md, force_close
>> session on storage side. mdx_raid1 is wait on free_array in D state,
>> and a lot of fio also in D state in wait_barrier.
>>
>> With the patch from Neil above, I can no longer trigger it anymore.
>>
>> The discussion was in link below:
>> http://www.spinics.net/lists/raid/msg54680.html
> 
> In addition to Jack's MD raid test there is a DM snapshot deadlock test,
> albeit unpolished/needy to get running, see:
> https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html

Can you run this patch with that test, reverting your DM workaround?

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1594455 — Re: blk: improve order of bio handling in generic_make_request()

On Tue, Mar 07 2017 at 12:05pm -0500,
Jens Axboe <axboe@kernel.dk> wrote:

> On 03/07/2017 09:52 AM, Mike Snitzer wrote:
> > On Tue, Mar 07 2017 at  3:49am -0500,
> > Jack Wang <jinpu.wang@profitbricks.com> wrote:
> > 
> >>
> >>
> >> On 06.03.2017 21:18, Jens Axboe wrote:
> >>> On 03/05/2017 09:40 PM, NeilBrown wrote:
> >>>> On Fri, Mar 03 2017, Jack Wang wrote:
> >>>>>
> >>>>> Thanks Neil for pushing the fix.
> >>>>>
> >>>>> We can optimize generic_make_request a little bit:
> >>>>> - assign bio_list struct hold directly instead init and merge
> >>>>> - remove duplicate code
> >>>>>
> >>>>> I think better to squash into your fix.
> >>>>
> >>>> Hi Jack,
> >>>>  I don't object to your changes, but I'd like to see a response from
> >>>>  Jens first.
> >>>>  My preference would be to get the original patch in, then other changes
> >>>>  that build on it, such as this one, can be added.  Until the core
> >>>>  changes lands, any other work is pointless.
> >>>>
> >>>>  Of course if Jens wants a this merged before he'll apply it, I'll
> >>>>  happily do that.
> >>>
> >>> I like the change, and thanks for tackling this. It's been a pending
> >>> issue for way too long. I do think we should squash Jack's patch
> >>> into the original, as it does clean up the code nicely.
> >>>
> >>> Do we have a proper test case for this, so we can verify that it
> >>> does indeed also work in practice?
> >>>
> >> Hi Jens,
> >>
> >> I can trigger deadlock with in RAID1 with test below:
> >>
> >> I create one md with one local loop device and one remote scsi
> >> exported by SRP. running fio with mix rw on top of md, force_close
> >> session on storage side. mdx_raid1 is wait on free_array in D state,
> >> and a lot of fio also in D state in wait_barrier.
> >>
> >> With the patch from Neil above, I can no longer trigger it anymore.
> >>
> >> The discussion was in link below:
> >> http://www.spinics.net/lists/raid/msg54680.html
> > 
> > In addition to Jack's MD raid test there is a DM snapshot deadlock test,
> > albeit unpolished/needy to get running, see:
> > https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html
> 
> Can you run this patch with that test, reverting your DM workaround?

Yeap, will do.  Last time Mikulas tried a similar patch it still
deadlocked.  But I'll give it a go (likely tomorrow).

[toc] | [prev] | [next] | [standalone]

#1594654 — Re: blk: improve order of bio handling in generic_make_request()

[Multipart message — attachments visible in raw view] — view raw

On Tue, Mar 07 2017, Mike Snitzer wrote:

> On Tue, Mar 07 2017 at 12:05pm -0500,
> Jens Axboe <axboe@kernel.dk> wrote:
>
>> On 03/07/2017 09:52 AM, Mike Snitzer wrote:
>> > On Tue, Mar 07 2017 at  3:49am -0500,
>> > Jack Wang <jinpu.wang@profitbricks.com> wrote:
>> > 
>> >>
>> >>
>> >> On 06.03.2017 21:18, Jens Axboe wrote:
>> >>> On 03/05/2017 09:40 PM, NeilBrown wrote:
>> >>>> On Fri, Mar 03 2017, Jack Wang wrote:
>> >>>>>
>> >>>>> Thanks Neil for pushing the fix.
>> >>>>>
>> >>>>> We can optimize generic_make_request a little bit:
>> >>>>> - assign bio_list struct hold directly instead init and merge
>> >>>>> - remove duplicate code
>> >>>>>
>> >>>>> I think better to squash into your fix.
>> >>>>
>> >>>> Hi Jack,
>> >>>>  I don't object to your changes, but I'd like to see a response from
>> >>>>  Jens first.
>> >>>>  My preference would be to get the original patch in, then other changes
>> >>>>  that build on it, such as this one, can be added.  Until the core
>> >>>>  changes lands, any other work is pointless.
>> >>>>
>> >>>>  Of course if Jens wants a this merged before he'll apply it, I'll
>> >>>>  happily do that.
>> >>>
>> >>> I like the change, and thanks for tackling this. It's been a pending
>> >>> issue for way too long. I do think we should squash Jack's patch
>> >>> into the original, as it does clean up the code nicely.
>> >>>
>> >>> Do we have a proper test case for this, so we can verify that it
>> >>> does indeed also work in practice?
>> >>>
>> >> Hi Jens,
>> >>
>> >> I can trigger deadlock with in RAID1 with test below:
>> >>
>> >> I create one md with one local loop device and one remote scsi
>> >> exported by SRP. running fio with mix rw on top of md, force_close
>> >> session on storage side. mdx_raid1 is wait on free_array in D state,
>> >> and a lot of fio also in D state in wait_barrier.
>> >>
>> >> With the patch from Neil above, I can no longer trigger it anymore.
>> >>
>> >> The discussion was in link below:
>> >> http://www.spinics.net/lists/raid/msg54680.html
>> > 
>> > In addition to Jack's MD raid test there is a DM snapshot deadlock test,
>> > albeit unpolished/needy to get running, see:
>> > https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html
>> 
>> Can you run this patch with that test, reverting your DM workaround?
>
> Yeap, will do.  Last time Mikulas tried a similar patch it still
> deadlocked.  But I'll give it a go (likely tomorrow).

I don't think this will fix the DM snapshot deadlock by itself.
Rather, it make it possible for some internal changes to DM to fix it.
The DM change might be something vaguely like:

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 3086da5664f3..06ee0960e415 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1216,6 +1216,14 @@ static int __split_and_process_non_flush(struct clone_info *ci)

 	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);

+	if (len < ci->sector_count) {
+		struct bio *split = bio_split(bio, len, GFP_NOIO, fs_bio_set);
+		bio_chain(split, bio);
+		generic_make_request(bio);
+		bio = split;
+		ci->sector_count = len;
+	}
+
 	r = __clone_and_map_data_bio(ci, ti, ci->sector, &len);
 	if (r < 0)
 		return r;

Instead of looping inside DM, this change causes the remainder to be
passed to generic_make_request() and DM only handles or region at a
time.  So there is only one loop, in the top generic_make_request().
That loop will not reliable handle bios in the "right" order.

Thanks,
NeilBrown

[toc] | [prev] | [next] | [standalone]

#1594775 — Re: blk: improve order of bio handling in generic_make_request()

On Tue, Mar 07 2017 at  3:29pm -0500,
NeilBrown <neilb@suse.com> wrote:

> On Tue, Mar 07 2017, Mike Snitzer wrote:
> 
> > On Tue, Mar 07 2017 at 12:05pm -0500,
> > Jens Axboe <axboe@kernel.dk> wrote:
> >
> >> On 03/07/2017 09:52 AM, Mike Snitzer wrote:
> >> > 
> >> > In addition to Jack's MD raid test there is a DM snapshot deadlock test,
> >> > albeit unpolished/needy to get running, see:
> >> > https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html
> >> 
> >> Can you run this patch with that test, reverting your DM workaround?
> >
> > Yeap, will do.  Last time Mikulas tried a similar patch it still
> > deadlocked.  But I'll give it a go (likely tomorrow).
> 
> I don't think this will fix the DM snapshot deadlock by itself.
> Rather, it make it possible for some internal changes to DM to fix it.
> The DM change might be something vaguely like:
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 3086da5664f3..06ee0960e415 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1216,6 +1216,14 @@ static int __split_and_process_non_flush(struct clone_info *ci)
> 
>  	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
> 
> +	if (len < ci->sector_count) {
> +		struct bio *split = bio_split(bio, len, GFP_NOIO, fs_bio_set);
> +		bio_chain(split, bio);
> +		generic_make_request(bio);
> +		bio = split;
> +		ci->sector_count = len;
> +	}
> +
>  	r = __clone_and_map_data_bio(ci, ti, ci->sector, &len);
>  	if (r < 0)
>  		return r;
> 
> Instead of looping inside DM, this change causes the remainder to be
> passed to generic_make_request() and DM only handles or region at a
> time.  So there is only one loop, in the top generic_make_request().
> That loop will not reliable handle bios in the "right" order.

s/not reliable/now reliably/ ? ;)

But thanks for the suggestion Neil.  Will dig in once I get through a
backlog of other DM target code I have queued for 4.12 review.

Mike

[toc] | [prev] | [next] | [standalone]

#1595377 — Re: blk: improve order of bio handling in generic_make_request()

On Wed, 8 Mar 2017, NeilBrown wrote:

> On Tue, Mar 07 2017, Mike Snitzer wrote:
> 
> > On Tue, Mar 07 2017 at 12:05pm -0500,
> > Jens Axboe <axboe@kernel.dk> wrote:
> >
> >> On 03/07/2017 09:52 AM, Mike Snitzer wrote:
> >> > On Tue, Mar 07 2017 at  3:49am -0500,
> >> > Jack Wang <jinpu.wang@profitbricks.com> wrote:
> >> > 
> >> >>
> >> >>
> >> >> On 06.03.2017 21:18, Jens Axboe wrote:
> >> >>> On 03/05/2017 09:40 PM, NeilBrown wrote:
> >> >>>> On Fri, Mar 03 2017, Jack Wang wrote:
> >> >>>>>
> >> >>>>> Thanks Neil for pushing the fix.
> >> >>>>>
> >> >>>>> We can optimize generic_make_request a little bit:
> >> >>>>> - assign bio_list struct hold directly instead init and merge
> >> >>>>> - remove duplicate code
> >> >>>>>
> >> >>>>> I think better to squash into your fix.
> >> >>>>
> >> >>>> Hi Jack,
> >> >>>>  I don't object to your changes, but I'd like to see a response from
> >> >>>>  Jens first.
> >> >>>>  My preference would be to get the original patch in, then other changes
> >> >>>>  that build on it, such as this one, can be added.  Until the core
> >> >>>>  changes lands, any other work is pointless.
> >> >>>>
> >> >>>>  Of course if Jens wants a this merged before he'll apply it, I'll
> >> >>>>  happily do that.
> >> >>>
> >> >>> I like the change, and thanks for tackling this. It's been a pending
> >> >>> issue for way too long. I do think we should squash Jack's patch
> >> >>> into the original, as it does clean up the code nicely.
> >> >>>
> >> >>> Do we have a proper test case for this, so we can verify that it
> >> >>> does indeed also work in practice?
> >> >>>
> >> >> Hi Jens,
> >> >>
> >> >> I can trigger deadlock with in RAID1 with test below:
> >> >>
> >> >> I create one md with one local loop device and one remote scsi
> >> >> exported by SRP. running fio with mix rw on top of md, force_close
> >> >> session on storage side. mdx_raid1 is wait on free_array in D state,
> >> >> and a lot of fio also in D state in wait_barrier.
> >> >>
> >> >> With the patch from Neil above, I can no longer trigger it anymore.
> >> >>
> >> >> The discussion was in link below:
> >> >> http://www.spinics.net/lists/raid/msg54680.html
> >> > 
> >> > In addition to Jack's MD raid test there is a DM snapshot deadlock test,
> >> > albeit unpolished/needy to get running, see:
> >> > https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html
> >> 
> >> Can you run this patch with that test, reverting your DM workaround?
> >
> > Yeap, will do.  Last time Mikulas tried a similar patch it still
> > deadlocked.  But I'll give it a go (likely tomorrow).
> 
> I don't think this will fix the DM snapshot deadlock by itself.
> Rather, it make it possible for some internal changes to DM to fix it.
> The DM change might be something vaguely like:
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 3086da5664f3..06ee0960e415 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1216,6 +1216,14 @@ static int __split_and_process_non_flush(struct clone_info *ci)
> 
>  	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
> 
> +	if (len < ci->sector_count) {
> +		struct bio *split = bio_split(bio, len, GFP_NOIO, fs_bio_set);

fs_bio_set is a shared bio set, so it is prone to deadlocks. For this 
change, we would need two bio sets per dm device, one for the split bio 
and one for the outgoing bio. (this also means having one more kernel 
thread per dm device)

It would be possible to avoid having two bio sets if the incoming bio were 
the same as the outgoing bio (allocate a small structure, move bi_end_io 
and bi_private into it, replace bi_end_io and bi_private with pointers to 
device mapper and send the bio to the target driver), but it would need 
much more changes - basically rewrite the whole bio handling code in dm.c 
and in the targets.

Mikulas

> +		bio_chain(split, bio);
> +		generic_make_request(bio);
> +		bio = split;
> +		ci->sector_count = len;
> +	}
> +
>  	r = __clone_and_map_data_bio(ci, ti, ci->sector, &len);
>  	if (r < 0)
>  		return r;
> 
> Instead of looping inside DM, this change causes the remainder to be
> passed to generic_make_request() and DM only handles or region at a
> time.  So there is only one loop, in the top generic_make_request().
> That loop will not reliable handle bios in the "right" order.
> 
> Thanks,
> NeilBrown
> 

[toc] | [prev] | [next] | [standalone]

#1595409 — Re: blk: improve order of bio handling in generic_make_request()

On 8 March 2017 at 17:40, Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> On Wed, 8 Mar 2017, NeilBrown wrote:
> > I don't think this will fix the DM snapshot deadlock by itself.
> > Rather, it make it possible for some internal changes to DM to fix it.
> > The DM change might be something vaguely like:
> >
> > diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> > index 3086da5664f3..06ee0960e415 100644
> > --- a/drivers/md/dm.c
> > +++ b/drivers/md/dm.c
> > @@ -1216,6 +1216,14 @@ static int __split_and_process_non_flush(struct clone_info *ci)
> >
> >       len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
> >
> > +     if (len < ci->sector_count) {
> > +             struct bio *split = bio_split(bio, len, GFP_NOIO, fs_bio_set);
>
> fs_bio_set is a shared bio set, so it is prone to deadlocks. For this
> change, we would need two bio sets per dm device, one for the split bio
> and one for the outgoing bio. (this also means having one more kernel
> thread per dm device)
>
> It would be possible to avoid having two bio sets if the incoming bio were
> the same as the outgoing bio (allocate a small structure, move bi_end_io
> and bi_private into it, replace bi_end_io and bi_private with pointers to
> device mapper and send the bio to the target driver), but it would need
> much more changes - basically rewrite the whole bio handling code in dm.c
> and in the targets.
>
> Mikulas

"back then" (see previously posted link into ML archive)
I suggested this:

...

A bit of conflict here may be that DM has all its own
split and clone and queue magic, and wants to process
"all of the bio" before returning back to generic_make_request().

To change that, __split_and_process_bio() and all its helpers
would need to learn to "push back" (pieces of) the bio they are
currently working on, and not push back via "DM_ENDIO_REQUEUE",
but by bio_list_add_head(&current->bio_lists->queue, piece_to_be_done_later).

Then, after they processed each piece,
*return* all the way up to the top-level generic_make_request(),
where the recursion-to-iteration logic would then
make sure that all deeper level bios, submitted via
recursive calls to generic_make_request() will be processed, before the
next, pushed back, piece of the "original incoming" bio.

And *not* do their own iteration over all pieces first.

Probably not as easy as dropping the while loop,
using bio_advance, and pushing that "advanced" bio back to
current->...queue?

static void __split_and_process_bio(struct mapped_device *md,
				    struct dm_table *map, struct bio *bio)
...
	ci.bio = bio;
	ci.sector_count = bio_sectors(bio);
	while (ci.sector_count && !error)
		error = __split_and_process_non_flush(&ci);
...
	error = __split_and_process_non_flush(&ci);
	if (ci.sector_count)
		bio_advance()
		bio_list_add_head(&current->bio_lists->queue, )
...

Something like that, maybe?


Needs to be adapted to this new and improved recursion-to-iteration
logic, obviously.  Would that be doable, or does device-mapper for some
reason really *need* its own iteration loop (which, because it is called
from generic_make_request(), won't be able to ever submit anything to
any device, ever, so needs all these helper threads just in case).

    Lars

[toc] | [prev] | [next] | [standalone]

#1595694 — Re: blk: improve order of bio handling in generic_make_request()

[Multipart message — attachments visible in raw view] — view raw

On Wed, Mar 08 2017, Mikulas Patocka wrote:

> On Wed, 8 Mar 2017, NeilBrown wrote:
>> 
>> I don't think this will fix the DM snapshot deadlock by itself.
>> Rather, it make it possible for some internal changes to DM to fix it.
>> The DM change might be something vaguely like:
>> 
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index 3086da5664f3..06ee0960e415 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -1216,6 +1216,14 @@ static int __split_and_process_non_flush(struct clone_info *ci)
>> 
>>  	len = min_t(sector_t, max_io_len(ci->sector, ti), ci->sector_count);
>> 
>> +	if (len < ci->sector_count) {
>> +		struct bio *split = bio_split(bio, len, GFP_NOIO, fs_bio_set);
>
> fs_bio_set is a shared bio set, so it is prone to deadlocks. For this 
> change, we would need two bio sets per dm device, one for the split bio 
> and one for the outgoing bio. (this also means having one more kernel 
> thread per dm device)

Yes, two local bio_sets would be best.
But we don't really need those extra kernel threads.  I'll start working
on patches to make them optional, and then to start removing them.

Thanks,
NeilBrown

[toc] | [prev] | [next] | [standalone]

#1594477 — Re: blk: improve order of bio handling in generic_make_request()

On Tue, Mar 07 2017 at  3:49am -0500,
Jack Wang <jinpu.wang@profitbricks.com> wrote:

> 
> 
> On 06.03.2017 21:18, Jens Axboe wrote:
> > On 03/05/2017 09:40 PM, NeilBrown wrote:
> >> On Fri, Mar 03 2017, Jack Wang wrote:
> >>>
> >>> Thanks Neil for pushing the fix.
> >>>
> >>> We can optimize generic_make_request a little bit:
> >>> - assign bio_list struct hold directly instead init and merge
> >>> - remove duplicate code
> >>>
> >>> I think better to squash into your fix.
> >>
> >> Hi Jack,
> >>  I don't object to your changes, but I'd like to see a response from
> >>  Jens first.
> >>  My preference would be to get the original patch in, then other changes
> >>  that build on it, such as this one, can be added.  Until the core
> >>  changes lands, any other work is pointless.
> >>
> >>  Of course if Jens wants a this merged before he'll apply it, I'll
> >>  happily do that.
> > 
> > I like the change, and thanks for tackling this. It's been a pending
> > issue for way too long. I do think we should squash Jack's patch
> > into the original, as it does clean up the code nicely.
> > 
> > Do we have a proper test case for this, so we can verify that it
> > does indeed also work in practice?
> > 
> Hi Jens,
> 
> I can trigger deadlock with in RAID1 with test below:
> 
> I create one md with one local loop device and one remote scsi
> exported by SRP. running fio with mix rw on top of md, force_close
> session on storage side. mdx_raid1 is wait on free_array in D state,
> and a lot of fio also in D state in wait_barrier.
> 
> With the patch from Neil above, I can no longer trigger it anymore.
> 
> The discussion was in link below:
> http://www.spinics.net/lists/raid/msg54680.html

In addition to Jack's MD raid test there is a DM snapshot deadlock test,
albeit unpolished/needy to get running, see:
https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html

But to actually test block core's ability to handle this, upstream
commit d67a5f4b5947aba4bfe9a80a2b86079c215ca755 ("dm: flush queued bios
when process blocks to avoid deadlock") would need to be reverted.

Also, I know Lars had a drbd deadlock too.  Not sure if Jack's MD test
is sufficient to coverage for drbd.  Lars?

[toc] | [prev] | [next] | [standalone]

#1595138 — Re: blk: improve order of bio handling in generic_make_request()

On 7 March 2017 at 17:52, Mike Snitzer <snitzer@redhat.com> wrote:

> > On 06.03.2017 21:18, Jens Axboe wrote:
> > > I like the change, and thanks for tackling this. It's been a pending
> > > issue for way too long. I do think we should squash Jack's patch
> > > into the original, as it does clean up the code nicely.
> > >
> > > Do we have a proper test case for this, so we can verify that it
> > > does indeed also work in practice?
> > >
> > Hi Jens,
> >
> > I can trigger deadlock with in RAID1 with test below:
> >
> > I create one md with one local loop device and one remote scsi
> > exported by SRP. running fio with mix rw on top of md, force_close
> > session on storage side. mdx_raid1 is wait on free_array in D state,
> > and a lot of fio also in D state in wait_barrier.
> >
> > With the patch from Neil above, I can no longer trigger it anymore.
> >
> > The discussion was in link below:
> > http://www.spinics.net/lists/raid/msg54680.html
>
> In addition to Jack's MD raid test there is a DM snapshot deadlock test,
> albeit unpolished/needy to get running, see:
> https://www.redhat.com/archives/dm-devel/2017-January/msg00064.html
>
> But to actually test block core's ability to handle this, upstream
> commit d67a5f4b5947aba4bfe9a80a2b86079c215ca755 ("dm: flush queued bios
> when process blocks to avoid deadlock") would need to be reverted.
>
> Also, I know Lars had a drbd deadlock too.  Not sure if Jack's MD test
> is sufficient to coverage for drbd.  Lars?
>

As this is just a slightly different implementation, trading some bytes of
stack
for more local, self-contained, "obvious" code changes (good job!),
but follows the same basic idea as my original RFC [*]  (see the
"inspired-by" tag)
I have no doubt it fixes the issues we are able to provoke with DRBD.
[*] https://lkml.org/lkml/2016/7/19/263
(where I also already suggest to fix the device-mapper issues
by losing the in-device-mapper loop,
relying on the loop in generic_make_request())

Cheers,
    Lars

[toc] | [prev] | [next] | [standalone]

#1594632 — [PATCH v2] blk: improve order of bio handling in generic_make_request()

[Multipart message — attachments visible in raw view] — view raw

To avoid recursion on the kernel stack when stacked block devices
are in use, generic_make_request() will, when called recursively,
queue new requests for later handling.  They will be handled when the
make_request_fn for the current bio completes.

If any bios are submitted by a make_request_fn, these will ultimately
be handled seqeuntially.  If the handling of one of those generates
further requests, they will be added to the end of the queue.

This strict first-in-first-out behaviour can lead to deadlocks in
various ways, normally because a request might need to wait for a
previous request to the same device to complete.  This can happen when
they share a mempool, and can happen due to interdependencies
particular to the device.  Both md and dm have examples where this happens.

These deadlocks can be erradicated by more selective ordering of bios.
Specifically by handling them in depth-first order.  That is: when the
handling of one bio generates one or more further bios, they are
handled immediately after the parent, before any siblings of the
parent.  That way, when generic_make_request() calls make_request_fn
for some particular device, we can be certain that all previously
submited requests for that device have been completely handled and are
not waiting for anything in the queue of requests maintained in
generic_make_request().

An easy way to achieve this would be to use a last-in-first-out stack
instead of a queue.  However this will change the order of consecutive
bios submitted by a make_request_fn, which could have unexpected consequences.
Instead we take a slightly more complex approach.
A fresh queue is created for each call to a make_request_fn.  After it completes,
any bios for a different device are placed on the front of the main queue, followed
by any bios for the same device, followed by all bios that were already on
the queue before the make_request_fn was called.
This provides the depth-first approach without reordering bios on the same level.

This, by itself, it not enough to remove all deadlocks.  It just makes
it possible for drivers to take the extra step required themselves.

To avoid deadlocks, drivers must never risk waiting for a request
after submitting one to generic_make_request.  This includes never
allocing from a mempool twice in the one call to a make_request_fn.

A common pattern in drivers is to call bio_split() in a loop, handling
the first part and then looping around to possibly split the next part.
Instead, a driver that finds it needs to split a bio should queue
(with generic_make_request) the second part, handle the first part,
and then return.  The new code in generic_make_request will ensure the
requests to underlying bios are processed first, then the second bio
that was split off.  If it splits again, the same process happens.  In
each case one bio will be completely handled before the next one is attempted.

With this is place, it should be possible to disable the
punt_bios_to_recover() recovery thread for many block devices, and
eventually it may be possible to remove it completely.

Ref: http://www.spinics.net/lists/raid/msg54680.html
Tested-by: Jinpu Wang <jinpu.wang@profitbricks.com>
Inspired-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 block/blk-core.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Changes since v1:
 - merge code improvements from Jack Wang
 - more edits to changelog comment
 - add Ref: link.
 - Add some lists to Cc, that should have been there the first time.
 


diff --git a/block/blk-core.c b/block/blk-core.c
index b9e857f4afe8..9520e82aa78c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2018,17 +2018,34 @@ blk_qc_t generic_make_request(struct bio *bio)
 		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 
 		if (likely(blk_queue_enter(q, false) == 0)) {
+			struct bio_list hold;
+			struct bio_list lower, same;
+
+			/* Create a fresh bio_list for all subordinate requests */
+			hold = bio_list_on_stack;
+			bio_list_init(&bio_list_on_stack);
 			ret = q->make_request_fn(q, bio);
 
 			blk_queue_exit(q);
 
-			bio = bio_list_pop(current->bio_list);
+			/* sort new bios into those for a lower level
+			 * and those for the same level
+			 */
+			bio_list_init(&lower);
+			bio_list_init(&same);
+			while ((bio = bio_list_pop(&bio_list_on_stack)) != NULL)
+				if (q == bdev_get_queue(bio->bi_bdev))
+					bio_list_add(&same, bio);
+				else
+					bio_list_add(&lower, bio);
+			/* now assemble so we handle the lowest level first */
+			bio_list_merge(&bio_list_on_stack, &lower);
+			bio_list_merge(&bio_list_on_stack, &same);
+			bio_list_merge(&bio_list_on_stack, &hold);
 		} else {
-			struct bio *bio_next = bio_list_pop(current->bio_list);
-
 			bio_io_error(bio);
-			bio = bio_next;
 		}
+		bio = bio_list_pop(current->bio_list);
 	} while (bio);
 	current->bio_list = NULL; /* deactivate */
 
-- 
2.12.0

[toc] | [prev] | [next] | [standalone]

#1596609 — Re: [PATCH v2] blk: improve order of bio handling in generic_make_request()

[Multipart message — attachments visible in raw view] — view raw

I started looking further at the improvements we can make once
generic_make_request is fixed, and realised that I had missed an
important detail in this patch.
Several places test current->bio_list, and two actually edit the list.
With this change, that cannot see the whole lists, so it could cause a
regression.

So I've revised the patch to make sure that the entire list of queued
bios remains visible, and change the relevant code to look at both
the new list and the old list.

Following that there are some patches which make the rescuer thread
optional, and then starts removing it from some easy-to-fix places.

The series summary is below.

NeilBrown


NeilBrown (5):
      blk: improve order of bio handling in generic_make_request()
      blk: remove bio_set arg from blk_queue_split()
      blk: make the bioset rescue_workqueue optional.
      blk: use non-rescuing bioset for q->bio_split.
      block_dev: make blkdev_dio_pool a non-rescuing bioset


 block/bio.c                         |   39 +++++++++++++++++++++++++++------
 block/blk-core.c                    |   42 ++++++++++++++++++++++++++++-------
 block/blk-merge.c                   |    7 +++---
 block/blk-mq.c                      |    4 ++-
 drivers/block/drbd/drbd_main.c      |    2 +-
 drivers/block/drbd/drbd_req.c       |    2 +-
 drivers/block/pktcdvd.c             |    2 +-
 drivers/block/ps3vram.c             |    2 +-
 drivers/block/rsxx/dev.c            |    2 +-
 drivers/block/umem.c                |    2 +-
 drivers/block/zram/zram_drv.c       |    2 +-
 drivers/lightnvm/rrpc.c             |    2 +-
 drivers/md/bcache/super.c           |    4 ++-
 drivers/md/dm-crypt.c               |    2 +-
 drivers/md/dm-io.c                  |    2 +-
 drivers/md/dm.c                     |   32 +++++++++++++++------------
 drivers/md/md.c                     |    4 ++-
 drivers/md/raid10.c                 |    3 ++-
 drivers/md/raid5-cache.c            |    2 +-
 drivers/s390/block/dcssblk.c        |    2 +-
 drivers/s390/block/xpram.c          |    2 +-
 drivers/target/target_core_iblock.c |    2 +-
 fs/btrfs/extent_io.c                |    4 ++-
 fs/xfs/xfs_super.c                  |    2 +-
 include/linux/bio.h                 |    2 ++
 include/linux/blkdev.h              |    3 +--
 26 files changed, 114 insertions(+), 60 deletions(-)

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web