Re: [PATCH 4.4 25/26] blk: Ensure users for current->bio_list can see the full list.

[Jinpu Wang <jinpu.wang@profitbricks.com>]

On Thu, Apr 6, 2017 at 8:17 PM, Ben Hutchings
<ben.hutchings@codethink.co.uk> wrote:
> On Thu, 2017-04-06 at 10:38 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: NeilBrown <neilb@suse.com>
>>
>> commit f5fe1b51905df7cfe4fdfd85c5fb7bc5b71a094f upstream.
>>
>> Commit 79bd99596b73 ("blk: improve order of bio handling in generic_make_request()")
>> changed current->bio_list so that it did not contain *all* of the
>> queued bios, but only those submitted by the currently running
>> make_request_fn.
>>
>> There are two places which walk the list and requeue selected bios,
>> and others that check if the list is empty.  These are no longer
>> correct.
>>
>> So redefine current->bio_list to point to an array of two lists, which
>> contain all queued bios, and adjust various code to test or walk both
>> lists.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> Fixes: 79bd99596b73 ("blk: improve order of bio handling in generic_make_request()")
>> Signed-off-by: Jens Axboe <axboe@fb.com>
>> [jwang: backport to 4.4]
>> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>  block/bio.c         |   12 +++++++++---
>>  block/blk-core.c    |   31 +++++++++++++++++++------------
>>  drivers/md/raid1.c  |    3 ++-
>>  drivers/md/raid10.c |    3 ++-
>>  4 files changed, 32 insertions(+), 17 deletions(-)
>
> Why did you drop the changes in drivers/md/dm.c?
>
> Ben.
>
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -373,10 +373,14 @@ static void punt_bios_to_rescuer(struct
>>       bio_list_init(&punt);
>>       bio_list_init(&nopunt);
>>
>> -     while ((bio = bio_list_pop(current->bio_list)))
>> +     while ((bio = bio_list_pop(&current->bio_list[0])))
>>               bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
>> +     current->bio_list[0] = nopunt;
>>
>> -     *current->bio_list = nopunt;
>> +     bio_list_init(&nopunt);
>> +     while ((bio = bio_list_pop(&current->bio_list[1])))
>> +             bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
>> +     current->bio_list[1] = nopunt;
>>
>>       spin_lock(&bs->rescue_lock);
>>       bio_list_merge(&bs->rescue_list, &punt);
>> @@ -464,7 +468,9 @@ struct bio *bio_alloc_bioset(gfp_t gfp_m
>>                * we retry with the original gfp_flags.
>>                */
>>
>> -             if (current->bio_list && !bio_list_empty(current->bio_list))
>> +             if (current->bio_list &&
>> +                 (!bio_list_empty(&current->bio_list[0]) ||
>> +                  !bio_list_empty(&current->bio_list[1])))
>>                       gfp_mask &= ~__GFP_DIRECT_RECLAIM;
>>
>>               p = mempool_alloc(bs->bio_pool, gfp_mask);
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -2021,7 +2021,14 @@ end_io:
>>   */
>>  blk_qc_t generic_make_request(struct bio *bio)
>>  {
>> -     struct bio_list bio_list_on_stack;
>> +     /*
>> +      * bio_list_on_stack[0] contains bios submitted by the current
>> +      * make_request_fn.
>> +      * bio_list_on_stack[1] contains bios that were submitted before
>> +      * the current make_request_fn, but that haven't been processed
>> +      * yet.
>> +      */
>> +     struct bio_list bio_list_on_stack[2];
>>       blk_qc_t ret = BLK_QC_T_NONE;
>>
>>       if (!generic_make_request_checks(bio))
>> @@ -2038,7 +2045,7 @@ blk_qc_t generic_make_request(struct bio
>>        * should be added at the tail
>>        */
>>       if (current->bio_list) {
>> -             bio_list_add(current->bio_list, bio);
>> +             bio_list_add(&current->bio_list[0], bio);
>>               goto out;
>>       }
>>
>> @@ -2057,17 +2064,17 @@ blk_qc_t generic_make_request(struct bio
>>        * bio_list, and call into ->make_request() again.
>>        */
>>       BUG_ON(bio->bi_next);
>> -     bio_list_init(&bio_list_on_stack);
>> -     current->bio_list = &bio_list_on_stack;
>> +     bio_list_init(&bio_list_on_stack[0]);
>> +     current->bio_list = bio_list_on_stack;
>>       do {
>>               struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>>
>>               if (likely(blk_queue_enter(q, __GFP_DIRECT_RECLAIM) == 0)) {
>> -                     struct bio_list lower, same, 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);
>> +                     bio_list_on_stack[1] = bio_list_on_stack[0];
>> +                     bio_list_init(&bio_list_on_stack[0]);
>>
>>                       ret = q->make_request_fn(q, bio);
>>
>> @@ -2077,19 +2084,19 @@ blk_qc_t generic_make_request(struct bio
>>                        */
>>                       bio_list_init(&lower);
>>                       bio_list_init(&same);
>> -                     while ((bio = bio_list_pop(&bio_list_on_stack)) != NULL)
>> +                     while ((bio = bio_list_pop(&bio_list_on_stack[0])) != 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_list_merge(&bio_list_on_stack[0], &lower);
>> +                     bio_list_merge(&bio_list_on_stack[0], &same);
>> +                     bio_list_merge(&bio_list_on_stack[0], &bio_list_on_stack[1]);
>>               } else {
>>                       bio_io_error(bio);
>>               }
>> -             bio = bio_list_pop(current->bio_list);
>> +             bio = bio_list_pop(&bio_list_on_stack[0]);
>>       } while (bio);
>>       current->bio_list = NULL; /* deactivate */
>>
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -877,7 +877,8 @@ static sector_t wait_barrier(struct r1co
>>                                    ((conf->start_next_window <
>>                                      conf->next_resync + RESYNC_SECTORS) &&
>>                                     current->bio_list &&
>> -                                   !bio_list_empty(current->bio_list))),
>> +                                  (!bio_list_empty(&current->bio_list[0]) ||
>> +                                   !bio_list_empty(&current->bio_list[1])))),
>>                                   conf->resync_lock);
>>               conf->nr_waiting--;
>>       }
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -946,7 +946,8 @@ static void wait_barrier(struct r10conf
>>                                   !conf->barrier ||
>>                                   (conf->nr_pending &&
>>                                    current->bio_list &&
>> -                                  !bio_list_empty(current->bio_list)),
>> +                                  (!bio_list_empty(&current->bio_list[0]) ||
>> +                                   !bio_list_empty(&current->bio_list[1]))),
>>                                   conf->resync_lock);
>>               conf->nr_waiting--;
>>       }
>>
>>
>>
>
> --
> Ben Hutchings
> Software Developer, Codethink Ltd.
>
>
Hi Ben,

Because the code snip doesn't exist in 4.4.


-- 
Jack Wang
Linux Kernel Developer