Commit a33801e8b473 ("block, bfq: move debug blkio stats behind
CONFIG_DEBUG_BLK_CGROUP") introduced two batches of confusing ifdefs:
one reported in [1], plus a similar one in another function. This
commit removes both batches, in the way suggested in [1].
[1] https://www.spinics.net/lists/linux-block/msg20043.html
Fixes: a33801e8b473 ("block, bfq: move debug blkio stats behind CONFIG_DEBUG_BLK_CGROUP")
Reported-by: Linus Torvalds <[email protected]>
Tested-by: Luca Miccio <[email protected]>
Signed-off-by: Paolo Valente <[email protected]>
---
block/bfq-iosched.c | 127 +++++++++++++++++++++++++++++-----------------------
1 file changed, 72 insertions(+), 55 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index bcb6d21..3feed64 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -3689,35 +3689,16 @@ static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
return rq;
}
-static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
-{
- struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
- struct request *rq;
#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
- struct bfq_queue *in_serv_queue, *bfqq;
- bool waiting_rq, idle_timer_disabled;
-#endif
-
- spin_lock_irq(&bfqd->lock);
-
-#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
- in_serv_queue = bfqd->in_service_queue;
- waiting_rq = in_serv_queue && bfq_bfqq_wait_request(in_serv_queue);
-
- rq = __bfq_dispatch_request(hctx);
-
- idle_timer_disabled =
- waiting_rq && !bfq_bfqq_wait_request(in_serv_queue);
-
-#else
- rq = __bfq_dispatch_request(hctx);
-#endif
- spin_unlock_irq(&bfqd->lock);
+static void bfq_update_dispatch_stats(struct request_queue *q,
+ struct request *rq,
+ struct bfq_queue *in_serv_queue,
+ bool idle_timer_disabled)
+{
+ struct bfq_queue *bfqq = rq ? RQ_BFQQ(rq) : NULL;
-#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
- bfqq = rq ? RQ_BFQQ(rq) : NULL;
if (!idle_timer_disabled && !bfqq)
- return rq;
+ return;
/*
* rq and bfqq are guaranteed to exist until this function
@@ -3732,7 +3713,7 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
* In addition, the following queue lock guarantees that
* bfqq_group(bfqq) exists as well.
*/
- spin_lock_irq(hctx->queue->queue_lock);
+ spin_lock_irq(q->queue_lock);
if (idle_timer_disabled)
/*
* Since the idle timer has been disabled,
@@ -3751,9 +3732,37 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
bfqg_stats_set_start_empty_time(bfqg);
bfqg_stats_update_io_remove(bfqg, rq->cmd_flags);
}
- spin_unlock_irq(hctx->queue->queue_lock);
+ spin_unlock_irq(q->queue_lock);
+}
+#else
+static inline void bfq_update_dispatch_stats(struct request_queue *q,
+ struct request *rq,
+ struct bfq_queue *in_serv_queue,
+ bool idle_timer_disabled) {}
#endif
+static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
+{
+ struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
+ struct request *rq;
+ struct bfq_queue *in_serv_queue;
+ bool waiting_rq, idle_timer_disabled;
+
+ spin_lock_irq(&bfqd->lock);
+
+ in_serv_queue = bfqd->in_service_queue;
+ waiting_rq = in_serv_queue && bfq_bfqq_wait_request(in_serv_queue);
+
+ rq = __bfq_dispatch_request(hctx);
+
+ idle_timer_disabled =
+ waiting_rq && !bfq_bfqq_wait_request(in_serv_queue);
+
+ spin_unlock_irq(&bfqd->lock);
+
+ bfq_update_dispatch_stats(hctx->queue, rq, in_serv_queue,
+ idle_timer_disabled);
+
return rq;
}
@@ -4276,16 +4285,46 @@ static bool __bfq_insert_request(struct bfq_data *bfqd, struct request *rq)
return idle_timer_disabled;
}
+#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
+static void bfq_update_insert_stats(struct request_queue *q,
+ struct bfq_queue *bfqq,
+ bool idle_timer_disabled,
+ unsigned int cmd_flags)
+{
+ if (!bfqq)
+ return;
+
+ /*
+ * bfqq still exists, because it can disappear only after
+ * either it is merged with another queue, or the process it
+ * is associated with exits. But both actions must be taken by
+ * the same process currently executing this flow of
+ * instructions.
+ *
+ * In addition, the following queue lock guarantees that
+ * bfqq_group(bfqq) exists as well.
+ */
+ spin_lock_irq(q->queue_lock);
+ bfqg_stats_update_io_add(bfqq_group(bfqq), bfqq, cmd_flags);
+ if (idle_timer_disabled)
+ bfqg_stats_update_idle_time(bfqq_group(bfqq));
+ spin_unlock_irq(q->queue_lock);
+}
+#else
+static inline void bfq_update_insert_stats(struct request_queue *q,
+ struct bfq_queue *bfqq,
+ bool idle_timer_disabled,
+ unsigned int cmd_flags) {}
+#endif
+
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;
-#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
struct bfq_queue *bfqq = RQ_BFQQ(rq);
bool idle_timer_disabled = false;
unsigned int cmd_flags;
-#endif
spin_lock_irq(&bfqd->lock);
if (blk_mq_sched_try_insert_merge(q, rq)) {
@@ -4304,7 +4343,6 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
else
list_add_tail(&rq->queuelist, &bfqd->dispatch);
} else {
-#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
idle_timer_disabled = __bfq_insert_request(bfqd, rq);
/*
* Update bfqq, because, if a queue merge has occurred
@@ -4312,9 +4350,6 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
* redirected into a new queue.
*/
bfqq = RQ_BFQQ(rq);
-#else
- __bfq_insert_request(bfqd, rq);
-#endif
if (rq_mergeable(rq)) {
elv_rqhash_add(q, rq);
@@ -4323,35 +4358,17 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
}
}
-#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
/*
* Cache cmd_flags before releasing scheduler lock, because rq
* may disappear afterwards (for example, because of a request
* merge).
*/
cmd_flags = rq->cmd_flags;
-#endif
+
spin_unlock_irq(&bfqd->lock);
-#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
- if (!bfqq)
- return;
- /*
- * bfqq still exists, because it can disappear only after
- * either it is merged with another queue, or the process it
- * is associated with exits. But both actions must be taken by
- * the same process currently executing this flow of
- * instruction.
- *
- * In addition, the following queue lock guarantees that
- * bfqq_group(bfqq) exists as well.
- */
- spin_lock_irq(q->queue_lock);
- bfqg_stats_update_io_add(bfqq_group(bfqq), bfqq, cmd_flags);
- if (idle_timer_disabled)
- bfqg_stats_update_idle_time(bfqq_group(bfqq));
- spin_unlock_irq(q->queue_lock);
-#endif
+ bfq_update_insert_stats(q, bfqq, idle_timer_disabled,
+ cmd_flags);
}
static void bfq_insert_requests(struct blk_mq_hw_ctx *hctx,
--
2.10.0
Hi Jens,
do you think this version could be ok?
Thanks,
Paolo
> Il giorno 04 dic 2017, alle ore 11:42, Paolo Valente <[email protected]> ha scritto:
>
> Commit a33801e8b473 ("block, bfq: move debug blkio stats behind
> CONFIG_DEBUG_BLK_CGROUP") introduced two batches of confusing ifdefs:
> one reported in [1], plus a similar one in another function. This
> commit removes both batches, in the way suggested in [1].
>
> [1] https://www.spinics.net/lists/linux-block/msg20043.html
>
> Fixes: a33801e8b473 ("block, bfq: move debug blkio stats behind CONFIG_DEBUG_BLK_CGROUP")
> Reported-by: Linus Torvalds <[email protected]>
> Tested-by: Luca Miccio <[email protected]>
> Signed-off-by: Paolo Valente <[email protected]>
> ---
> block/bfq-iosched.c | 127 +++++++++++++++++++++++++++++-----------------------
> 1 file changed, 72 insertions(+), 55 deletions(-)
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index bcb6d21..3feed64 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -3689,35 +3689,16 @@ static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
> return rq;
> }
>
> -static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
> -{
> - struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
> - struct request *rq;
> #if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
> - struct bfq_queue *in_serv_queue, *bfqq;
> - bool waiting_rq, idle_timer_disabled;
> -#endif
> -
> - spin_lock_irq(&bfqd->lock);
> -
> -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
> - in_serv_queue = bfqd->in_service_queue;
> - waiting_rq = in_serv_queue && bfq_bfqq_wait_request(in_serv_queue);
> -
> - rq = __bfq_dispatch_request(hctx);
> -
> - idle_timer_disabled =
> - waiting_rq && !bfq_bfqq_wait_request(in_serv_queue);
> -
> -#else
> - rq = __bfq_dispatch_request(hctx);
> -#endif
> - spin_unlock_irq(&bfqd->lock);
> +static void bfq_update_dispatch_stats(struct request_queue *q,
> + struct request *rq,
> + struct bfq_queue *in_serv_queue,
> + bool idle_timer_disabled)
> +{
> + struct bfq_queue *bfqq = rq ? RQ_BFQQ(rq) : NULL;
>
> -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
> - bfqq = rq ? RQ_BFQQ(rq) : NULL;
> if (!idle_timer_disabled && !bfqq)
> - return rq;
> + return;
>
> /*
> * rq and bfqq are guaranteed to exist until this function
> @@ -3732,7 +3713,7 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
> * In addition, the following queue lock guarantees that
> * bfqq_group(bfqq) exists as well.
> */
> - spin_lock_irq(hctx->queue->queue_lock);
> + spin_lock_irq(q->queue_lock);
> if (idle_timer_disabled)
> /*
> * Since the idle timer has been disabled,
> @@ -3751,9 +3732,37 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
> bfqg_stats_set_start_empty_time(bfqg);
> bfqg_stats_update_io_remove(bfqg, rq->cmd_flags);
> }
> - spin_unlock_irq(hctx->queue->queue_lock);
> + spin_unlock_irq(q->queue_lock);
> +}
> +#else
> +static inline void bfq_update_dispatch_stats(struct request_queue *q,
> + struct request *rq,
> + struct bfq_queue *in_serv_queue,
> + bool idle_timer_disabled) {}
> #endif
>
> +static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
> +{
> + struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
> + struct request *rq;
> + struct bfq_queue *in_serv_queue;
> + bool waiting_rq, idle_timer_disabled;
> +
> + spin_lock_irq(&bfqd->lock);
> +
> + in_serv_queue = bfqd->in_service_queue;
> + waiting_rq = in_serv_queue && bfq_bfqq_wait_request(in_serv_queue);
> +
> + rq = __bfq_dispatch_request(hctx);
> +
> + idle_timer_disabled =
> + waiting_rq && !bfq_bfqq_wait_request(in_serv_queue);
> +
> + spin_unlock_irq(&bfqd->lock);
> +
> + bfq_update_dispatch_stats(hctx->queue, rq, in_serv_queue,
> + idle_timer_disabled);
> +
> return rq;
> }
>
> @@ -4276,16 +4285,46 @@ static bool __bfq_insert_request(struct bfq_data *bfqd, struct request *rq)
> return idle_timer_disabled;
> }
>
> +#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
> +static void bfq_update_insert_stats(struct request_queue *q,
> + struct bfq_queue *bfqq,
> + bool idle_timer_disabled,
> + unsigned int cmd_flags)
> +{
> + if (!bfqq)
> + return;
> +
> + /*
> + * bfqq still exists, because it can disappear only after
> + * either it is merged with another queue, or the process it
> + * is associated with exits. But both actions must be taken by
> + * the same process currently executing this flow of
> + * instructions.
> + *
> + * In addition, the following queue lock guarantees that
> + * bfqq_group(bfqq) exists as well.
> + */
> + spin_lock_irq(q->queue_lock);
> + bfqg_stats_update_io_add(bfqq_group(bfqq), bfqq, cmd_flags);
> + if (idle_timer_disabled)
> + bfqg_stats_update_idle_time(bfqq_group(bfqq));
> + spin_unlock_irq(q->queue_lock);
> +}
> +#else
> +static inline void bfq_update_insert_stats(struct request_queue *q,
> + struct bfq_queue *bfqq,
> + bool idle_timer_disabled,
> + unsigned int cmd_flags) {}
> +#endif
> +
> 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;
> -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
> struct bfq_queue *bfqq = RQ_BFQQ(rq);
> bool idle_timer_disabled = false;
> unsigned int cmd_flags;
> -#endif
>
> spin_lock_irq(&bfqd->lock);
> if (blk_mq_sched_try_insert_merge(q, rq)) {
> @@ -4304,7 +4343,6 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
> else
> list_add_tail(&rq->queuelist, &bfqd->dispatch);
> } else {
> -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
> idle_timer_disabled = __bfq_insert_request(bfqd, rq);
> /*
> * Update bfqq, because, if a queue merge has occurred
> @@ -4312,9 +4350,6 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
> * redirected into a new queue.
> */
> bfqq = RQ_BFQQ(rq);
> -#else
> - __bfq_insert_request(bfqd, rq);
> -#endif
>
> if (rq_mergeable(rq)) {
> elv_rqhash_add(q, rq);
> @@ -4323,35 +4358,17 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
> }
> }
>
> -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
> /*
> * Cache cmd_flags before releasing scheduler lock, because rq
> * may disappear afterwards (for example, because of a request
> * merge).
> */
> cmd_flags = rq->cmd_flags;
> -#endif
> +
> spin_unlock_irq(&bfqd->lock);
>
> -#if defined(CONFIG_BFQ_GROUP_IOSCHED) && defined(CONFIG_DEBUG_BLK_CGROUP)
> - if (!bfqq)
> - return;
> - /*
> - * bfqq still exists, because it can disappear only after
> - * either it is merged with another queue, or the process it
> - * is associated with exits. But both actions must be taken by
> - * the same process currently executing this flow of
> - * instruction.
> - *
> - * In addition, the following queue lock guarantees that
> - * bfqq_group(bfqq) exists as well.
> - */
> - spin_lock_irq(q->queue_lock);
> - bfqg_stats_update_io_add(bfqq_group(bfqq), bfqq, cmd_flags);
> - if (idle_timer_disabled)
> - bfqg_stats_update_idle_time(bfqq_group(bfqq));
> - spin_unlock_irq(q->queue_lock);
> -#endif
> + bfq_update_insert_stats(q, bfqq, idle_timer_disabled,
> + cmd_flags);
> }
>
> static void bfq_insert_requests(struct blk_mq_hw_ctx *hctx,
> --
> 2.10.0
>
On 12/13/17 11:10 PM, Paolo Valente wrote:
> Hi Jens,
> do you think this version could be ok?
It's definitely an improvement. I will add it for 4.16.
--
Jens Axboe