| Message ID | 20220513023507.2625717-3-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block, bfq: make bfq_has_work() more accurate | expand |
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 >
> 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
> 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 >
在 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 >> > > . >
> 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 >>> >> .
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)
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(-)