cgroup: Fix split bio been throttled more than once

cgroup: Fix split bio been throttled more than once

[Jiale Li]

Hi Tejun,

These days, we have tested the cgroup blkio throttle function use fio,
and we found a problem that when we use buffered IO or set the big block
size like 1M, then the IO performance cannot reach the value we set.
For example we set blkio.throttle.read_bps_device as 10M, in kernel 
version 4.3 IO performance can only reach 6M, and in kernel version 
4.4 the actual IO bps is only 3.1M. 

Then we did some research and find that in kernel version 4.3 brought in
blk_queue_split() function to split the big size bio into several parts,
and some of them are calling the generic_make_request() again, this result
the bio been throttled more than once. so the actual bio sent to device is 
less than we expected.

We have checked the newest kernel of 4.7-rc5, this problem is still exist.

Based on this kind of situation, we propose a fix solution to add a flag bit
in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
we used to fix this problem.

Thanks

>From b5ea98c9fc5612f9390b65bd9cf4ff344b6cfe92 Mon Sep 17 00:00:00 2001
From: Jiale Li <aaronlee0817@163.com>
Date: Mon, 4 Jul 2016 09:23:32 -0400
Subject: [PATCH] cgroup: Fix split bio been throttled more than once

>From kernel version 4.3, blk_queue_split() been added into the kernel,
it calls the generic_make_request() function,  witch means a part of 
the bio will be counted more than once in blk_throtl_bio(). This result
the throttle function of cgroup cannot work as we expected.

This patch add a new flag bit in bio, to let the split bio bypass the 
blk_throtl_bio().

Signed-off-by: Jiale Li <aaronlee0817@163.com>
---
 block/blk-merge.c         | 1 +
 block/blk-throttle.c      | 4 ++++
 include/linux/blk_types.h | 1 +
 3 files changed, 6 insertions(+)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 2613531..7b17a65 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -190,6 +190,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
 
 		bio_chain(split, *bio);
 		trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
+		bio_set_flag(*bio, BIO_SPLITED);
 		generic_make_request(*bio);
 		*bio = split;
 	}
diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 47a3e54..4ffde95 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1403,6 +1403,10 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
 
 	WARN_ON_ONCE(!rcu_read_lock_held());
 
