[PATCH V1] raidd5:Only move IO_THRESHOLD stripes from delay_list to hold_list once.

[PATCH V1] raidd5:Only move IO_THRESHOLD stripes from delay_list to hold_list once.

[majianpeng]

To improve write perfomance by decreasing the preread stripe,only move
IO_THRESHOLD stripes from delay_list to hold_list once.

Using the follow command:
dd if=/dev/zero of=/dev/md0 bs=2M count=52100.

At default condition: speed is 95MB/s.
At the condition of preread_bypass_threshold was equal zero:speed is 105MB/s.
Using this patch:speed is 123MB/s.

If preread_bypass_threshold was zero,the performance will be better,but
not better than this patch.
I think maybe two reason:
1:If bio is REQ_SYNC
2:In function __get_priority_stripe():
>> } else if (!list_empty(&conf->hold_list) &&
>>		   ((conf->bypass_threshold &&
>>		     conf->bypass_count > conf->bypass_threshold) ||
>>		    atomic_read(&conf->pending_full_writes) == 0)) {
Preread_bypass_threshold is one condition of getting stripe from
hold_list.So only control the number of hold_list can get better
performance.

Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
---
 drivers/md/raid5.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 04348d7..a6749bb 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3662,6 +3662,7 @@ finish:
 
 static void raid5_activate_delayed(struct r5conf *conf)
 {
+	int count = 0;
 	if (atomic_read(&conf->preread_active_stripes) < IO_THRESHOLD) {
 		while (!list_empty(&conf->delayed_list)) {
 			struct list_head *l = conf->delayed_list.next;
@@ -3672,6 +3673,8 @@ static void raid5_activate_delayed(struct r5conf *conf)
 			if (!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				atomic_inc(&conf->preread_active_stripes);
 			list_add_tail(&sh->lru, &conf->hold_list);
+			if (++count >= IO_THRESHOLD)
+				break;
 		}
 	}
 }
-- 
1.7.5.4

Re: [PATCH V1] raidd5:Only move IO_THRESHOLD stripes from delay_list to hold_list once.

[Dan Williams]

[ adding Shaohua ]

On Fri, Jul 13, 2012 at 3:31 AM, majianpeng <majianpeng@gmail.com> wrote:
> To improve write perfomance by decreasing the preread stripe,only move
> IO_THRESHOLD stripes from delay_list to hold_list once.
>
> Using the follow command:
> dd if=/dev/zero of=/dev/md0 bs=2M count=52100.
>
> At default condition: speed is 95MB/s.
> At the condition of preread_bypass_threshold was equal zero:speed is 105MB/s.
> Using this patch:speed is 123MB/s.
>
> If preread_bypass_threshold was zero,the performance will be better,but
> not better than this patch.
> I think maybe two reason:
> 1:If bio is REQ_SYNC
> 2:In function __get_priority_stripe():
>>> } else if (!list_empty(&conf->hold_list) &&
>>>                 ((conf->bypass_threshold &&
>>>                   conf->bypass_count > conf->bypass_threshold) ||
>>>                  atomic_read(&conf->pending_full_writes) == 0)) {
> Preread_bypass_threshold is one condition of getting stripe from
> hold_list.So only control the number of hold_list can get better
> performance.

So this is a pretty obvious tradeoff of increased latency for improved
throughput.  Any idea how much this change affects latency?
Especially in the fast device case?

Re: Re: [PATCH V1] raidd5:Only move IO_THRESHOLD stripes from delay_list to hold_list once.

[majianpeng]

On 2012-07-14 07:56 Dan Williams <dan.j.williams@gmail.com> Wrote:
>[ adding Shaohua ]
>
>On Fri, Jul 13, 2012 at 3:31 AM, majianpeng <majianpeng@gmail.com> wrote:
>> To improve write perfomance by decreasing the preread stripe,only move
>> IO_THRESHOLD stripes from delay_list to hold_list once.
>>
>> Using the follow command:
>> dd if=/dev/zero of=/dev/md0 bs=2M count=52100.
>>
>> At default condition: speed is 95MB/s.
>> At the condition of preread_bypass_threshold was equal zero:speed is 105MB/s.
>> Using this patch:speed is 123MB/s.
>>
>> If preread_bypass_threshold was zero,the performance will be better,but
>> not better than this patch.
>> I think maybe two reason:
>> 1:If bio is REQ_SYNC
>> 2:In function __get_priority_stripe():
>>>> } else if (!list_empty(&conf->hold_list) &&
>>>>                 ((conf->bypass_threshold &&
>>>>                   conf->bypass_count > conf->bypass_threshold) ||
>>>>                  atomic_read(&conf->pending_full_writes) == 0)) {
>> Preread_bypass_threshold is one condition of getting stripe from
>> hold_list.So only control the number of hold_list can get better
>> performance.
>
>So this is a pretty obvious tradeoff of increased latency for improved
>throughput.  Any idea how much this change affects latency?
>Especially in the fast device case?
I did not think the latency.If it only fetch preread_bypass_threshold stripes from delay_list to 
host_list,the latency can be control by userspace.
The code like :

static void raid5_activate_delayed(struct r5conf *conf)
 {
+ int count = 0;
  if (atomic_read(&conf->preread_active_stripes) < IO_THRESHOLD) {
  while (!list_empty(&conf->delayed_list)) {
  struct list_head *l = conf->delayed_list.next;
@@ -3672,6 +3673,8 @@ static void raid5_activate_delayed(struct r5conf *conf)
  if (!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
  atomic_inc(&conf->preread_active_stripes);
  list_add_tail(&sh->lru, &conf->hold_list);
+ if (++count >= conf->preread_active_stripes)
+ break;
  }
 

Re: [PATCH V1] raidd5:Only move IO_THRESHOLD stripes from delay_list to hold_list once.

[NeilBrown]

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

On Fri, 13 Jul 2012 18:31:11 +0800 majianpeng <majianpeng@gmail.com> wrote:

> To improve write perfomance by decreasing the preread stripe,only move
> IO_THRESHOLD stripes from delay_list to hold_list once.
> 
> Using the follow command:
> dd if=/dev/zero of=/dev/md0 bs=2M count=52100.
> 
> At default condition: speed is 95MB/s.
> At the condition of preread_bypass_threshold was equal zero:speed is 105MB/s.
> Using this patch:speed is 123MB/s.
> 
> If preread_bypass_threshold was zero,the performance will be better,but
> not better than this patch.
> I think maybe two reason:
> 1:If bio is REQ_SYNC
> 2:In function __get_priority_stripe():
> >> } else if (!list_empty(&conf->hold_list) &&
> >>		   ((conf->bypass_threshold &&
> >>		     conf->bypass_count > conf->bypass_threshold) ||
> >>		    atomic_read(&conf->pending_full_writes) == 0)) {
> Preread_bypass_threshold is one condition of getting stripe from
> hold_list.So only control the number of hold_list can get better
> performance.
> 
> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
> ---
>  drivers/md/raid5.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 04348d7..a6749bb 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -3662,6 +3662,7 @@ finish:
>  
>  static void raid5_activate_delayed(struct r5conf *conf)
>  {
> +	int count = 0;
>  	if (atomic_read(&conf->preread_active_stripes) < IO_THRESHOLD) {
>  		while (!list_empty(&conf->delayed_list)) {
>  			struct list_head *l = conf->delayed_list.next;
> @@ -3672,6 +3673,8 @@ static void raid5_activate_delayed(struct r5conf *conf)
>  			if (!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>  				atomic_inc(&conf->preread_active_stripes);
>  			list_add_tail(&sh->lru, &conf->hold_list);
> +			if (++count >= IO_THRESHOLD)
> +				break;
>  		}
>  	}
>  }


I tried this patch - against my current for-next tree - on my own modest
hardware and could not measure any difference in write throughput.

Maybe some other patch has fixed something.

However it is still reading a lot during a write-only test and that is not
ideal.  It would be nice if we could arrange that it didn't read at all.

NeilBRown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Re: [PATCH V1] raidd5:Only move IO_THRESHOLD stripes from delay_list to hold_list once.

[majianpeng]

On 2012-07-16 15:46 NeilBrown <neilb@suse.de> Wrote:
>On Fri, 13 Jul 2012 18:31:11 +0800 majianpeng <majianpeng@gmail.com> wrote:
>
>> To improve write perfomance by decreasing the preread stripe,only move
>> IO_THRESHOLD stripes from delay_list to hold_list once.
>> 
>> Using the follow command:
>> dd if=/dev/zero of=/dev/md0 bs=2M count=52100.
>> 
>> At default condition: speed is 95MB/s.
>> At the condition of preread_bypass_threshold was equal zero:speed is 105MB/s.
>> Using this patch:speed is 123MB/s.
>> 
>> If preread_bypass_threshold was zero,the performance will be better,but
>> not better than this patch.
>> I think maybe two reason:
>> 1:If bio is REQ_SYNC
>> 2:In function __get_priority_stripe():
>> >> } else if (!list_empty(&conf->hold_list) &&
>> >>		   ((conf->bypass_threshold &&
>> >>		     conf->bypass_count > conf->bypass_threshold) ||
>> >>		    atomic_read(&conf->pending_full_writes) == 0)) {
>> Preread_bypass_threshold is one condition of getting stripe from
>> hold_list.So only control the number of hold_list can get better
>> performance.
>> 
>> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
>> ---
>>  drivers/md/raid5.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>> 
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index 04348d7..a6749bb 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -3662,6 +3662,7 @@ finish:
>>  
>>  static void raid5_activate_delayed(struct r5conf *conf)
>>  {
>> +	int count = 0;
>>  	if (atomic_read(&conf->preread_active_stripes) < IO_THRESHOLD) {
>>  		while (!list_empty(&conf->delayed_list)) {
>>  			struct list_head *l = conf->delayed_list.next;
>> @@ -3672,6 +3673,8 @@ static void raid5_activate_delayed(struct r5conf *conf)
>>  			if (!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>>  				atomic_inc(&conf->preread_active_stripes);
>>  			list_add_tail(&sh->lru, &conf->hold_list);
>> +			if (++count >= IO_THRESHOLD)
>> +				break;
>>  		}
>>  	}
>>  }
>
>
>I tried this patch - against my current for-next tree - on my own modest
>hardware and could not measure any difference in write throughput.
>
>Maybe some other patch has fixed something.
>
>However it is still reading a lot during a write-only test and that is not
>ideal.  It would be nice if we could arrange that it didn't read at all.
>
By compare to kernel 2.6.18/2.6.32, there are not any reading.
So i think  it should more work to do .
>NeilBRown
>