Re: [PATCH 3/3] raid5: introduce MD_BROKEN

Hi Guoqing,

On Fri, 17 Dec 2021 10:26:27 +0800
Guoqing Jiang [off-list ref] wrote:

quoted

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 1240a5c16af8..8b5561811431 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c

@@ -690,6 +690,9 @@ static int has_failed(struct r5conf *conf)
  {
  	int degraded;
  
+	if (test_bit(MD_BROKEN, &conf->mddev->flags))
+		return 1;
+
  	if (conf->mddev->reshape_position == MaxSector)
  		return conf->mddev->degraded > conf->max_degraded;
  

@@ -2877,34 +2880,29 @@ static void raid5_error(struct mddev

*mddev, struct md_rdev *rdev) unsigned long flags;
  	pr_debug("raid456: error called\n");
  
-	spin_lock_irqsave(&conf->device_lock, flags);
-
-	if (test_bit(In_sync, &rdev->flags) &&
-	    mddev->degraded == conf->max_degraded) {
-		/*
-		 * Don't allow to achieve failed state
-		 * Don't try to recover this device
-		 */
-		conf->recovery_disabled = mddev->recovery_disabled;
-		spin_unlock_irqrestore(&conf->device_lock, flags);
-		return;
-	}
+	pr_crit("md/raid:%s: Disk failure on %s, disabling
device.\n",
+		mdname(mddev), bdevname(rdev->bdev, b));
  
+	spin_lock_irqsave(&conf->device_lock, flags);
  	set_bit(Faulty, &rdev->flags);
  	clear_bit(In_sync, &rdev->flags);
  	mddev->degraded = raid5_calc_degraded(conf);
+
+	if (has_failed(conf)) {
+		set_bit(MD_BROKEN, &mddev->flags);  

What about other callers of has_failed? Do they need to set BROKEN
flag? Or set the flag in has_failed if it returns true, just FYI.

The function checks rdev->state for faulty. There are two, places
where it can be set:
- raid5_error (handled here)
- raid5_spare_active (not a case IMO).

I left it in raid5_error to be consistent with other levels.
I think that moving it t has_failed is safe but I don't see any
additional value in it.
I see that in raid5_error we hold device_lock. It is not true for
all has_failed calls.

Do you have any recommendations?

Thanks,
Mariusz