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

[Guoqing Jiang <guoqing.jiang@linux.dev>]

On 12/17/21 4:37 PM, Mariusz Tkaczyk wrote:
> Hi Guoqing,
>
> On Fri, 17 Dec 2021 10:26:27 +0800
> Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>>> 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.

 From raid5's point of view, it is broken in case has_failed returns 1, 
so it is
kind of inconsistent I think.

> I see that in raid5_error we hold device_lock. It is not true for
> all has_failed calls.

Didn't go detail of each caller, not sure the lock  is needed for set/clear
mddev->flags.

> Do you have any recommendations?

I don't have strong opinion for it.

Thanks,
Guoqing