raid5: When add stripe_head to inactive_list, it should remove hash.

raid5: When add stripe_head to inactive_list, it should remove hash.

[Jianpeng Ma]

If it released stripe_head to inactive_list and soon get it from hash
list. So the stripe_head didn't call init_stripe, the dev.flags isn't
clear. So the debug info is error which printed the before info.
Although, i can't find logic error because this.I think it should be
correct.

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

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index adda94d..03d2f64 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -196,6 +196,24 @@ static int stripe_operations_active(struct stripe_head *sh)
 	       test_bit(STRIPE_COMPUTE_RUN, &sh->state);
 }
 
+static inline void remove_hash(struct stripe_head *sh)
+{
+	pr_debug("remove_hash(), stripe %llu\n",
+		(unsigned long long)sh->sector);
+
+	hlist_del_init(&sh->hash);
+}
+
+static inline void insert_hash(struct r5conf *conf, struct stripe_head *sh)
+{
+	struct hlist_head *hp = stripe_hash(conf, sh->sector);
+
+	pr_debug("insert_hash(), stripe %llu\n",
+		(unsigned long long)sh->sector);
+
+	hlist_add_head(&sh->hash, hp);
+}
+
 static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh)
 {
 	BUG_ON(!list_empty(&sh->lru));
@@ -222,6 +240,7 @@ static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh)
 		atomic_dec(&conf->active_stripes);
 		if (!test_bit(STRIPE_EXPANDING, &sh->state)) {
 			list_add_tail(&sh->lru, &conf->inactive_list);
+			remove_hash(sh);
 			wake_up(&conf->wait_for_stripe);
 			if (conf->retry_read_aligned)
 				md_wakeup_thread(conf->mddev->thread);
@@ -248,23 +267,7 @@ static void release_stripe(struct stripe_head *sh)
 	local_irq_restore(flags);
 }
 
-static inline void remove_hash(struct stripe_head *sh)
-{
-	pr_debug("remove_hash(), stripe %llu\n",
-		(unsigned long long)sh->sector);
 
-	hlist_del_init(&sh->hash);
-}
-
-static inline void insert_hash(struct r5conf *conf, struct stripe_head *sh)
-{
-	struct hlist_head *hp = stripe_hash(conf, sh->sector);
-
-	pr_debug("insert_hash(), stripe %llu\n",
-		(unsigned long long)sh->sector);
-
-	hlist_add_head(&sh->hash, hp);
-}
 
 
 /* find an idle stripe, make sure it is unhashed, and return it. */
-- 
1.7.9.5
\x02

Re: raid5: When add stripe_head to inactive_list, it should remove hash.

[Jianpeng Ma]

