raid5: fix directio regression

PATCH Superseded Shaohua Li 4 participants 28 messages Aug 7, 2012 – Sep 11, 2012 MD RAID

Share & tools

Shaohua Li

author

2012-08-07 03:22 UTC #0 lore raw

PATCH 1/1 raid5: fix directio regression

My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/md/raid5.c +8 -1

Index: linux/drivers/md/raid5.c
===================================================================
@@ -4076,6 +4076,7 @@ static void make_request(struct mddev *m
 	struct stripe_head *sh;
 	const int rw = bio_data_dir(bi);
 	int remaining;
+	bool large_request;
 
 	if (unlikely(bi->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bi);

@@ -4089,6 +4090,11 @@ static void make_request(struct mddev *m
 	     chunk_aligned_read(mddev,bi))
 		return;
 
+	if (mddev->new_chunk_sectors < mddev->chunk_sectors)
+		large_request = (bi->bi_size >> 9) > mddev->new_chunk_sectors;
+	else
+		large_request = (bi->bi_size >> 9) > mddev->chunk_sectors;
+
 	logical_sector = bi->bi_sector & ~((sector_t)STRIPE_SECTORS-1);
 	last_sector = bi->bi_sector + (bi->bi_size>>9);
 	bi->bi_next = NULL;

@@ -4192,7 +4198,8 @@ static void make_request(struct mddev *m
 			finish_wait(&conf->wait_for_overlap, &w);
 			set_bit(STRIPE_HANDLE, &sh->state);
 			clear_bit(STRIPE_DELAYED, &sh->state);
-			if ((bi->bi_rw & REQ_NOIDLE) &&
+			if ((bi->bi_rw & REQ_SYNC) &&
+			    ((bi->bi_rw & REQ_NOIDLE) || !large_request) &&
 			    !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				atomic_inc(&conf->preread_active_stripes);
 			release_stripe_plug(mddev, sh);

Jianpeng Ma

2012-08-07 05:13 UTC #1 lore raw

On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:

Quoted 35 lines

My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
---
drivers/md/raid5.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
Index: linux/drivers/md/raid5.c
===================================================================
drivers/md/raid5.c +6 -0
@@ -4076,6 +4076,7 @@ static void make_request(struct mddev *m
 	struct stripe_head *sh;
 	const int rw = bio_data_dir(bi);
 	int remaining;
+	bool large_request;
 
 	if (unlikely(bi->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bi);
@@ -4089,6 +4090,11 @@ static void make_request(struct mddev *m
 	     chunk_aligned_read(mddev,bi))
 		return;
 
+	if (mddev->new_chunk_sectors < mddev->chunk_sectors)
+		large_request = (bi->bi_size >> 9) > mddev->new_chunk_sectors;
+	else
+		large_request = (bi->bi_size >> 9) > mddev->chunk_sectors;
+
 	logical_sector = bi->bi_sector & ~((sector_t)STRIPE_SECTORS-1);
 	last_sector = bi->bi_sector + (bi->bi_size>>9);
 	bi->bi_next = NULL;

drivers/md/raid5.c static void make_request(struct mddev *m

Quoted 10 lines

drivers/md/raid5.c +2 -2

@@ -4192,7 +4198,8 @@ static void make_request(struct mddev *m
 			finish_wait(&conf->wait_for_overlap, &w);
 			set_bit(STRIPE_HANDLE, &sh->state);
 			clear_bit(STRIPE_DELAYED, &sh->state);
-			if ((bi->bi_rw & REQ_NOIDLE) &&
+			if ((bi->bi_rw & REQ_SYNC) &&
+			    ((bi->bi_rw & REQ_NOIDLE) || !large_request) &&
 			    !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				atomic_inc(&conf->preread_active_stripes);
 			release_stripe_plug(mddev, sh);
--

May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.

Shaohua Li

author

2012-08-07 05:32 UTC #2 lore raw

2012/8/7 Jianpeng Ma <[email protected]>:

Quoted 44 lines

On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:

Quoted 43 lines

Signed-off-by Shaohua Li <[email protected]>

---
drivers/md/raid5.c | 9 ++++++++-

1 file changed, 8 insertions(+), 1 deletion(-)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c 2012-08-07 11:09:08.743562203 +0800 renamed from drivers/md/raid5.c 2012-08-07 11:04:48.442834682 +0800 +8 -1

@@ -4076,6 +4076,7 @@ static void make_request(struct mddev *m
      struct stripe_head *sh;
      const int rw = bio_data_dir(bi);
      int remaining;
+      bool large_request;
 
      if (unlikely(bi->bi_rw & REQ_FLUSH)) {
              md_flush_request(mddev, bi);

@@ -4089,6 +4090,11 @@ static void make_request(struct mddev *m
           chunk_aligned_read(mddev,bi))
              return;
 
+      if (mddev->new_chunk_sectors < mddev->chunk_sectors)
+              large_request = (bi->bi_size >> 9) > mddev->new_chunk_sectors;
+      else
+              large_request = (bi->bi_size >> 9) > mddev->chunk_sectors;
+
      logical_sector = bi->bi_sector & ~((sector_t)STRIPE_SECTORS-1);
      last_sector = bi->bi_sector + (bi->bi_size>>9);
      bi->bi_next = NULL;

@@ -4192,7 +4198,8 @@ static void make_request(struct mddev *m
                      finish_wait(&conf->wait_for_overlap, &w);
                      set_bit(STRIPE_HANDLE, &sh->state);
                      clear_bit(STRIPE_DELAYED, &sh->state);
-                      if ((bi->bi_rw & REQ_NOIDLE) &&
+                      if ((bi->bi_rw & REQ_SYNC) &&
+                          ((bi->bi_rw & REQ_NOIDLE) || !large_request) &&
                          !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              atomic_inc(&conf->preread_active_stripes);

Quoted 8 lines

drivers/md/raid5.c 2012-08-07 11:09:08.743562203 +0800 static void make_request(struct mddev *m
Quoted 2 lines
drivers/md/raid5.c 2012-08-07 11:09:08.743562203 +0800 renamed from drivers/md/raid5.c 2012-08-07 11:04:48.442834682 +0800 +0 -1
                      release_stripe_plug(mddev, sh);
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.

I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?

Thanks,
Shaohua

Jianpeng Ma

2012-08-07 05:42 UTC #3 lore raw

On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:

Quoted 11 lines

2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 10 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 9 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
---
drivers/md/raid5.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

[snip]

Quoted 8 lines

Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.

Quoted 6 lines

I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua

The problem is when to set flag STRIPE_PREREAD_ACTIVE.
When setting this flag, it will to handle stripe instead of delaying for full-write.
I think it like the IOPS and throughput.
When in random-small-write workload, it't hardly to achieve full-write.So no need to dealy.This like sync-write. But for larger-write workload, it's easly to achieve full-write.
This is why send the path. My workload is the latter.
For regular file, it controled by fs.But for raw-block,i think we do the work by fs, so we can control to do the best performance. Of cource, My work is do this,so my point may be limited.

Jianpeng Ma

2012-08-07 06:21 UTC #4 lore raw

In reply to Shaohua Li

On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:

Quoted 8 lines

2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>

[snip]

Quoted 8 lines

Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.

Quoted 6 lines

I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua

Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.

Shaohua Li

author

2012-08-08 02:58 UTC #5 lore raw

2012/8/7 Jianpeng Ma <[email protected]>:

Quoted 22 lines

On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 12 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?

Quoted 6 lines

Quoted 2 lines
Thanks,
Shaohua
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.

I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.

Jianpeng Ma

2012-08-08 05:21 UTC #6 lore raw

On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:

Quoted 25 lines

2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua

Quoted 7 lines

Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like

Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.

Quoted 6 lines

only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.

If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.

Shaohua Li

author

2012-08-08 12:53 UTC #7 lore raw

2012/8/8 Jianpeng Ma <[email protected]>:

Quoted 41 lines

On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.

Quoted 3 lines

If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.

Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.

Jianpeng Ma

2012-08-09 01:20 UTC #8 lore raw

On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:

Quoted 42 lines

2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 41 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.

Quoted 7 lines

Quoted 3 lines
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.

A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.

NeilBrown

2012-08-09 01:32 UTC #9 lore raw

Quoted 47 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:

On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.

Quoted 6 lines

Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.

This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.

The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.

Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.

NeilBrown

Jianpeng Ma

2012-08-09 02:27 UTC #10 lore raw

On 2012-08-09 09:32 NeilBrown <[email protected]> Wrote:

Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.

Quoted 6 lines

Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
NeilBrown

Hi neil:
Thanks your suggestion.
Direct-write can set REQ_NOIDLE because only finish this write-operation the next can do. But direct-write(struct dio) can break up to some bios(struct bios).
For those bios, they have releationSo they may not set REQ_NOIDLE unless the last bio. I think this may increase the performance, because random-direct-write at most only one bio?

Shaohua Li

author

2012-08-09 05:07 UTC #11 lore raw

In reply to NeilBrown

2012/8/9 NeilBrown <[email protected]>:

Quoted 59 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.

Quoted 7 lines

The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.

A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.

Shaohua Li

author

2012-08-14 06:33 UTC #12 lore raw

PATCH v2 1/1 raid5: fix directio regression

Quoted 64 lines · Shaohua Li

2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.

Quoted 8 lines

Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.

Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.

Thanks,
Shaohua

Subject: raid5: fix directio regression

My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.

DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.

REQ_NOIDLE doesn't mean about request merge, I deleted it.

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/md/raid5.c +8 -1

Index: linux/drivers/md/raid5.c
===================================================================
@@ -4076,6 +4076,7 @@ static void make_request(struct mddev *m
 	struct stripe_head *sh;
 	const int rw = bio_data_dir(bi);
 	int remaining;
+	int chunk_sectors;
 
 	if (unlikely(bi->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bi);

@@ -4089,6 +4090,11 @@ static void make_request(struct mddev *m
 	     chunk_aligned_read(mddev,bi))
 		return;
 
+	if (mddev->new_chunk_sectors < mddev->chunk_sectors)
+		chunk_sectors = mddev->new_chunk_sectors;
+	else
+		chunk_sectors = mddev->chunk_sectors;
+
 	logical_sector = bi->bi_sector & ~((sector_t)STRIPE_SECTORS-1);
 	last_sector = bi->bi_sector + (bi->bi_size>>9);
 	bi->bi_next = NULL;

@@ -4192,7 +4198,8 @@ static void make_request(struct mddev *m
 			finish_wait(&conf->wait_for_overlap, &w);
 			set_bit(STRIPE_HANDLE, &sh->state);
 			clear_bit(STRIPE_DELAYED, &sh->state);
-			if ((bi->bi_rw & REQ_NOIDLE) &&
+			if ((bi->bi_rw & REQ_SYNC) &&
+			    (last_sector - logical_sector < chunk_sectors) &&
 			    !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				atomic_inc(&conf->preread_active_stripes);
 			release_stripe_plug(mddev, sh);

NeilBrown

2012-08-15 00:56 UTC #13 lore raw

Quoted 89 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:

Quoted 64 lines · Shaohua Li
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.

Quoted 8 lines

DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.

Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.

You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.

Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.

So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.

I suspect that the original patch should be reverted because while it does improve one case, it causes a regression in another and regressions should be avoided. It would be nice to find a way for both to go fast though...

Thanks,
NeilBrown

Quoted 40 lines

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/md/raid5.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: linux/drivers/md/raid5.c
===================================================================

drivers/md/raid5.c +8 -1

@@ -4076,6 +4076,7 @@ static void make_request(struct mddev *m
 	struct stripe_head *sh;
 	const int rw = bio_data_dir(bi);
 	int remaining;
+	int chunk_sectors;
 
 	if (unlikely(bi->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bi);

@@ -4089,6 +4090,11 @@ static void make_request(struct mddev *m
 	     chunk_aligned_read(mddev,bi))
 		return;
 
+	if (mddev->new_chunk_sectors < mddev->chunk_sectors)
+		chunk_sectors = mddev->new_chunk_sectors;
+	else
+		chunk_sectors = mddev->chunk_sectors;
+
 	logical_sector = bi->bi_sector & ~((sector_t)STRIPE_SECTORS-1);
 	last_sector = bi->bi_sector + (bi->bi_size>>9);
 	bi->bi_next = NULL;

@@ -4192,7 +4198,8 @@ static void make_request(struct mddev *m
 			finish_wait(&conf->wait_for_overlap, &w);
 			set_bit(STRIPE_HANDLE, &sh->state);
 			clear_bit(STRIPE_DELAYED, &sh->state);
-			if ((bi->bi_rw & REQ_NOIDLE) &&
+			if ((bi->bi_rw & REQ_SYNC) &&
+			    (last_sector - logical_sector < chunk_sectors) &&
 			    !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
 				atomic_inc(&conf->preread_active_stripes);
 			release_stripe_plug(mddev, sh);

kedacomkernel

2012-08-15 01:20 UTC #14 lore raw

On 2012-08-15 08:56 NeilBrown <[email protected]> Wrote:

Quoted 121 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:

Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.

Quoted 6 lines

I suspect that the original patch should be reverted because while it does improve one case, it causes a regression in another and regressions should be avoided. It would be nice to find a way for both to go fast though...
Thanks,
NeilBrown

Hi all:
In md-layer, we hardly decide to judge the large-sequential-direct-writes or random write(at most small size) by
bio.
I insist my option: we only judge bio from fs-layer. For one direct-write, it can send more bios to md-driver.
Those bios are sequential.So if the last the bio set REQ_NOFLAG which tell md-driver, it the last and no bio can arrive unless the previous completed. This may good for single process.But for mutiple thread, i think it maybe not good.

Shaohua Li

author

2012-08-15 01:44 UTC #15 lore raw

In reply to NeilBrown

Quoted 105 lines · NeilBrown

Quoted 89 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 64 lines · Shaohua Li
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. directIO usually is random IO and if request size isn't big (which is the common case), delay handling of the stripe hasn't any advantages. For big size request, delay can still reduce IO.
Signed-off-by Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.

Quoted 6 lines

You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.

I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?

Quoted 8 lines

Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.

Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?

Jianpeng Ma

2012-08-15 01:54 UTC #16 lore raw

On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:

Quoted 121 lines · NeilBrown

Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.

Quoted 7 lines

Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?

Ok.

Quoted 1 line

Jianpeng Ma

2012-08-16 07:36 UTC #17 lore raw

In reply to Shaohua Li

On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:

Quoted 121 lines · NeilBrown

Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
Quoted 6 lines
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
Quoted 1 line
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file. Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.

Quoted 8 lines

Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?

I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :

Quoted 4 lines

                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);

The speed is about 76MB/s.With those code the speed is 200MB/s.

BTW, why are you and neil not to answer my option which adding REQ_NOIDLE for last bio of one dio?

Shaohua Li

author

2012-08-16 09:42 UTC #18 lore raw

2012/8/16 Jianpeng Ma <[email protected]>:

Quoted 130 lines

On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 7 lines
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
Quoted 7 lines
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?

Quoted 8 lines

I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
The speed is about 76MB/s.With those code the speed is 200MB/s.

Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.

Quoted 1 line

BTW, why are you and neil not to answer my option which adding REQ_NOIDLE for last bio of one dio?

I'm not quite positive to this. each io_submit can submit several requests, and each request has a dio. Setting the flag for the first dio doesn't make sense, for example

Thanks,
Shaohua

Jianpeng Ma

2012-08-17 01:00 UTC #19 lore raw

On 2012-08-16 17:42 Shaohua Li <[email protected]> Wrote:

Quoted 140 lines

2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 130 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
Quoted 8 lines
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
The speed is about 76MB/s.With those code the speed is 200MB/s.

Quoted 9 lines

Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.
Quoted 1 line
BTW, why are you and neil not to answer my option which adding REQ_NOIDLE for last bio of one dio?
I'm not quite positive to this. each io_submit can submit several requests, and each request has a dio. Setting the flag for the first dio doesn't make sense, for example

Thanks, i didn't know the aio. So, hmm. Thanks again.

Shaohua Li

author

2012-08-23 06:08 UTC #20 lore raw

In reply to Shaohua Li

2012/8/16 Shaohua Li <[email protected]>:

Quoted 134 lines

2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 133 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
Quoted 8 lines
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
Quoted 14 lines
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not? A syscall write is a dio and a dio may be split more bios. For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
The speed is about 76MB/s.With those code the speed is 200MB/s.
Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.

Did you have data for your original workload with 895e3c5c58a80bb reverted now?

Jianpeng Ma

2012-08-23 06:46 UTC #21 lore raw

On 2012-08-23 14:08 Shaohua Li <[email protected]> Wrote:

Quoted 139 lines

2012/8/16 Shaohua Li <[email protected]>:
Quoted 138 lines
2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 137 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 25 lines
2012/8/7 Jianpeng Ma <[email protected]>:
Quoted 24 lines
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Quoted 7 lines
Quoted 4 lines
Another problem is the size. How to judge the size is large or not?
A syscall write is a dio and a dio may be split more bios.
For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size judge is arbitrary. I can optimize it to be only consider stripe which hits two or more disks in one bio, but not sure if it's worthy doing. Not ware big size directio is common, and even is, big size request IOPS is low, a bit delay maybe not a big deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
The speed is about 76MB/s.With those code the speed is 200MB/s.
Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.
Did you have data for your original workload with 895e3c5c58a80bb reverted now?

our raid5 which had 14 SATA HDDs.

with 895e3c5c58a80bb reverted:
using dd to test 55MB/s
using our-fs 200-250Mb/s

with 895e3c5c58a80bb:
using dd to test 275MB/s
using our-fs 500-550Mb/s

Shaohua Li

author

2012-08-23 07:55 UTC #22 lore raw

2012/8/23 Jianpeng Ma <[email protected]>:

Quoted 149 lines

On 2012-08-23 14:08 Shaohua Li <[email protected]> Wrote:
Quoted 139 lines
2012/8/16 Shaohua Li <[email protected]>:
Quoted 138 lines
2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 137 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
Quoted 32 lines
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Another problem is the size. How to judge the size is large or not?
A syscall write is a dio and a dio may be split more bios.
For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though
your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write and as possible as to no pre-read operation.
Quoted 6 lines
only to consider big size random directio. I agree the size
judge is arbitrary. I can optimize it to be only consider stripe
which hits two or more disks in one bio, but not sure if it's
worthy doing. Not ware big size directio is common, and even
is, big size request IOPS is low, a bit delay maybe not a big
deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The speed is about 76MB/s.With those code the speed is 200MB/s.
Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.
Did you have data for your original workload with 895e3c5c58a80bb reverted now?
our raid5 which had 14 SATA HDDs.

Quoted 7 lines

with 895e3c5c58a80bb reverted:
using dd to test 55MB/s
using our-fs 200-250Mb/s
with 895e3c5c58a80bb:
using dd to test 275MB/s
using our-fs 500-550Mb/s

what's block size of dd in this test? In your original test, your BS covers chunk_sector*data_disks. In that case, 895e3c5c58a80bb is likely not required.

I guess you are using a smaller bs this time, so we need merge different bios to a stripe overwrite?

Jianpeng Ma

2012-08-23 08:11 UTC #23 lore raw

On 2012-08-23 15:55 Shaohua Li <[email protected]> Wrote:

Quoted 154 lines

2012/8/23 Jianpeng Ma <[email protected]>:
Quoted 153 lines
On 2012-08-23 14:08 Shaohua Li <[email protected]> Wrote:
Quoted 139 lines
2012/8/16 Shaohua Li <[email protected]>:
Quoted 138 lines
2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 137 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Another problem is the size. How to judge the size is large or not?
A syscall write is a dio and a dio may be split more bios.
For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though
your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write
and as possible as to no pre-read operation.
only to consider big size random directio. I agree the size
judge is arbitrary. I can optimize it to be only consider stripe
which hits two or more disks in one bio, but not sure if it's
worthy doing. Not ware big size directio is common, and even
is, big size request IOPS is low, a bit delay maybe not a big
deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The speed is about 76MB/s.With those code the speed is 200MB/s.
Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.
Did you have data for your original workload with 895e3c5c58a80bb reverted now?
our raid5 which had 14 SATA HDDs.
with 895e3c5c58a80bb reverted:
using dd to test 55MB/s
using our-fs 200-250Mb/s

Quoted 7 lines

Quoted 3 lines
with 895e3c5c58a80bb:
using dd to test 275MB/s
using our-fs 500-550Mb/s
what's block size of dd in this test? In your original test, your BS covers chunk_sector*data_disks. In that case, 895e3c5c58a80bb is likely not required.

Maybe it's my fault. Kerel version is 3.4.4 which did not contain the patch 8811b5968f6216e97c. So i will retest it.

Quoted 3 lines

I guess you are using a smaller bs this time, so we need merge different bios to a stripe overwrite?

At present, in our situation,we control our-fs to do it.
If md can support this function,like io-schedule. it's better for all who can't contorl use-date to cover chunk_sector*data_disks.

Jianpeng Ma

2012-08-23 12:17 UTC #24 lore raw

In reply to Shaohua Li

On 2012-08-23 15:55 Shaohua Li <[email protected]> Wrote:

Quoted 154 lines

2012/8/23 Jianpeng Ma <[email protected]>:
Quoted 153 lines
On 2012-08-23 14:08 Shaohua Li <[email protected]> Wrote:
Quoted 139 lines
2012/8/16 Shaohua Li <[email protected]>:
Quoted 138 lines
2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 137 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
Quoted 44 lines
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Another problem is the size. How to judge the size is large or not?
A syscall write is a dio and a dio may be split more bios.
For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though
your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write
and as possible as to no pre-read operation.
only to consider big size random directio. I agree the size
judge is arbitrary. I can optimize it to be only consider stripe
which hits two or more disks in one bio, but not sure if it's
worthy doing. Not ware big size directio is common, and even
is, big size request IOPS is low, a bit delay maybe not a big
deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly. How do you choose the expire time? A time works for harddisk definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The speed is about 76MB/s.With those code the speed is 200MB/s.
Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.
Did you have data for your original workload with 895e3c5c58a80bb reverted now?
our raid5 which had 14 SATA HDDs.
with 895e3c5c58a80bb reverted:
using dd to test 55MB/s
using our-fs 200-250Mb/s

Quoted 8 lines

Quoted 3 lines
with 895e3c5c58a80bb:
using dd to test 275MB/s
using our-fs 500-550Mb/s
what's block size of dd in this test? In your original test, your BS covers chunk_sector*data_disks. In that case, 895e3c5c58a80bb is likely not required.

With latest kernel(3.6-rc3), w/ or w/o 895e3c5c58a80bb, the result is the same. The block size of dd is chunk_sector * data_disks.
Your patch(8811b5968f6216e97) is good.
I think it shoul revert 8811b5968f6216e97.

Quoted 2 lines

I guess you are using a smaller bs this time, so we need merge different bios to a stripe overwrite?

If the size is smaller, the w/o 895e3c5c58a80bb looks like better than w/ 895e3c5c58a80bb.

Shaohua Li

author

2012-08-24 03:12 UTC #25 lore raw

2012/8/23 Jianpeng Ma <[email protected]>:

Quoted 159 lines

On 2012-08-23 15:55 Shaohua Li <[email protected]> Wrote:
Quoted 158 lines
2012/8/23 Jianpeng Ma <[email protected]>:
Quoted 156 lines
On 2012-08-23 14:08 Shaohua Li <[email protected]> Wrote:
Quoted 139 lines
2012/8/16 Shaohua Li <[email protected]>:
Quoted 138 lines
2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 137 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 48 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
Quoted 46 lines
2012/8/8 Jianpeng Ma <[email protected]>:
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Another problem is the size. How to judge the size is large or not?
A syscall write is a dio and a dio may be split more bios.
For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though
your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write
and as possible as to no pre-read operation.
only to consider big size random directio. I agree the size
judge is arbitrary. I can optimize it to be only consider stripe
which hits two or more disks in one bio, but not sure if it's
worthy doing. Not ware big size directio is common, and even
is, big size request IOPS is low, a bit delay maybe not a big
deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Quoted 6 lines
Quoted 3 lines
Do you want to add a timer for each stripe? This is even ugly.
How do you choose the expire time? A time works for harddisk
definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The speed is about 76MB/s.With those code the speed is 200MB/s.
Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.
Did you have data for your original workload with 895e3c5c58a80bb reverted now?
our raid5 which had 14 SATA HDDs.
with 895e3c5c58a80bb reverted:
using dd to test 55MB/s
using our-fs 200-250Mb/s
with 895e3c5c58a80bb:
using dd to test 275MB/s
using our-fs 500-550Mb/s

Quoted 8 lines

Quoted 4 lines
what's block size of dd in this test? In your original test, your BS covers chunk_sector*data_disks. In that case, 895e3c5c58a80bb is likely not required.
With latest kernel(3.6-rc3), w/ or w/o 895e3c5c58a80bb, the result is the same. The block size of dd is chunk_sector * data_disks.
Your patch(8811b5968f6216e97) is good.
I think it shoul revert 8811b5968f6216e97.

revert 895e3c5c58a80bb, right?

kedacomkernel

2012-08-24 04:21 UTC #26 lore raw

On 2012-08-24 11:12 Shaohua Li <[email protected]> Wrote:

Quoted 164 lines

2012/8/23 Jianpeng Ma <[email protected]>:
Quoted 163 lines
On 2012-08-23 15:55 Shaohua Li <[email protected]> Wrote:
Quoted 162 lines
2012/8/23 Jianpeng Ma <[email protected]>:
Quoted 149 lines
On 2012-08-23 14:08 Shaohua Li <[email protected]> Wrote:
Quoted 139 lines
2012/8/16 Shaohua Li <[email protected]>:
Quoted 138 lines
2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 137 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
Quoted 54 lines
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
2012/8/8 Jianpeng Ma <[email protected]>:
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Another problem is the size. How to judge the size is large or not?
A syscall write is a dio and a dio may be split more bios.
For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though
your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write
and as possible as to no pre-read operation.
only to consider big size random directio. I agree the size
judge is arbitrary. I can optimize it to be only consider stripe
which hits two or more disks in one bio, but not sure if it's
worthy doing. Not ware big size directio is common, and even
is, big size request IOPS is low, a bit delay maybe not a big
deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Do you want to add a timer for each stripe? This is even ugly.
How do you choose the expire time? A time works for harddisk
definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that merge with this one are expected. If some use cases sends random requests, maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't include REQ_NOIDLE. Understanding that would help understand the current problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The speed is about 76MB/s.With those code the speed is 200MB/s.
Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.
Did you have data for your original workload with 895e3c5c58a80bb reverted now?
our raid5 which had 14 SATA HDDs.
Quoted 7 lines
with 895e3c5c58a80bb reverted:
using dd to test 55MB/s
using our-fs 200-250Mb/s
with 895e3c5c58a80bb:
using dd to test 275MB/s
using our-fs 500-550Mb/s
what's block size of dd in this test? In your original test, your BS covers chunk_sector*data_disks. In that case, 895e3c5c58a80bb is likely not required.

Quoted 6 lines

Quoted 4 lines
With latest kernel(3.6-rc3), w/ or w/o 895e3c5c58a80bb, the result is the same. The block size of dd is chunk_sector * data_disks.
Your patch(8811b5968f6216e97) is good.
I think it shoul revert 8811b5968f6216e97.
revert 895e3c5c58a80bb, right?

yes. Because the 8811b5968f6216e97, it can revert.

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

NeilBrown

2012-09-11 00:44 UTC #27 lore raw

Quoted 165 lines · On Fri, 24 Aug 2012 12:21:30 +0800 kedacomkernel <[email protected]>…

On 2012-08-24 11:12 Shaohua Li <[email protected]> Wrote:
Quoted 164 lines
2012/8/23 Jianpeng Ma <[email protected]>:
Quoted 163 lines
On 2012-08-23 15:55 Shaohua Li <[email protected]> Wrote:
Quoted 162 lines
2012/8/23 Jianpeng Ma <[email protected]>:
Quoted 149 lines
On 2012-08-23 14:08 Shaohua Li <[email protected]> Wrote:
Quoted 139 lines
2012/8/16 Shaohua Li <[email protected]>:
Quoted 138 lines
2012/8/16 Jianpeng Ma <[email protected]>:
Quoted 137 lines
On 2012-08-15 09:44 Shaohua Li <[email protected]> Wrote:
Quoted 121 lines · NeilBrown
Quoted 105 lines · On Tue, 14 Aug 2012 14:33:43 +0800 Shaohua Li <[email protected]> wrote:
Quoted 90 lines · Shaohua Li
Quoted 64 lines
2012/8/9 NeilBrown <[email protected]>:
Quoted 63 lines · On Thu, 9 Aug 2012 09:20:05 +0800 "Jianpeng Ma" <[email protected]> wrote:
On 2012-08-08 20:53 Shaohua Li <[email protected]> Wrote:
2012/8/8 Jianpeng Ma <[email protected]>:
On 2012-08-08 10:58 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 13:32 Shaohua Li <[email protected]> Wrote:
2012/8/7 Jianpeng Ma <[email protected]>:
On 2012-08-07 11:22 Shaohua Li <[email protected]> Wrote:
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit
895e3c5c58a80bb. directIO usually is random IO and if request size isn't big
(which is the common case), delay handling of the stripe hasn't any advantages.
For big size request, delay can still reduce IO.
Signed-off-by: Shaohua Li <[email protected]>
[snip]
--
May be used size to judge is not a good method.
I firstly sended this patch, only want to control direct-write-block,not for reqular file.
Because i think if someone used direct-write-block for raid5,he should know the feature of raid5 and he can control
for write to full-write.
But at that time, i did know how to differentiate between regular file and block-device.
I thik we should do something to do this.
I don't think it's possible user can control his write to be a
full-write even for
raw disk IO. Why regular file and block device io matters here?
Thanks,
Shaohua
Another problem is the size. How to judge the size is large or not?
A syscall write is a dio and a dio may be split more bios.
For my workload, i usualy write chunk-size.
But your patch is judge by bio-size.
I'd ignore workload which does sequential directIO, though
your workload is, but I bet no real workloads are. So I'd like
Sorry,my explain maybe not corcrect. I write data once which size is almost chunks-size * devices,in order to full-write
and as possible as to no pre-read operation.
only to consider big size random directio. I agree the size
judge is arbitrary. I can optimize it to be only consider stripe
which hits two or more disks in one bio, but not sure if it's
worthy doing. Not ware big size directio is common, and even
is, big size request IOPS is low, a bit delay maybe not a big
deal.
If add a acc_time for 'striep_head' to control?
When get_active_stripe() is ok, update acc_time.
For some time, stripe_head did not access and it shold pre-read.
Do you want to add a timer for each stripe? This is even ugly.
How do you choose the expire time? A time works for harddisk
definitely will not work for a fast SSD.
A time is like the size which is arbitrary.
How about add a interface in sysfs to control by user?
Only user can judge the workload, which sequatial write or random write.
This is getting worse by the minute. A sysfs interface for this is
definitely not a good idea.
The REQ_NOIDLE flag is a pretty clear statement that no more requests that
merge with this one are expected. If some use cases sends random requests,
maybe it should be setting REQ_NOIDLE.
Quoted 8 lines
Quoted 3 lines
Maybe someone should do some research and find out why WRITE_ODIRECT doesn't
include REQ_NOIDLE. Understanding that would help understand the current
problem.
A quick search shows only cfq-iosched uses REQ_NOIDLE. In cfq, a queue is idled to avoid losing its share. REQ_NOIDLE tells cfq to avoid idle, since the task will not dispatch further requests any more. Note this isn't no merge.
Since REQ_NOIDLE has no relationship with request merge, we'd better remove it. I came out a new patch, which doesn't depend on request size any more. With this patch, sequential directio will still introduce unnecessary raid5 preread (especially for small size IO), but I bet no app does sequential small size directIO.
Thanks,
Shaohua
Subject: raid5: fix directio regression
My directIO randomwrite 4k workload shows a 10~20% regression caused by commit 895e3c5c58a80bb. This commit isn't friendly for small size random IO, because delaying such request hasn't any advantages.
Quoted 8 lines
DirectIO usually is random IO. I thought we can ignore request merge between bios from different io_submit. So we only consider one bio which can drive unnecessary preread in raid5, which is large request. If a bio is large enough and some of its stripes will access two or more disks, such stripes should be delayed to avoid unnecessary preread till bio for the last disk of the strips is added.
REQ_NOIDLE doesn't mean about request merge, I deleted it.
Hi,
Have you tested what effect this has on large sequential direct writes? Because it don't make sense to me and I would be surprised if it improves things.
Quoted 6 lines
You are delaying setting the STRIPE_PREREAD_ACTIVE bit until you think you have submitted all the writes from this bio that apply to the give stripe. That does make some sense, however it doesn't seem to deal with the possibility that the one bio covers parts of two different stripes. In that case the first stripe never gets STRIPE_PREREAD_ACTIVE set, so it is delayed despite having 'REQ_SYNC' set.
I didn't get your point. Isn't last_sector - logical_sector < chunk_sectors true in the case?
Quoted 5 lines
Also, and more significantly, plugging should mean that the various stripe_heads are not even looked at until all of the original bio is processed, so while STRIPE_PREREAD_ACTIVE might get set early, it should not get processed until the whole bio is processed and the queue is unplugged.
Quoted 8 lines
Quoted 3 lines
So I don't think this patch should make a difference on large direct writes, and if it does then something strange is going on that I'd like to understand first.
Aha, ok, this makes sense. recent delayed stripe release should make the problem go away. So Jianpeng, can you try your workload with the commit reverted with a recent kernel please?
I tested used your patch in my workload.
Like the neil said, the performance does not regress. But if the code is :
Quoted 4 lines
                      if (test_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
                              release_stripe(sh);
                      else
                              release_stripe_plug(mddev, sh);
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The speed is about 76MB/s.With those code the speed is 200MB/s.
Hmm, what I want to test is upstream kernel with commit 895e3c5c58a80bb reverted. don't apply my patch. We want to just revert the commit.
Did you have data for your original workload with 895e3c5c58a80bb reverted now?
our raid5 which had 14 SATA HDDs.
Quoted 7 lines
with 895e3c5c58a80bb reverted:
using dd to test 55MB/s
using our-fs 200-250Mb/s
with 895e3c5c58a80bb:
using dd to test 275MB/s
using our-fs 500-550Mb/s
what's block size of dd in this test? In your original test, your BS covers chunk_sector*data_disks. In that case, 895e3c5c58a80bb is likely not required.

Quoted 7 lines

Quoted 6 lines
Quoted 4 lines
With latest kernel(3.6-rc3), w/ or w/o 895e3c5c58a80bb, the result is the same. The block size of dd is chunk_sector * data_disks.
Your patch(8811b5968f6216e97) is good.
I think it shoul revert 8811b5968f6216e97.
revert 895e3c5c58a80bb, right?
yes. Because the 8811b5968f6216e97, it can revert.

Thanks. I've reverted 895e3c5c58a80bb and will submit to -next later today.

NeilBrown