On Thursday April 24, bernd-schubert@xxxxxx wrote:
>
> @@ -1157,19 +1159,20 @@ static void raid5_end_read_request(struct bio * bi, int error)
>
> clear_bit(R5_UPTODATE, &sh->dev[i].flags);
> atomic_inc(&rdev->read_errors);
> - if (conf->mddev->degraded)
> + if (conf->mddev->degraded && printk_ratelimit())
> printk(KERN_WARNING "raid5:%s: read error not correctable (sector %llu on %s).\n",
> mdname(conf->mddev),
> (unsigned long long)(sh->sector + rdev->data_offset),
> bdn);
> - else if (test_bit(R5_ReWrite, &sh->dev[i].flags))
> + else if (test_bit(R5_ReWrite, &sh->dev[i].flags) &&
> + printk_ratelimit())
> /* Oh, no!!! */
> printk(KERN_WARNING "raid5:%s: read error NOT corrected!! (sector %llu on %s).\n",
> mdname(conf->mddev),
> (unsigned long long)(sh->sector + rdev->data_offset),
> bdn);
> else if (atomic_read(&rdev->read_errors)
> - > conf->max_nr_stripes)
> + > conf->max_nr_stripes && printk_ratelimit())
> printk(KERN_WARNING
> "raid5:%s: Too many read errors, failing device %s.\n",
> mdname(conf->mddev), bdn);
This is not good, you are subtly changing semantics.
You have changed:
if (A)
X;
else if (B)
Y;
else if (C)
Z;
else
W;
to
if (A && foo())
X;
else if (B && foo())
Y;
else if (C && foo())
Z;
else
W;
If 'A' and not 'foo()', you want nothing to happen, but you will
actually get one of Y, Z, or W happening, all of which are wrong.
>
> Btw, from my point of view the
>
> if (printk_ratelimit())
> printk("print output");
>
> looks odd. I just don't see why the API isn't
>
> printk_ratelimit("print output");
Very sensible. I've put
#define printk_rl printk_ratelimit() ?: printk
at the start of raid5.c, and used it to fix the problematic printks.
Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