On 2012-08-18 10:53 Jianpeng Ma <majianpeng@gmail.com> Wrote:
>If it released stripe_head to inactive_list and soon get it from hash
>list. So the stripe_head didn't call init_stripe, the dev.flags isn't
>clear. So the debug info is error which printed the before info.
>Although, i can't find logic error because this.I think it should be
>correct.
>
>Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
>---
> drivers/md/raid5.c |   35 +++++++++++++++++++----------------
> 1 file changed, 19 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>index adda94d..03d2f64 100644
>--- a/drivers/md/raid5.c
>+++ b/drivers/md/raid5.c
>@@ -196,6 +196,24 @@ static int stripe_operations_active(struct stripe_head *sh)
> 	       test_bit(STRIPE_COMPUTE_RUN, &sh->state);
> }
> 
>+static inline void remove_hash(struct stripe_head *sh)
>+{
>+	pr_debug("remove_hash(), stripe %llu\n",
>+		(unsigned long long)sh->sector);
>+
>+	hlist_del_init(&sh->hash);
>+}
>+
>+static inline void insert_hash(struct r5conf *conf, struct stripe_head *sh)
>+{
>+	struct hlist_head *hp = stripe_hash(conf, sh->sector);
>+
>+	pr_debug("insert_hash(), stripe %llu\n",
>+		(unsigned long long)sh->sector);
>+
>+	hlist_add_head(&sh->hash, hp);
>+}
>+
> static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh)
> {
> 	BUG_ON(!list_empty(&sh->lru));
>@@ -222,6 +240,7 @@ static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh)
> 		atomic_dec(&conf->active_stripes);
> 		if (!test_bit(STRIPE_EXPANDING, &sh->state)) {
> 			list_add_tail(&sh->lru, &conf->inactive_list);
>+			remove_hash(sh);
> 			wake_up(&conf->wait_for_stripe);
> 			if (conf->retry_read_aligned)
> 				md_wakeup_thread(conf->mddev->thread);
>@@ -248,23 +267,7 @@ static void release_stripe(struct stripe_head *sh)
> 	local_irq_restore(flags);
> }
> 
>-static inline void remove_hash(struct stripe_head *sh)
>-{
>-	pr_debug("remove_hash(), stripe %llu\n",
>-		(unsigned long long)sh->sector);
> 
>-	hlist_del_init(&sh->hash);
>-}
>-
>-static inline void insert_hash(struct r5conf *conf, struct stripe_head *sh)
>-{
>-	struct hlist_head *hp = stripe_hash(conf, sh->sector);
>-
>-	pr_debug("insert_hash(), stripe %llu\n",
>-		(unsigned long long)sh->sector);
>-
>-	hlist_add_head(&sh->hash, hp);
>-}
> 
> 
> /* find an idle stripe, make sure it is unhashed, and return it. */
>-- 
>1.7.9.5
>
Hi neil:
	Today, i found a problem about this.
1:mdadm -CR /dev/md0 -l5 -c4 -n4 /dev/sd[bcd] missing
2:dd if=/dev/zero of=/dev/md0 bs=4K count=1 oflag=direct
3:hdparm --make-bad-sector 2048 --yes-i-know-what-i-am-doing /dev/sdb
  (2048 is the data-offset of sdb)
4:dd if=/dev/md0 of=/dev/zero bs=4K count=1 iflag=direct

