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

["Jianpeng Ma" <majianpeng@gmail.com>]

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.