* [PATCH -next v2 0/2] block, bfq: make bfq_has_work() more accurate
@ 2022-05-13 2:35 Yu Kuai
2022-05-13 2:35 ` [PATCH -next v2 1/2] block, bfq: protect 'bfqd->queued' by 'bfqd->lock' Yu Kuai
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Yu Kuai @ 2022-05-13 2:35 UTC (permalink / raw)
To: jack, paolo.valente, axboe; +Cc: linux-block, linux-kernel, yukuai3, yi.zhang
Changes in v2:
- add reviewed-by tag for patch 1
- use WRITE_ONCE() for updating of 'bfqd->queued' in patch 2
This patchset try to make bfq_has_work() more accurate, patch 1 is a
small problem found by code review.
BTW, I not sure why blk_mq_run_hw_queues() is called with 'bfqd->lock'
held, I think this is not necessary. And bfq_has_work() can be more
accurate by reading 'bfqd->queued' with 'bfqd->lock' held after patch 2.
Previous versions:
v1: https://lore.kernel.org/all/20220510131629.1964415-1-yukuai3@huawei.com/
Yu Kuai (2):
block, bfq: protect 'bfqd->queued' by 'bfqd->lock'
block, bfq: make bfq_has_work() more accurate
block/bfq-iosched.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH -next v2 1/2] block, bfq: protect 'bfqd->queued' by 'bfqd->lock' 2022-05-13 2:35 [PATCH -next v2 0/2] block, bfq: make bfq_has_work() more accurate Yu Kuai @ 2022-05-13 2:35 ` Yu Kuai 2022-05-13 6:13 ` Chaitanya Kulkarni 2022-05-13 2:35 ` [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate Yu Kuai 2022-05-16 17:39 ` [PATCH -next v2 0/2] " Jens Axboe 2 siblings, 1 reply; 10+ messages in thread From: Yu Kuai @ 2022-05-13 2:35 UTC (permalink / raw) To: jack, paolo.valente, axboe; +Cc: linux-block, linux-kernel, yukuai3, yi.zhang If bfq_schedule_dispatch() is called from bfq_idle_slice_timer_body(), then 'bfqd->queued' is read without holding 'bfqd->lock'. This is wrong since it can be wrote concurrently. Fix the problem by holding 'bfqd->lock' in such case. Signed-off-by: Yu Kuai <yukuai3@huawei.com> Reviewed-by: Jan Kara <jack@suse.cz> --- block/bfq-iosched.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 272d48d8f326..61750696e87f 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -456,6 +456,8 @@ static struct bfq_io_cq *bfq_bic_lookup(struct request_queue *q) */ void bfq_schedule_dispatch(struct bfq_data *bfqd) { + lockdep_assert_held(&bfqd->lock); + if (bfqd->queued != 0) { bfq_log(bfqd, "schedule dispatch"); blk_mq_run_hw_queues(bfqd->queue, true); @@ -6898,8 +6900,8 @@ bfq_idle_slice_timer_body(struct bfq_data *bfqd, struct bfq_queue *bfqq) bfq_bfqq_expire(bfqd, bfqq, true, reason); schedule_dispatch: - spin_unlock_irqrestore(&bfqd->lock, flags); bfq_schedule_dispatch(bfqd); + spin_unlock_irqrestore(&bfqd->lock, flags); } /* -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2 1/2] block, bfq: protect 'bfqd->queued' by 'bfqd->lock' 2022-05-13 2:35 ` [PATCH -next v2 1/2] block, bfq: protect 'bfqd->queued' by 'bfqd->lock' Yu Kuai @ 2022-05-13 6:13 ` Chaitanya Kulkarni 0 siblings, 0 replies; 10+ messages in thread From: Chaitanya Kulkarni @ 2022-05-13 6:13 UTC (permalink / raw) To: Yu Kuai, jack, paolo.valente, axboe; +Cc: linux-block, linux-kernel, yi.zhang On 5/12/2022 7:35 PM, Yu Kuai wrote: > If bfq_schedule_dispatch() is called from bfq_idle_slice_timer_body(), > then 'bfqd->queued' is read without holding 'bfqd->lock'. This is > wrong since it can be wrote concurrently. > > Fix the problem by holding 'bfqd->lock' in such case. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > Reviewed-by: Jan Kara <jack@suse.cz> > --- Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate 2022-05-13 2:35 [PATCH -next v2 0/2] block, bfq: make bfq_has_work() more accurate Yu Kuai 2022-05-13 2:35 ` [PATCH -next v2 1/2] block, bfq: protect 'bfqd->queued' by 'bfqd->lock' Yu Kuai @ 2022-05-13 2:35 ` Yu Kuai 2022-05-16 9:56 ` Jan Kara 2022-05-16 17:39 ` [PATCH -next v2 0/2] " Jens Axboe 2 siblings, 1 reply; 10+ messages in thread From: Yu Kuai @ 2022-05-13 2:35 UTC (permalink / raw) To: jack, paolo.valente, axboe; +Cc: linux-block, linux-kernel, yukuai3, yi.zhang bfq_has_work() is using busy_queues currently, which is not accurate because bfq_queue is busy doesn't represent that it has requests. Since bfqd aready has a counter 'queued' to record how many requests are in bfq, use it instead of busy_queues. Noted that bfq_has_work() can be called with 'bfqd->lock' held, thus the lock can't be held in bfq_has_work() to protect 'bfqd->queued'. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- block/bfq-iosched.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 61750696e87f..740dd83853a6 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -2210,7 +2210,11 @@ static void bfq_add_request(struct request *rq) bfq_log_bfqq(bfqd, bfqq, "add_request %d", rq_is_sync(rq)); bfqq->queued[rq_is_sync(rq)]++; - bfqd->queued++; + /* + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it + * may be read without holding the lock in bfq_has_work(). + */ + WRITE_ONCE(bfqd->queued, bfqd->queued + 1); if (RB_EMPTY_ROOT(&bfqq->sort_list) && bfq_bfqq_sync(bfqq)) { bfq_check_waker(bfqd, bfqq, now_ns); @@ -2402,7 +2406,11 @@ static void bfq_remove_request(struct request_queue *q, if (rq->queuelist.prev != &rq->queuelist) list_del_init(&rq->queuelist); bfqq->queued[sync]--; - bfqd->queued--; + /* + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it + * may be read without holding the lock in bfq_has_work(). + */ + WRITE_ONCE(bfqd->queued, bfqd->queued - 1); elv_rb_del(&bfqq->sort_list, rq); elv_rqhash_del(q, rq); @@ -5063,11 +5071,11 @@ static bool bfq_has_work(struct blk_mq_hw_ctx *hctx) struct bfq_data *bfqd = hctx->queue->elevator->elevator_data; /* - * Avoiding lock: a race on bfqd->busy_queues should cause at + * Avoiding lock: a race on bfqd->queued should cause at * most a call to dispatch for nothing */ return !list_empty_careful(&bfqd->dispatch) || - bfq_tot_busy_queues(bfqd) > 0; + READ_ONCE(bfqd->queued); } static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) -- 2.31.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate 2022-05-13 2:35 ` [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate Yu Kuai @ 2022-05-16 9:56 ` Jan Kara 2022-05-17 14:21 ` Paolo Valente 0 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2022-05-16 9:56 UTC (permalink / raw) To: Yu Kuai; +Cc: jack, paolo.valente, axboe, linux-block, linux-kernel, yi.zhang On Fri 13-05-22 10:35:07, Yu Kuai wrote: > bfq_has_work() is using busy_queues currently, which is not accurate > because bfq_queue is busy doesn't represent that it has requests. Since > bfqd aready has a counter 'queued' to record how many requests are in > bfq, use it instead of busy_queues. > > Noted that bfq_has_work() can be called with 'bfqd->lock' held, thus the > lock can't be held in bfq_has_work() to protect 'bfqd->queued'. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > block/bfq-iosched.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 61750696e87f..740dd83853a6 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -2210,7 +2210,11 @@ static void bfq_add_request(struct request *rq) > > bfq_log_bfqq(bfqd, bfqq, "add_request %d", rq_is_sync(rq)); > bfqq->queued[rq_is_sync(rq)]++; > - bfqd->queued++; > + /* > + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it > + * may be read without holding the lock in bfq_has_work(). > + */ > + WRITE_ONCE(bfqd->queued, bfqd->queued + 1); > > if (RB_EMPTY_ROOT(&bfqq->sort_list) && bfq_bfqq_sync(bfqq)) { > bfq_check_waker(bfqd, bfqq, now_ns); > @@ -2402,7 +2406,11 @@ static void bfq_remove_request(struct request_queue *q, > if (rq->queuelist.prev != &rq->queuelist) > list_del_init(&rq->queuelist); > bfqq->queued[sync]--; > - bfqd->queued--; > + /* > + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it > + * may be read without holding the lock in bfq_has_work(). > + */ > + WRITE_ONCE(bfqd->queued, bfqd->queued - 1); > elv_rb_del(&bfqq->sort_list, rq); > > elv_rqhash_del(q, rq); > @@ -5063,11 +5071,11 @@ static bool bfq_has_work(struct blk_mq_hw_ctx *hctx) > struct bfq_data *bfqd = hctx->queue->elevator->elevator_data; > > /* > - * Avoiding lock: a race on bfqd->busy_queues should cause at > + * Avoiding lock: a race on bfqd->queued should cause at > * most a call to dispatch for nothing > */ > return !list_empty_careful(&bfqd->dispatch) || > - bfq_tot_busy_queues(bfqd) > 0; > + READ_ONCE(bfqd->queued); > } > > static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) > -- > 2.31.1 > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate 2022-05-16 9:56 ` Jan Kara @ 2022-05-17 14:21 ` Paolo Valente 2022-05-17 15:06 ` Paolo Valente 0 siblings, 1 reply; 10+ messages in thread From: Paolo Valente @ 2022-05-17 14:21 UTC (permalink / raw) To: Jan Kara; +Cc: Yu Kuai, Jens Axboe, linux-block, linux-kernel, yi.zhang > Il giorno 16 mag 2022, alle ore 11:56, Jan Kara <jack@suse.cz> ha scritto: > > On Fri 13-05-22 10:35:07, Yu Kuai wrote: >> bfq_has_work() is using busy_queues currently, which is not accurate >> because bfq_queue is busy doesn't represent that it has requests. Since >> bfqd aready has a counter 'queued' to record how many requests are in >> bfq, use it instead of busy_queues. >> The number of requests queued is not equal to the number of busy queues (it is >=). If this patch is based on this assumption then unfortunately it is wrong :( Paolo >> Noted that bfq_has_work() can be called with 'bfqd->lock' held, thus the >> lock can't be held in bfq_has_work() to protect 'bfqd->queued'. >> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > Looks good. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > Honza > >> --- >> block/bfq-iosched.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c >> index 61750696e87f..740dd83853a6 100644 >> --- a/block/bfq-iosched.c >> +++ b/block/bfq-iosched.c >> @@ -2210,7 +2210,11 @@ static void bfq_add_request(struct request *rq) >> >> bfq_log_bfqq(bfqd, bfqq, "add_request %d", rq_is_sync(rq)); >> bfqq->queued[rq_is_sync(rq)]++; >> - bfqd->queued++; >> + /* >> + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it >> + * may be read without holding the lock in bfq_has_work(). >> + */ >> + WRITE_ONCE(bfqd->queued, bfqd->queued + 1); >> >> if (RB_EMPTY_ROOT(&bfqq->sort_list) && bfq_bfqq_sync(bfqq)) { >> bfq_check_waker(bfqd, bfqq, now_ns); >> @@ -2402,7 +2406,11 @@ static void bfq_remove_request(struct request_queue *q, >> if (rq->queuelist.prev != &rq->queuelist) >> list_del_init(&rq->queuelist); >> bfqq->queued[sync]--; >> - bfqd->queued--; >> + /* >> + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it >> + * may be read without holding the lock in bfq_has_work(). >> + */ >> + WRITE_ONCE(bfqd->queued, bfqd->queued - 1); >> elv_rb_del(&bfqq->sort_list, rq); >> >> elv_rqhash_del(q, rq); >> @@ -5063,11 +5071,11 @@ static bool bfq_has_work(struct blk_mq_hw_ctx *hctx) >> struct bfq_data *bfqd = hctx->queue->elevator->elevator_data; >> >> /* >> - * Avoiding lock: a race on bfqd->busy_queues should cause at >> + * Avoiding lock: a race on bfqd->queued should cause at >> * most a call to dispatch for nothing >> */ >> return !list_empty_careful(&bfqd->dispatch) || >> - bfq_tot_busy_queues(bfqd) > 0; >> + READ_ONCE(bfqd->queued); >> } >> >> static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) >> -- >> 2.31.1 >> > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate 2022-05-17 14:21 ` Paolo Valente @ 2022-05-17 15:06 ` Paolo Valente 2022-05-18 1:17 ` yukuai (C) 0 siblings, 1 reply; 10+ messages in thread From: Paolo Valente @ 2022-05-17 15:06 UTC (permalink / raw) To: Jan Kara; +Cc: Yu Kuai, Jens Axboe, linux-block, linux-kernel, yi.zhang > Il giorno 17 mag 2022, alle ore 16:21, Paolo Valente <paolo.valente@linaro.org> ha scritto: > > > >> Il giorno 16 mag 2022, alle ore 11:56, Jan Kara <jack@suse.cz> ha scritto: >> >> On Fri 13-05-22 10:35:07, Yu Kuai wrote: >>> bfq_has_work() is using busy_queues currently, which is not accurate >>> because bfq_queue is busy doesn't represent that it has requests. Since >>> bfqd aready has a counter 'queued' to record how many requests are in >>> bfq, use it instead of busy_queues. >>> > > The number of requests queued is not equal to the number of busy > queues (it is >=). No, sorry. It is actually != in general. In particular, if queued == 0 but there are busy queues (although still waiting for I/O to arrive), then responding that there is no work caused blk-mq to stop asking, and hence an I/O freeze. IOW I/O eventually arrives for a busy queue, but blk-mq does not ask for a new request any longer. But maybe things have changed around bfq since then. Paolo > If this patch is based on this assumption then > unfortunately it is wrong :( > > Paolo > >>> Noted that bfq_has_work() can be called with 'bfqd->lock' held, thus the >>> lock can't be held in bfq_has_work() to protect 'bfqd->queued'. >>> >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> >> Looks good. Feel free to add: >> >> Reviewed-by: Jan Kara <jack@suse.cz> >> >> Honza >> >>> --- >>> block/bfq-iosched.c | 16 ++++++++++++---- >>> 1 file changed, 12 insertions(+), 4 deletions(-) >>> >>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c >>> index 61750696e87f..740dd83853a6 100644 >>> --- a/block/bfq-iosched.c >>> +++ b/block/bfq-iosched.c >>> @@ -2210,7 +2210,11 @@ static void bfq_add_request(struct request *rq) >>> >>> bfq_log_bfqq(bfqd, bfqq, "add_request %d", rq_is_sync(rq)); >>> bfqq->queued[rq_is_sync(rq)]++; >>> - bfqd->queued++; >>> + /* >>> + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it >>> + * may be read without holding the lock in bfq_has_work(). >>> + */ >>> + WRITE_ONCE(bfqd->queued, bfqd->queued + 1); >>> >>> if (RB_EMPTY_ROOT(&bfqq->sort_list) && bfq_bfqq_sync(bfqq)) { >>> bfq_check_waker(bfqd, bfqq, now_ns); >>> @@ -2402,7 +2406,11 @@ static void bfq_remove_request(struct request_queue *q, >>> if (rq->queuelist.prev != &rq->queuelist) >>> list_del_init(&rq->queuelist); >>> bfqq->queued[sync]--; >>> - bfqd->queued--; >>> + /* >>> + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it >>> + * may be read without holding the lock in bfq_has_work(). >>> + */ >>> + WRITE_ONCE(bfqd->queued, bfqd->queued - 1); >>> elv_rb_del(&bfqq->sort_list, rq); >>> >>> elv_rqhash_del(q, rq); >>> @@ -5063,11 +5071,11 @@ static bool bfq_has_work(struct blk_mq_hw_ctx *hctx) >>> struct bfq_data *bfqd = hctx->queue->elevator->elevator_data; >>> >>> /* >>> - * Avoiding lock: a race on bfqd->busy_queues should cause at >>> + * Avoiding lock: a race on bfqd->queued should cause at >>> * most a call to dispatch for nothing >>> */ >>> return !list_empty_careful(&bfqd->dispatch) || >>> - bfq_tot_busy_queues(bfqd) > 0; >>> + READ_ONCE(bfqd->queued); >>> } >>> >>> static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) >>> -- >>> 2.31.1 >>> >> -- >> Jan Kara <jack@suse.com> >> SUSE Labs, CR > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate 2022-05-17 15:06 ` Paolo Valente @ 2022-05-18 1:17 ` yukuai (C) 2022-05-18 13:40 ` Paolo VALENTE 0 siblings, 1 reply; 10+ messages in thread From: yukuai (C) @ 2022-05-18 1:17 UTC (permalink / raw) To: Paolo Valente, Jan Kara; +Cc: Jens Axboe, linux-block, linux-kernel, yi.zhang 在 2022/05/17 23:06, Paolo Valente 写道: > > >> Il giorno 17 mag 2022, alle ore 16:21, Paolo Valente <paolo.valente@linaro.org> ha scritto: >> >> >> >>> Il giorno 16 mag 2022, alle ore 11:56, Jan Kara <jack@suse.cz> ha scritto: >>> >>> On Fri 13-05-22 10:35:07, Yu Kuai wrote: >>>> bfq_has_work() is using busy_queues currently, which is not accurate >>>> because bfq_queue is busy doesn't represent that it has requests. Since >>>> bfqd aready has a counter 'queued' to record how many requests are in >>>> bfq, use it instead of busy_queues. >>>> >> >> The number of requests queued is not equal to the number of busy >> queues (it is >=). > > No, sorry. It is actually != in general. Hi, Paolo I'm aware that number of requests queued is not equal to the number of busy queues, and that is the motivation of this patch. > > In particular, if queued == 0 but there are busy queues (although > still waiting for I/O to arrive), then responding that there is no > work caused blk-mq to stop asking, and hence an I/O freeze. IOW I/O > eventually arrives for a busy queue, but blk-mq does not ask for a new > request any longer. But maybe things have changed around bfq since > then. The problem is that if queued == 0 while there are busy queues, is there any point to return true in bfq_has_work() ? IMO, it will only cause unecessary run queue. And if new request arrives, blk_mq_sched_insert_request() will trigger a run queue. Thanks, Kuai > > Paolo > >> If this patch is based on this assumption then >> unfortunately it is wrong :( >> >> Paolo >> >>>> Noted that bfq_has_work() can be called with 'bfqd->lock' held, thus the >>>> lock can't be held in bfq_has_work() to protect 'bfqd->queued'. >>>> >>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>> >>> Looks good. Feel free to add: >>> >>> Reviewed-by: Jan Kara <jack@suse.cz> >>> >>> Honza >>> >>>> --- >>>> block/bfq-iosched.c | 16 ++++++++++++---- >>>> 1 file changed, 12 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c >>>> index 61750696e87f..740dd83853a6 100644 >>>> --- a/block/bfq-iosched.c >>>> +++ b/block/bfq-iosched.c >>>> @@ -2210,7 +2210,11 @@ static void bfq_add_request(struct request *rq) >>>> >>>> bfq_log_bfqq(bfqd, bfqq, "add_request %d", rq_is_sync(rq)); >>>> bfqq->queued[rq_is_sync(rq)]++; >>>> - bfqd->queued++; >>>> + /* >>>> + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it >>>> + * may be read without holding the lock in bfq_has_work(). >>>> + */ >>>> + WRITE_ONCE(bfqd->queued, bfqd->queued + 1); >>>> >>>> if (RB_EMPTY_ROOT(&bfqq->sort_list) && bfq_bfqq_sync(bfqq)) { >>>> bfq_check_waker(bfqd, bfqq, now_ns); >>>> @@ -2402,7 +2406,11 @@ static void bfq_remove_request(struct request_queue *q, >>>> if (rq->queuelist.prev != &rq->queuelist) >>>> list_del_init(&rq->queuelist); >>>> bfqq->queued[sync]--; >>>> - bfqd->queued--; >>>> + /* >>>> + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it >>>> + * may be read without holding the lock in bfq_has_work(). >>>> + */ >>>> + WRITE_ONCE(bfqd->queued, bfqd->queued - 1); >>>> elv_rb_del(&bfqq->sort_list, rq); >>>> >>>> elv_rqhash_del(q, rq); >>>> @@ -5063,11 +5071,11 @@ static bool bfq_has_work(struct blk_mq_hw_ctx *hctx) >>>> struct bfq_data *bfqd = hctx->queue->elevator->elevator_data; >>>> >>>> /* >>>> - * Avoiding lock: a race on bfqd->busy_queues should cause at >>>> + * Avoiding lock: a race on bfqd->queued should cause at >>>> * most a call to dispatch for nothing >>>> */ >>>> return !list_empty_careful(&bfqd->dispatch) || >>>> - bfq_tot_busy_queues(bfqd) > 0; >>>> + READ_ONCE(bfqd->queued); >>>> } >>>> >>>> static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) >>>> -- >>>> 2.31.1 >>>> >>> -- >>> Jan Kara <jack@suse.com> >>> SUSE Labs, CR >> > > . > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate 2022-05-18 1:17 ` yukuai (C) @ 2022-05-18 13:40 ` Paolo VALENTE 0 siblings, 0 replies; 10+ messages in thread From: Paolo VALENTE @ 2022-05-18 13:40 UTC (permalink / raw) To: yukuai (C); +Cc: Jan Kara, Jens Axboe, linux-block, linux-kernel, yi.zhang > Il giorno 18 mag 2022, alle ore 03:17, yukuai (C) <yukuai3@huawei.com> ha scritto: > > 在 2022/05/17 23:06, Paolo Valente 写道: >>> Il giorno 17 mag 2022, alle ore 16:21, Paolo Valente <paolo.valente@linaro.org> ha scritto: >>> >>> >>> >>>> Il giorno 16 mag 2022, alle ore 11:56, Jan Kara <jack@suse.cz> ha scritto: >>>> >>>> On Fri 13-05-22 10:35:07, Yu Kuai wrote: >>>>> bfq_has_work() is using busy_queues currently, which is not accurate >>>>> because bfq_queue is busy doesn't represent that it has requests. Since >>>>> bfqd aready has a counter 'queued' to record how many requests are in >>>>> bfq, use it instead of busy_queues. >>>>> >>> >>> The number of requests queued is not equal to the number of busy >>> queues (it is >=). >> No, sorry. It is actually != in general. > Hi, Paolo > > I'm aware that number of requests queued is not equal to the number of > busy queues, and that is the motivation of this patch. > >> In particular, if queued == 0 but there are busy queues (although >> still waiting for I/O to arrive), then responding that there is no >> work caused blk-mq to stop asking, and hence an I/O freeze. IOW I/O >> eventually arrives for a busy queue, but blk-mq does not ask for a new >> request any longer. But maybe things have changed around bfq since >> then. > > The problem is that if queued == 0 while there are busy queues, is there > any point to return true in bfq_has_work() ? IMO, it will only cause > unecessary run queue. And if new request arrives, > blk_mq_sched_insert_request() will trigger a run queue. Great, if this is the scheme now, then the patch is correct and optimizing. Thanks, Paolo > > Thanks, > Kuai >> Paolo >>> If this patch is based on this assumption then >>> unfortunately it is wrong :( >>> >>> Paolo >>> >>>>> Noted that bfq_has_work() can be called with 'bfqd->lock' held, thus the >>>>> lock can't be held in bfq_has_work() to protect 'bfqd->queued'. >>>>> >>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>>> >>>> Looks good. Feel free to add: >>>> >>>> Reviewed-by: Jan Kara <jack@suse.cz> >>>> >>>> Honza >>>> >>>>> --- >>>>> block/bfq-iosched.c | 16 ++++++++++++---- >>>>> 1 file changed, 12 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c >>>>> index 61750696e87f..740dd83853a6 100644 >>>>> --- a/block/bfq-iosched.c >>>>> +++ b/block/bfq-iosched.c >>>>> @@ -2210,7 +2210,11 @@ static void bfq_add_request(struct request *rq) >>>>> >>>>> bfq_log_bfqq(bfqd, bfqq, "add_request %d", rq_is_sync(rq)); >>>>> bfqq->queued[rq_is_sync(rq)]++; >>>>> - bfqd->queued++; >>>>> + /* >>>>> + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it >>>>> + * may be read without holding the lock in bfq_has_work(). >>>>> + */ >>>>> + WRITE_ONCE(bfqd->queued, bfqd->queued + 1); >>>>> >>>>> if (RB_EMPTY_ROOT(&bfqq->sort_list) && bfq_bfqq_sync(bfqq)) { >>>>> bfq_check_waker(bfqd, bfqq, now_ns); >>>>> @@ -2402,7 +2406,11 @@ static void bfq_remove_request(struct request_queue *q, >>>>> if (rq->queuelist.prev != &rq->queuelist) >>>>> list_del_init(&rq->queuelist); >>>>> bfqq->queued[sync]--; >>>>> - bfqd->queued--; >>>>> + /* >>>>> + * Updating of 'bfqd->queued' is protected by 'bfqd->lock', however, it >>>>> + * may be read without holding the lock in bfq_has_work(). >>>>> + */ >>>>> + WRITE_ONCE(bfqd->queued, bfqd->queued - 1); >>>>> elv_rb_del(&bfqq->sort_list, rq); >>>>> >>>>> elv_rqhash_del(q, rq); >>>>> @@ -5063,11 +5071,11 @@ static bool bfq_has_work(struct blk_mq_hw_ctx *hctx) >>>>> struct bfq_data *bfqd = hctx->queue->elevator->elevator_data; >>>>> >>>>> /* >>>>> - * Avoiding lock: a race on bfqd->busy_queues should cause at >>>>> + * Avoiding lock: a race on bfqd->queued should cause at >>>>> * most a call to dispatch for nothing >>>>> */ >>>>> return !list_empty_careful(&bfqd->dispatch) || >>>>> - bfq_tot_busy_queues(bfqd) > 0; >>>>> + READ_ONCE(bfqd->queued); >>>>> } >>>>> >>>>> static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) >>>>> -- >>>>> 2.31.1 >>>>> >>>> -- >>>> Jan Kara <jack@suse.com> >>>> SUSE Labs, CR >>> >> . ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -next v2 0/2] block, bfq: make bfq_has_work() more accurate 2022-05-13 2:35 [PATCH -next v2 0/2] block, bfq: make bfq_has_work() more accurate Yu Kuai 2022-05-13 2:35 ` [PATCH -next v2 1/2] block, bfq: protect 'bfqd->queued' by 'bfqd->lock' Yu Kuai 2022-05-13 2:35 ` [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate Yu Kuai @ 2022-05-16 17:39 ` Jens Axboe 2 siblings, 0 replies; 10+ messages in thread From: Jens Axboe @ 2022-05-16 17:39 UTC (permalink / raw) To: yukuai3, jack, paolo.valente; +Cc: linux-kernel, linux-block, yi.zhang On Fri, 13 May 2022 10:35:05 +0800, Yu Kuai wrote: > Changes in v2: > - add reviewed-by tag for patch 1 > - use WRITE_ONCE() for updating of 'bfqd->queued' in patch 2 > > This patchset try to make bfq_has_work() more accurate, patch 1 is a > small problem found by code review. > > [...] Applied, thanks! [1/2] block, bfq: protect 'bfqd->queued' by 'bfqd->lock' commit: 181490d5321806e537dc5386db5ea640b826bf78 [2/2] block, bfq: make bfq_has_work() more accurate commit: ddc25c86b466d2359b57bc7798f167baa1735a44 Best regards, -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-05-18 13:41 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-13 2:35 [PATCH -next v2 0/2] block, bfq: make bfq_has_work() more accurate Yu Kuai 2022-05-13 2:35 ` [PATCH -next v2 1/2] block, bfq: protect 'bfqd->queued' by 'bfqd->lock' Yu Kuai 2022-05-13 6:13 ` Chaitanya Kulkarni 2022-05-13 2:35 ` [PATCH -next v2 2/2] block, bfq: make bfq_has_work() more accurate Yu Kuai 2022-05-16 9:56 ` Jan Kara 2022-05-17 14:21 ` Paolo Valente 2022-05-17 15:06 ` Paolo Valente 2022-05-18 1:17 ` yukuai (C) 2022-05-18 13:40 ` Paolo VALENTE 2022-05-16 17:39 ` [PATCH -next v2 0/2] " Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.