Re: [PATCH -next v7 2/3] block, bfq: refactor the counting of 'num_groups_with_pending_reqs'

Il giorno 31 mag 2022, alle ore 12:01, Jan Kara [off-list ref] ha scritto:

On Tue 31-05-22 17:33:25, Yu Kuai wrote:

quoted

在 2022/05/31 17:19, Paolo Valente 写道:

quoted

quoted

Il giorno 31 mag 2022, alle ore 11:06, Yu Kuai [off-list ref] ha scritto:

在 2022/05/31 16:36, Paolo VALENTE 写道:

quoted

quoted

Il giorno 30 mag 2022, alle ore 10:40, Yu Kuai [off-list ref] ha scritto:

在 2022/05/30 16:34, Yu Kuai 写道:

quoted

在 2022/05/30 16:10, Paolo Valente 写道:

quoted

quoted

Il giorno 28 mag 2022, alle ore 11:50, Yu Kuai [off-list ref] ha scritto:

Currently, bfq can't handle sync io concurrently as long as they
are not issued from root group. This is because
'bfqd->num_groups_with_pending_reqs > 0' is always true in
bfq_asymmetric_scenario().

The way that bfqg is counted into 'num_groups_with_pending_reqs':

Before this patch:
1) root group will never be counted.
2) Count if bfqg or it's child bfqgs have pending requests.
3) Don't count if bfqg and it's child bfqgs complete all the requests.

After this patch:
1) root group is counted.
2) Count if bfqg have at least one bfqq that is marked busy.
3) Don't count if bfqg doesn't have any busy bfqqs.

Unfortunately, I see a last problem here. I see a double change:
(1) a bfqg is now counted only as a function of the state of its child
     queues, and not of also its child bfqgs
(2) the state considered for counting a bfqg moves from having pending
     requests to having busy queues

I'm ok with with (1), which is a good catch (you are lady explained
the idea to me some time ago IIRC).

Yet I fear that (2) is not ok.  A bfqq can become non busy even if it
still has in-flight I/O, i.e.  I/O being served in the drive.  The
weight of such a bfqq must still be considered in the weights_tree,
and the group containing such a queue must still be counted when
checking whether the scenario is asymmetric.  Otherwise service
guarantees are broken.  The reason is that, if a scenario is deemed as
symmetric because in-flight I/O is not taken into account, then idling
will not be performed to protect some bfqq, and in-flight I/O may
steal bandwidth to that bfqq in an uncontrolled way.

Hi, Paolo
Thanks for your explanation.
My orginal thoughts was using weights_tree insertion/removal, however,
Jan convinced me that using bfq_add/del_bfqq_busy() is ok.
From what I see, when bfqq dispatch the last request,
bfq_del_bfqq_busy() will not be called from __bfq_bfqq_expire() if
idling is needed, and it will delayed to when such bfqq get scheduled as
in-service queue again. Which means the weight of such bfqq should still
be considered in the weights_tree.
I also run some tests on null_blk with "irqmode=2
completion_nsec=100000000(100ms) hw_queue_depth=1", and tests show
that service guarantees are still preserved on slow device.
Do you this is strong enough to cover your concern?

Unfortunately it is not.  Your very argument is what made be believe
that considering busy queues was enough, in the first place.  But, as
I found out, the problem is caused by the queues that do not enjoy
idling.  With your patch (as well as in my initial version) they are
not counted when they remain without requests queued.  And this makes
asymmetric scenarios be considered erroneously as symmetric.  The
consequence is that idling gets switched off when it had to be kept
on, and control on bandwidth is lost for the victim in-service queues.

Hi，Paolo

Thanks for your explanation, are you thinking that if bfqq doesn't enjoy
idling, then such bfqq will clear busy after dispatching the last
request?

Please kindly correct me if I'm wrong in the following process:

If there are more than one bfqg that is activatied, then bfqqs that are
not enjoying idle are still left busy after dispatching the last
request.

details in __bfq_bfqq_expire:

       if (RB_EMPTY_ROOT(&bfqq->sort_list) &&
       ┊   !(reason == BFQQE_PREEMPTED &&
       ┊     idling_needed_for_service_guarantees(bfqd, bfqq))) {
-> idling_needed_for_service_guarantees will always return true,

It returns true only is the scenario is symmetric.  Not counting bfqqs
with in-flight requests makes an asymmetric scenario be considered
wrongly symmetric.  See function bfq_asymmetric_scenario().

Hi, Paolo

Do you mean this gap?

1. io1 is issued from bfqq1(from bfqg1)
2. bfqq1 dispatched this io, it's busy is cleared
3. *before io1 is completed*, io2 is issued from bfqq2(bfqg2)

Yes. So as far as I understand Paolo is concerned about this scenario.

quoted

4. with this patchset, while dispatching io2 from bfqq2, the scenario
should be symmetric while it's considered wrongly asymmetric.

But with this patchset, we will consider this scenario symmetric because at
any point in time there is only one busy bfqq. Before, we considered this
scenario asymmetric because two different bfq groups have bfqq in their
weights_tree. So before this patchset
idling_needed_for_service_guarantees() returned true, after this patchset
the function returns false so we won't idle anymore and Paolo argues that
bfqq1 does not get adequate protection from bfqq2 as a result.

I agree with Paolo this seems possible. The fix is relatively simple though
- instead of changing how weights_tree is used for weight raised queues as
you did originally, I'd move the accounting of groups with pending requests
to bfq_add/del_bfqq_busy() and bfq_completed_request().

Why don't we use simply the existing logic? I mean, as for the changes made by this patch, we could simply turn the loop:

void bfq_weights_tree_remove(struct bfq_data *bfqd,
			     struct bfq_queue *bfqq)
{
	...
	for_each_entity(entity) {
		struct bfq_sched_data *sd = entity->my_sched_data;

		...
		if (entity->in_groups_with_pending_reqs) {
			entity->in_groups_with_pending_reqs = false;
			bfqd->num_groups_with_pending_reqs--;
		}
	}
	...
}

into a single:

	bfqd->num_groups_with_pending_reqs--;

so that only the parent group is concerned.

Thanks,
Paolo

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR