raid5: Avoid doing more read on dev of a stripe at the same time

In func 'ops_run_bio' if you read the dev which the last reading of this dev didn't return,it will destrory the req/rreq'source of rdev. It may call hung-task.
For example, for badsector or other reasons, read-operation only used stripe instead of chunk_aligned_read.
First:stripe 0;second: stripe 8;third:stripe 16.At the block-layer,three bios merged.
Because media error of sector from 0 to 7, the request retried. At this time, raid5d readed stripe0 again.But it will set 'bio->next = NULL'.So the stripe 8 and 16 didn't return.

Signed-off-by Jianpeng Ma <[email protected]>

---
 drivers/md/raid5.c |   13 ++++++++++---
 drivers/md/raid5.h |    1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

drivers/md/raid5.c +10 -3

index adda94d..e52ba1b 100644
@@ -547,9 +547,14 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
 				rw = WRITE_FUA;
 			else
 				rw = WRITE;
-		} else if (test_and_clear_bit(R5_Wantread, &sh->dev[i].flags))
-			rw = READ;
-		else if (test_and_clear_bit(R5_WantReplace,
+		} else if (test_and_clear_bit(R5_Wantread, &sh->dev[i].flags)) {
+			/*The last read did not return,so skip this read*/
+			if (test_and_set_bit(R5_Reading, &sh->dev[i].flags)) {
+				clear_bit(R5_LOCKED, &sh->dev[i].flags);
+				continue;
+			} else
+				rw = READ;
+		} else if (test_and_clear_bit(R5_WantReplace,
 					    &sh->dev[i].flags)) {
 			rw = WRITE;
 			replace_only = 1;

@@ -700,6 +705,7 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
 			pr_debug("skip op %ld on disc %d for sector %llu\n",
 				bi->bi_rw, i, (unsigned long long)sh->sector);
 			clear_bit(R5_LOCKED, &sh->dev[i].flags);
+			clear_bit(R5_Reading, &sh->dev[i].flags);
 			set_bit(STRIPE_HANDLE, &sh->state);
 		}
 	}

@@ -1818,6 +1824,7 @@ static void raid5_end_read_request(struct bio * bi, int error)
 	}
 	rdev_dec_pending(rdev, conf->mddev);
 	clear_bit(R5_LOCKED, &sh->dev[i].flags);
+	clear_bit(R5_Reading, &sh->dev[i].flags);
 	set_bit(STRIPE_HANDLE, &sh->state);
 	release_stripe(sh);
 }

drivers/md/raid5.h +1 -0

index a9fc249..a596df8 100644
@@ -298,6 +298,7 @@ enum r5dev_flags {
 	R5_WantReplace, /* We need to update the replacement, we have read
 			 * data in, and now is a good time to write it out.
 			 */
+	R5_Reading,	/* this dev on reading on lld*/
 };
 
 /*

Quoted 12 lines

In func 'ops_run_bio' if you read the dev which the last reading of this dev didn't return,it will destrory the req/rreq'source of rdev. It may call hung-task.
For example, for badsector or other reasons, read-operation only used stripe instead of chunk_aligned_read.
First:stripe 0;second: stripe 8;third:stripe 16.At the block-layer,three bios merged.
Because media error of sector from 0 to 7, the request retried. At this time, raid5d readed stripe0 again.But it will set 'bio->next = NULL'.So the stripe 8 and 16 didn't return.

Quoted 1 line

Signed-off-by Jianpeng Ma <[email protected]>

Hi,
I'm really trying, but I cannot understand what you are saying.

I think the situation that you are describing involves a 24 sector request. This is attached to 3 stripe_heads - 8 sectors each - at address 0, 8, 16.

So 'toread' on the first device of each stripe points to this bio, and bi_next is NULL.

The "req" bio for each device is filled out to read one page and these three 'req' bios are submitted. The block layer merges these into a single request.

This request reports an error because there is a read error somewhere in the first 8 sectors.

So one, or maybe all, of the 'req' bios return with an error?

Then RAID5 marks that device as being bad and reads from the other devices to recover the data.

Where exactly does something go wrong? which bio gets bi_next set to NULL when it shouldn't?

If you could explain with lots of detail it would help a lot.

Thanks,
NeilBrown

Quoted 55 lines

---
 drivers/md/raid5.c |   13 ++++++++++---
 drivers/md/raid5.h |    1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

drivers/md/raid5.c +10 -3

index adda94d..e52ba1b 100644
@@ -547,9 +547,14 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
 				rw = WRITE_FUA;
 			else
 				rw = WRITE;
-		} else if (test_and_clear_bit(R5_Wantread, &sh->dev[i].flags))
-			rw = READ;
-		else if (test_and_clear_bit(R5_WantReplace,
+		} else if (test_and_clear_bit(R5_Wantread, &sh->dev[i].flags)) {
+			/*The last read did not return,so skip this read*/
+			if (test_and_set_bit(R5_Reading, &sh->dev[i].flags)) {
+				clear_bit(R5_LOCKED, &sh->dev[i].flags);
+				continue;
+			} else
+				rw = READ;
+		} else if (test_and_clear_bit(R5_WantReplace,
 					    &sh->dev[i].flags)) {
 			rw = WRITE;
 			replace_only = 1;

@@ -700,6 +705,7 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
 			pr_debug("skip op %ld on disc %d for sector %llu\n",
 				bi->bi_rw, i, (unsigned long long)sh->sector);
 			clear_bit(R5_LOCKED, &sh->dev[i].flags);
+			clear_bit(R5_Reading, &sh->dev[i].flags);
 			set_bit(STRIPE_HANDLE, &sh->state);
 		}
 	}

@@ -1818,6 +1824,7 @@ static void raid5_end_read_request(struct bio * bi, int error)
 	}
 	rdev_dec_pending(rdev, conf->mddev);
 	clear_bit(R5_LOCKED, &sh->dev[i].flags);
+	clear_bit(R5_Reading, &sh->dev[i].flags);
 	set_bit(STRIPE_HANDLE, &sh->state);
 	release_stripe(sh);
 }

drivers/md/raid5.h +1 -0

index a9fc249..a596df8 100644
@@ -298,6 +298,7 @@ enum r5dev_flags {
 	R5_WantReplace, /* We need to update the replacement, we have read
 			 * data in, and now is a good time to write it out.
 			 */
+	R5_Reading,	/* this dev on reading on lld*/
 };
 
 /*

On 2012-09-20 10:51 NeilBrown <[email protected]> Wrote:

Quoted 13 lines · On Sat, 15 Sep 2012 10:20:35 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 12 lines

In func 'ops_run_bio' if you read the dev which the last reading of this dev didn't return,it will destrory the req/rreq'source of rdev. It may call hung-task.
For example, for badsector or other reasons, read-operation only used stripe instead of chunk_aligned_read.
First:stripe 0;second: stripe 8;third:stripe 16.At the block-layer,three bios merged.
Because media error of sector from 0 to 7, the request retried. At this time, raid5d readed stripe0 again.But it will set 'bio->next = NULL'.So the stripe 8 and 16 didn't return.

Quoted 5 lines

Quoted 1 line

Signed-off-by Jianpeng Ma <[email protected]>

Hi,
I'm really trying, but I cannot understand what you are saying.

Sorry for my bad english.

Quoted 9 lines

I think the situation that you are describing involves a 24 sector request. This is attached to 3 stripe_heads - 8 sectors each - at address 0, 8, 16.

So 'toread' on the first device of each stripe points to this bio, and bi_next is NULL.

The "req" bio for each device is filled out to read one page and these three 'req' bios are submitted. The block layer merges these into a single request.

Quoted 3 lines

This request reports an error because there is a read error somewhere in the first 8 sectors.

Yes,

Quoted 1 line

So one, or maybe all, of the 'req' bios return with an error?

From my test, when req did not return and at the same time, the bio(stripe 0) send. So this operation will set bi_next is NULL.

Why am i mention the badsector? Because my disks which had badsector can reproduced easily. For disks which didn't have badsector,i can't reproduced this bug.

Quoted 8 lines

Then RAID5 marks that device as being bad and reads from the other devices to recover the data.

Where exactly does something go wrong? which bio gets bi_next set to NULL when it shouldn't?

If you could explain with lots of detail it would help a lot.

The question is to ensure the first-read returned and the next to send on same dev of stripe.

Quoted 61 lines

Thanks,
NeilBrown

Quoted 55 lines

---
 drivers/md/raid5.c |   13 ++++++++++---
 drivers/md/raid5.h |    1 +
 2 files changed, 11 insertions(+), 3 deletions(-)

drivers/md/raid5.c +10 -3

index adda94d..e52ba1b 100644
@@ -547,9 +547,14 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
 				rw = WRITE_FUA;
 			else
 				rw = WRITE;
-		} else if (test_and_clear_bit(R5_Wantread, &sh->dev[i].flags))
-			rw = READ;
-		else if (test_and_clear_bit(R5_WantReplace,
+		} else if (test_and_clear_bit(R5_Wantread, &sh->dev[i].flags)) {
+			/*The last read did not return,so skip this read*/
+			if (test_and_set_bit(R5_Reading, &sh->dev[i].flags)) {
+				clear_bit(R5_LOCKED, &sh->dev[i].flags);
+				continue;
+			} else
+				rw = READ;
+		} else if (test_and_clear_bit(R5_WantReplace,
 					    &sh->dev[i].flags)) {
 			rw = WRITE;
 			replace_only = 1;

@@ -700,6 +705,7 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
 			pr_debug("skip op %ld on disc %d for sector %llu\n",
 				bi->bi_rw, i, (unsigned long long)sh->sector);
 			clear_bit(R5_LOCKED, &sh->dev[i].flags);
+			clear_bit(R5_Reading, &sh->dev[i].flags);
 			set_bit(STRIPE_HANDLE, &sh->state);
 		}
 	}

@@ -1818,6 +1824,7 @@ static void raid5_end_read_request(struct bio * bi, int error)
 	}
 	rdev_dec_pending(rdev, conf->mddev);
 	clear_bit(R5_LOCKED, &sh->dev[i].flags);
+	clear_bit(R5_Reading, &sh->dev[i].flags);
 	set_bit(STRIPE_HANDLE, &sh->state);
 	release_stripe(sh);
 }

drivers/md/raid5.h +1 -0

index a9fc249..a596df8 100644
@@ -298,6 +298,7 @@ enum r5dev_flags {
 	R5_WantReplace, /* We need to update the replacement, we have read
 			 * data in, and now is a good time to write it out.
 			 */
+	R5_Reading,	/* this dev on reading on lld*/
 };
 
 /*

Quoted 30 lines

On 2012-09-20 10:51 NeilBrown <[email protected]> Wrote:

Quoted 13 lines · On Sat, 15 Sep 2012 10:20:35 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 12 lines

In func 'ops_run_bio' if you read the dev which the last reading of this dev didn't return,it will destrory the req/rreq'source of rdev. It may call hung-task.
For example, for badsector or other reasons, read-operation only used stripe instead of chunk_aligned_read.
First:stripe 0;second: stripe 8;third:stripe 16.At the block-layer,three bios merged.
Because media error of sector from 0 to 7, the request retried. At this time, raid5d readed stripe0 again.But it will set 'bio->next = NULL'.So the stripe 8 and 16 didn't return.

Quoted 5 lines

Quoted 1 line

Signed-off-by Jianpeng Ma <[email protected]>

Hi,
I'm really trying, but I cannot understand what you are saying.

Sorry for my bad english.

Quoted 9 lines

I think the situation that you are describing involves a 24 sector request. This is attached to 3 stripe_heads - 8 sectors each - at address 0, 8, 16.

So 'toread' on the first device of each stripe points to this bio, and bi_next is NULL.

The "req" bio for each device is filled out to read one page and these three 'req' bios are submitted. The block layer merges these into a single request.

Quoted 7 lines

Quoted 3 lines

This request reports an error because there is a read error somewhere in the first 8 sectors.

Yes,

Quoted 1 line

So one, or maybe all, of the 'req' bios return with an error?

From my test, when req did not return and at the same time, the bio(stripe 0) send. So this operation will set bi_next is NULL.

Are you saying that we send another bio before the first one has returned? That shouldn't be possible as sh->count will prevent it from happening. While there is an outstanding request, sh->count will be >0, and until sh->count is 0, we won't try to send any more requests.

So I still don't understand. Please try to provide as much detail as possible. If it is easier, write in your own language and use translate.google.com to convert to english. ??

Quoted 30 lines

On 2012-09-20 10:51 NeilBrown <[email protected]> Wrote:

Quoted 13 lines · On Sat, 15 Sep 2012 10:20:35 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 12 lines

In func 'ops_run_bio' if you read the dev which the last reading of this dev didn't return,it will destrory the req/rreq'source of rdev. It may call hung-task.
For example, for badsector or other reasons, read-operation only used stripe instead of chunk_aligned_read.
First:stripe 0;second: stripe 8;third:stripe 16.At the block-layer,three bios merged.
Because media error of sector from 0 to 7, the request retried. At this time, raid5d readed stripe0 again.But it will set 'bio->next = NULL'.So the stripe 8 and 16 didn't return.

Quoted 5 lines

Quoted 1 line

Signed-off-by Jianpeng Ma <[email protected]>

Hi,
I'm really trying, but I cannot understand what you are saying.

Sorry for my bad english.

Quoted 9 lines

I think the situation that you are describing involves a 24 sector request. This is attached to 3 stripe_heads - 8 sectors each - at address 0, 8, 16.

So 'toread' on the first device of each stripe points to this bio, and bi_next is NULL.

The "req" bio for each device is filled out to read one page and these three 'req' bios are submitted. The block layer merges these into a single request.

Quoted 7 lines

Quoted 3 lines

This request reports an error because there is a read error somewhere in the first 8 sectors.

Yes,

Quoted 1 line

So one, or maybe all, of the 'req' bios return with an error?

From my test, when req did not return and at the same time, the bio(stripe 0) send. So this operation will set bi_next is NULL.

Are you saying that we send another bio before the first one has returned? That shouldn't be possible as sh->count will prevent it from happening. While there is an outstanding request, sh->count will be >0, and until sh->count is 0, we won't try to send any more requests.

So I still don't understand. Please try to provide as much detail as possible. If it is easier, write in your own language and use translate.google.com to convert to english. ??

Thanks,
NeilBrown

On 2012-09-20 11:24 NeilBrown <[email protected]> Wrote:

Quoted 44 lines · On Thu, 20 Sep 2012 11:04:46 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 30 lines

On 2012-09-20 10:51 NeilBrown <[email protected]> Wrote:

Quoted 13 lines · On Sat, 15 Sep 2012 10:20:35 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 12 lines

In func 'ops_run_bio' if you read the dev which the last reading of this dev didn't return,it will destrory the req/rreq'source of rdev. It may call hung-task.
For example, for badsector or other reasons, read-operation only used stripe instead of chunk_aligned_read.
First:stripe 0;second: stripe 8;third:stripe 16.At the block-layer,three bios merged.
Because media error of sector from 0 to 7, the request retried. At this time, raid5d readed stripe0 again.But it will set 'bio->next = NULL'.So the stripe 8 and 16 didn't return.

Quoted 5 lines

Quoted 1 line

Signed-off-by Jianpeng Ma <[email protected]>

Hi,
I'm really trying, but I cannot understand what you are saying.

Sorry for my bad english.

Quoted 9 lines

I think the situation that you are describing involves a 24 sector request. This is attached to 3 stripe_heads - 8 sectors each - at address 0, 8, 16.

So 'toread' on the first device of each stripe points to this bio, and bi_next is NULL.

The "req" bio for each device is filled out to read one page and these three 'req' bios are submitted. The block layer merges these into a single request.

Quoted 7 lines

Quoted 3 lines

This request reports an error because there is a read error somewhere in the first 8 sectors.

Yes,

Quoted 1 line

So one, or maybe all, of the 'req' bios return with an error?

From my test, when req did not return and at the same time, the bio(stripe 0) send. So this operation will set bi_next is NULL.

Are you saying that we send another bio before the first one has returned? That shouldn't be possible as sh->count will prevent it from happening. While there is an outstanding request, sh->count will be >0, and until sh->count is 0, we won't try to send any more requests.

Quoted 6 lines

So I still don't understand. Please try to provide as much detail as possible. If it is easier, write in your own language and use translate.google.com to convert to english. ??

Thanks,
NeilBrown

Hi,
i wrote a shell-script can reproduct this bug.
Note: mdadm -V
mdadm - v3.3-pre - Unreleased

#!/bin/bash

declare -i count
declare -i sector
count=0
sector=2048
while true
do

	hdparm --make-bad-sector $sector --yes-i-know-what-i-am-doing /dev/sdc > /dev/null
	hdparm --make-bad-sector $sector --yes-i-know-what-i-am-doing /dev/sdd > /dev/null
	hdparm --make-bad-sector $sector --yes-i-know-what-i-am-doing /dev/sde > /dev/null
	let count++
	let sector+=$count*8
	if (($count == 40));then
		break
	fi

done

while true
do
mdadm -S /dev/md0
mdadm -CR /dev/md0 -l5 -c4 -n4 missing /dev/sd[cde] dd if=/dev/md0 of=/dev/null bs=10M count=1 iflag=direct sleep 1
done

Thanks

Quoted 5 lines · On Fri, 21 Sep 2012 10:24:45 +0800 "Jianpeng Ma" <[email protected]> wrote:

Hi,
i wrote a shell-script can reproduct this bug.
Note: mdadm -V
mdadm - v3.3-pre - Unreleased

Thanks a lot!
I'll try this out and see what I find.

Quoted 8 lines · On Fri, 21 Sep 2012 10:24:45 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 6 lines

Hi,
i wrote a shell-script can reproduct this bug.
Note: mdadm -V
mdadm - v3.3-pre - Unreleased

...

Quoted 2 lines

Thanks a lot!
I'll try this out and see what I find.

I've been running this script (with the mentioned version of mdadm) and nothing has gone wrong.
The 'dd' keeps reporting read errors of course, as does the kernel, but not crash or freeze or anything.

What exactly happens when you run it? Do you get a kernel stack trace at all? Can you post logs showing where it goes wrong?

Quoted 8 lines · On Fri, 21 Sep 2012 10:24:45 +0800 "Jianpeng Ma" <[email protected]> wrote:

Quoted 6 lines

Hi,
i wrote a shell-script can reproduct this bug.
Note: mdadm -V
mdadm - v3.3-pre - Unreleased

...

Quoted 2 lines

Thanks a lot!
I'll try this out and see what I find.

I've been running this script (with the mentioned version of mdadm) and nothing has gone wrong.
The 'dd' keeps reporting read errors of course, as does the kernel, but not crash or freeze or anything.

What exactly happens when you run it? Do you get a kernel stack trace at all? Can you post logs showing where it goes wrong?

Thanks,
NeilBrown