Re: [PATCH V2] blk-mq: avoid to starve tag allocation after allocation process migrates

[Ming Lei <tom.leiming@gmail.com>]

On Thu, May 24, 2018 at 1:40 AM, Omar Sandoval <osandov@osandov.com> wrote:
> On Sat, May 19, 2018 at 03:44:06PM +0800, Ming Lei wrote:
>> When the allocation process is scheduled back and the mapped hw queue is
>> changed, do one extra wake up on orignal queue for compensating wake up
>> miss, so other allocations on the orignal queue won't be starved.
>>
>> This patch fixes one request allocation hang issue, which can be
>> triggered easily in case of very low nr_request.
>>
>> Cc: <stable@vger.kernel.org>
>> Cc: Omar Sandoval <osandov@fb.com>
>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> ---
>>
>> V2:
>>       fix build failure
>>
>>  block/blk-mq-tag.c      | 13 +++++++++++++
>>  include/linux/sbitmap.h |  7 +++++++
>>  lib/sbitmap.c           |  6 ++++++
>>  3 files changed, 26 insertions(+)
>>
>> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
>> index 336dde07b230..77607f89d205 100644
>> --- a/block/blk-mq-tag.c
>> +++ b/block/blk-mq-tag.c
>> @@ -134,6 +134,8 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>>       ws = bt_wait_ptr(bt, data->hctx);
>>       drop_ctx = data->ctx == NULL;
>>       do {
>> +             struct sbitmap_queue *bt_orig;
>> +
>>               /*
>>                * We're out of tags on this hardware queue, kick any
>>                * pending IO submits before going to sleep waiting for
>> @@ -159,6 +161,7 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>>               if (data->ctx)
>>                       blk_mq_put_ctx(data->ctx);
>>
>> +             bt_orig = bt;
>>               io_schedule();
>>
>>               data->ctx = blk_mq_get_ctx(data->q);
>> @@ -170,6 +173,16 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>>                       bt = &tags->bitmap_tags;
>>
>>               finish_wait(&ws->wait, &wait);
>> +
>> +             /*
>> +              * If destination hw queue is changed, wake up original
>> +              * queue one extra time for compensating the wake up
>> +              * miss, so other allocations on original queue won't
>> +              * be starved.
>> +              */
>> +             if (bt != bt_orig)
>> +                     sbitmap_queue_wake_up(bt_orig);
>> +
>>               ws = bt_wait_ptr(bt, data->hctx);
>>       } while (1);
>>
>> diff --git a/include/linux/sbitmap.h b/include/linux/sbitmap.h
>> index 841585f6e5f2..b23f50355281 100644
>> --- a/include/linux/sbitmap.h
>> +++ b/include/linux/sbitmap.h
>> @@ -484,6 +484,13 @@ static inline struct sbq_wait_state *sbq_wait_ptr(struct sbitmap_queue *sbq,
>>  void sbitmap_queue_wake_all(struct sbitmap_queue *sbq);
>>
>>  /**
>> + * sbitmap_wake_up() - Do a regular wake up compensation if the queue
>> + * allocated from is changed after scheduling back.
>> + * @sbq: Bitmap queue to wake up.
>> + */
>> +void sbitmap_queue_wake_up(struct sbitmap_queue *sbq);
>> +
>> +/**
>>   * sbitmap_queue_show() - Dump &struct sbitmap_queue information to a &struct
>>   * seq_file.
>>   * @sbq: Bitmap queue to show.
>> diff --git a/lib/sbitmap.c b/lib/sbitmap.c
>> index e6a9c06ec70c..c6ae4206bcb1 100644
>> --- a/lib/sbitmap.c
>> +++ b/lib/sbitmap.c
>> @@ -466,6 +466,12 @@ static void sbq_wake_up(struct sbitmap_queue *sbq)
>>       }
>>  }
>>
>> +void sbitmap_queue_wake_up(struct sbitmap_queue *sbq)
>> +{
>> +     sbq_wake_up(sbq);
>> +}
>> +EXPORT_SYMBOL_GPL(sbitmap_queue_wake_up);
>
> There's no reason to wrap an internal helper with a function taking the
> same arguments, just rename sbq_wake_up() and export it.

This way may keep sbq_wake_up() inlined to sbitmap_queue_clear(), so
we won't introduce any cost to fast path.

Thanks,
Ming Lei