Re: [PATCH BUGFIX] block, bfq: postpone rq preparation to insert or merge

Oleksandr Natalenko
2018-05-06T07:33:41Z

Hi.

On 04.05.2018 19:17, Paolo Valente wrote:
> When invoked for an I/O request rq, the prepare_request hook of bfq
> increments reference counters in the destination bfq_queue for rq. In
> this respect, after this hook has been invoked, rq may still be
> transformed into a request with no icq attached, i.e., for bfq, a
> request not associated with any bfq_queue. No further hook is invoked
> to signal this tranformation to bfq (in general, to the destination
> elevator for rq). This leads bfq into an inconsistent state, because
> bfq has no chance to correctly lower these counters back. This
> inconsistency may in its turn cause incorrect scheduling and hangs. It
> certainly causes memory leaks, by making it impossible for bfq to free
> the involved bfq_queue.
>
> On the bright side, no transformation can still happen for rq after rq
> has been inserted into bfq, or merged with another, already inserted,
> request. Exploiting this fact, this commit addresses the above issue
> by delaying the preparation of an I/O request to when the request is
> inserted or merged.
>
> This change also gives a performance bonus: a lock-contention point
> gets removed. To prepare a request, bfq needs to hold its scheduler
> lock. After postponing request preparation to insertion or merging, no
> lock needs to be grabbed any longer in the prepare_request hook, while
> the lock already taken to perform insertion or merging is used to
> preparare the request as well.
>
> Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
> ---
> block/bfq-iosched.c | 86
> +++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 57 insertions(+), 29 deletions(-)
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 771ae9730ac6..ea02162df6c7 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -1858,6 +1858,8 @@ static int bfq_request_merge(struct
> request_queue *q, struct request **req,
> return ELEVATOR_NO_MERGE;
> }
>
> +static struct bfq_queue *bfq_init_rq(struct request *rq);
> +
> static void bfq_request_merged(struct request_queue *q, struct request
> *req,
> enum elv_merge type)
> {
> @@ -1866,7 +1868,7 @@ static void bfq_request_merged(struct
> request_queue *q, struct request *req,
> blk_rq_pos(req) <
> blk_rq_pos(container_of(rb_prev(&req->rb_node),
> struct request, rb_node))) {
> - struct bfq_queue *bfqq = RQ_BFQQ(req);
> + struct bfq_queue *bfqq = bfq_init_rq(req);
> struct bfq_data *bfqd = bfqq->bfqd;
> struct request *prev, *next_rq;
>
> @@ -1894,7 +1896,8 @@ static void bfq_request_merged(struct
> request_queue *q, struct request *req,
> static void bfq_requests_merged(struct request_queue *q, struct
> request *rq,
> struct request *next)
> {
> - struct bfq_queue *bfqq = RQ_BFQQ(rq), *next_bfqq = RQ_BFQQ(next);
> + struct bfq_queue *bfqq = bfq_init_rq(rq),
> + *next_bfqq = bfq_init_rq(next);
>
> if (!RB_EMPTY_NODE(&rq->rb_node))
> goto end;
> @@ -4540,14 +4543,12 @@ static inline void
> bfq_update_insert_stats(struct request_queue *q,
> unsigned int cmd_flags) {}
> #endif
>
> -static void bfq_prepare_request(struct request *rq, struct bio *bio);
> -
> static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct
> request *rq,
> bool at_head)
> {
> struct request_queue *q = hctx->queue;
> struct bfq_data *bfqd = q->elevator->elevator_data;
> - struct bfq_queue *bfqq = RQ_BFQQ(rq);
> + struct bfq_queue *bfqq;
> bool idle_timer_disabled = false;
> unsigned int cmd_flags;
>
> @@ -4562,24 +4563,13 @@ static void bfq_insert_request(struct
> blk_mq_hw_ctx *hctx, struct request *rq,
> blk_mq_sched_request_inserted(rq);
>
> spin_lock_irq(&bfqd->lock);
> + bfqq = bfq_init_rq(rq);
> if (at_head || blk_rq_is_passthrough(rq)) {
> if (at_head)
> list_add(&rq->queuelist, &bfqd->dispatch);
> else
> list_add_tail(&rq->queuelist, &bfqd->dispatch);
> - } else {
> - if (WARN_ON_ONCE(!bfqq)) {
> - /*
> - * This should never happen. Most likely rq is
> - * a requeued regular request, being
> - * re-inserted without being first
> - * re-prepared. Do a prepare, to avoid
> - * failure.
> - */
> - bfq_prepare_request(rq, rq->bio);
> - bfqq = RQ_BFQQ(rq);
> - }
> -
> + } else { /* bfqq is assumed to be non null here */
> idle_timer_disabled = __bfq_insert_request(bfqd, rq);
> /*
> * Update bfqq, because, if a queue merge has occurred
> @@ -4922,11 +4912,48 @@ static struct bfq_queue
> *bfq_get_bfqq_handle_split(struct bfq_data *bfqd,
> }
>
> /*
> - * Allocate bfq data structures associated with this request.
> + * Only reset private fields. The actual request preparation will be
> + * performed by bfq_init_rq, when rq is either inserted or merged. See
> + * comments on bfq_init_rq for the reason behind this delayed
> + * preparation.
> */
> static void bfq_prepare_request(struct request *rq, struct bio *bio)
> +{
> + /*
> + * Regardless of whether we have an icq attached, we have to
> + * clear the scheduler pointers, as they might point to
> + * previously allocated bic/bfqq structs.
> + */
> + rq->elv.priv[0] = rq->elv.priv[1] = NULL;
> +}
> +
> +/*
> + * If needed, init rq, allocate bfq data structures associated with
> + * rq, and increment reference counters in the destination bfq_queue
> + * for rq. Return the destination bfq_queue for rq, or NULL is rq is
> + * not associated with any bfq_queue.
> + *
> + * This function is invoked by the functions that perform rq insertion
> + * or merging. One may have expected the above preparation operations
> + * to be performed in bfq_prepare_request, and not delayed to when rq
> + * is inserted or merged. The rationale behind this delayed
> + * preparation is that, after the prepare_request hook is invoked for
> + * rq, rq may still be transformed into a request with no icq, i.e., a
> + * request not associated with any queue. No bfq hook is invoked to
> + * signal this tranformation. As a consequence, should these
> + * preparation operations be performed when the prepare_request hook
> + * is invoked, and should rq be transformed one moment later, bfq
> + * would end up in an inconsistent state, because it would have
> + * incremented some queue counters for an rq destined to
> + * transformation, without any chance to correctly lower these
> + * counters back. In contrast, no transformation can still happen for
> + * rq after rq has been inserted or merged. So, it is safe to execute
> + * these preparation operations when rq is finally inserted or merged.
> + */
> +static struct bfq_queue *bfq_init_rq(struct request *rq)
> {
> struct request_queue *q = rq->q;
> + struct bio *bio = rq->bio;
> struct bfq_data *bfqd = q->elevator->elevator_data;
> struct bfq_io_cq *bic;
> const int is_sync = rq_is_sync(rq);
> @@ -4934,20 +4961,21 @@ static void bfq_prepare_request(struct request
> *rq, struct bio *bio)
> bool new_queue = false;
> bool bfqq_already_existing = false, split = false;
>
> + if (unlikely(!rq->elv.icq))
> + return NULL;
> +
> /*
> - * Even if we don't have an icq attached, we should still clear
> - * the scheduler pointers, as they might point to previously
> - * allocated bic/bfqq structs.
> + * Assuming that elv.priv[1] is set only if everything is set
> + * for this rq. This holds true, because this function is
> + * invoked only for insertion or merging, and, after such
> + * events, a request cannot be manipulated any longer before
> + * being removed from bfq.
> */
> - if (!rq->elv.icq) {
> - rq->elv.priv[0] = rq->elv.priv[1] = NULL;
> - return;
> - }
> + if (rq->elv.priv[1])
> + return rq->elv.priv[1];
>
> bic = icq_to_bic(rq->elv.icq);
>
> - spin_lock_irq(&bfqd->lock);
> -
> bfq_check_ioprio_change(bic, bio);
>
> bfq_bic_update_cgroup(bic, bio);
> @@ -5006,7 +5034,7 @@ static void bfq_prepare_request(struct request
> *rq, struct bio *bio)
> if (unlikely(bfq_bfqq_just_created(bfqq)))
> bfq_handle_burst(bfqd, bfqq);
>
> - spin_unlock_irq(&bfqd->lock);
> + return bfqq;
> }
>
> static void bfq_idle_slice_timer_body(struct bfq_queue *bfqq)

No harm is observed on both test VM with smartctl hammer and my laptop.
So,

Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>

Thanks.

--
Oleksandr Natalenko (post-factum)