I think step4 maybe return ioerror.But it's success.
The kernel message like :
[19277.581245] ata3.00: exception Emask 0x0 SAct 0x1 SErr 0x0 action 0x0
[19277.581250] ata3.00: irq_stat 0x40000008
[19277.581253] ata3.00: failed command: READ FPDMA QUEUED
[19277.581260] ata3.00: cmd 60/08:00:00:08:00/00:00:00:00:00/40 tag 0 ncq 4096 in
[19277.581260]          res 41/40:08:00:08:00/00:00:00:00:00/00 Emask 0x409 (media error) <F>
[19277.581263] ata3.00: status: { DRDY ERR }
[19277.581265] ata3.00: error: { UNC }
[19277.585809] ata3.00: configured for UDMA/133
[19277.585828] sd 2:0:0:0: \x010[sdb] Unhandled sense code
[19277.585831] sd 2:0:0:0: \x010[sdb]  
[19277.585833] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[19277.585835] sd 2:0:0:0: \x010[sdb]  
[19277.585837] Sense Key : Medium Error [current] [descriptor]
[19277.585842] Descriptor sense data with sense descriptors (in hex):
[19277.585844]         72 03 11 04 00 00 00 0c 00 0a 80 00 00 00 00 00 
[19277.585856]         00 00 08 00 
[19277.585862] sd 2:0:0:0: \x010[sdb]  
[19277.585864] Add. Sense: Unrecovered read error - auto reallocate failed
[19277.585867] sd 2:0:0:0: \x010[sdb] CDB: 
[19277.585869] Read(10): 28 00 00 00 08 00 00 00 08 00
[19277.585879] end_request: I/O error, dev sdb, sector 2048
[19277.585889] raid5_align_endio : io error...handing IO for a retry
[19277.585901] +++ raid5d active
[19277.585906] get_stripe, sector 0
[19277.585908] __find_stripe, sector 0
[19277.585911] adding bi b#0 to stripe s#0
[19277.585914] added bi b#0 to stripe s#0, disk 0.
[19277.585918] handling stripe 0, state=0x1091 cnt=1, pd_idx=3, qd_idx=-1
[19277.585918] , check:0, reconstruct:0
[19277.585922] check 3: state 0x1 read           (null) write           (null) written           (null)
[19277.585925] check 2: state 0x11 read           (null) write           (null) written           (null)
[19277.585927] ata3: EH complete
[19277.585930] check 1: state 0x11 read           (null) write           (null) written           (null)
[19277.585933] check 0: state 0x119 read ffff8800b69cd900 write           (null) written           (null)
[19277.585936] locked=0 uptodate=4 to_read=0 to_write=0 failed=1 failed_num=3,-1
[19277.585939] ops_run_biofill: stripe 0
[19277.585944] ops_complete_biofill: stripe 0
[19277.585955] __get_priority_stripe: handle: busy hold: empty full_writes: 0 bypass_count: 0
[19277.585958] __get_priority_stripe: handle: empty hold: empty full_writes: 0 bypass_count: 0
[19277.585961] handling stripe 0, state=0x1091 cnt=1, pd_idx=3, qd_idx=-1
[19277.585961] , check:0, reconstruct:0
[19277.585964] check 3: state 0x1 read           (null) write           (null) written           (null)
[19277.585967] check 2: state 0x11 read           (null) write           (null) written           (null)
[19277.585969] check 1: state 0x11 read           (null) write           (null) written           (null)
[19277.585972] check 0: state 0x119 read           (null) write           (null) written           (null)
[19277.585975] locked=0 uptodate=4 to_read=0 to_write=0 failed=1 failed_num=3,-1
[19277.585979] __get_priority_stripe: handle: empty hold: empty full_writes: 0 bypass_count: 0
[19277.585981] 2 stripes handled
[19277.585983] --- raid5d inactive
[19277.585984] +++ raid5d active
[19277.585987] __get_priority_stripe: handle: empty hold: empty full_writes: 0 bypass_count: 0
[19277.585989] 0 stripes handled
[19277.585991] --- raid5d inactive

Re: raid5: When add stripe_head to inactive_list, it should remove hash.

[Dan Williams]

