Re: [patch] limit error rate

[Neil Brown <neilb@suse.de>]

On Thursday April 24, bernd-schubert@gmx.de 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