+	/* if the bio has been splited, should not throttle again */
+	if (bio_flagged(bio, BIO_SPLITED))
+		goto out;
+
 	/* see throtl_charge_bio() */
 	if ((bio->bi_rw & REQ_THROTTLED) || !tg->has_rules[rw])
 		goto out;
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 77e5d81..b294780 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -120,6 +120,7 @@ struct bio {
 #define BIO_QUIET	6	/* Make BIO Quiet */
 #define BIO_CHAIN	7	/* chained bio, ->bi_remaining in effect */
 #define BIO_REFFED	8	/* bio has elevated ->bi_cnt */
+#define BIO_SPLITED     9       /* bio has been splited */
 
 /*
  * Flags starting here get preserved by bio_reset() - this includes
-- 
1.9.1

Re: cgroup: Fix split bio been throttled more than once

[Tejun Heo]

Hello,

On Mon, Jul 04, 2016 at 10:46:54PM +0800, Jiale Li wrote:
> These days, we have tested the cgroup blkio throttle function use fio,
> and we found a problem that when we use buffered IO or set the big block
> size like 1M, then the IO performance cannot reach the value we set.
> For example we set blkio.throttle.read_bps_device as 10M, in kernel 
> version 4.3 IO performance can only reach 6M, and in kernel version 
> 4.4 the actual IO bps is only 3.1M. 
> 
> Then we did some research and find that in kernel version 4.3 brought in
> blk_queue_split() function to split the big size bio into several parts,
> and some of them are calling the generic_make_request() again, this result
> the bio been throttled more than once. so the actual bio sent to device is 
> less than we expected.
> 
> We have checked the newest kernel of 4.7-rc5, this problem is still exist.
> 
> Based on this kind of situation, we propose a fix solution to add a flag bit
> in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
> we used to fix this problem.

Thanks a lot for the report.  Hmmm... there was another brekage around
split bios, I wonder whether the two can be handled together somehow.

 http://lkml.kernel.org/g/1466583730-28595-1-git-send-email-lars.ellenberg@linbit.com

> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 2613531..7b17a65 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -190,6 +190,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
>  
>  		bio_chain(split, *bio);
>  		trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
> +		bio_set_flag(*bio, BIO_SPLITED);

Split's past participle form is split, so BIO_SPLIT would be right
here.

>  		generic_make_request(*bio);
>  		*bio = split;
>  	}
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 47a3e54..4ffde95 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -1403,6 +1403,10 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
>  
>  	WARN_ON_ONCE(!rcu_read_lock_held());
>  
> +	/* if the bio has been splited, should not throttle again */
> +	if (bio_flagged(bio, BIO_SPLITED))
> +		goto out;
> +

But that said, can't we just copy BIO_THROTLED while splitting?

Thanks.

-- 
tejun

Re: cgroup: Fix split bio been throttled more than once

[Ming Lei]

Cc block list

On Mon, Jul 4, 2016 at 10:46 PM, Jiale Li <aaronlee0817@163.com> wrote:
> Hi Tejun,
>
> These days, we have tested the cgroup blkio throttle function use fio,
> and we found a problem that when we use buffered IO or set the big block
> size like 1M, then the IO performance cannot reach the value we set.
> For example we set blkio.throttle.read_bps_device as 10M, in kernel
> version 4.3 IO performance can only reach 6M, and in kernel version
> 4.4 the actual IO bps is only 3.1M.
>
> Then we did some research and find that in kernel version 4.3 brought in
> blk_queue_split() function to split the big size bio into several parts,
> and some of them are calling the generic_make_request() again, this result
> the bio been throttled more than once. so the actual bio sent to device is
> less than we expected.

Except for blk_queue_split(), there are other(stacked) drivers which call
generic_make_request() too, such as drbd, dm, md and bcache.

>
> We have checked the newest kernel of 4.7-rc5, this problem is still exist.
>
> Based on this kind of situation, we propose a fix solution to add a flag bit
> in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
> we used to fix this problem.

The splitted bio is just a fast-cloned bio(except for discard bio) and not very
special compared with other fast-cloned bio, which is quite common used.

So I guess what you need is to bypass BIO_CLONED bio for this purpose
since all fast-cloned bio shares the same bvec table of the source bio.

Thanks,
Ming

>
> Thanks
>
> From b5ea98c9fc5612f9390b65bd9cf4ff344b6cfe92 Mon Sep 17 00:00:00 2001
> From: Jiale Li <aaronlee0817@163.com>
> Date: Mon, 4 Jul 2016 09:23:32 -0400
> Subject: [PATCH] cgroup: Fix split bio been throttled more than once
>
> From kernel version 4.3, blk_queue_split() been added into the kernel,
> it calls the generic_make_request() function,  witch means a part of
> the bio will be counted more than once in blk_throtl_bio(). This result
> the throttle function of cgroup cannot work as we expected.
>
> This patch add a new flag bit in bio, to let the split bio bypass the
> blk_throtl_bio().
>
> Signed-off-by: Jiale Li <aaronlee0817@163.com>
> ---
>  block/blk-merge.c         | 1 +
>  block/blk-throttle.c      | 4 ++++
>  include/linux/blk_types.h | 1 +
>  3 files changed, 6 insertions(+)
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 2613531..7b17a65 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -190,6 +190,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
>
>                 bio_chain(split, *bio);
>                 trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
> +               bio_set_flag(*bio, BIO_SPLITED);
>                 generic_make_request(*bio);
>                 *bio = split;
>         }
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 47a3e54..4ffde95 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -1403,6 +1403,10 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
>
>         WARN_ON_ONCE(!rcu_read_lock_held());
>
> +       /* if the bio has been splited, should not throttle again */
> +       if (bio_flagged(bio, BIO_SPLITED))
> +               goto out;
> +
>         /* see throtl_charge_bio() */
>         if ((bio->bi_rw & REQ_THROTTLED) || !tg->has_rules[rw])
>                 goto out;
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index 77e5d81..b294780 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -120,6 +120,7 @@ struct bio {
>  #define BIO_QUIET      6       /* Make BIO Quiet */
>  #define BIO_CHAIN      7       /* chained bio, ->bi_remaining in effect */
>  #define BIO_REFFED     8       /* bio has elevated ->bi_cnt */
> +#define BIO_SPLITED     9       /* bio has been splited */
>
>  /*
>   * Flags starting here get preserved by bio_reset() - this includes
> --
> 1.9.1
>



-- 
Ming Lei

Re: cgroup: Fix split bio been throttled more than once

[Ming Lei]

On Wed, Jul 6, 2016 at 6:26 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Mon, Jul 04, 2016 at 10:46:54PM +0800, Jiale Li wrote:
>> These days, we have tested the cgroup blkio throttle function use fio,
>> and we found a problem that when we use buffered IO or set the big block
>> size like 1M, then the IO performance cannot reach the value we set.
>> For example we set blkio.throttle.read_bps_device as 10M, in kernel
>> version 4.3 IO performance can only reach 6M, and in kernel version
>> 4.4 the actual IO bps is only 3.1M.
>>
>> Then we did some research and find that in kernel version 4.3 brought in
>> blk_queue_split() function to split the big size bio into several parts,
>> and some of them are calling the generic_make_request() again, this result
>> the bio been throttled more than once. so the actual bio sent to device is
>> less than we expected.
>>
>> We have checked the newest kernel of 4.7-rc5, this problem is still exist.
>>
>> Based on this kind of situation, we propose a fix solution to add a flag bit
>> in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
>> we used to fix this problem.
>
> Thanks a lot for the report.  Hmmm... there was another brekage around
> split bios, I wonder whether the two can be handled together somehow.
>
>  http://lkml.kernel.org/g/1466583730-28595-1-git-send-email-lars.ellenberg@linbit.com
>

That is another issue, which is a deadlock caused by queuing the remainder
bio from splitting before BIOs generated in .make_request_fn().

>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 2613531..7b17a65 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -190,6 +190,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
>>
>>               bio_chain(split, *bio);
>>               trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
>> +             bio_set_flag(*bio, BIO_SPLITED);
>
> Split's past participle form is split, so BIO_SPLIT would be right
> here.
>
>>               generic_make_request(*bio);
>>               *bio = split;
>>       }
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 47a3e54..4ffde95 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -1403,6 +1403,10 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
>>
>>       WARN_ON_ONCE(!rcu_read_lock_held());
>>
>> +     /* if the bio has been splited, should not throttle again */
>> +     if (bio_flagged(bio, BIO_SPLITED))
>> +             goto out;
>> +
>
> But that said, can't we just copy BIO_THROTLED while splitting?
>
> Thanks.
>
> --
> tejun


Thanks,
Ming Lei

Re: cgroup: Fix split bio been throttled more than once

[Tejun Heo]

Hello, Ming.

On Wed, Jul 06, 2016 at 09:10:00AM +0800, Ming Lei wrote:
> > Then we did some research and find that in kernel version 4.3 brought in
> > blk_queue_split() function to split the big size bio into several parts,
> > and some of them are calling the generic_make_request() again, this result
> > the bio been throttled more than once. so the actual bio sent to device is
> > less than we expected.
> 
> Except for blk_queue_split(), there are other(stacked) drivers which call
> generic_make_request() too, such as drbd, dm, md and bcache.

So, blk-throtl already uses REQ_THROTTLED to avoid throttling the same
bio multiple times.  The problem seems that the flag isn't maintained
through clone.

> >
> > We have checked the newest kernel of 4.7-rc5, this problem is still exist.
> >
> > Based on this kind of situation, we propose a fix solution to add a flag bit
> > in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
> > we used to fix this problem.
> 
> The splitted bio is just a fast-cloned bio(except for discard bio) and not very
> special compared with other fast-cloned bio, which is quite common used.
> 
> So I guess what you need is to bypass BIO_CLONED bio for this purpose
> since all fast-cloned bio shares the same bvec table of the source bio.

Depending on how a device handles a bio, that could allow bios to
bypass throttling entirely, no?  Wouldn't adding REQ_THROTTLED to
REQ_CLONE_MASK work?

Thanks.

-- 
tejun

Re: cgroup: Fix split bio been throttled more than once

[Ming Lei]

On Wed, Jul 6, 2016 at 10:09 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Ming.
>
> On Wed, Jul 06, 2016 at 09:10:00AM +0800, Ming Lei wrote:
>> > Then we did some research and find that in kernel version 4.3 brought in
>> > blk_queue_split() function to split the big size bio into several parts,
>> > and some of them are calling the generic_make_request() again, this result
>> > the bio been throttled more than once. so the actual bio sent to device is
>> > less than we expected.
>>
>> Except for blk_queue_split(), there are other(stacked) drivers which call
>> generic_make_request() too, such as drbd, dm, md and bcache.
>
> So, blk-throtl already uses REQ_THROTTLED to avoid throttling the same
> bio multiple times.  The problem seems that the flag isn't maintained
> through clone.

Actually the flag(bio->bi_rw) has been maintained during clone, please
see __bio_clone_fast() and bio_clone_bioset().

>
>> >
>> > We have checked the newest kernel of 4.7-rc5, this problem is still exist.
>> >
>> > Based on this kind of situation, we propose a fix solution to add a flag bit
>> > in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
>> > we used to fix this problem.
>>
>> The splitted bio is just a fast-cloned bio(except for discard bio) and not very
>> special compared with other fast-cloned bio, which is quite common used.
>>
>> So I guess what you need is to bypass BIO_CLONED bio for this purpose
>> since all fast-cloned bio shares the same bvec table of the source bio.
>
> Depending on how a device handles a bio, that could allow bios to
> bypass throttling entirely, no?  Wouldn't adding REQ_THROTTLED to
> REQ_CLONE_MASK work?
>
> Thanks.
>
> --
> tejun


Thanks,
Ming Lei

Re:Re: cgroup: Fix split bio been throttled more than once

[aaronlee0817]

[-- Attachment #1: Type: text/plain, Size: 4667 bytes --]

Hi Ming,

>At 2016-07-06 09:10:00, "Ming Lei" <tom.leiming@gmail.com> wrote:
>Cc block list
>
>On Mon, Jul 4, 2016 at 10:46 PM, Jiale Li <aaronlee0817@163.com> wrote:
>> Hi Tejun,
>>
>> These days, we have tested the cgroup blkio throttle function use fio,
>> and we found a problem that when we use buffered IO or set the big block
>> size like 1M, then the IO performance cannot reach the value we set.
>> For example we set blkio.throttle.read_bps_device as 10M, in kernel
>> version 4.3 IO performance can only reach 6M, and in kernel version
>> 4.4 the actual IO bps is only 3.1M.
>>
>> Then we did some research and find that in kernel version 4.3 brought in
>> blk_queue_split() function to split the big size bio into several parts,
>> and some of them are calling the generic_make_request() again, this result
>> the bio been throttled more than once. so the actual bio sent to device is
>> less than we expected.
>
>Except for blk_queue_split(), there are other(stacked) drivers which call

>generic_make_request() too, such as drbd, dm, md and bcache.


Yes, but only when using cgroup blkio throttle function will cause problem.


>
>>
>> We have checked the newest kernel of 4.7-rc5, this problem is still exist.
>>
>> Based on this kind of situation, we propose a fix solution to add a flag bit
>> in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
>> we used to fix this problem.
>
>The splitted bio is just a fast-cloned bio(except for discard bio) and not very
>special compared with other fast-cloned bio, which is quite common used.
>
>So I guess what you need is to bypass BIO_CLONED bio for this purpose

>since all fast-cloned bio shares the same bvec table of the source bio.


Sorry about my mistake, not bypass the blk_queue_split(), should be let
the remainder bio to bypass the blk_throtl_bio() in generic_make_request().
So, bypass BIO_CLONED bio might not fix this, the problem I introduced is
in blk_throtl_bio(), it counts the dispatched bytes of the same bio more than 
once, once for a bio and remainder bio of this bio after split. we need to count
one bio for only once.


>
>Thanks,
>Ming
>
>>
>> Thanks
>>
>> From b5ea98c9fc5612f9390b65bd9cf4ff344b6cfe92 Mon Sep 17 00:00:00 2001
>> From: Jiale Li <aaronlee0817@163.com>
>> Date: Mon, 4 Jul 2016 09:23:32 -0400
>> Subject: [PATCH] cgroup: Fix split bio been throttled more than once
>>
>> From kernel version 4.3, blk_queue_split() been added into the kernel,
>> it calls the generic_make_request() function,  witch means a part of
>> the bio will be counted more than once in blk_throtl_bio(). This result
>> the throttle function of cgroup cannot work as we expected.
>>
>> This patch add a new flag bit in bio, to let the split bio bypass the
>> blk_throtl_bio().
>>
>> Signed-off-by: Jiale Li <aaronlee0817@163.com>
>> ---
>>  block/blk-merge.c         | 1 +
>>  block/blk-throttle.c      | 4 ++++
>>  include/linux/blk_types.h | 1 +
>>  3 files changed, 6 insertions(+)
>>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 2613531..7b17a65 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -190,6 +190,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
>>
>>                 bio_chain(split, *bio);
>>                 trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
>> +               bio_set_flag(*bio, BIO_SPLITED);
>>                 generic_make_request(*bio);
>>                 *bio = split;
>>         }
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 47a3e54..4ffde95 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -1403,6 +1403,10 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
>>
>>         WARN_ON_ONCE(!rcu_read_lock_held());
>>
>> +       /* if the bio has been splited, should not throttle again */
>> +       if (bio_flagged(bio, BIO_SPLITED))
>> +               goto out;
>> +
>>         /* see throtl_charge_bio() */
>>         if ((bio->bi_rw & REQ_THROTTLED) || !tg->has_rules[rw])
>>                 goto out;
>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>> index 77e5d81..b294780 100644
>> --- a/include/linux/blk_types.h
>> +++ b/include/linux/blk_types.h
>> @@ -120,6 +120,7 @@ struct bio {
>>  #define BIO_QUIET      6       /* Make BIO Quiet */
>>  #define BIO_CHAIN      7       /* chained bio, ->bi_remaining in effect */
>>  #define BIO_REFFED     8       /* bio has elevated ->bi_cnt */
>> +#define BIO_SPLITED     9       /* bio has been splited */
>>
>>  /*
>>   * Flags starting here get preserved by bio_reset() - this includes
>> --
>> 1.9.1
>>

Thanks,


Jiale

[-- Attachment #2: Type: text/html, Size: 5902 bytes --]

Re:Re: cgroup: Fix split bio been throttled more than once

[aaronlee0817]

[-- Attachment #1: Type: text/plain, Size: 2782 bytes --]

Hi Tejun Heo

>At 2016-07-06 06:26:26, "Tejun Heo" <tj@kernel.org> wrote:
>Hello,
>
>On Mon, Jul 04, 2016 at 10:46:54PM +0800, Jiale Li wrote:
>> These days, we have tested the cgroup blkio throttle function use fio,
>> and we found a problem that when we use buffered IO or set the big block
>> size like 1M, then the IO performance cannot reach the value we set.
>> For example we set blkio.throttle.read_bps_device as 10M, in kernel 
>> version 4.3 IO performance can only reach 6M, and in kernel version 
>> 4.4 the actual IO bps is only 3.1M. 
>> 
>> Then we did some research and find that in kernel version 4.3 brought in
>> blk_queue_split() function to split the big size bio into several parts,
>> and some of them are calling the generic_make_request() again, this result
>> the bio been throttled more than once. so the actual bio sent to device is 
>> less than we expected.
>> 
>> We have checked the newest kernel of 4.7-rc5, this problem is still exist.
>> 
>> Based on this kind of situation, we propose a fix solution to add a flag bit
>> in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
>> we used to fix this problem.
>
>Thanks a lot for the report.  Hmmm... there was another brekage around
>split bios, I wonder whether the two can be handled together somehow.
>
> http://lkml.kernel.org/g/1466583730-28595-1-git-send-email-lars.ellenberg@linbit.com 


About this patch, I agree with Ming Lei, this is another issue, they are independent
of each other.
  
>
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 2613531..7b17a65 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -190,6 +190,7 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
>>  
>>  bio_chain(split, *bio);
>>  trace_block_split(q, split, (*bio)->bi_iter.bi_sector);
>> +bio_set_flag(*bio, BIO_SPLITED);
>
>Split's past participle form is split, so BIO_SPLIT would be right
>here.


Yea, you are right, it should be BIO_SPLIT


>
>>  generic_make_request(*bio);
>>  *bio = split;
>>  }
>> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
>> index 47a3e54..4ffde95 100644
>> --- a/block/blk-throttle.c
>> +++ b/block/blk-throttle.c
>> @@ -1403,6 +1403,10 @@ bool blk_throtl_bio(struct request_queue *q, struct blkcg_gq *blkg,
>>  
>>  WARN_ON_ONCE(!rcu_read_lock_held());
>>  
>> +/* if the bio has been splited, should not throttle again */
>> +if (bio_flagged(bio, BIO_SPLITED))
>> +goto out;
>> +
>
>But that said, can't we just copy BIO_THROTLED while splitting?


In my opinion， copying BIO_THROTLED while splitting doesn't work
with this, because, the bio within the limit will not set BIO_THROTLED
but will be split, and back to call generic_make_request(), and will be 
count twice.


Thanks,


Jiale


[-- Attachment #2: Type: text/html, Size: 9926 bytes --]

Re: Re: cgroup: Fix split bio been throttled more than once

[Tejun Heo]

Hello,

On Wed, Jul 06, 2016 at 10:58:55PM +0800, aaronlee0817 wrote:
> >But that said, can't we just copy BIO_THROTLED while splitting?
> 
> In my opinion， copying BIO_THROTLED while splitting doesn't work
> with this, because, the bio within the limit will not set BIO_THROTLED
> but will be split, and back to call generic_make_request(), and will be 
> count twice.

I see.  It's just a bit weird that we're marking this from clone site
when it's an issue which is specific to throttling.  The thing is
depending on how bio clone is used, it could misbehave in the other
direction (bio not accounted at all).  What we want to know is whether
a bio has been previously accounted or not, rather than whether a bio
is a result of splitting.  I think it'd be better to track that
instead.

Thanks.

-- 
tejun

Re:Re: Re: cgroup: Fix split bio been throttled more than once

[aaronlee0817]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]


>At 2016-07-06 23:06:29, "Tejun Heo" <tj@kernel.org> wrote:
>Hello,
>
>On Wed, Jul 06, 2016 at 10:58:55PM +0800, aaronlee0817 wrote:
>> >But that said, can't we just copy BIO_THROTLED while splitting?
>> 
>> In my opinion， copying BIO_THROTLED while splitting doesn't work
>> with this, because, the bio within the limit will not set BIO_THROTLED
>> but will be split, and back to call generic_make_request(), and will be 
>> count twice.
>
>I see.  It's just a bit weird that we're marking this from clone site
>when it's an issue which is specific to throttling.  The thing is
>depending on how bio clone is used, it could misbehave in the other
>direction (bio not accounted at all).  What we want to know is whether
>a bio has been previously accounted or not, rather than whether a bio
>is a result of splitting.  I think it'd be better to track that

>instead.


Yea, I agree with you, we should consider this more carefully, I will
check more about what you mentioned and to see if we have better
way to fix this.


>
>Thanks.
>
>-- 

>tejun


Thanks,


Jiale

[-- Attachment #2: Type: text/html, Size: 1437 bytes --]

Re: Re: cgroup: Fix split bio been throttled more than once

[Ming Lei]

On Wed, Jul 6, 2016 at 11:06 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Wed, Jul 06, 2016 at 10:58:55PM +0800, aaronlee0817 wrote:
>> >But that said, can't we just copy BIO_THROTLED while splitting?
>>
>> In my opinion=EF=BC=8C copying BIO_THROTLED while splitting doesn't work
>> with this, because, the bio within the limit will not set BIO_THROTLED
>> but will be split, and back to call generic_make_request(), and will be
>> count twice.
>
> I see.  It's just a bit weird that we're marking this from clone site
> when it's an issue which is specific to throttling.  The thing is
> depending on how bio clone is used, it could misbehave in the other

The cloned bio often need to be submitted via generic_make_request().

> direction (bio not accounted at all).  What we want to know is whether
> a bio has been previously accounted or not, rather than whether a bio
> is a result of splitting.  I think it'd be better to track that
> instead.

Looks the flag of BIO_THROTLED doesn't work well, and it should have
been set if the bio is accounted, and seems not related with splitting.

Thanks,
Ming

>
> Thanks.
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--=20
Ming Lei

Re: Re: cgroup: Fix split bio been throttled more than once

[Ming Lei]

On Wed, Jul 6, 2016 at 11:06 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Wed, Jul 06, 2016 at 10:58:55PM +0800, aaronlee0817 wrote:
>> >But that said, can't we just copy BIO_THROTLED while splitting?
>>
>> In my opinion， copying BIO_THROTLED while splitting doesn't work
>> with this, because, the bio within the limit will not set BIO_THROTLED
>> but will be split, and back to call generic_make_request(), and will be
>> count twice.
>
> I see.  It's just a bit weird that we're marking this from clone site
> when it's an issue which is specific to throttling.  The thing is
> depending on how bio clone is used, it could misbehave in the other

The cloned bio often need to be submitted via generic_make_request().

> direction (bio not accounted at all).  What we want to know is whether
> a bio has been previously accounted or not, rather than whether a bio
> is a result of splitting.  I think it'd be better to track that
> instead.

Looks the flag of BIO_THROTLED doesn't work well, and it should have
been set if the bio is accounted, and seems not related with splitting.

Thanks,
Ming

>
> Thanks.
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Ming Lei

Re:Re: cgroup: Fix split bio been throttled more than once

[aaronlee0817]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

>At 2016-07-06 22:09:19, "Tejun Heo" <tj@kernel.org> wrote:
>Hello, Ming.
>
>On Wed, Jul 06, 2016 at 09:10:00AM +0800, Ming Lei wrote:
>> > Then we did some research and find that in kernel version 4.3 brought in
>> > blk_queue_split() function to split the big size bio into several parts,
>> > and some of them are calling the generic_make_request() again, this result
>> > the bio been throttled more than once. so the actual bio sent to device is
>> > less than we expected.
>> 
>> Except for blk_queue_split(), there are other(stacked) drivers which call
>> generic_make_request() too, such as drbd, dm, md and bcache.
>
>So, blk-throtl already uses REQ_THROTTLED to avoid throttling the same
>bio multiple times.  The problem seems that the flag isn't maintained
>through clone.

Yea， REQ_THROTTLED is used to avoid throttling the same bio multiple times, 
but cannot avoid counting the same bio multiple times, this makes the counts of
dispatched bytes no real.
Before bringing in splitting, it works well.But now, this cannot cover all  scenarios.
For example:


generic_make_request()
blk_throtl_bio
    bool throttled = false
    throtl_charge_bio()      count the bytes, set REQ_THROTTLED
    goto out
    if(!throttled)
        clean REQ_THROTTLED       
    return 


Before return, REQ_THROTTLED will be cleaned.
While splitting will copy bio->rw, but REQ_THROTTLED has been cleaned.
So throttling function will count the bytes again. In this scenario  cannot 
avoid counting the same bio multiple times.  


>
>> >
>> > We have checked the newest kernel of 4.7-rc5, this problem is still exist.
>> >
>> > Based on this kind of situation, we propose a fix solution to add a flag bit
>> > in bio to let the splited bio bypass the blk_queue_split(). Below is the patch
>> > we used to fix this problem.
>> 
>> The splitted bio is just a fast-cloned bio(except for discard bio) and not very
>> special compared with other fast-cloned bio, which is quite common used.
>> 
>> So I guess what you need is to bypass BIO_CLONED bio for this purpose
>> since all fast-cloned bio shares the same bvec table of the source bio.
>
>Depending on how a device handles a bio, that could allow bios to
>bypass throttling entirely, no?  Wouldn't adding REQ_THROTTLED to
>REQ_CLONE_MASK work?
>
>Thanks.
>
>-- 
>tejun

[-- Attachment #2: Type: text/html, Size: 2972 bytes --]

Re: Re: cgroup: Fix split bio been throttled more than once

[Ming Lei]

On Thu, Jul 7, 2016 at 9:48 PM, aaronlee0817 <aaronlee0817@163.com> wrote:
>>At 2016-07-06 22:09:19, "Tejun Heo" <tj@kernel.org> wrote:
>>Hello, Ming.
>>
>>On Wed, Jul 06, 2016 at 09:10:00AM +0800, Ming Lei wrote:
>>> > Then we did some research and find that in kernel version 4.3 brought
>>> > in
>>> > blk_queue_split() function to split the big size bio into several
>>> > parts,
>>> > and some of them are calling the generic_make_request() again, this
>>> > result
>>> > the bio been throttled more than once. so the actual bio sent to devi=
ce
>>> > is
>>> > less than we expected.
>>>
>>> Except for blk_queue_split(), there are other(stacked) drivers which ca=
ll
>>> generic_make_request() too, such as drbd, dm, md and bcache.
>>
>>So, blk-throtl already uses REQ_THROTTLED to avoid throttling the same
>>bio multiple times.  The problem seems that the flag isn't maintained
>>through clone.
>
> Yea=EF=BC=8C REQ_THROTTLED is used to avoid throttling the same bio multi=
ple times,
> but cannot avoid counting the same bio multiple times, this makes the cou=
nts
> of
> dispatched bytes no real.
> Before bringing in splitting, it works well.But now, this cannot cover al=
l
> scenarios.
> For example:
>
> generic_make_request()
> blk_throtl_bio
>     bool throttled =3D false
>     throtl_charge_bio()      count the bytes, set REQ_THROTTLED
>     goto out
>     if(!throttled)
>         clean REQ_THROTTLED
>     return
>
> Before return, REQ_THROTTLED will be cleaned.

I am wondering why REQ_THROTTLED is cleared for the original bio
even it has been charged and will be issued to driver, and is it allowed
to throttle and charge the same bio for many times?

If the original bio is accounted, looks the splitted bio and the remainder(=
same
bio with original bio but size is decreased, and is resubmitted) shouldn't
have been accounted again because size of the splitted and the remainder
just equals with size of orignal bio.

Thanks,


> While splitting will copy bio->rw, but REQ_THROTTLED has been cleaned.
> So throttling function will count the bytes again. In this scenario  cann=
ot
> avoid counting the same bio multiple times.
>
>>
>>> >
>>> > We have checked the newest kernel of 4.7-rc5, this problem is still
>>> > exist.
>>> >
>>> > Based on this kind of situation, we propose a fix solution to add a
>>> > flag bit
>>> > in bio to let the splited bio bypass the blk_queue_split(). Below is
>>> > the patch
>>> > we used to fix this problem.
>>>
>>> The splitted bio is just a fast-cloned bio(except for discard bio) and
>>> not very
>>> special compared with other fast-cloned bio, which is quite common used=
.
>>>
>>> So I guess what you need is to bypass BIO_CLONED bio for this purpose
>>> since all fast-cloned bio shares the same bvec table of the source bio.
>>
>>Depending on how a device handles a bio, that could allow bios to
>>bypass throttling entirely, no?  Wouldn't adding REQ_THROTTLED to
>>REQ_CLONE_MASK work?
>>
>>Thanks.
>>
>>--
>>tejun
>
>
>
>



--=20
Ming Lei

Re: Re: cgroup: Fix split bio been throttled more than once

[Ming Lei]

On Thu, Jul 7, 2016 at 9:48 PM, aaronlee0817 <aaronlee0817@163.com> wrote:
>>At 2016-07-06 22:09:19, "Tejun Heo" <tj@kernel.org> wrote:
>>Hello, Ming.
>>
>>On Wed, Jul 06, 2016 at 09:10:00AM +0800, Ming Lei wrote:
>>> > Then we did some research and find that in kernel version 4.3 brought
>>> > in
>>> > blk_queue_split() function to split the big size bio into several
>>> > parts,
>>> > and some of them are calling the generic_make_request() again, this
>>> > result
>>> > the bio been throttled more than once. so the actual bio sent to device
>>> > is
>>> > less than we expected.
>>>
>>> Except for blk_queue_split(), there are other(stacked) drivers which call
>>> generic_make_request() too, such as drbd, dm, md and bcache.
>>
>>So, blk-throtl already uses REQ_THROTTLED to avoid throttling the same
>>bio multiple times.  The problem seems that the flag isn't maintained
>>through clone.
>
> Yea， REQ_THROTTLED is used to avoid throttling the same bio multiple times,
> but cannot avoid counting the same bio multiple times, this makes the counts
> of
> dispatched bytes no real.
> Before bringing in splitting, it works well.But now, this cannot cover all
> scenarios.
> For example:
>
> generic_make_request()
> blk_throtl_bio
>     bool throttled = false
>     throtl_charge_bio()      count the bytes, set REQ_THROTTLED
>     goto out
>     if(!throttled)
>         clean REQ_THROTTLED
>     return
>
> Before return, REQ_THROTTLED will be cleaned.

I am wondering why REQ_THROTTLED is cleared for the original bio
even it has been charged and will be issued to driver, and is it allowed
to throttle and charge the same bio for many times?

If the original bio is accounted, looks the splitted bio and the remainder(same
bio with original bio but size is decreased, and is resubmitted) shouldn't
have been accounted again because size of the splitted and the remainder
just equals with size of orignal bio.

Thanks,


> While splitting will copy bio->rw, but REQ_THROTTLED has been cleaned.
> So throttling function will count the bytes again. In this scenario  cannot
> avoid counting the same bio multiple times.
>
>>
>>> >
>>> > We have checked the newest kernel of 4.7-rc5, this problem is still
>>> > exist.
>>> >
>>> > Based on this kind of situation, we propose a fix solution to add a
>>> > flag bit
>>> > in bio to let the splited bio bypass the blk_queue_split(). Below is
>>> > the patch
>>> > we used to fix this problem.
>>>
>>> The splitted bio is just a fast-cloned bio(except for discard bio) and
>>> not very
>>> special compared with other fast-cloned bio, which is quite common used.
>>>
>>> So I guess what you need is to bypass BIO_CLONED bio for this purpose
>>> since all fast-cloned bio shares the same bvec table of the source bio.
>>
>>Depending on how a device handles a bio, that could allow bios to
>>bypass throttling entirely, no?  Wouldn't adding REQ_THROTTLED to
>>REQ_CLONE_MASK work?
>>
>>Thanks.
>>
>>--
>>tejun
>
>
>
>



-- 
Ming Lei

Re: Re: cgroup: Fix split bio been throttled more than once

[Tejun Heo]

Hello, Ming.

On Fri, Jul 08, 2016 at 06:35:06PM +0800, Ming Lei wrote:
> I am wondering why REQ_THROTTLED is cleared for the original bio
> even it has been charged and will be issued to driver, and is it allowed
> to throttle and charge the same bio for many times?

So, IIUC, the flag is just to prevent the bio from recursing while
being issued from blk-throtl after queued there for throttling.  We
can probably extend the flag.  I'm not sure how it'd interact with
stacked drivers tho.  It'd definitely need to be cleared before
traveling down to a lower level device.

Thanks.

-- 
tejun

Re: Re: cgroup: Fix split bio been throttled more than once

[Ming Lei]

On Sat, Jul 9, 2016 at 10:53 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Ming.
>
> On Fri, Jul 08, 2016 at 06:35:06PM +0800, Ming Lei wrote:
>> I am wondering why REQ_THROTTLED is cleared for the original bio
>> even it has been charged and will be issued to driver, and is it allowed
>> to throttle and charge the same bio for many times?
>
> So, IIUC, the flag is just to prevent the bio from recursing while
> being issued from blk-throtl after queued there for throttling.  We
> can probably extend the flag.  I'm not sure how it'd interact with
> stacked drivers tho.  It'd definitely need to be cleared before
> traveling down to a lower level device.

I think I understand it now, in case of stacked driver, the cloned bio
will be submitted
to a new request queue belonging to lower disk, and orignal bio's
REQ_THROTTLED flag should be cleared.

But in case of bio splitting, the remainder bio is just the orignal
bio with front part
splitted out, and it need to submit to same queue again, so it should be
bypassed of throttling because it has been charged already.

So looks Jiale's patch is correct, also seems 'blkg_rwstat_add()' should be
avoided for the remainder bio too in blkcg_bio_issue_check().

thanks,
Ming Lei