On Mon, Aug 20, 2012 at 1:17 AM, Jianpeng Ma <majianpeng@gmail.com> wrote:
> On 2012-08-18 10:53 Jianpeng Ma <majianpeng@gmail.com> Wrote:
>>If it released stripe_head to inactive_list and soon get it from hash
>>list. So the stripe_head didn't call init_stripe, the dev.flags isn't
>>clear. So the debug info is error which printed the before info.
>>Although, i can't find logic error because this.I think it should be
>>correct.
>>
>>Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
>>---
>> drivers/md/raid5.c |   35 +++++++++++++++++++----------------
>> 1 file changed, 19 insertions(+), 16 deletions(-)
>>
>>diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>>index adda94d..03d2f64 100644
>>--- a/drivers/md/raid5.c
>>+++ b/drivers/md/raid5.c
>>@@ -196,6 +196,24 @@ static int stripe_operations_active(struct stripe_head *sh)
>>              test_bit(STRIPE_COMPUTE_RUN, &sh->state);
>> }
>>
>>+static inline void remove_hash(struct stripe_head *sh)
>>+{
>>+      pr_debug("remove_hash(), stripe %llu\n",
>>+              (unsigned long long)sh->sector);
>>+
>>+      hlist_del_init(&sh->hash);
>>+}
>>+
>>+static inline void insert_hash(struct r5conf *conf, struct stripe_head *sh)
>>+{
>>+      struct hlist_head *hp = stripe_hash(conf, sh->sector);
>>+
>>+      pr_debug("insert_hash(), stripe %llu\n",
>>+              (unsigned long long)sh->sector);
>>+
>>+      hlist_add_head(&sh->hash, hp);
>>+}
>>+
>> static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh)
>> {
>>       BUG_ON(!list_empty(&sh->lru));
>>@@ -222,6 +240,7 @@ static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh)
>>               atomic_dec(&conf->active_stripes);
>>               if (!test_bit(STRIPE_EXPANDING, &sh->state)) {
>>                       list_add_tail(&sh->lru, &conf->inactive_list);
>>+                      remove_hash(sh);
>>                       wake_up(&conf->wait_for_stripe);
>>                       if (conf->retry_read_aligned)
>>                               md_wakeup_thread(conf->mddev->thread);
>>@@ -248,23 +267,7 @@ static void release_stripe(struct stripe_head *sh)
>>       local_irq_restore(flags);
>> }
>>
>>-static inline void remove_hash(struct stripe_head *sh)
>>-{
>>-      pr_debug("remove_hash(), stripe %llu\n",
>>-              (unsigned long long)sh->sector);
>>
>>-      hlist_del_init(&sh->hash);
>>-}
>>-
>>-static inline void insert_hash(struct r5conf *conf, struct stripe_head *sh)
>>-{
>>-      struct hlist_head *hp = stripe_hash(conf, sh->sector);
>>-
>>-      pr_debug("insert_hash(), stripe %llu\n",
>>-              (unsigned long long)sh->sector);
>>-
>>-      hlist_add_head(&sh->hash, hp);
>>-}
>>
>>
>> /* find an idle stripe, make sure it is unhashed, and return it. */
>>--
>>1.7.9.5
>>
> Hi neil:
>         Today, i found a problem about this.
> 1:mdadm -CR /dev/md0 -l5 -c4 -n4 /dev/sd[bcd] missing
> 2:dd if=/dev/zero of=/dev/md0 bs=4K count=1 oflag=direct
> 3:hdparm --make-bad-sector 2048 --yes-i-know-what-i-am-doing /dev/sdb
>   (2048 is the data-offset of sdb)
> 4:dd if=/dev/md0 of=/dev/zero bs=4K count=1 iflag=direct
>
> I think step4 maybe return ioerror.But it's success.

You would need to check that stripe0 got recycled between step2 and
step3.  If not then the uptodate stripe is in the cache and the read
can be serviced from the cached data.

...which seems to be the case because of:

> [19277.585936] locked=0 uptodate=4 to_read=0 to_write=0 failed=1 failed_num=3,-1

--
Dan

Re: Re: raid5: When add stripe_head to inactive_list, it should remove hash.

[Jianpeng Ma]

