[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-raid
Subject:    Re: [patch] limit error rate
From:       Neil Brown <neilb () suse ! de>
Date:       2008-04-28 1:44:59
Message-ID: 18453.11291.797493.538474 () notabene ! brown
[Download RAW message or body]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]