On 2012-08-21 01:51 Dan Williams <djbw@fb.com> Wrote:
>On Mon, Aug 20, 2012 at 1:17 AM, Jianpeng Ma <majianpeng@gmail.com> wrote:
>> On 2012-08-18 10:53 Jianpeng Ma <majianpeng@gmail.com> Wrote:
>>>If it released stripe_head to inactive_list and soon get it from hash
>>>list. So the stripe_head didn't call init_stripe, the dev.flags isn't
>>>clear. So the debug info is error which printed the before info.
>>>Although, i can't find logic error because this.I think it should be
>>>correct.
>>>
>>>Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
>>>---
>>> drivers/md/raid5.c |   35 +++++++++++++++++++----------------
>>> 1 file changed, 19 insertions(+), 16 deletions(-)
>>>
>>>diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>>>index adda94d..03d2f64 100644
>>>--- a/drivers/md/raid5.c
>>>+++ b/drivers/md/raid5.c
>>>@@ -196,6 +196,24 @@ static int stripe_operations_active(struct stripe_head *sh)
>>>              test_bit(STRIPE_COMPUTE_RUN, &sh->state);
>>> }
>>>
>>>+static inline void remove_hash(struct stripe_head *sh)
>>>+{
>>>+      pr_debug("remove_hash(), stripe %llu\n",
>>>+              (unsigned long long)sh->sector);
>>>+
>>>+      hlist_del_init(&sh->hash);
>>>+}
>>>+
>>>+static inline void insert_hash(struct r5conf *conf, struct stripe_head *sh)
>>>+{
>>>+      struct hlist_head *hp = stripe_hash(conf, sh->sector);
>>>+
>>>+      pr_debug("insert_hash(), stripe %llu\n",
>>>+              (unsigned long long)sh->sector);
>>>+
>>>+      hlist_add_head(&sh->hash, hp);
>>>+}
>>>+
>>> static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh)
>>> {
>>>       BUG_ON(!list_empty(&sh->lru));
>>>@@ -222,6 +240,7 @@ static void do_release_stripe(struct r5conf *conf, struct stripe_head *sh)
>>>               atomic_dec(&conf->active_stripes);
>>>               if (!test_bit(STRIPE_EXPANDING, &sh->state)) {
>>>                       list_add_tail(&sh->lru, &conf->inactive_list);
>>>+                      remove_hash(sh);
>>>                       wake_up(&conf->wait_for_stripe);
>>>                       if (conf->retry_read_aligned)
>>>                               md_wakeup_thread(conf->mddev->thread);
>>>@@ -248,23 +267,7 @@ static void release_stripe(struct stripe_head *sh)
>>>       local_irq_restore(flags);
>>> }
>>>
>>>-static inline void remove_hash(struct stripe_head *sh)
>>>-{
>>>-      pr_debug("remove_hash(), stripe %llu\n",
>>>-              (unsigned long long)sh->sector);
>>>
>>>-      hlist_del_init(&sh->hash);
>>>-}
>>>-
>>>-static inline void insert_hash(struct r5conf *conf, struct stripe_head *sh)
>>>-{
>>>-      struct hlist_head *hp = stripe_hash(conf, sh->sector);
>>>-
>>>-      pr_debug("insert_hash(), stripe %llu\n",
>>>-              (unsigned long long)sh->sector);
>>>-
>>>-      hlist_add_head(&sh->hash, hp);
>>>-}
>>>
>>>
>>> /* find an idle stripe, make sure it is unhashed, and return it. */
>>>--
>>>1.7.9.5
>>>
>> Hi neil:
>>         Today, i found a problem about this.
>> 1:mdadm -CR /dev/md0 -l5 -c4 -n4 /dev/sd[bcd] missing
>> 2:dd if=/dev/zero of=/dev/md0 bs=4K count=1 oflag=direct
>> 3:hdparm --make-bad-sector 2048 --yes-i-know-what-i-am-doing /dev/sdb
>>   (2048 is the data-offset of sdb)
>> 4:dd if=/dev/md0 of=/dev/zero bs=4K count=1 iflag=direct
>>
>> I think step4 maybe return ioerror.But it's success.
>
>You would need to check that stripe0 got recycled between step2 and
>step3.  If not then the uptodate stripe is in the cache and the read
>can be serviced from the cached data.
>
>...which seems to be the case because of:
>
>> [19277.585936] locked=0 uptodate=4 to_read=0 to_write=0 failed=1 failed_num=3,-1
>
>--
>Dan
Thanks! If stripe had a cache function.I think the chunk_aligned_read should use it for small read situation.

Re: Re: raid5: When add stripe_head to inactive_list, it should remove hash.

[Dan Williams]

On Mon, Aug 20, 2012 at 8:14 PM, Jianpeng Ma <majianpeng@gmail.com> wrote:
> On 2012-08-21 01:51 Dan Williams <djbw@fb.com> Wrote:
>>On Mon, Aug 20, 2012 at 1:17 AM, Jianpeng Ma <majianpeng@gmail.com> wrote:
>>> I think step4 maybe return ioerror.But it's success.
>>
>>You would need to check that stripe0 got recycled between step2 and
>>step3.  If not then the uptodate stripe is in the cache and the read
>>can be serviced from the cached data.
>>
>>...which seems to be the case because of:
>>
>>> [19277.585936] locked=0 uptodate=4 to_read=0 to_write=0 failed=1 failed_num=3,-1
>>
>
> Thanks! If stripe had a cache function.I think the chunk_aligned_read should use it for small read situation.

But the stripe cache is not meant to be a read cache as it would be
duplicating what the page cache offers.  Most aligned reads are likely
to miss the stripe cache so adding a lookup for every read would be
mostly wasted overhead.

--
Dan

Re: Re: raid5: When add stripe_head to inactive_list, it should remove hash.

[kedacomkernel]

On 2012-08-22 00:43 Dan Williams <djbw@fb.com> Wrote:
>On Mon, Aug 20, 2012 at 8:14 PM, Jianpeng Ma <majianpeng@gmail.com> wrote:
>> On 2012-08-21 01:51 Dan Williams <djbw@fb.com> Wrote:
>>>On Mon, Aug 20, 2012 at 1:17 AM, Jianpeng Ma <majianpeng@gmail.com> wrote:
>>>> I think step4 maybe return ioerror.But it's success.
>>>
>>>You would need to check that stripe0 got recycled between step2 and
>>>step3.  If not then the uptodate stripe is in the cache and the read
>>>can be serviced from the cached data.
>>>
>>>...which seems to be the case because of:
>>>
>>>> [19277.585936] locked=0 uptodate=4 to_read=0 to_write=0 failed=1 failed_num=3,-1
>>>
>>
>> Thanks! If stripe had a cache function.I think the chunk_aligned_read should use it for small read situation.
>
>But the stripe cache is not meant to be a read cache as it would be
>duplicating what the page cache offers.  Most aligned reads are likely
>to miss the stripe cache so adding a lookup for every read would be
>mostly wasted overhead.
>
I think most aligned reads are likely to missed the stripe cache because they did not use stripe-cache(when fristly reaed)
For direct-io workload, there is no page-cache.
FYI, most hardware raid-controlers have own memory and don't use host-memeory. The stripe-cache of raid456 is the same as
raid-controller memeory.They also should provide the page-cache for read, rather only for write.

RE: Re: raid5: When add stripe_head to inactive_list, it should remove hash.

[Jiang, Dave]

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of kedacomkernel
> Sent: Tuesday, August 21, 2012 6:26 PM
> To: Dan Williams; majianpeng
> Cc: Neil Brown; linux-raid
> Subject: Re: Re: raid5: When add stripe_head to inactive_list, it should
> remove hash.
> 
> On 2012-08-22 00:43 Dan Williams <djbw@fb.com> Wrote:
> >On Mon, Aug 20, 2012 at 8:14 PM, Jianpeng Ma <majianpeng@gmail.com>
> wrote:
> >> On 2012-08-21 01:51 Dan Williams <djbw@fb.com> Wrote:
> >>>On Mon, Aug 20, 2012 at 1:17 AM, Jianpeng Ma
> <majianpeng@gmail.com> wrote:
> >>>> I think step4 maybe return ioerror.But it's success.
> >>>
> >>>You would need to check that stripe0 got recycled between step2 and
> >>>step3.  If not then the uptodate stripe is in the cache and the read
> >>>can be serviced from the cached data.
> >>>
> >>>...which seems to be the case because of:
> >>>
> >>>> [19277.585936] locked=0 uptodate=4 to_read=0 to_write=0 failed=1
> >>>> failed_num=3,-1
> >>>
> >>
> >> Thanks! If stripe had a cache function.I think the chunk_aligned_read
> should use it for small read situation.
> >
> >But the stripe cache is not meant to be a read cache as it would be
> >duplicating what the page cache offers.  Most aligned reads are likely
> >to miss the stripe cache so adding a lookup for every read would be
> >mostly wasted overhead.
> >
> I think most aligned reads are likely to missed the stripe cache because they
> did not use stripe-cache(when fristly reaed) For direct-io workload, there is
> no page-cache.
> FYI, most hardware raid-controlers have own memory and don't use host-
> memeory. The stripe-cache of raid456 is the same as raid-controller
> memeory.They also should provide the page-cache for read, rather only for
> write.

So if a portion of the system memory can be battery backed (something like the ADR feature on the Intel Xeon E5 series), would it be possible for us to put the stripe cache in that battery protected region and make it a protected write back cache to improve performance and provide data protection for software RAID on Linux?