[PATCH v2 0/3] Use MD_BROKEN for redundant arrays

[PATCH v2 0/3] Use MD_BROKEN for redundant arrays

[Mariusz Tkaczyk]

Hi Song,
As discussed in first version[1], I've done following:
- errors handler for RAID0 and linear added
- raid5 bug described
- MD_BROKEN described
- removed MD_BROKEN check for superblock update.

[1] https://lore.kernel.org/linux-raid/20210917153452.5593-1-mariusz.tkaczyk@linux.intel.com/

Mariusz Tkaczyk (3):
  raid0, linear, md: add error_handlers for raid0 and linear
  md: Set MD_BROKEN for RAID1 and RAID10
  raid5: introduce MD_BROKEN

 drivers/md/md-linear.c | 15 ++++++++++++++-
 drivers/md/md.c        | 23 +++++++++++++++--------
 drivers/md/md.h        | 14 ++++----------
 drivers/md/raid0.c     | 15 ++++++++++++++-
 drivers/md/raid1.c     |  1 +
 drivers/md/raid10.c    |  1 +
 drivers/md/raid5.c     | 34 ++++++++++++++++------------------
 7 files changed, 65 insertions(+), 38 deletions(-)

-- 
2.26.2

[PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Mariusz Tkaczyk]

Patch 62f7b1989c0 ("md raid0/linear: Mark array as 'broken' and fail BIOs
if a member is gone") allowed to finish writes earlier (before level
dependent actions) for non-redundant arrays.

To achieve that MD_BROKEN is added to mddev->flags if drive disappearance
is detected. This is done in is_mddev_broken() which is confusing and not
consistent with other levels where error_handler() is used.
This patch adds appropriate error_handler for raid0 and linear.

It also adopts md_error(), we only want to call .error_handler for those
levels. mddev->pers->sync_request is additionally checked, its existence
implies a level with redundancy.

Usage of error_handler causes that disk failure can be requested from
userspace. User can fail the array via #mdadm --set-faulty command. This
is not safe and will be fixed in mdadm. It is correctable because failed
state is not recorded in the metadata. After next assembly array will be
read-write again. For safety reason is better to keep MD_BROKEN in
runtime only.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 drivers/md/md-linear.c | 15 ++++++++++++++-
 drivers/md/md.c        |  6 +++++-
 drivers/md/md.h        | 10 ++--------
 drivers/md/raid0.c     | 15 ++++++++++++++-
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
index 1ff51647a682..415d2615d17a 100644
--- a/drivers/md/md-linear.c
+++ b/drivers/md/md-linear.c
@@ -233,7 +233,8 @@ static bool linear_make_request(struct mddev *mddev, struct bio *bio)
 		     bio_sector < start_sector))
 		goto out_of_bounds;
 
-	if (unlikely(is_mddev_broken(tmp_dev->rdev, "linear"))) {
+	if (unlikely(is_rdev_broken(tmp_dev->rdev))) {
+		md_error(mddev, tmp_dev->rdev);
 		bio_io_error(bio);
 		return true;
 	}
@@ -281,6 +282,17 @@ static void linear_status (struct seq_file *seq, struct mddev *mddev)
 	seq_printf(seq, " %dk rounding", mddev->chunk_sectors / 2);
 }
 
+static void linear_error(struct mddev *mddev, struct md_rdev *rdev)
+{
+	char b[BDEVNAME_SIZE];
+
+	if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
+		pr_crit("md/linear%s: Disk failure on %s detected.\n"
+			"md/linear:%s: Cannot continue, failing array.\n",
+			mdname(mddev), bdevname(rdev->bdev, b),
+			mdname(mddev));
+}
+
 static void linear_quiesce(struct mddev *mddev, int state)
 {
 }
@@ -297,6 +309,7 @@ static struct md_personality linear_personality =
 	.hot_add_disk	= linear_add,
 	.size		= linear_size,
 	.quiesce	= linear_quiesce,
+	.error_handler	= linear_error,
 };
 
 static int __init linear_init (void)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index e8666bdc0d28..f888ef197765 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7982,7 +7982,11 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
 
 	if (!mddev->pers || !mddev->pers->error_handler)
 		return;
-	mddev->pers->error_handler(mddev,rdev);
+	mddev->pers->error_handler(mddev, rdev);
+
+	if (!mddev->pers->sync_request)
+		return;
+
 	if (mddev->degraded)
 		set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
 	sysfs_notify_dirent_safe(rdev->sysfs_state);
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 53ea7a6961de..bc3f2094d0b6 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -762,15 +762,9 @@ extern void mddev_destroy_serial_pool(struct mddev *mddev, struct md_rdev *rdev,
 struct md_rdev *md_find_rdev_nr_rcu(struct mddev *mddev, int nr);
 struct md_rdev *md_find_rdev_rcu(struct mddev *mddev, dev_t dev);
 
-static inline bool is_mddev_broken(struct md_rdev *rdev, const char *md_type)
+static inline bool is_rdev_broken(struct md_rdev *rdev)
 {
-	if (!disk_live(rdev->bdev->bd_disk)) {
-		if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
-			pr_warn("md: %s: %s array has a missing/failed member\n",
-				mdname(rdev->mddev), md_type);
-		return true;
-	}
-	return false;
+	return !disk_live(rdev->bdev->bd_disk);
 }
 
 static inline void rdev_dec_pending(struct md_rdev *rdev, struct mddev *mddev)
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index 62c8b6adac70..cb9edd65d8a9 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -564,8 +564,9 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
 		return true;
 	}
 
-	if (unlikely(is_mddev_broken(tmp_dev, "raid0"))) {
+	if (unlikely(is_rdev_broken(tmp_dev))) {
 		bio_io_error(bio);
+		md_error(mddev, tmp_dev);
 		return true;
 	}
 
@@ -588,6 +589,17 @@ static void raid0_status(struct seq_file *seq, struct mddev *mddev)
 	return;
 }
 
+static void raid0_error(struct mddev *mddev, struct md_rdev *rdev)
+{
+	char b[BDEVNAME_SIZE];
+
+	if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
+		pr_crit("md/raid0%s: Disk failure on %s detected.\n"
+			"md/raid0:%s: Cannot continue, failing array.\n",
+			mdname(mddev), bdevname(rdev->bdev, b),
+			mdname(mddev));
+}
+
 static void *raid0_takeover_raid45(struct mddev *mddev)
 {
 	struct md_rdev *rdev;
@@ -763,6 +775,7 @@ static struct md_personality raid0_personality=
 	.size		= raid0_size,
 	.takeover	= raid0_takeover,
 	.quiesce	= raid0_quiesce,
+	.error_handler	= raid0_error,
 };
 
 static int __init raid0_init (void)
-- 
2.26.2

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Guoqing Jiang]

On 12/16/21 10:52 PM, Mariusz Tkaczyk wrote:
> Patch 62f7b1989c0 ("md raid0/linear: Mark array as 'broken' and fail BIOs
> if a member is gone") allowed to finish writes earlier (before level
> dependent actions) for non-redundant arrays.
>
> To achieve that MD_BROKEN is added to mddev->flags if drive disappearance
> is detected. This is done in is_mddev_broken() which is confusing and not
> consistent with other levels where error_handler() is used.
> This patch adds appropriate error_handler for raid0 and linear.
>
> It also adopts md_error(), we only want to call .error_handler for those
> levels. mddev->pers->sync_request is additionally checked, its existence
> implies a level with redundancy.
>
> Usage of error_handler causes that disk failure can be requested from
> userspace. User can fail the array via #mdadm --set-faulty command. This
> is not safe and will be fixed in mdadm. It is correctable because failed
> state is not recorded in the metadata. After next assembly array will be
> read-write again. For safety reason is better to keep MD_BROKEN in
> runtime only.
>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>   drivers/md/md-linear.c | 15 ++++++++++++++-
>   drivers/md/md.c        |  6 +++++-
>   drivers/md/md.h        | 10 ++--------
>   drivers/md/raid0.c     | 15 ++++++++++++++-
>   4 files changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
> index 1ff51647a682..415d2615d17a 100644
> --- a/drivers/md/md-linear.c
> +++ b/drivers/md/md-linear.c
> @@ -233,7 +233,8 @@ static bool linear_make_request(struct mddev *mddev, struct bio *bio)
>   		     bio_sector < start_sector))
>   		goto out_of_bounds;
>   
> -	if (unlikely(is_mddev_broken(tmp_dev->rdev, "linear"))) {
> +	if (unlikely(is_rdev_broken(tmp_dev->rdev))) {
> +		md_error(mddev, tmp_dev->rdev);
>   		bio_io_error(bio);
>   		return true;
>   	}
> @@ -281,6 +282,17 @@ static void linear_status (struct seq_file *seq, struct mddev *mddev)
>   	seq_printf(seq, " %dk rounding", mddev->chunk_sectors / 2);
>   }
>   
> +static void linear_error(struct mddev *mddev, struct md_rdev *rdev)
> +{
> +	char b[BDEVNAME_SIZE];
> +
> +	if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
> +		pr_crit("md/linear%s: Disk failure on %s detected.\n"
> +			"md/linear:%s: Cannot continue, failing array.\n",
> +			mdname(mddev), bdevname(rdev->bdev, b),
> +			mdname(mddev));
> +}
> +

Do you consider to use %pg to print block device?

>   static void linear_quiesce(struct mddev *mddev, int state)
>   {
>   }
> @@ -297,6 +309,7 @@ static struct md_personality linear_personality =
>   	.hot_add_disk	= linear_add,
>   	.size		= linear_size,
>   	.quiesce	= linear_quiesce,
> +	.error_handler	= linear_error,
>   };
>   
>   static int __init linear_init (void)
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index e8666bdc0d28..f888ef197765 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7982,7 +7982,11 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
>   
>   	if (!mddev->pers || !mddev->pers->error_handler)
>   		return;
> -	mddev->pers->error_handler(mddev,rdev);
> +	mddev->pers->error_handler(mddev, rdev);
> +
> +	if (!mddev->pers->sync_request)
> +		return;
> +

What is the reason of the above change? I suppose dm event can be missed.

>   	if (mddev->degraded)
>   		set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
>   	sysfs_notify_dirent_safe(rdev->sysfs_state);
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 53ea7a6961de..bc3f2094d0b6 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -762,15 +762,9 @@ extern void mddev_destroy_serial_pool(struct mddev *mddev, struct md_rdev *rdev,
>   struct md_rdev *md_find_rdev_nr_rcu(struct mddev *mddev, int nr);
>   struct md_rdev *md_find_rdev_rcu(struct mddev *mddev, dev_t dev);
>   
> -static inline bool is_mddev_broken(struct md_rdev *rdev, const char *md_type)
> +static inline bool is_rdev_broken(struct md_rdev *rdev)
>   {
> -	if (!disk_live(rdev->bdev->bd_disk)) {
> -		if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
> -			pr_warn("md: %s: %s array has a missing/failed member\n",
> -				mdname(rdev->mddev), md_type);
> -		return true;
> -	}
> -	return false;
> +	return !disk_live(rdev->bdev->bd_disk);
>   }

I would suggest to only rename the function, at least md_type info is 
useful.
Besides, if MD_BROKEN is not set here then I think you can also delete the
flag from other places as well.

>    static inline void rdev_dec_pending(struct md_rdev *rdev, struct mddev *mddev)
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index 62c8b6adac70..cb9edd65d8a9 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -564,8 +564,9 @@ static bool raid0_make_request(struct mddev *mddev, struct bio *bio)
>   		return true;
>   	}
>   
> -	if (unlikely(is_mddev_broken(tmp_dev, "raid0"))) {
> +	if (unlikely(is_rdev_broken(tmp_dev))) {
>   		bio_io_error(bio);
> +		md_error(mddev, tmp_dev);
>   		return true;
>   	}
>   
> @@ -588,6 +589,17 @@ static void raid0_status(struct seq_file *seq, struct mddev *mddev)
>   	return;
>   }
>   
> +static void raid0_error(struct mddev *mddev, struct md_rdev *rdev)
> +{
> +	char b[BDEVNAME_SIZE];
> +
> +	if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
> +		pr_crit("md/raid0%s: Disk failure on %s detected.\n"
> +			"md/raid0:%s: Cannot continue, failing array.\n",
> +			mdname(mddev), bdevname(rdev->bdev, b),
> +			mdname(mddev));
> +}
> +
>   static void *raid0_takeover_raid45(struct mddev *mddev)
>   {
>   	struct md_rdev *rdev;
> @@ -763,6 +775,7 @@ static struct md_personality raid0_personality=
>   	.size		= raid0_size,
>   	.takeover	= raid0_takeover,
>   	.quiesce	= raid0_quiesce,
> +	.error_handler	= raid0_error,
>   };
>   

What is the advantage of adding error_handler for raid0 and linear? IOW,
without implement the error_handler, is there some existing issue?

Thanks,
Guoqing

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Guoqing Jiang]

On 12/17/21 10:00 AM, Guoqing Jiang wrote:
>> +static inline bool is_rdev_broken(struct md_rdev *rdev)
>>   {
>> -    if (!disk_live(rdev->bdev->bd_disk)) {
>> -        if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
>> -            pr_warn("md: %s: %s array has a missing/failed member\n",
>> -                mdname(rdev->mddev), md_type);
>> -        return true;
>> -    }
>> -    return false;
>> +    return !disk_live(rdev->bdev->bd_disk);
>>   }
>
>
> Besides, if MD_BROKEN is not set here then I think you can also delete 
> the
> flag from other places as well. 

Oops, I didn't notice the flag is set elsewhere.

Thanks,
Guoqing

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Xiao Ni]

On Fri, Dec 17, 2021 at 10:00 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>
>
> On 12/16/21 10:52 PM, Mariusz Tkaczyk wrote:
> > Patch 62f7b1989c0 ("md raid0/linear: Mark array as 'broken' and fail BIOs
> > if a member is gone") allowed to finish writes earlier (before level
> > dependent actions) for non-redundant arrays.
> >
> > To achieve that MD_BROKEN is added to mddev->flags if drive disappearance
> > is detected. This is done in is_mddev_broken() which is confusing and not
> > consistent with other levels where error_handler() is used.
> > This patch adds appropriate error_handler for raid0 and linear.
> >
> > It also adopts md_error(), we only want to call .error_handler for those
> > levels. mddev->pers->sync_request is additionally checked, its existence
> > implies a level with redundancy.
> >
> > Usage of error_handler causes that disk failure can be requested from
> > userspace. User can fail the array via #mdadm --set-faulty command. This
> > is not safe and will be fixed in mdadm. It is correctable because failed
> > state is not recorded in the metadata. After next assembly array will be
> > read-write again. For safety reason is better to keep MD_BROKEN in
> > runtime only.
> >
> > Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> > ---
> >   drivers/md/md-linear.c | 15 ++++++++++++++-
> >   drivers/md/md.c        |  6 +++++-
> >   drivers/md/md.h        | 10 ++--------
> >   drivers/md/raid0.c     | 15 ++++++++++++++-
> >   4 files changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
> > index 1ff51647a682..415d2615d17a 100644
> > --- a/drivers/md/md-linear.c
> > +++ b/drivers/md/md-linear.c
> > @@ -233,7 +233,8 @@ static bool linear_make_request(struct mddev *mddev, struct bio *bio)
> >                    bio_sector < start_sector))
> >               goto out_of_bounds;
> >
> > -     if (unlikely(is_mddev_broken(tmp_dev->rdev, "linear"))) {
> > +     if (unlikely(is_rdev_broken(tmp_dev->rdev))) {
> > +             md_error(mddev, tmp_dev->rdev);
> >               bio_io_error(bio);
> >               return true;
> >       }
> > @@ -281,6 +282,17 @@ static void linear_status (struct seq_file *seq, struct mddev *mddev)
> >       seq_printf(seq, " %dk rounding", mddev->chunk_sectors / 2);
> >   }
> >
> > +static void linear_error(struct mddev *mddev, struct md_rdev *rdev)
> > +{
> > +     char b[BDEVNAME_SIZE];
> > +
> > +     if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
> > +             pr_crit("md/linear%s: Disk failure on %s detected.\n"
> > +                     "md/linear:%s: Cannot continue, failing array.\n",
> > +                     mdname(mddev), bdevname(rdev->bdev, b),
> > +                     mdname(mddev));
> > +}
> > +
>
> Do you consider to use %pg to print block device?
>
> >   static void linear_quiesce(struct mddev *mddev, int state)
> >   {
> >   }
> > @@ -297,6 +309,7 @@ static struct md_personality linear_personality =
> >       .hot_add_disk   = linear_add,
> >       .size           = linear_size,
> >       .quiesce        = linear_quiesce,
> > +     .error_handler  = linear_error,
> >   };
> >
> >   static int __init linear_init (void)
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index e8666bdc0d28..f888ef197765 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -7982,7 +7982,11 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
> >
> >       if (!mddev->pers || !mddev->pers->error_handler)
> >               return;
> > -     mddev->pers->error_handler(mddev,rdev);
> > +     mddev->pers->error_handler(mddev, rdev);
> > +
> > +     if (!mddev->pers->sync_request)
> > +             return;
> > +
>
> What is the reason of the above change? I suppose dm event can be missed.

Hi Guoqing

What's the dm event here? From my understanding, this patch only wants
to set MD_BROKEN
in error handler. Now raid0 and linear only need to do this job in
md_error. It checks ->sync_request
here and can return for raid0 and linear. In the error handler raid0
and linear already set MD_BROKEN.

Best Regards
Xiao

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Guoqing Jiang]

On 12/19/21 11:26 AM, Xiao Ni wrote:
> On Fri, Dec 17, 2021 at 10:00 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>>
>>
>> On 12/16/21 10:52 PM, Mariusz Tkaczyk wrote:
>>> Patch 62f7b1989c0 ("md raid0/linear: Mark array as 'broken' and fail BIOs
>>> if a member is gone") allowed to finish writes earlier (before level
>>> dependent actions) for non-redundant arrays.
>>>
>>> To achieve that MD_BROKEN is added to mddev->flags if drive disappearance
>>> is detected. This is done in is_mddev_broken() which is confusing and not
>>> consistent with other levels where error_handler() is used.
>>> This patch adds appropriate error_handler for raid0 and linear.
>>>
>>> It also adopts md_error(), we only want to call .error_handler for those
>>> levels. mddev->pers->sync_request is additionally checked, its existence
>>> implies a level with redundancy.
>>>
>>> Usage of error_handler causes that disk failure can be requested from
>>> userspace. User can fail the array via #mdadm --set-faulty command. This
>>> is not safe and will be fixed in mdadm. It is correctable because failed
>>> state is not recorded in the metadata. After next assembly array will be
>>> read-write again. For safety reason is better to keep MD_BROKEN in
>>> runtime only.
>>>
>>> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
>>> ---
>>>    drivers/md/md-linear.c | 15 ++++++++++++++-
>>>    drivers/md/md.c        |  6 +++++-
>>>    drivers/md/md.h        | 10 ++--------
>>>    drivers/md/raid0.c     | 15 ++++++++++++++-
>>>    4 files changed, 35 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/md/md-linear.c b/drivers/md/md-linear.c
>>> index 1ff51647a682..415d2615d17a 100644
>>> --- a/drivers/md/md-linear.c
>>> +++ b/drivers/md/md-linear.c
>>> @@ -233,7 +233,8 @@ static bool linear_make_request(struct mddev *mddev, struct bio *bio)
>>>                     bio_sector < start_sector))
>>>                goto out_of_bounds;
>>>
>>> -     if (unlikely(is_mddev_broken(tmp_dev->rdev, "linear"))) {
>>> +     if (unlikely(is_rdev_broken(tmp_dev->rdev))) {
>>> +             md_error(mddev, tmp_dev->rdev);
>>>                bio_io_error(bio);
>>>                return true;
>>>        }
>>> @@ -281,6 +282,17 @@ static void linear_status (struct seq_file *seq, struct mddev *mddev)
>>>        seq_printf(seq, " %dk rounding", mddev->chunk_sectors / 2);
>>>    }
>>>
>>> +static void linear_error(struct mddev *mddev, struct md_rdev *rdev)
>>> +{
>>> +     char b[BDEVNAME_SIZE];
>>> +
>>> +     if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
>>> +             pr_crit("md/linear%s: Disk failure on %s detected.\n"
>>> +                     "md/linear:%s: Cannot continue, failing array.\n",
>>> +                     mdname(mddev), bdevname(rdev->bdev, b),
>>> +                     mdname(mddev));
>>> +}
>>> +
>> Do you consider to use %pg to print block device?
>>
>>>    static void linear_quiesce(struct mddev *mddev, int state)
>>>    {
>>>    }
>>> @@ -297,6 +309,7 @@ static struct md_personality linear_personality =
>>>        .hot_add_disk   = linear_add,
>>>        .size           = linear_size,
>>>        .quiesce        = linear_quiesce,
>>> +     .error_handler  = linear_error,
>>>    };
>>>
>>>    static int __init linear_init (void)
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index e8666bdc0d28..f888ef197765 100644
>>> --- a/drivers/md/md.c
>>> +++ b/drivers/md/md.c
>>> @@ -7982,7 +7982,11 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
>>>
>>>        if (!mddev->pers || !mddev->pers->error_handler)
>>>                return;
>>> -     mddev->pers->error_handler(mddev,rdev);
>>> +     mddev->pers->error_handler(mddev, rdev);
>>> +
>>> +     if (!mddev->pers->sync_request)
>>> +             return;
>>> +
>> What is the reason of the above change? I suppose dm event can be missed.
> Hi Guoqing
>
> What's the dm event here?

Pls see mddev->event_work after above line, also commit 768e587e18 for dm
which might be relevant.

Thanks,
Guoqing

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Mariusz Tkaczyk]

Hi Guoqing,
On Fri, 17 Dec 2021 10:00:25 +0800
Guoqing Jiang <guoqing.jiang@linux.dev> wrote:

> > @@ -281,6 +282,17 @@ static void linear_status (struct seq_file
> > *seq, struct mddev *mddev) seq_printf(seq, " %dk rounding",
> > mddev->chunk_sectors / 2); }
> >   
> > +static void linear_error(struct mddev *mddev, struct md_rdev *rdev)
> > +{
> > +	char b[BDEVNAME_SIZE];
> > +
> > +	if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
> > +		pr_crit("md/linear%s: Disk failure on %s
> > detected.\n"
> > +			"md/linear:%s: Cannot continue, failing
> > array.\n",
> > +			mdname(mddev), bdevname(rdev->bdev, b),
> > +			mdname(mddev));
> > +}
> > +  
> 
> Do you consider to use %pg to print block device?
 
Will do.
 
> > @@ -588,6 +589,17 @@ static void raid0_status(struct seq_file *seq,
> > struct mddev *mddev) return;
> >   }
> >   
> > +static void raid0_error(struct mddev *mddev, struct md_rdev *rdev)
> > +{
> > +	char b[BDEVNAME_SIZE];
> > +
> > +	if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
> > +		pr_crit("md/raid0%s: Disk failure on %s
> > detected.\n"
> > +			"md/raid0:%s: Cannot continue, failing
> > array.\n",
> > +			mdname(mddev), bdevname(rdev->bdev, b),
> > +			mdname(mddev));
> > +}
> > +
> >   static void *raid0_takeover_raid45(struct mddev *mddev)
> >   {
> >   	struct md_rdev *rdev;
> > @@ -763,6 +775,7 @@ static struct md_personality raid0_personality=
> >   	.size		= raid0_size,
> >   	.takeover	= raid0_takeover,
> >   	.quiesce	= raid0_quiesce,
> > +	.error_handler	= raid0_error,
> >   };
> >     
> 
> What is the advantage of adding error_handler for raid0 and linear?
> IOW, without implement the error_handler, is there some existing
> issue?
> 
There is no issue. It was suggested by Song:
https://lore.kernel.org/linux-raid/CAPhsuW4X94eJ8aG6i7F0zCmgjuWHSRWuBH2gOJjTe5uWg_rMvQ@mail.gmail.com/

Thanks,
Mariusz

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Xiao Ni]

On Thu, Dec 16, 2021 at 10:53 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Patch 62f7b1989c0 ("md raid0/linear: Mark array as 'broken' and fail BIOs
> if a member is gone") allowed to finish writes earlier (before level
> dependent actions) for non-redundant arrays.
>
> To achieve that MD_BROKEN is added to mddev->flags if drive disappearance
> is detected. This is done in is_mddev_broken() which is confusing and not
> consistent with other levels where error_handler() is used.
> This patch adds appropriate error_handler for raid0 and linear.
>
> It also adopts md_error(), we only want to call .error_handler for those
> levels. mddev->pers->sync_request is additionally checked, its existence
> implies a level with redundancy.
>
> Usage of error_handler causes that disk failure can be requested from
> userspace. User can fail the array via #mdadm --set-faulty command. This
> is not safe and will be fixed in mdadm. It is correctable because failed
> state is not recorded in the metadata. After next assembly array will be
> read-write again. For safety reason is better to keep MD_BROKEN in
> runtime only.

Hi Mariusz

Let me call them chapter[1-4]

Could you explain more about 'mdadm --set-faulty' part? I've read this
patch. But I don't
know the relationship between the patch and chapter4.

In patch2, you write "As in previous commit, it causes that #mdadm
--set-faulty is able to
mark array as failed." I tried to run command `mdadm /dev/md0 -f
/dev/sda`. md0 is a raid0.
It can't remove sda from md0.

Best Regards
Xiao

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Mariusz Tkaczyk]

Hi Xiao,

On Sun, 19 Dec 2021 11:20:59 +0800
Xiao Ni <xni@redhat.com> wrote:

> > Usage of error_handler causes that disk failure can be requested
> > from userspace. User can fail the array via #mdadm --set-faulty
> > command. This is not safe and will be fixed in mdadm. It is
> > correctable because failed state is not recorded in the metadata.
> > After next assembly array will be read-write again. For safety
> > reason is better to keep MD_BROKEN in runtime only.  
> 
> Hi Mariusz
> 
> Let me call them chapter[1-4]
> 
> Could you explain more about 'mdadm --set-faulty' part? I've read this
> patch. But I don't
> know the relationship between the patch and chapter4.
> 
> In patch2, you write "As in previous commit, it causes that #mdadm
> --set-faulty is able to
> mark array as failed." I tried to run command `mdadm /dev/md0 -f
> /dev/sda`. md0 is a raid0.
> It can't remove sda from md0.

Did you test kernel with my patchset applied?

I've added chapter 4 because I'm aware of behavior change.
Now for r0, nothing happens when we are trying to write failure to
md/<disk>/state.
 
After the change, drive is not remove too, but MD_BROKEN is set and
any new write will be rejected. The drive will be still visible
in array (I didn't change that). Should I add it to description?

Thanks,
Mariusz

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Xiao Ni]

On Mon, Dec 20, 2021 at 4:45 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Hi Xiao,
>
> On Sun, 19 Dec 2021 11:20:59 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > > Usage of error_handler causes that disk failure can be requested
> > > from userspace. User can fail the array via #mdadm --set-faulty
> > > command. This is not safe and will be fixed in mdadm. It is
> > > correctable because failed state is not recorded in the metadata.
> > > After next assembly array will be read-write again. For safety
> > > reason is better to keep MD_BROKEN in runtime only.
> >
> > Hi Mariusz
> >
> > Let me call them chapter[1-4]
> >
> > Could you explain more about 'mdadm --set-faulty' part? I've read this
> > patch. But I don't
> > know the relationship between the patch and chapter4.
> >
> > In patch2, you write "As in previous commit, it causes that #mdadm
> > --set-faulty is able to
> > mark array as failed." I tried to run command `mdadm /dev/md0 -f
> > /dev/sda`. md0 is a raid0.
> > It can't remove sda from md0.
>
> Did you test kernel with my patchset applied?
>
> I've added chapter 4 because I'm aware of behavior change.
> Now for r0, nothing happens when we are trying to write failure to
> md/<disk>/state.
>
> After the change, drive is not remove too, but MD_BROKEN is set and
> any new write will be rejected. The drive will be still visible
> in array (I didn't change that). Should I add it to description?

Thanks for the explanation. I understand now. But I still have one question.

Now for a raid0, it can't remove one member disk from raid0. It returns
EBUSY and the raid0 still can work well. It makes sense. Because all member
disks are busy, the admin can't remove the member disk and mdadm gives a
proper error.

With this patch, it changes the situation. In raid0_error, it sets MD_BROKEN.
In fact, it isn't broken. So is it really good to set MD_BROKEN here? In patch
62f7b1989c0 ("md raid0/linear: Mark array as 'broken'...), MD_BROKEN
is introduced
when the member disk disappears and the disk is really broken. For raid0/linear,
the raid device can't work anymore.

Best Regards
Xiao

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Mariusz Tkaczyk]

Hi Xiao,
On Tue, 21 Dec 2021 09:40:50 +0800
Xiao Ni <xni@redhat.com> wrote:

> Now for a raid0, it can't remove one member disk from raid0. It
> returns EBUSY and the raid0 still can work well. It makes sense.
> Because all member disks are busy, the admin can't remove the member
> disk and mdadm gives a proper error.

EBUSY means that drive is busy but it is not. Just drive cannot be
safety removed. As I wrote in patch 2:

If "faulty" was not set then -EBUSY was returned to
userspace. It causes that mdadm expects -EBUSY if the array
becomes failed. There are some reasons to not consider this mechanism
as correct:
- drive can't be failed for different reasons.
- there are path where -EBUSY is not reported and drive removal leads
to failed array, without notification for userspace.
- in the array failure case -EBUSY seems to be wrong status. Array is
not busy, but removal process cannot proceed safe.

For compatibility reasons i cannot remove EBUSY. I left more detailed
explanation in patch 2.

> With this patch, it changes the situation. In raid0_error, it sets
> MD_BROKEN. In fact, it isn't broken. So is it really good to set
> MD_BROKEN here? In patch 62f7b1989c0 ("md raid0/linear: Mark array as
> 'broken'...), MD_BROKEN is introduced
> when the member disk disappears and the disk is really broken. For
> raid0/linear, the raid device can't work anymore.

It is broken, any md_error() call should end with appropriate action:
- drive removal (if possible)
- failing array (if cannot degrade array)

We cannot trust drive if md_error() was called, so writes will be
blocked. IMO it is reasonable- to not engage level stack, because one
or more members cannot be trusted (even if it is still accessible). Just
allow to read old data (if still possible).
 
Thanks,
Mariusz

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Guoqing Jiang]

On 12/21/21 9:56 PM, Mariusz Tkaczyk wrote:
> Hi Xiao,
> On Tue, 21 Dec 2021 09:40:50 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
>> Now for a raid0, it can't remove one member disk from raid0. It
>> returns EBUSY and the raid0 still can work well. It makes sense.
>> Because all member disks are busy, the admin can't remove the member
>> disk and mdadm gives a proper error.
> EBUSY means that drive is busy but it is not. Just drive cannot be
> safety removed. As I wrote in patch 2:
>
> If "faulty" was not set then -EBUSY was returned to
> userspace. It causes that mdadm expects -EBUSY if the array
> becomes failed. There are some reasons to not consider this mechanism
> as correct:
> - drive can't be failed for different reasons.
> - there are path where -EBUSY is not reported and drive removal leads
> to failed array, without notification for userspace.
> - in the array failure case -EBUSY seems to be wrong status. Array is
> not busy, but removal process cannot proceed safe.
>
> For compatibility reasons i cannot remove EBUSY. I left more detailed
> explanation in patch 2.
>
>> With this patch, it changes the situation. In raid0_error, it sets
>> MD_BROKEN. In fact, it isn't broken. So is it really good to set
>> MD_BROKEN here? In patch 62f7b1989c0 ("md raid0/linear: Mark array as
>> 'broken'...), MD_BROKEN is introduced
>> when the member disk disappears and the disk is really broken. For
>> raid0/linear, the raid device can't work anymore.
> It is broken, any md_error() call should end with appropriate action:
> - drive removal (if possible)
> - failing array (if cannot degrade array)
>
> We cannot trust drive if md_error() was called, so writes will be
> blocked. IMO it is reasonable- to not engage level stack, because one
> or more members cannot be trusted (even if it is still accessible). Just
> allow to read old data (if still possible).

Agree, especially for  raid0 and linear, the array is literally broken 
in case one
of member device can't be accessed.

Thanks,
Guoqing

Re: [PATCH 1/3] raid0, linear, md: add error_handlers for raid0 and linear

[Xiao Ni]

On Tue, Dec 21, 2021 at 9:56 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Hi Xiao,
> On Tue, 21 Dec 2021 09:40:50 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > Now for a raid0, it can't remove one member disk from raid0. It
> > returns EBUSY and the raid0 still can work well. It makes sense.
> > Because all member disks are busy, the admin can't remove the member
> > disk and mdadm gives a proper error.
>
> EBUSY means that drive is busy but it is not. Just drive cannot be
> safety removed. As I wrote in patch 2:
>
> If "faulty" was not set then -EBUSY was returned to
> userspace. It causes that mdadm expects -EBUSY if the array
> becomes failed. There are some reasons to not consider this mechanism
> as correct:
> - drive can't be failed for different reasons.
> - there are path where -EBUSY is not reported and drive removal leads
> to failed array, without notification for userspace.
> - in the array failure case -EBUSY seems to be wrong status. Array is
> not busy, but removal process cannot proceed safe.
>
> For compatibility reasons i cannot remove EBUSY. I left more detailed
> explanation in patch 2.
>
> > With this patch, it changes the situation. In raid0_error, it sets
> > MD_BROKEN. In fact, it isn't broken. So is it really good to set
> > MD_BROKEN here? In patch 62f7b1989c0 ("md raid0/linear: Mark array as
> > 'broken'...), MD_BROKEN is introduced
> > when the member disk disappears and the disk is really broken. For
> > raid0/linear, the raid device can't work anymore.
>
> It is broken, any md_error() call should end with appropriate action:
> - drive removal (if possible)
> - failing array (if cannot degrade array)
>
> We cannot trust drive if md_error() was called, so writes will be
> blocked. IMO it is reasonable- to not engage level stack, because one
> or more members cannot be trusted (even if it is still accessible). Just
> allow to read old data (if still possible).
>

Thanks for the explanation. It's ok for me. Just want to have the same
understanding. Now raid0 works
in the way that it will fail when calling md_error.

Regards
Xiao

[PATCH 2/3] md: Set MD_BROKEN for RAID1 and RAID10

[Mariusz Tkaczyk]

There was no direct mechanism to determine raid failure outside
personality. It was done by checking rdev->flags after executing
md_error(). If "faulty" was not set then -EBUSY was returned to
userspace. It causes that mdadm expects -EBUSY if the array
becomes failed. There are some reasons to not consider this mechanism
as correct:
- drive can't be failed for different reasons.
- there are path where -EBUSY is not reported and drive removal leads
to failed array, without notification for userspace.
- in the array failure case -EBUSY seems to be wrong status. Array is
not busy, but removal process cannot proceed safe.

-EBUSY expectation cannot be removed without breaking compatibility
with userspace, but we can adopt the failed state verification method.

In this patch MD_BROKEN flag support, used to mark non-redundant array
as dead, is added to RAID1 and RAID10. Support for RAID456 is added in
next commit.

Now the array failure can be checked, so verify MD_BROKEN flag, however
still return -EBUSY to userspace.

As in previous commit, it causes that #mdadm --set-faulty is able to
mark array as failed. Previously proposed workaround is valid if optional
functionality 9a567843f79("md: allow last device to be forcibly
removed from RAID1/RAID10.") is disabled.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 drivers/md/md.c     | 17 ++++++++++-------
 drivers/md/md.h     |  4 ++--
 drivers/md/raid1.c  |  1 +
 drivers/md/raid10.c |  1 +
 4 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index f888ef197765..fda8473f96b8 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2983,10 +2983,11 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
 
 	if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
 		md_error(rdev->mddev, rdev);
-		if (test_bit(Faulty, &rdev->flags))
-			err = 0;
-		else
+
+		if (test_bit(MD_BROKEN, &rdev->mddev->flags))
 			err = -EBUSY;
+		else
+			err = 0;
 	} else if (cmd_match(buf, "remove")) {
 		if (rdev->mddev->pers) {
 			clear_bit(Blocked, &rdev->flags);
@@ -7441,7 +7442,7 @@ static int set_disk_faulty(struct mddev *mddev, dev_t dev)
 		err =  -ENODEV;
 	else {
 		md_error(mddev, rdev);
-		if (!test_bit(Faulty, &rdev->flags))
+		if (test_bit(MD_BROKEN, &mddev->flags))
 			err = -EBUSY;
 	}
 	rcu_read_unlock();
@@ -7987,12 +7988,14 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
 	if (!mddev->pers->sync_request)
 		return;
 
-	if (mddev->degraded)
+	if (mddev->degraded && !test_bit(MD_BROKEN, &mddev->flags))
 		set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
 	sysfs_notify_dirent_safe(rdev->sysfs_state);
 	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
-	md_wakeup_thread(mddev->thread);
+	if (!test_bit(MD_BROKEN, &mddev->flags)) {
+		set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
+		md_wakeup_thread(mddev->thread);
+	}
 	if (mddev->event_work.func)
 		queue_work(md_misc_wq, &mddev->event_work);
 	md_new_event();
diff --git a/drivers/md/md.h b/drivers/md/md.h
index bc3f2094d0b6..d3a897868695 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -259,8 +259,8 @@ enum mddev_flags {
 	MD_NOT_READY,		/* do_md_run() is active, so 'array_state'
 				 * must not report that array is ready yet
 				 */
-	MD_BROKEN,              /* This is used in RAID-0/LINEAR only, to stop
-				 * I/O in case an array member is gone/failed.
+	MD_BROKEN,              /* This is used to stop I/O and mark device as
+				 * dead in case an array becomes failed.
 				 */
 };
 
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 7dc8026cf6ee..45dc75f90476 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1638,6 +1638,7 @@ static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
 		 */
 		conf->recovery_disabled = mddev->recovery_disabled;
 		spin_unlock_irqrestore(&conf->device_lock, flags);
+		set_bit(MD_BROKEN, &mddev->flags);
 		return;
 	}
 	set_bit(Blocked, &rdev->flags);
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index dde98f65bd04..d7cefd212e6b 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1964,6 +1964,7 @@ static void raid10_error(struct mddev *mddev, struct md_rdev *rdev)
 		 * Don't fail the drive, just return an IO error.
 		 */
 		spin_unlock_irqrestore(&conf->device_lock, flags);
+		set_bit(MD_BROKEN, &mddev->flags);
 		return;
 	}
 	if (test_and_clear_bit(In_sync, &rdev->flags))
-- 
2.26.2

Re: [PATCH 2/3] md: Set MD_BROKEN for RAID1 and RAID10

[Guoqing Jiang]

On 12/16/21 10:52 PM, Mariusz Tkaczyk wrote:
> There was no direct mechanism to determine raid failure outside
> personality. It was done by checking rdev->flags after executing
> md_error(). If "faulty" was not set then -EBUSY was returned to
> userspace. It causes that mdadm expects -EBUSY if the array
> becomes failed. There are some reasons to not consider this mechanism
> as correct:
> - drive can't be failed for different reasons.
> - there are path where -EBUSY is not reported and drive removal leads
> to failed array, without notification for userspace.
> - in the array failure case -EBUSY seems to be wrong status. Array is
> not busy, but removal process cannot proceed safe.
>
> -EBUSY expectation cannot be removed without breaking compatibility
> with userspace, but we can adopt the failed state verification method.
>
> In this patch MD_BROKEN flag support, used to mark non-redundant array
> as dead, is added to RAID1 and RAID10. Support for RAID456 is added in
> next commit.
>
> Now the array failure can be checked, so verify MD_BROKEN flag, however
> still return -EBUSY to userspace.
>
> As in previous commit, it causes that #mdadm --set-faulty is able to
> mark array as failed. Previously proposed workaround is valid if optional
> functionality 9a567843f79("md: allow last device to be forcibly

s/9a567843f79/9a567843f7ce/

> removed from RAID1/RAID10.") is disabled.
>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>   drivers/md/md.c     | 17 ++++++++++-------
>   drivers/md/md.h     |  4 ++--
>   drivers/md/raid1.c  |  1 +
>   drivers/md/raid10.c |  1 +
>   4 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f888ef197765..fda8473f96b8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2983,10 +2983,11 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
>   
>   	if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
>   		md_error(rdev->mddev, rdev);
> -		if (test_bit(Faulty, &rdev->flags))
> -			err = 0;
> -		else
> +
> +		if (test_bit(MD_BROKEN, &rdev->mddev->flags))
>   			err = -EBUSY;
> +		else
> +			err = 0;
>   	} else if (cmd_match(buf, "remove")) {
>   		if (rdev->mddev->pers) {
>   			clear_bit(Blocked, &rdev->flags);
> @@ -7441,7 +7442,7 @@ static int set_disk_faulty(struct mddev *mddev, dev_t dev)
>   		err =  -ENODEV;
>   	else {
>   		md_error(mddev, rdev);
> -		if (!test_bit(Faulty, &rdev->flags))
> +		if (test_bit(MD_BROKEN, &mddev->flags))
>   			err = -EBUSY;
>   	}
>   	rcu_read_unlock();
> @@ -7987,12 +7988,14 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
>   	if (!mddev->pers->sync_request)
>   		return;
>   
> -	if (mddev->degraded)
> +	if (mddev->degraded && !test_bit(MD_BROKEN, &mddev->flags))
>   		set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
>   	sysfs_notify_dirent_safe(rdev->sysfs_state);
>   	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> -	set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> -	md_wakeup_thread(mddev->thread);
> +	if (!test_bit(MD_BROKEN, &mddev->flags)) {
> +		set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> +		md_wakeup_thread(mddev->thread);
> +	}
>   	if (mddev->event_work.func)
>   		queue_work(md_misc_wq, &mddev->event_work);
>   	md_new_event();
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index bc3f2094d0b6..d3a897868695 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -259,8 +259,8 @@ enum mddev_flags {
>   	MD_NOT_READY,		/* do_md_run() is active, so 'array_state'
>   				 * must not report that array is ready yet
>   				 */
> -	MD_BROKEN,              /* This is used in RAID-0/LINEAR only, to stop
> -				 * I/O in case an array member is gone/failed.
> +	MD_BROKEN,              /* This is used to stop I/O and mark device as

IIUC, 'device' actually means array, if so, could you change it to array 
to make it clear?

> +				 * dead in case an array becomes failed.
>   				 */
>   };
>   
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 7dc8026cf6ee..45dc75f90476 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1638,6 +1638,7 @@ static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
>   		 */
>   		conf->recovery_disabled = mddev->recovery_disabled;
>   		spin_unlock_irqrestore(&conf->device_lock, flags);
> +		set_bit(MD_BROKEN, &mddev->flags);
>   		return;
>   	}
>   	set_bit(Blocked, &rdev->flags);
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index dde98f65bd04..d7cefd212e6b 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1964,6 +1964,7 @@ static void raid10_error(struct mddev *mddev, struct md_rdev *rdev)
>   		 * Don't fail the drive, just return an IO error.
>   		 */
>   		spin_unlock_irqrestore(&conf->device_lock, flags);
> +		set_bit(MD_BROKEN, &mddev->flags);
>   		return;
>   	}
>   	if (test_and_clear_bit(In_sync, &rdev->flags))

Thanks,
Guoqing

Re: [PATCH 2/3] md: Set MD_BROKEN for RAID1 and RAID10

[Xiao Ni]

On Thu, Dec 16, 2021 at 10:53 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> There was no direct mechanism to determine raid failure outside
> personality. It was done by checking rdev->flags after executing
> md_error(). If "faulty" was not set then -EBUSY was returned to
> userspace. It causes that mdadm expects -EBUSY if the array
> becomes failed. There are some reasons to not consider this mechanism
> as correct:
> - drive can't be failed for different reasons.
> - there are path where -EBUSY is not reported and drive removal leads
> to failed array, without notification for userspace.
> - in the array failure case -EBUSY seems to be wrong status. Array is
> not busy, but removal process cannot proceed safe.
>
> -EBUSY expectation cannot be removed without breaking compatibility
> with userspace, but we can adopt the failed state verification method.
>
> In this patch MD_BROKEN flag support, used to mark non-redundant array
> as dead, is added to RAID1 and RAID10. Support for RAID456 is added in
> next commit.
>
> Now the array failure can be checked, so verify MD_BROKEN flag, however
> still return -EBUSY to userspace.
>
> As in previous commit, it causes that #mdadm --set-faulty is able to
> mark array as failed. Previously proposed workaround is valid if optional
> functionality 9a567843f79("md: allow last device to be forcibly
> removed from RAID1/RAID10.") is disabled.
>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>  drivers/md/md.c     | 17 ++++++++++-------
>  drivers/md/md.h     |  4 ++--
>  drivers/md/raid1.c  |  1 +
>  drivers/md/raid10.c |  1 +
>  4 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f888ef197765..fda8473f96b8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2983,10 +2983,11 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len)
>
>         if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
>                 md_error(rdev->mddev, rdev);
> -               if (test_bit(Faulty, &rdev->flags))
> -                       err = 0;
> -               else
> +
> +               if (test_bit(MD_BROKEN, &rdev->mddev->flags))
>                         err = -EBUSY;
> +               else
> +                       err = 0;
>         } else if (cmd_match(buf, "remove")) {
>                 if (rdev->mddev->pers) {
>                         clear_bit(Blocked, &rdev->flags);
> @@ -7441,7 +7442,7 @@ static int set_disk_faulty(struct mddev *mddev, dev_t dev)
>                 err =  -ENODEV;
>         else {
>                 md_error(mddev, rdev);
> -               if (!test_bit(Faulty, &rdev->flags))
> +               if (test_bit(MD_BROKEN, &mddev->flags))
>                         err = -EBUSY;
>         }
>         rcu_read_unlock();
> @@ -7987,12 +7988,14 @@ void md_error(struct mddev *mddev, struct md_rdev *rdev)
>         if (!mddev->pers->sync_request)
>                 return;
>
> -       if (mddev->degraded)
> +       if (mddev->degraded && !test_bit(MD_BROKEN, &mddev->flags))
>                 set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
>         sysfs_notify_dirent_safe(rdev->sysfs_state);
>         set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> -       set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> -       md_wakeup_thread(mddev->thread);
> +       if (!test_bit(MD_BROKEN, &mddev->flags)) {
> +               set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> +               md_wakeup_thread(mddev->thread);
> +       }
>         if (mddev->event_work.func)
>                 queue_work(md_misc_wq, &mddev->event_work);
>         md_new_event();
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index bc3f2094d0b6..d3a897868695 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -259,8 +259,8 @@ enum mddev_flags {
>         MD_NOT_READY,           /* do_md_run() is active, so 'array_state'
>                                  * must not report that array is ready yet
>                                  */
> -       MD_BROKEN,              /* This is used in RAID-0/LINEAR only, to stop
> -                                * I/O in case an array member is gone/failed.
> +       MD_BROKEN,              /* This is used to stop I/O and mark device as
> +                                * dead in case an array becomes failed.
>                                  */
>  };
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 7dc8026cf6ee..45dc75f90476 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1638,6 +1638,7 @@ static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
>                  */
>                 conf->recovery_disabled = mddev->recovery_disabled;
>                 spin_unlock_irqrestore(&conf->device_lock, flags);
> +               set_bit(MD_BROKEN, &mddev->flags);
>                 return;
>         }
>         set_bit(Blocked, &rdev->flags);
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index dde98f65bd04..d7cefd212e6b 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1964,6 +1964,7 @@ static void raid10_error(struct mddev *mddev, struct md_rdev *rdev)
>                  * Don't fail the drive, just return an IO error.
>                  */
>                 spin_unlock_irqrestore(&conf->device_lock, flags);
> +               set_bit(MD_BROKEN, &mddev->flags);
>                 return;
>         }
>         if (test_and_clear_bit(In_sync, &rdev->flags))
> --
> 2.26.2
>

Hi Mariusz

In your first Version, it checks MD_BROKEN in super_written. Does it
need to do this in this version?
From my understanding, it needs to retry the write when failfast is
supported. If it checks MD_BROKEN
in super_written, it can't send re-write anymore. Am I right?

Regards
Xiao

Re: [PATCH 2/3] md: Set MD_BROKEN for RAID1 and RAID10

[Mariusz Tkaczyk]

On Wed, 22 Dec 2021 15:24:54 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Thu, Dec 16, 2021 at 10:53 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > There was no direct mechanism to determine raid failure outside
> > personality. It was done by checking rdev->flags after executing
> > md_error(). If "faulty" was not set then -EBUSY was returned to
> > userspace. It causes that mdadm expects -EBUSY if the array
> > becomes failed. There are some reasons to not consider this
> > mechanism as correct:
> > - drive can't be failed for different reasons.
> > - there are path where -EBUSY is not reported and drive removal
> > leads to failed array, without notification for userspace.
> > - in the array failure case -EBUSY seems to be wrong status. Array
> > is not busy, but removal process cannot proceed safe.
> >
> > -EBUSY expectation cannot be removed without breaking compatibility
> > with userspace, but we can adopt the failed state verification
> > method.
> >
> > In this patch MD_BROKEN flag support, used to mark non-redundant
> > array as dead, is added to RAID1 and RAID10. Support for RAID456 is
> > added in next commit.
> >
> > Now the array failure can be checked, so verify MD_BROKEN flag,
> > however still return -EBUSY to userspace.
> >
> > As in previous commit, it causes that #mdadm --set-faulty is able to
> > mark array as failed. Previously proposed workaround is valid if
> > optional functionality 9a567843f79("md: allow last device to be
> > forcibly removed from RAID1/RAID10.") is disabled.
> >
> > Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> > ---
> >  drivers/md/md.c     | 17 ++++++++++-------
> >  drivers/md/md.h     |  4 ++--
> >  drivers/md/raid1.c  |  1 +
> >  drivers/md/raid10.c |  1 +
> >  4 files changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index f888ef197765..fda8473f96b8 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -2983,10 +2983,11 @@ state_store(struct md_rdev *rdev, const
> > char *buf, size_t len)
> >
> >         if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
> >                 md_error(rdev->mddev, rdev);
> > -               if (test_bit(Faulty, &rdev->flags))
> > -                       err = 0;
> > -               else
> > +
> > +               if (test_bit(MD_BROKEN, &rdev->mddev->flags))
> >                         err = -EBUSY;
> > +               else
> > +                       err = 0;
> >         } else if (cmd_match(buf, "remove")) {
> >                 if (rdev->mddev->pers) {
> >                         clear_bit(Blocked, &rdev->flags);
> > @@ -7441,7 +7442,7 @@ static int set_disk_faulty(struct mddev
> > *mddev, dev_t dev) err =  -ENODEV;
> >         else {
> >                 md_error(mddev, rdev);
> > -               if (!test_bit(Faulty, &rdev->flags))
> > +               if (test_bit(MD_BROKEN, &mddev->flags))
> >                         err = -EBUSY;
> >         }
> >         rcu_read_unlock();
> > @@ -7987,12 +7988,14 @@ void md_error(struct mddev *mddev, struct
> > md_rdev *rdev) if (!mddev->pers->sync_request)
> >                 return;
> >
> > -       if (mddev->degraded)
> > +       if (mddev->degraded && !test_bit(MD_BROKEN, &mddev->flags))
> >                 set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> >         sysfs_notify_dirent_safe(rdev->sysfs_state);
> >         set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> > -       set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> > -       md_wakeup_thread(mddev->thread);
> > +       if (!test_bit(MD_BROKEN, &mddev->flags)) {
> > +               set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> > +               md_wakeup_thread(mddev->thread);
> > +       }
> >         if (mddev->event_work.func)
> >                 queue_work(md_misc_wq, &mddev->event_work);
> >         md_new_event();
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index bc3f2094d0b6..d3a897868695 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -259,8 +259,8 @@ enum mddev_flags {
> >         MD_NOT_READY,           /* do_md_run() is active, so
> > 'array_state'
> >                                  * must not report that array is
> > ready yet */
> > -       MD_BROKEN,              /* This is used in RAID-0/LINEAR
> > only, to stop
> > -                                * I/O in case an array member is
> > gone/failed.
> > +       MD_BROKEN,              /* This is used to stop I/O and
> > mark device as
> > +                                * dead in case an array becomes
> > failed. */
> >  };
> >
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index 7dc8026cf6ee..45dc75f90476 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -1638,6 +1638,7 @@ static void raid1_error(struct mddev *mddev,
> > struct md_rdev *rdev) */
> >                 conf->recovery_disabled = mddev->recovery_disabled;
> >                 spin_unlock_irqrestore(&conf->device_lock, flags);
> > +               set_bit(MD_BROKEN, &mddev->flags);
> >                 return;
> >         }
> >         set_bit(Blocked, &rdev->flags);
> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> > index dde98f65bd04..d7cefd212e6b 100644
> > --- a/drivers/md/raid10.c
> > +++ b/drivers/md/raid10.c
> > @@ -1964,6 +1964,7 @@ static void raid10_error(struct mddev *mddev,
> > struct md_rdev *rdev)
> >                  * Don't fail the drive, just return an IO error.
> >                  */
> >                 spin_unlock_irqrestore(&conf->device_lock, flags);
> > +               set_bit(MD_BROKEN, &mddev->flags);
> >                 return;
> >         }
> >         if (test_and_clear_bit(In_sync, &rdev->flags))
> > --
> > 2.26.2
> >  
> 
> Hi Mariusz
> 
> In your first Version, it checks MD_BROKEN in super_written. Does it
> need to do this in this version?
> From my understanding, it needs to retry the write when failfast is
> supported. If it checks MD_BROKEN
> in super_written, it can't send re-write anymore. Am I right?
> 
Hi Xiao,
We have discussion about it with Song and as a result a dropped it.
I just left it as before. V2 shouldn't race with failfast because it
still takes care about faulty flag on device (MD_BROKEN is set
additionally).

I don't know a lot about failfast so, idea proposed in first patch could
be wrong.  I did it this way because I've updated all md_error() and
later test_bit(Faulty, &rdev->flags) paths, with strict assumption
faulty == MD_BROKEN. For failfast and last_dev it is not obvious.
If you have any recommendation, I will be grateful.

Thanks,
Mariusz

[PATCH 3/3] raid5: introduce MD_BROKEN

[Mariusz Tkaczyk]

Raid456 module had allowed to achieve failed state. It was fixed by
fb73b357fb9 ("raid5: block failing device if raid will be failed").
This fix introduces a bug, now if raid5 fails during IO, it may result
with a hung task without completion. Faulty flag on the device is
necessary to process all requests and is checked many times, mainly in
analyze_stripe().
Allow to set faulty on drive again and set MD_BROKEN if raid is failed.

As a result, this level is allowed to achieve failed state again, but
communication with userspace (via -EBUSY status) will be preserved.

This restores possibility to fail array via #mdadm --set-faulty command
and will be fixed by additional verification on mdadm side.

Reproduction steps:
 mdadm -CR imsm -e imsm -n 3 /dev/nvme[0-2]n1
 mdadm -CR r5 -e imsm -l5 -n3 /dev/nvme[0-2]n1 --assume-clean
 mkfs.xfs /dev/md126 -f
 mount /dev/md126 /mnt/root/

 fio --filename=/mnt/root/file --size=5GB --direct=1 --rw=randrw
--bs=64k --ioengine=libaio --iodepth=64 --runtime=240 --numjobs=4
--time_based --group_reporting --name=throughput-test-job
--eta-newline=1 &

 echo 1 > /sys/block/nvme2n1/device/device/remove
 echo 1 > /sys/block/nvme1n1/device/device/remove

 [ 1475.787779] Call Trace:
 [ 1475.793111] __schedule+0x2a6/0x700
 [ 1475.799460] schedule+0x38/0xa0
 [ 1475.805454] raid5_get_active_stripe+0x469/0x5f0 [raid456]
 [ 1475.813856] ? finish_wait+0x80/0x80
 [ 1475.820332] raid5_make_request+0x180/0xb40 [raid456]
 [ 1475.828281] ? finish_wait+0x80/0x80
 [ 1475.834727] ? finish_wait+0x80/0x80
 [ 1475.841127] ? finish_wait+0x80/0x80
 [ 1475.847480] md_handle_request+0x119/0x190
 [ 1475.854390] md_make_request+0x8a/0x190
 [ 1475.861041] generic_make_request+0xcf/0x310
 [ 1475.868145] submit_bio+0x3c/0x160
 [ 1475.874355] iomap_dio_submit_bio.isra.20+0x51/0x60
 [ 1475.882070] iomap_dio_bio_actor+0x175/0x390
 [ 1475.889149] iomap_apply+0xff/0x310
 [ 1475.895447] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.902736] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.909974] iomap_dio_rw+0x2f2/0x490
 [ 1475.916415] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.923680] ? atime_needs_update+0x77/0xe0
 [ 1475.930674] ? xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
 [ 1475.938455] xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
 [ 1475.946084] xfs_file_read_iter+0xba/0xd0 [xfs]
 [ 1475.953403] aio_read+0xd5/0x180
 [ 1475.959395] ? _cond_resched+0x15/0x30
 [ 1475.965907] io_submit_one+0x20b/0x3c0
 [ 1475.972398] __x64_sys_io_submit+0xa2/0x180
 [ 1475.979335] ? do_io_getevents+0x7c/0xc0
 [ 1475.986009] do_syscall_64+0x5b/0x1a0
 [ 1475.992419] entry_SYSCALL_64_after_hwframe+0x65/0xca
 [ 1476.000255] RIP: 0033:0x7f11fc27978d
 [ 1476.006631] Code: Bad RIP value.
 [ 1476.073251] INFO: task fio:3877 blocked for more than 120 seconds.

Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 drivers/md/raid5.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

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);
+		conf->recovery_disabled = mddev->recovery_disabled;
+		pr_crit("md/raid:%s: Cannot continue on %d devices.\n",
+			mdname(mddev), conf->raid_disks - mddev->degraded);
+	} else
+		pr_crit("md/raid:%s: Operation continuing on %d devices.\n",
+			mdname(mddev), conf->raid_disks - mddev->degraded);
+
 	spin_unlock_irqrestore(&conf->device_lock, flags);
 	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 
 	set_bit(Blocked, &rdev->flags);
 	set_mask_bits(&mddev->sb_flags, 0,
 		      BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
-	pr_crit("md/raid:%s: Disk failure on %s, disabling device.\n"
-		"md/raid:%s: Operation continuing on %d devices.\n",
-		mdname(mddev),
-		bdevname(rdev->bdev, b),
-		mdname(mddev),
-		conf->raid_disks - mddev->degraded);
 	r5c_update_on_rdev_error(mddev, rdev);
 }
 
-- 
2.26.2

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

[Guoqing Jiang]

On 12/16/21 10:52 PM, Mariusz Tkaczyk wrote:
> Raid456 module had allowed to achieve failed state. It was fixed by
> fb73b357fb9 ("raid5: block failing device if raid will be failed").
> This fix introduces a bug, now if raid5 fails during IO, it may result
> with a hung task without completion. Faulty flag on the device is
> necessary to process all requests and is checked many times, mainly in
> analyze_stripe().
> Allow to set faulty on drive again and set MD_BROKEN if raid is failed.
>
> As a result, this level is allowed to achieve failed state again, but
> communication with userspace (via -EBUSY status) will be preserved.
>
> This restores possibility to fail array via #mdadm --set-faulty command
> and will be fixed by additional verification on mdadm side.
>
> Reproduction steps:
>   mdadm -CR imsm -e imsm -n 3 /dev/nvme[0-2]n1
>   mdadm -CR r5 -e imsm -l5 -n3 /dev/nvme[0-2]n1 --assume-clean
>   mkfs.xfs /dev/md126 -f
>   mount /dev/md126 /mnt/root/
>
>   fio --filename=/mnt/root/file --size=5GB --direct=1 --rw=randrw
> --bs=64k --ioengine=libaio --iodepth=64 --runtime=240 --numjobs=4
> --time_based --group_reporting --name=throughput-test-job
> --eta-newline=1 &
>
>   echo 1 > /sys/block/nvme2n1/device/device/remove
>   echo 1 > /sys/block/nvme1n1/device/device/remove
>
>   [ 1475.787779] Call Trace:
>   [ 1475.793111] __schedule+0x2a6/0x700
>   [ 1475.799460] schedule+0x38/0xa0
>   [ 1475.805454] raid5_get_active_stripe+0x469/0x5f0 [raid456]
>   [ 1475.813856] ? finish_wait+0x80/0x80
>   [ 1475.820332] raid5_make_request+0x180/0xb40 [raid456]
>   [ 1475.828281] ? finish_wait+0x80/0x80
>   [ 1475.834727] ? finish_wait+0x80/0x80
>   [ 1475.841127] ? finish_wait+0x80/0x80
>   [ 1475.847480] md_handle_request+0x119/0x190
>   [ 1475.854390] md_make_request+0x8a/0x190
>   [ 1475.861041] generic_make_request+0xcf/0x310
>   [ 1475.868145] submit_bio+0x3c/0x160
>   [ 1475.874355] iomap_dio_submit_bio.isra.20+0x51/0x60
>   [ 1475.882070] iomap_dio_bio_actor+0x175/0x390
>   [ 1475.889149] iomap_apply+0xff/0x310
>   [ 1475.895447] ? iomap_dio_bio_actor+0x390/0x390
>   [ 1475.902736] ? iomap_dio_bio_actor+0x390/0x390
>   [ 1475.909974] iomap_dio_rw+0x2f2/0x490
>   [ 1475.916415] ? iomap_dio_bio_actor+0x390/0x390
>   [ 1475.923680] ? atime_needs_update+0x77/0xe0
>   [ 1475.930674] ? xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
>   [ 1475.938455] xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
>   [ 1475.946084] xfs_file_read_iter+0xba/0xd0 [xfs]
>   [ 1475.953403] aio_read+0xd5/0x180
>   [ 1475.959395] ? _cond_resched+0x15/0x30
>   [ 1475.965907] io_submit_one+0x20b/0x3c0
>   [ 1475.972398] __x64_sys_io_submit+0xa2/0x180
>   [ 1475.979335] ? do_io_getevents+0x7c/0xc0
>   [ 1475.986009] do_syscall_64+0x5b/0x1a0
>   [ 1475.992419] entry_SYSCALL_64_after_hwframe+0x65/0xca
>   [ 1476.000255] RIP: 0033:0x7f11fc27978d
>   [ 1476.006631] Code: Bad RIP value.
>   [ 1476.073251] INFO: task fio:3877 blocked for more than 120 seconds.
>
> Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>   drivers/md/raid5.c | 34 ++++++++++++++++------------------
>   1 file changed, 16 insertions(+), 18 deletions(-)
>
> 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.

Thanks,
Guoqing

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

[Mariusz Tkaczyk]

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.
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 

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

[Guoqing Jiang]

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

Re: [PATCH v2 0/3] Use MD_BROKEN for redundant arrays

[Song Liu]

On Thu, Dec 16, 2021 at 6:52 AM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Hi Song,
> As discussed in first version[1], I've done following:
> - errors handler for RAID0 and linear added
> - raid5 bug described
> - MD_BROKEN described
> - removed MD_BROKEN check for superblock update.
>
> [1] https://lore.kernel.org/linux-raid/20210917153452.5593-1-mariusz.tkaczyk@linux.intel.com/
>
> Mariusz Tkaczyk (3):
>   raid0, linear, md: add error_handlers for raid0 and linear
>   md: Set MD_BROKEN for RAID1 and RAID10
>   raid5: introduce MD_BROKEN

The set looks good to me. The only concern is that we changed some messages.
While dmesg is not a stable API, I believe there are people grep on it
to detect errors.
Therefore, please try to keep these messages same as before (as much
as possible).

Thanks,
Song

Re: [PATCH v2 0/3] Use MD_BROKEN for redundant arrays

[Mariusz Tkaczyk]

Hi Song,

On Thu, 16 Dec 2021 16:52:23 -0800
Song Liu <song@kernel.org> wrote:
> > Mariusz Tkaczyk (3):
> >   raid0, linear, md: add error_handlers for raid0 and linear
> >   md: Set MD_BROKEN for RAID1 and RAID10
> >   raid5: introduce MD_BROKEN  
> 
> The set looks good to me. The only concern is that we changed some
> messages. While dmesg is not a stable API, I believe there are people
> grep on it to detect errors.
> Therefore, please try to keep these messages same as before (as much
> as possible).

Will do.
After sending it, I realized that my approach is not correct when
mddev->fail_last_dev is on. MD_BRKOEN should be set even if we agree to
remove the "last" drive. I will fix it too.

Thanks,
Mariusz

Re: [PATCH v2 0/3] Use MD_BROKEN for redundant arrays

[Mariusz Tkaczyk]

On Fri, 17 Dec 2021 09:02:38 +0100
Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:

> Hi Song,
> 
> On Thu, 16 Dec 2021 16:52:23 -0800
> Song Liu <song@kernel.org> wrote:
> > > Mariusz Tkaczyk (3):
> > >   raid0, linear, md: add error_handlers for raid0 and linear
> > >   md: Set MD_BROKEN for RAID1 and RAID10
> > >   raid5: introduce MD_BROKEN  
> > 
> > The set looks good to me. The only concern is that we changed some
> > messages. While dmesg is not a stable API, I believe there are
> > people grep on it to detect errors.
> > Therefore, please try to keep these messages same as before (as much
> > as possible).
> 
> Will do.
> After sending it, I realized that my approach is not correct when
> mddev->fail_last_dev is on. MD_BRKOEN should be set even if we agree
> to remove the "last" drive. I will fix it too.
> 

Hi Song,
For raid0 and linear i added new messages so it shouldn't be a problem.
I added one message in raid5 for failed state:
+		pr_crit("md/raid:%s: Cannot continue on %d devices.\n",

Do you want to remove it?
Other errors are same. Order is also preserved.

Thanks,
Mariusz

Re: [PATCH v2 0/3] Use MD_BROKEN for redundant arrays

[Song Liu]

On Tue, Jan 25, 2022 at 7:52 AM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Fri, 17 Dec 2021 09:02:38 +0100
> Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:
>
> > Hi Song,
> >
> > On Thu, 16 Dec 2021 16:52:23 -0800
> > Song Liu <song@kernel.org> wrote:
> > > > Mariusz Tkaczyk (3):
> > > >   raid0, linear, md: add error_handlers for raid0 and linear
> > > >   md: Set MD_BROKEN for RAID1 and RAID10
> > > >   raid5: introduce MD_BROKEN
> > >
> > > The set looks good to me. The only concern is that we changed some
> > > messages. While dmesg is not a stable API, I believe there are
> > > people grep on it to detect errors.
> > > Therefore, please try to keep these messages same as before (as much
> > > as possible).
> >
> > Will do.
> > After sending it, I realized that my approach is not correct when
> > mddev->fail_last_dev is on. MD_BRKOEN should be set even if we agree
> > to remove the "last" drive. I will fix it too.
> >
>
> Hi Song,
> For raid0 and linear i added new messages so it shouldn't be a problem.
> I added one message in raid5 for failed state:
> +               pr_crit("md/raid:%s: Cannot continue on %d devices.\n",
>
> Do you want to remove it?
> Other errors are same. Order is also preserved.

I think we should print in this case. How about we change it to something
similar to the one in raid5_run():

                pr_crit("md/raid:%s: Cannot continue operation (%d/%d
failed)\n",
                        mdname(mddev), mddev->degraded, conf->raid_disks);

Thanks,
Song

[PATCH v3 0/3] Improve failed arrays handling.

[Mariusz Tkaczyk]

Hi Song,
I've made changes as discussed in v2[1]. I did manual testing with IMSM
metadata.

Patch 1:
- "%pg" propagated and raid0/linear_error refactored as Suggested by Guoqing.
- missed dm-event, suggested by Guoqing- verified. IMO the behavior is same as
  before.

Patch 2:
- Commit id fixed, suggested by Gouqing.
- Description rework, suggested by Xiao (discussed offline).
- fail_last_dev handling added (and verified).
- MD_BROKEN description modified, suggested by Gouqing.
- Descriptions for raid1_error and raid10_error are added, redundant comments
  are removed.

Patch3:
- Error message for failed array changed, suggested by you.
- MD_BROKEN setter moved to has_failed(), suggested by Gouqing.
- has_failed() refactored

Other notes:
I followed kernel-doc style guidelines when editing or adding new descriptions.
Please let me know if you consider it as unnecessary and messy.

I also noticed potential issue during refactor related to MD_FAILFAST_SUPPORTED,
please see the flag definition. I'm wondering if fail_last_dev functionality is
not against failfast. Should I start separate thread for that?

[1] https://lore.kernel.org/linux-raid/CAPhsuW43QfDNGEu72o2_eqDZ5vGq3tbFvdZ-W+dxVqcEhHmJ5w@mail.gmail.com/T/#t

Mariusz Tkaczyk (3):
  raid0, linear, md: add error_handlers for raid0 and linear
  md: Set MD_BROKEN for RAID1 and RAID10
  raid5: introduce MD_BROKEN

 drivers/md/md-linear.c | 15 ++++++++-
 drivers/md/md.c        | 23 +++++++++-----
 drivers/md/md.h        | 72 ++++++++++++++++++++++--------------------
 drivers/md/raid0.c     | 15 ++++++++-
 drivers/md/raid1.c     | 42 ++++++++++++++----------
 drivers/md/raid10.c    | 33 ++++++++++++-------
 drivers/md/raid5.c     | 49 ++++++++++++++--------------
 7 files changed, 151 insertions(+), 98 deletions(-)

-- 
2.26.2

[PATCH 3/3] raid5: introduce MD_BROKEN

[Mariusz Tkaczyk]

Raid456 module had allowed to achieve failed state. It was fixed by
fb73b357fb9 ("raid5: block failing device if raid will be failed").
This fix introduces a bug, now if raid5 fails during IO, it may result
with a hung task without completion. Faulty flag on the device is
necessary to process all requests and is checked many times, mainly in
analyze_stripe().
Allow to set faulty on drive again and set MD_BROKEN if raid is failed.

As a result, this level is allowed to achieve failed state again, but
communication with userspace (via -EBUSY status) will be preserved.

This restores possibility to fail array via #mdadm --set-faulty command
and will be fixed by additional verification on mdadm side.

Reproduction steps:
 mdadm -CR imsm -e imsm -n 3 /dev/nvme[0-2]n1
 mdadm -CR r5 -e imsm -l5 -n3 /dev/nvme[0-2]n1 --assume-clean
 mkfs.xfs /dev/md126 -f
 mount /dev/md126 /mnt/root/

 fio --filename=/mnt/root/file --size=5GB --direct=1 --rw=randrw
--bs=64k --ioengine=libaio --iodepth=64 --runtime=240 --numjobs=4
--time_based --group_reporting --name=throughput-test-job
--eta-newline=1 &

 echo 1 > /sys/block/nvme2n1/device/device/remove
 echo 1 > /sys/block/nvme1n1/device/device/remove

 [ 1475.787779] Call Trace:
 [ 1475.793111] __schedule+0x2a6/0x700
 [ 1475.799460] schedule+0x38/0xa0
 [ 1475.805454] raid5_get_active_stripe+0x469/0x5f0 [raid456]
 [ 1475.813856] ? finish_wait+0x80/0x80
 [ 1475.820332] raid5_make_request+0x180/0xb40 [raid456]
 [ 1475.828281] ? finish_wait+0x80/0x80
 [ 1475.834727] ? finish_wait+0x80/0x80
 [ 1475.841127] ? finish_wait+0x80/0x80
 [ 1475.847480] md_handle_request+0x119/0x190
 [ 1475.854390] md_make_request+0x8a/0x190
 [ 1475.861041] generic_make_request+0xcf/0x310
 [ 1475.868145] submit_bio+0x3c/0x160
 [ 1475.874355] iomap_dio_submit_bio.isra.20+0x51/0x60
 [ 1475.882070] iomap_dio_bio_actor+0x175/0x390
 [ 1475.889149] iomap_apply+0xff/0x310
 [ 1475.895447] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.902736] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.909974] iomap_dio_rw+0x2f2/0x490
 [ 1475.916415] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.923680] ? atime_needs_update+0x77/0xe0
 [ 1475.930674] ? xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
 [ 1475.938455] xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
 [ 1475.946084] xfs_file_read_iter+0xba/0xd0 [xfs]
 [ 1475.953403] aio_read+0xd5/0x180
 [ 1475.959395] ? _cond_resched+0x15/0x30
 [ 1475.965907] io_submit_one+0x20b/0x3c0
 [ 1475.972398] __x64_sys_io_submit+0xa2/0x180
 [ 1475.979335] ? do_io_getevents+0x7c/0xc0
 [ 1475.986009] do_syscall_64+0x5b/0x1a0
 [ 1475.992419] entry_SYSCALL_64_after_hwframe+0x65/0xca
 [ 1476.000255] RIP: 0033:0x7f11fc27978d
 [ 1476.006631] Code: Bad RIP value.
 [ 1476.073251] INFO: task fio:3877 blocked for more than 120 seconds.

Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 drivers/md/raid1.c |  1 +
 drivers/md/raid5.c | 49 +++++++++++++++++++++++-----------------------
 2 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index b222bafa1196..58c8eddb0f55 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1627,6 +1627,7 @@ static void raid1_status(struct seq_file *seq, struct mddev *mddev)
  * - if is on @rdev is removed.
  * - if is off, @rdev is not removed, but recovery from it is disabled (@rdev is
  *   very likely to fail).
+ *
  * In both cases, &MD_BROKEN will be set in &mddev->flags.
  */
 static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 1240a5c16af8..bee953c8007f 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -686,17 +686,21 @@ int raid5_calc_degraded(struct r5conf *conf)
 	return degraded;
 }
 
-static int has_failed(struct r5conf *conf)
+static bool has_failed(struct r5conf *conf)
 {
-	int degraded;
+	int degraded = conf->mddev->degraded;
 
-	if (conf->mddev->reshape_position == MaxSector)
-		return conf->mddev->degraded > conf->max_degraded;
+	if (test_bit(MD_BROKEN, &conf->mddev->flags))
+		return true;
 
-	degraded = raid5_calc_degraded(conf);
-	if (degraded > conf->max_degraded)
-		return 1;
-	return 0;
+	if (conf->mddev->reshape_position != MaxSector)
+		degraded = raid5_calc_degraded(conf);
+
+	if (degraded > conf->max_degraded) {
+		set_bit(MD_BROKEN, &conf->mddev->flags);
+		return true;
+	}
+	return false;
 }
 
 struct stripe_head *
@@ -2877,34 +2881,29 @@ static void raid5_error(struct mddev *mddev, struct md_rdev *rdev)
 	unsigned long flags;
 	pr_debug("raid456: error called\n");
 
+	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 (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
-		 */
+	if (has_failed(conf)) {
 		conf->recovery_disabled = mddev->recovery_disabled;
-		spin_unlock_irqrestore(&conf->device_lock, flags);
-		return;
+		pr_crit("md/raid:%s: Cannot continue operation (%d/%d failed).\n",
+			mdname(mddev), mddev->degraded, conf->raid_disks);
+	} else {
+		pr_crit("md/raid:%s: Operation continuing on %d devices.\n",
+			mdname(mddev), conf->raid_disks - mddev->degraded);
 	}
 
-	set_bit(Faulty, &rdev->flags);
-	clear_bit(In_sync, &rdev->flags);
-	mddev->degraded = raid5_calc_degraded(conf);
 	spin_unlock_irqrestore(&conf->device_lock, flags);
 	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 
 	set_bit(Blocked, &rdev->flags);
 	set_mask_bits(&mddev->sb_flags, 0,
 		      BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
-	pr_crit("md/raid:%s: Disk failure on %s, disabling device.\n"
-		"md/raid:%s: Operation continuing on %d devices.\n",
-		mdname(mddev),
-		bdevname(rdev->bdev, b),
-		mdname(mddev),
-		conf->raid_disks - mddev->degraded);
 	r5c_update_on_rdev_error(mddev, rdev);
 }
 
-- 
2.26.2

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

[Xiao Ni]

On Thu, Jan 27, 2022 at 11:39 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Raid456 module had allowed to achieve failed state. It was fixed by
> fb73b357fb9 ("raid5: block failing device if raid will be failed").
> This fix introduces a bug, now if raid5 fails during IO, it may result
> with a hung task without completion. Faulty flag on the device is
> necessary to process all requests and is checked many times, mainly in
> analyze_stripe().
> Allow to set faulty on drive again and set MD_BROKEN if raid is failed.
>
> As a result, this level is allowed to achieve failed state again, but
> communication with userspace (via -EBUSY status) will be preserved.
>
> This restores possibility to fail array via #mdadm --set-faulty command
> and will be fixed by additional verification on mdadm side.
>
> Reproduction steps:
>  mdadm -CR imsm -e imsm -n 3 /dev/nvme[0-2]n1
>  mdadm -CR r5 -e imsm -l5 -n3 /dev/nvme[0-2]n1 --assume-clean
>  mkfs.xfs /dev/md126 -f
>  mount /dev/md126 /mnt/root/
>
>  fio --filename=/mnt/root/file --size=5GB --direct=1 --rw=randrw
> --bs=64k --ioengine=libaio --iodepth=64 --runtime=240 --numjobs=4
> --time_based --group_reporting --name=throughput-test-job
> --eta-newline=1 &
>
>  echo 1 > /sys/block/nvme2n1/device/device/remove
>  echo 1 > /sys/block/nvme1n1/device/device/remove
>
>  [ 1475.787779] Call Trace:
>  [ 1475.793111] __schedule+0x2a6/0x700
>  [ 1475.799460] schedule+0x38/0xa0
>  [ 1475.805454] raid5_get_active_stripe+0x469/0x5f0 [raid456]
>  [ 1475.813856] ? finish_wait+0x80/0x80
>  [ 1475.820332] raid5_make_request+0x180/0xb40 [raid456]
>  [ 1475.828281] ? finish_wait+0x80/0x80
>  [ 1475.834727] ? finish_wait+0x80/0x80
>  [ 1475.841127] ? finish_wait+0x80/0x80
>  [ 1475.847480] md_handle_request+0x119/0x190
>  [ 1475.854390] md_make_request+0x8a/0x190
>  [ 1475.861041] generic_make_request+0xcf/0x310
>  [ 1475.868145] submit_bio+0x3c/0x160
>  [ 1475.874355] iomap_dio_submit_bio.isra.20+0x51/0x60
>  [ 1475.882070] iomap_dio_bio_actor+0x175/0x390
>  [ 1475.889149] iomap_apply+0xff/0x310
>  [ 1475.895447] ? iomap_dio_bio_actor+0x390/0x390
>  [ 1475.902736] ? iomap_dio_bio_actor+0x390/0x390
>  [ 1475.909974] iomap_dio_rw+0x2f2/0x490
>  [ 1475.916415] ? iomap_dio_bio_actor+0x390/0x390
>  [ 1475.923680] ? atime_needs_update+0x77/0xe0
>  [ 1475.930674] ? xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
>  [ 1475.938455] xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
>  [ 1475.946084] xfs_file_read_iter+0xba/0xd0 [xfs]
>  [ 1475.953403] aio_read+0xd5/0x180
>  [ 1475.959395] ? _cond_resched+0x15/0x30
>  [ 1475.965907] io_submit_one+0x20b/0x3c0
>  [ 1475.972398] __x64_sys_io_submit+0xa2/0x180
>  [ 1475.979335] ? do_io_getevents+0x7c/0xc0
>  [ 1475.986009] do_syscall_64+0x5b/0x1a0
>  [ 1475.992419] entry_SYSCALL_64_after_hwframe+0x65/0xca
>  [ 1476.000255] RIP: 0033:0x7f11fc27978d
>  [ 1476.006631] Code: Bad RIP value.
>  [ 1476.073251] INFO: task fio:3877 blocked for more than 120 seconds.
>
> Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>  drivers/md/raid1.c |  1 +
>  drivers/md/raid5.c | 49 +++++++++++++++++++++++-----------------------
>  2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index b222bafa1196..58c8eddb0f55 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1627,6 +1627,7 @@ static void raid1_status(struct seq_file *seq, struct mddev *mddev)
>   * - if is on @rdev is removed.
>   * - if is off, @rdev is not removed, but recovery from it is disabled (@rdev is
>   *   very likely to fail).
> + *

type error? If not, it should be better in the second patch.

Regards
Xiao

>   * In both cases, &MD_BROKEN will be set in &mddev->flags.
>   */
>  static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 1240a5c16af8..bee953c8007f 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -686,17 +686,21 @@ int raid5_calc_degraded(struct r5conf *conf)
>         return degraded;
>  }
>
> -static int has_failed(struct r5conf *conf)
> +static bool has_failed(struct r5conf *conf)
>  {
> -       int degraded;
> +       int degraded = conf->mddev->degraded;
>
> -       if (conf->mddev->reshape_position == MaxSector)
> -               return conf->mddev->degraded > conf->max_degraded;
> +       if (test_bit(MD_BROKEN, &conf->mddev->flags))
> +               return true;
>
> -       degraded = raid5_calc_degraded(conf);
> -       if (degraded > conf->max_degraded)
> -               return 1;
> -       return 0;
> +       if (conf->mddev->reshape_position != MaxSector)
> +               degraded = raid5_calc_degraded(conf);
> +
> +       if (degraded > conf->max_degraded) {
> +               set_bit(MD_BROKEN, &conf->mddev->flags);
> +               return true;
> +       }
> +       return false;
>  }
>
>  struct stripe_head *
> @@ -2877,34 +2881,29 @@ static void raid5_error(struct mddev *mddev, struct md_rdev *rdev)
>         unsigned long flags;
>         pr_debug("raid456: error called\n");
>
> +       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 (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
> -                */
> +       if (has_failed(conf)) {
>                 conf->recovery_disabled = mddev->recovery_disabled;
> -               spin_unlock_irqrestore(&conf->device_lock, flags);
> -               return;
> +               pr_crit("md/raid:%s: Cannot continue operation (%d/%d failed).\n",
> +                       mdname(mddev), mddev->degraded, conf->raid_disks);
> +       } else {
> +               pr_crit("md/raid:%s: Operation continuing on %d devices.\n",
> +                       mdname(mddev), conf->raid_disks - mddev->degraded);
>         }
>
> -       set_bit(Faulty, &rdev->flags);
> -       clear_bit(In_sync, &rdev->flags);
> -       mddev->degraded = raid5_calc_degraded(conf);
>         spin_unlock_irqrestore(&conf->device_lock, flags);
>         set_bit(MD_RECOVERY_INTR, &mddev->recovery);
>
>         set_bit(Blocked, &rdev->flags);
>         set_mask_bits(&mddev->sb_flags, 0,
>                       BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
> -       pr_crit("md/raid:%s: Disk failure on %s, disabling device.\n"
> -               "md/raid:%s: Operation continuing on %d devices.\n",
> -               mdname(mddev),
> -               bdevname(rdev->bdev, b),
> -               mdname(mddev),
> -               conf->raid_disks - mddev->degraded);
>         r5c_update_on_rdev_error(mddev, rdev);
>  }
>
> --
> 2.26.2
>

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

[Guoqing Jiang]

On 1/27/22 11:39 PM, Mariusz Tkaczyk wrote:
> Raid456 module had allowed to achieve failed state. It was fixed by
> fb73b357fb9 ("raid5: block failing device if raid will be failed").
> This fix introduces a bug, now if raid5 fails during IO, it may result
> with a hung task without completion. Faulty flag on the device is
> necessary to process all requests and is checked many times, mainly in
> analyze_stripe().
> Allow to set faulty on drive again and set MD_BROKEN if raid is failed.
>
> As a result, this level is allowed to achieve failed state again, but
> communication with userspace (via -EBUSY status) will be preserved.
>
> This restores possibility to fail array via #mdadm --set-faulty command
> and will be fixed by additional verification on mdadm side.

Again, you better to send mdadm change along with the series.

> Reproduction steps:
>   mdadm -CR imsm -e imsm -n 3 /dev/nvme[0-2]n1
>   mdadm -CR r5 -e imsm -l5 -n3 /dev/nvme[0-2]n1 --assume-clean
>   mkfs.xfs /dev/md126 -f
>   mount /dev/md126 /mnt/root/
>
>   fio --filename=/mnt/root/file --size=5GB --direct=1 --rw=randrw
> --bs=64k --ioengine=libaio --iodepth=64 --runtime=240 --numjobs=4
> --time_based --group_reporting --name=throughput-test-job
> --eta-newline=1 &
>
>   echo 1 > /sys/block/nvme2n1/device/device/remove
>   echo 1 > /sys/block/nvme1n1/device/device/remove
>
>   [ 1475.787779] Call Trace:
>   [ 1475.793111] __schedule+0x2a6/0x700
>   [ 1475.799460] schedule+0x38/0xa0
>   [ 1475.805454] raid5_get_active_stripe+0x469/0x5f0 [raid456]
>   [ 1475.813856] ? finish_wait+0x80/0x80
>   [ 1475.820332] raid5_make_request+0x180/0xb40 [raid456]
>   [ 1475.828281] ? finish_wait+0x80/0x80
>   [ 1475.834727] ? finish_wait+0x80/0x80
>   [ 1475.841127] ? finish_wait+0x80/0x80
>   [ 1475.847480] md_handle_request+0x119/0x190
>   [ 1475.854390] md_make_request+0x8a/0x190
>   [ 1475.861041] generic_make_request+0xcf/0x310
>   [ 1475.868145] submit_bio+0x3c/0x160
>   [ 1475.874355] iomap_dio_submit_bio.isra.20+0x51/0x60
>   [ 1475.882070] iomap_dio_bio_actor+0x175/0x390
>   [ 1475.889149] iomap_apply+0xff/0x310
>   [ 1475.895447] ? iomap_dio_bio_actor+0x390/0x390
>   [ 1475.902736] ? iomap_dio_bio_actor+0x390/0x390
>   [ 1475.909974] iomap_dio_rw+0x2f2/0x490
>   [ 1475.916415] ? iomap_dio_bio_actor+0x390/0x390
>   [ 1475.923680] ? atime_needs_update+0x77/0xe0
>   [ 1475.930674] ? xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
>   [ 1475.938455] xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
>   [ 1475.946084] xfs_file_read_iter+0xba/0xd0 [xfs]
>   [ 1475.953403] aio_read+0xd5/0x180
>   [ 1475.959395] ? _cond_resched+0x15/0x30
>   [ 1475.965907] io_submit_one+0x20b/0x3c0
>   [ 1475.972398] __x64_sys_io_submit+0xa2/0x180
>   [ 1475.979335] ? do_io_getevents+0x7c/0xc0
>   [ 1475.986009] do_syscall_64+0x5b/0x1a0
>   [ 1475.992419] entry_SYSCALL_64_after_hwframe+0x65/0xca
>   [ 1476.000255] RIP: 0033:0x7f11fc27978d
>   [ 1476.006631] Code: Bad RIP value.
>   [ 1476.073251] INFO: task fio:3877 blocked for more than 120 seconds.

Does it also happen to non imsm array? And did you try to reproduce it with
revert fb73b357fb? I suppose fb73b357fb9 introduced the regression given
it is fixed by this one.

> Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
> Signed-off-by: Mariusz Tkaczyk<mariusz.tkaczyk@linux.intel.com>
> ---
>   drivers/md/raid1.c |  1 +
>   drivers/md/raid5.c | 49 +++++++++++++++++++++++-----------------------
>   2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index b222bafa1196..58c8eddb0f55 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1627,6 +1627,7 @@ static void raid1_status(struct seq_file *seq, struct mddev *mddev)
>    * - if is on @rdev is removed.
>    * - if is off, @rdev is not removed, but recovery from it is disabled (@rdev is
>    *   very likely to fail).
> + *

You probably don't want to touch raid1 file in this patch.

>    * In both cases, &MD_BROKEN will be set in &mddev->flags.
>    */
>   static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 1240a5c16af8..bee953c8007f 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -686,17 +686,21 @@ int raid5_calc_degraded(struct r5conf *conf)
>   	return degraded;
>   }
>   
> -static int has_failed(struct r5conf *conf)
> +static bool has_failed(struct r5conf *conf)
>   {
> -	int degraded;
> +	int degraded = conf->mddev->degraded;
>   
> -	if (conf->mddev->reshape_position == MaxSector)
> -		return conf->mddev->degraded > conf->max_degraded;
> +	if (test_bit(MD_BROKEN, &conf->mddev->flags))
> +		return true;

If one member disk was set Faulty which caused BROKEN was set, is it
possible to re-add the same member disk again?

[root@vm ~]# echo faulty > /sys/block/md0/md/dev-loop1/state
[root@vm ~]# cat /proc/mdstat
Personalities : [raid6] [raid5] [raid4]
md0 : active raid5 loop2[2] loop1[0](F)
       1046528 blocks super 1.2 level 5, 512k chunk, algorithm 2 [2/1] [_U]
       bitmap: 0/1 pages [0KB], 65536KB chunk

unused devices: <none>
[root@vm ~]# echo re-add > /sys/block/md0/md/dev-loop1/state
[root@vm ~]# cat /proc/mdstat
Personalities : [raid6] [raid5] [raid4]
md0 : active raid5 loop2[2] loop1[0]
       1046528 blocks super 1.2 level 5, 512k chunk, algorithm 2 [2/2] [UU]
       bitmap: 0/1 pages [0KB], 65536KB chunk

unused devices: <none>

And have you run mdadm test against the series?

> -	degraded = raid5_calc_degraded(conf);
> -	if (degraded > conf->max_degraded)
> -		return 1;
> -	return 0;
> +	if (conf->mddev->reshape_position != MaxSector)
> +		degraded = raid5_calc_degraded(conf);
> +
> +	if (degraded > conf->max_degraded) {
> +		set_bit(MD_BROKEN, &conf->mddev->flags);

Why not set BROKEN flags in err handler to align with other levels? Or
do it in md_error only.

> +		return true;
> +	}
> +	return false;
>   }
>   
>   struct stripe_head *
> @@ -2877,34 +2881,29 @@ static void raid5_error(struct mddev *mddev, struct md_rdev *rdev)
>   	unsigned long flags;
>   	pr_debug("raid456: error called\n");
>   
> +	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 (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
> -		 */
> +	if (has_failed(conf)) {
>   		conf->recovery_disabled = mddev->recovery_disabled;
> -		spin_unlock_irqrestore(&conf->device_lock, flags);
> -		return;

Ok, I think commit fb73b357fb985cc652a72a41541d25915c7f9635 is
effectively reverted by this hunk. So I would prefer to separate the
revert part from this patch, just my 0.02$.

Thanks,
Guoqing

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

[Mariusz Tkaczyk]

Hi Guoqing,
Sorry for the delay, I missed it.

On Sat, 12 Feb 2022 09:47:38 +0800
Guoqing Jiang <guoqing.jiang@linux.dev> wrote:

> On 1/27/22 11:39 PM, Mariusz Tkaczyk wrote:
> > Raid456 module had allowed to achieve failed state. It was fixed by
> > fb73b357fb9 ("raid5: block failing device if raid will be failed").
> > This fix introduces a bug, now if raid5 fails during IO, it may
> > result with a hung task without completion. Faulty flag on the
> > device is necessary to process all requests and is checked many
> > times, mainly in analyze_stripe().
> > Allow to set faulty on drive again and set MD_BROKEN if raid is
> > failed.
> >
> > As a result, this level is allowed to achieve failed state again,
> > but communication with userspace (via -EBUSY status) will be
> > preserved.
> >
> > This restores possibility to fail array via #mdadm --set-faulty
> > command and will be fixed by additional verification on mdadm side.
> >  
> 
> Again, you better to send mdadm change along with the series.

I understand your objections. Unfortunately, I was unable to handle it
yet. I focused on kernel part first because mdadm was in freeze.
In mdadm, I need to block manual removal which should be simple, there
is already enough() function defined. It is on top of my TODO lst.
  
> 
> > Reproduction steps:
> >   mdadm -CR imsm -e imsm -n 3 /dev/nvme[0-2]n1
> >   mdadm -CR r5 -e imsm -l5 -n3 /dev/nvme[0-2]n1 --assume-clean
> >   mkfs.xfs /dev/md126 -f
> >   mount /dev/md126 /mnt/root/
> >
> >   fio --filename=/mnt/root/file --size=5GB --direct=1 --rw=randrw
> > --bs=64k --ioengine=libaio --iodepth=64 --runtime=240 --numjobs=4
> > --time_based --group_reporting --name=throughput-test-job
> > --eta-newline=1 &
> >
> >   echo 1 > /sys/block/nvme2n1/device/device/remove
> >   echo 1 > /sys/block/nvme1n1/device/device/remove
> >
> >   [ 1475.787779] Call Trace:
> >   [ 1475.793111] __schedule+0x2a6/0x700
> >   [ 1475.799460] schedule+0x38/0xa0
> >   [ 1475.805454] raid5_get_active_stripe+0x469/0x5f0 [raid456]
> >   [ 1475.813856] ? finish_wait+0x80/0x80
> >   [ 1475.820332] raid5_make_request+0x180/0xb40 [raid456]
> >   [ 1475.828281] ? finish_wait+0x80/0x80
> >   [ 1475.834727] ? finish_wait+0x80/0x80
> >   [ 1475.841127] ? finish_wait+0x80/0x80
> >   [ 1475.847480] md_handle_request+0x119/0x190
> >   [ 1475.854390] md_make_request+0x8a/0x190
> >   [ 1475.861041] generic_make_request+0xcf/0x310
> >   [ 1475.868145] submit_bio+0x3c/0x160
> >   [ 1475.874355] iomap_dio_submit_bio.isra.20+0x51/0x60
> >   [ 1475.882070] iomap_dio_bio_actor+0x175/0x390
> >   [ 1475.889149] iomap_apply+0xff/0x310
> >   [ 1475.895447] ? iomap_dio_bio_actor+0x390/0x390
> >   [ 1475.902736] ? iomap_dio_bio_actor+0x390/0x390
> >   [ 1475.909974] iomap_dio_rw+0x2f2/0x490
> >   [ 1475.916415] ? iomap_dio_bio_actor+0x390/0x390
> >   [ 1475.923680] ? atime_needs_update+0x77/0xe0
> >   [ 1475.930674] ? xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
> >   [ 1475.938455] xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
> >   [ 1475.946084] xfs_file_read_iter+0xba/0xd0 [xfs]
> >   [ 1475.953403] aio_read+0xd5/0x180
> >   [ 1475.959395] ? _cond_resched+0x15/0x30
> >   [ 1475.965907] io_submit_one+0x20b/0x3c0
> >   [ 1475.972398] __x64_sys_io_submit+0xa2/0x180
> >   [ 1475.979335] ? do_io_getevents+0x7c/0xc0
> >   [ 1475.986009] do_syscall_64+0x5b/0x1a0
> >   [ 1475.992419] entry_SYSCALL_64_after_hwframe+0x65/0xca
> >   [ 1476.000255] RIP: 0033:0x7f11fc27978d
> >   [ 1476.006631] Code: Bad RIP value.
> >   [ 1476.073251] INFO: task fio:3877 blocked for more than 120
> > seconds.  
> 
> Does it also happen to non imsm array? And did you try to reproduce
> it with revert fb73b357fb? I suppose fb73b357fb9 introduced the
> regression given it is fixed by this one.
> 
It is reproducible on native and IMSM. Yes, we can fix it by revert.

> 
> >    * In both cases, &MD_BROKEN will be set in &mddev->flags.
> >    */
> >   static void raid1_error(struct mddev *mddev, struct md_rdev *rdev)
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index 1240a5c16af8..bee953c8007f 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> > @@ -686,17 +686,21 @@ int raid5_calc_degraded(struct r5conf *conf)
> >   	return degraded;
> >   }
> >   
> > -static int has_failed(struct r5conf *conf)
> > +static bool has_failed(struct r5conf *conf)
> >   {
> > -	int degraded;
> > +	int degraded = conf->mddev->degraded;
> >   
> > -	if (conf->mddev->reshape_position == MaxSector)
> > -		return conf->mddev->degraded > conf->max_degraded;
> > +	if (test_bit(MD_BROKEN, &conf->mddev->flags))
> > +		return true;  
> 
> If one member disk was set Faulty which caused BROKEN was set, is it
> possible to re-add the same member disk again?
> 
Is possible to re-add drive to failed raid5 array now? From my
understanding of raid5_add_disk it is not possible.

> [root@vm ~]# echo faulty > /sys/block/md0/md/dev-loop1/state
> [root@vm ~]# cat /proc/mdstat
> Personalities : [raid6] [raid5] [raid4]
> md0 : active raid5 loop2[2] loop1[0](F)
>        1046528 blocks super 1.2 level 5, 512k chunk, algorithm 2
> [2/1] [_U] bitmap: 0/1 pages [0KB], 65536KB chunk
> 
> unused devices: <none>
> [root@vm ~]# echo re-add > /sys/block/md0/md/dev-loop1/state
> [root@vm ~]# cat /proc/mdstat
> Personalities : [raid6] [raid5] [raid4]
> md0 : active raid5 loop2[2] loop1[0]
>        1046528 blocks super 1.2 level 5, 512k chunk, algorithm 2
> [2/2] [UU] bitmap: 0/1 pages [0KB], 65536KB chunk
> 
> unused devices: <none>
> 
> And have you run mdadm test against the series?
> 
I run imsm test suite and our internal IMSM scope. I will take the
challenge and will verify with native. Thanks for suggestion.

> > -	degraded = raid5_calc_degraded(conf);
> > -	if (degraded > conf->max_degraded)
> > -		return 1;
> > -	return 0;
> > +	if (conf->mddev->reshape_position != MaxSector)
> > +		degraded = raid5_calc_degraded(conf);
> > +
> > +	if (degraded > conf->max_degraded) {
> > +		set_bit(MD_BROKEN, &conf->mddev->flags);  
> 
> Why not set BROKEN flags in err handler to align with other levels? Or
> do it in md_error only.

https://lore.kernel.org/linux-raid/3da9324e-01e7-2a07-4bcd-14245db56693@linux.dev/

You suggested that.
Other levels doesn't have dedicates has_failed() routines. For raid5 it
is reasonable to set it in has_failed().

I can't do that in md_error because I don't have such information in
all cases. !test_bit("Faulty", rdev->flags) result varies.

> 
> > +		return true;
> > +	}
> > +	return false;
> >   }
> >   
> >   struct stripe_head *
> > @@ -2877,34 +2881,29 @@ static void raid5_error(struct mddev
> > *mddev, struct md_rdev *rdev) unsigned long flags;
> >   	pr_debug("raid456: error called\n");
> >   
> > +	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 (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
> > -		 */
> > +	if (has_failed(conf)) {
> >   		conf->recovery_disabled =
> > mddev->recovery_disabled;
> > -		spin_unlock_irqrestore(&conf->device_lock, flags);
> > -		return;  
> 
> Ok, I think commit fb73b357fb985cc652a72a41541d25915c7f9635 is
> effectively reverted by this hunk. So I would prefer to separate the
> revert part from this patch, just my 0.02$.

Song,
do you want revert?

Thanks,
Mariusz

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

[Guoqing Jiang]

Hi Mariusz,

On 2/22/22 10:18 PM, Mariusz Tkaczyk wrote:
>
>>>    
>>> -static int has_failed(struct r5conf *conf)
>>> +static bool has_failed(struct r5conf *conf)
>>>    {
>>> -	int degraded;
>>> +	int degraded = conf->mddev->degraded;
>>>    
>>> -	if (conf->mddev->reshape_position == MaxSector)
>>> -		return conf->mddev->degraded > conf->max_degraded;
>>> +	if (test_bit(MD_BROKEN, &conf->mddev->flags))
>>> +		return true;
>> If one member disk was set Faulty which caused BROKEN was set, is it
>> possible to re-add the same member disk again?
>>
> Is possible to re-add drive to failed raid5 array now? From my
> understanding of raid5_add_disk it is not possible.

I mean the below steps, it works as you can see.

>> [root@vm ~]# echo faulty > /sys/block/md0/md/dev-loop1/state
>> [root@vm ~]# cat /proc/mdstat
>> Personalities : [raid6] [raid5] [raid4]
>> md0 : active raid5 loop2[2] loop1[0](F)
>>         1046528 blocks super 1.2 level 5, 512k chunk, algorithm 2
>> [2/1] [_U] bitmap: 0/1 pages [0KB], 65536KB chunk
>>
>> unused devices: <none>
>> [root@vm ~]# echo re-add > /sys/block/md0/md/dev-loop1/state
>> [root@vm ~]# cat /proc/mdstat
>> Personalities : [raid6] [raid5] [raid4]
>> md0 : active raid5 loop2[2] loop1[0]
>>         1046528 blocks super 1.2 level 5, 512k chunk, algorithm 2
>> [2/2] [UU] bitmap: 0/1 pages [0KB], 65536KB chunk
>>
>> unused devices: <none>
>>
>> And have you run mdadm test against the series?
>>
> I run imsm test suite and our internal IMSM scope. I will take the
> challenge and will verify with native. Thanks for suggestion.

Cool, thank you.

BTW, I know the mdadm test suite is kind of broke, at least this one
which I aware.

https://lore.kernel.org/all/20220119055501.GD27703@xsang-OptiPlex-9020/

And given the complexity of md, the more we test, the less bug we can
avoid.


>>> -	degraded = raid5_calc_degraded(conf);
>>> -	if (degraded > conf->max_degraded)
>>> -		return 1;
>>> -	return 0;
>>> +	if (conf->mddev->reshape_position != MaxSector)
>>> +		degraded = raid5_calc_degraded(conf);
>>> +
>>> +	if (degraded > conf->max_degraded) {
>>> +		set_bit(MD_BROKEN, &conf->mddev->flags);
>> Why not set BROKEN flags in err handler to align with other levels? Or
>> do it in md_error only.
> https://lore.kernel.org/linux-raid/3da9324e-01e7-2a07-4bcd-14245db56693@linux.dev/
>
> You suggested that.
> Other levels doesn't have dedicates has_failed() routines. For raid5 it
> is reasonable to set it in has_failed().

When has_failed returns true which means MD_BROKEN should be set, if so,
then it makes sense to set it in raid5_error.


> I can't do that in md_error because I don't have such information in
> all cases. !test_bit("Faulty", rdev->flags) result varies.

Fair enough.

Thanks,
Guoqing

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

[Mariusz Tkaczyk]

On Fri, 25 Feb 2022 15:22:00 +0800
Guoqing Jiang <guoqing.jiang@linux.dev> wrote:

> >> If one member disk was set Faulty which caused BROKEN was set, is
> >> it possible to re-add the same member disk again?
> >>  
> > Is possible to re-add drive to failed raid5 array now? From my
> > understanding of raid5_add_disk it is not possible.  
> 
> I mean the below steps, it works as you can see.
> 
> >> [root@vm ~]# echo faulty > /sys/block/md0/md/dev-loop1/state
> >> [root@vm ~]# cat /proc/mdstat
> >> Personalities : [raid6] [raid5] [raid4]
> >> md0 : active raid5 loop2[2] loop1[0](F)
> >>         1046528 blocks super 1.2 level 5, 512k chunk, algorithm 2
> >> [2/1] [_U] bitmap: 0/1 pages [0KB], 65536KB chunk
> >>
> >> unused devices: <none>
> >> [root@vm ~]# echo re-add > /sys/block/md0/md/dev-loop1/state
> >> [root@vm ~]# cat /proc/mdstat
> >> Personalities : [raid6] [raid5] [raid4]
> >> md0 : active raid5 loop2[2] loop1[0]
> >>         1046528 blocks super 1.2 level 5, 512k chunk, algorithm 2
> >> [2/2] [UU] bitmap: 0/1 pages [0KB], 65536KB chunk
> >>
> >> unused devices: <none>


In this case array is not failed (it is degraded). For that reason I
think that my changes are not related.

# cat /proc/mdstat
Personalities : [raid6] [raid5] [raid4] [raid1] [raid10]
md127 : active raid5 nvme5n1[1] nvme4n1[0](F)
      5242880 blocks super 1.2 level 5, 512k chunk, algorithm 2 [2/1]
[_U]

unused devices: <none>
# cat /sys/block/md127/md/array_state clean

# mdadm -D /dev/md127
/dev/md127:
           Version : 1.2
     Creation Time : Thu Mar  3 18:49:53 2022
        Raid Level : raid5
        Array Size : 5242880 (5.00 GiB 5.37 GB)
     Used Dev Size : 5242880 (5.00 GiB 5.37 GB)
      Raid Devices : 2
     Total Devices : 2
       Persistence : Superblock is persistent

       Update Time : Thu Mar  3 18:52:46 2022
             State : clean, degraded
    Active Devices : 1
   Working Devices : 1
    Failed Devices : 1
     Spare Devices : 0

            Layout : left-symmetric
        Chunk Size : 512K

Consistency Policy : resync

              Name : gklab-localhost:vol  (local to host
              gklab-localhost)
              UUID : 711594e8:73ef988c:87a85085:b30c838d
            Events : 8

    Number   Major   Minor   RaidDevice State
       -       0        0        0      removed
       1     259        9        1      active sync   /dev/nvme5n1

       0     259        5        -      faulty   /dev/nvme4n1


Do I miss something?

Thanks,
Mariusz

[PATCH 0/3] Failed array handling improvements

[Mariusz Tkaczyk]

Hi Song,
In v4 following changes were made:
- raid1_error and raid10_error description reworked, suggested by Guoqing.
- Error messages for raid0 and linear reworked, suggested by Guoqing.
- check for sync_request replaced by level checks, suggested by Guoqing.

I did manual (mainly on IMSM) tests, here my next TODOs in mdadm:
- broken state handling for redundant arrays in mdadm (it is exposed in sysfs).
  Currently it is working same as before, because broken is not a case for
  redundant arrays in mdadm. It requires to deal with already defined "FAILED"
  state in mdadm.
- Blocking manual removal of devices (#mdadm --set-faulty).

I run following native mdadm tests with and without changes, all passed:
#./test --disks=/dev/nullb[1-5] --tests=00raid1,00raid4,00raid5,00raid6,01r1fail,
01r5fail,01replace,02r1add,05r1failfast,05r1re-add,05r1re-add-nosuper

Mariusz Tkaczyk (3):
  raid0, linear, md: add error_handlers for raid0 and linear
  md: Set MD_BROKEN for RAID1 and RAID10
  raid5: introduce MD_BROKEN

 drivers/md/md-linear.c | 14 +++++++-
 drivers/md/md.c        | 30 +++++++++++-------
 drivers/md/md.h        | 72 ++++++++++++++++++++++--------------------
 drivers/md/raid0.c     | 14 +++++++-
 drivers/md/raid1.c     | 43 +++++++++++++++----------
 drivers/md/raid10.c    | 40 +++++++++++++----------
 drivers/md/raid5.c     | 48 ++++++++++++++--------------
 7 files changed, 155 insertions(+), 106 deletions(-)

-- 
2.26.2

[PATCH 3/3] raid5: introduce MD_BROKEN

[Mariusz Tkaczyk]

Raid456 module had allowed to achieve failed state. It was fixed by
fb73b357fb9 ("raid5: block failing device if raid will be failed").
This fix introduces a bug, now if raid5 fails during IO, it may result
with a hung task without completion. Faulty flag on the device is
necessary to process all requests and is checked many times, mainly in
analyze_stripe().
Allow to set faulty on drive again and set MD_BROKEN if raid is failed.

As a result, this level is allowed to achieve failed state again, but
communication with userspace (via -EBUSY status) will be preserved.

This restores possibility to fail array via #mdadm --set-faulty command
and will be fixed by additional verification on mdadm side.

Reproduction steps:
 mdadm -CR imsm -e imsm -n 3 /dev/nvme[0-2]n1
 mdadm -CR r5 -e imsm -l5 -n3 /dev/nvme[0-2]n1 --assume-clean
 mkfs.xfs /dev/md126 -f
 mount /dev/md126 /mnt/root/

 fio --filename=/mnt/root/file --size=5GB --direct=1 --rw=randrw
--bs=64k --ioengine=libaio --iodepth=64 --runtime=240 --numjobs=4
--time_based --group_reporting --name=throughput-test-job
--eta-newline=1 &

 echo 1 > /sys/block/nvme2n1/device/device/remove
 echo 1 > /sys/block/nvme1n1/device/device/remove

 [ 1475.787779] Call Trace:
 [ 1475.793111] __schedule+0x2a6/0x700
 [ 1475.799460] schedule+0x38/0xa0
 [ 1475.805454] raid5_get_active_stripe+0x469/0x5f0 [raid456]
 [ 1475.813856] ? finish_wait+0x80/0x80
 [ 1475.820332] raid5_make_request+0x180/0xb40 [raid456]
 [ 1475.828281] ? finish_wait+0x80/0x80
 [ 1475.834727] ? finish_wait+0x80/0x80
 [ 1475.841127] ? finish_wait+0x80/0x80
 [ 1475.847480] md_handle_request+0x119/0x190
 [ 1475.854390] md_make_request+0x8a/0x190
 [ 1475.861041] generic_make_request+0xcf/0x310
 [ 1475.868145] submit_bio+0x3c/0x160
 [ 1475.874355] iomap_dio_submit_bio.isra.20+0x51/0x60
 [ 1475.882070] iomap_dio_bio_actor+0x175/0x390
 [ 1475.889149] iomap_apply+0xff/0x310
 [ 1475.895447] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.902736] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.909974] iomap_dio_rw+0x2f2/0x490
 [ 1475.916415] ? iomap_dio_bio_actor+0x390/0x390
 [ 1475.923680] ? atime_needs_update+0x77/0xe0
 [ 1475.930674] ? xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
 [ 1475.938455] xfs_file_dio_aio_read+0x6b/0xe0 [xfs]
 [ 1475.946084] xfs_file_read_iter+0xba/0xd0 [xfs]
 [ 1475.953403] aio_read+0xd5/0x180
 [ 1475.959395] ? _cond_resched+0x15/0x30
 [ 1475.965907] io_submit_one+0x20b/0x3c0
 [ 1475.972398] __x64_sys_io_submit+0xa2/0x180
 [ 1475.979335] ? do_io_getevents+0x7c/0xc0
 [ 1475.986009] do_syscall_64+0x5b/0x1a0
 [ 1475.992419] entry_SYSCALL_64_after_hwframe+0x65/0xca
 [ 1476.000255] RIP: 0033:0x7f11fc27978d
 [ 1476.006631] Code: Bad RIP value.
 [ 1476.073251] INFO: task fio:3877 blocked for more than 120 seconds.

Cc: stable@vger.kernel.org
Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 drivers/md/raid5.c | 48 +++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 7c119208a214..4d76e3a89aa5 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -686,17 +686,20 @@ int raid5_calc_degraded(struct r5conf *conf)
 	return degraded;
 }
 
-static int has_failed(struct r5conf *conf)
+static bool has_failed(struct r5conf *conf)
 {
-	int degraded;
+	int degraded = conf->mddev->degraded;
 
-	if (conf->mddev->reshape_position == MaxSector)
-		return conf->mddev->degraded > conf->max_degraded;
+	if (test_bit(MD_BROKEN, &conf->mddev->flags))
+		return true;
+
+	if (conf->mddev->reshape_position != MaxSector)
+		degraded = raid5_calc_degraded(conf);
 
-	degraded = raid5_calc_degraded(conf);
 	if (degraded > conf->max_degraded)
-		return 1;
-	return 0;
+		return true;
+
+	return false;
 }
 
 struct stripe_head *
@@ -2877,34 +2880,31 @@ static void raid5_error(struct mddev *mddev, struct md_rdev *rdev)
 	unsigned long flags;
 	pr_debug("raid456: error called\n");
 
+	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 (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
-		 */
+	if (has_failed(conf)) {
+		set_bit(MD_BROKEN, &conf->mddev->flags);
 		conf->recovery_disabled = mddev->recovery_disabled;
-		spin_unlock_irqrestore(&conf->device_lock, flags);
-		return;
+
+		pr_crit("md/raid:%s: Cannot continue operation (%d/%d failed).\n",
+			mdname(mddev), mddev->degraded, conf->raid_disks);
+	} else {
+		pr_crit("md/raid:%s: Operation continuing on %d devices.\n",
+			mdname(mddev), conf->raid_disks - mddev->degraded);
 	}
 
-	set_bit(Faulty, &rdev->flags);
-	clear_bit(In_sync, &rdev->flags);
-	mddev->degraded = raid5_calc_degraded(conf);
 	spin_unlock_irqrestore(&conf->device_lock, flags);
 	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 
 	set_bit(Blocked, &rdev->flags);
 	set_mask_bits(&mddev->sb_flags, 0,
 		      BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
-	pr_crit("md/raid:%s: Disk failure on %s, disabling device.\n"
-		"md/raid:%s: Operation continuing on %d devices.\n",
-		mdname(mddev),
-		bdevname(rdev->bdev, b),
-		mdname(mddev),
-		conf->raid_disks - mddev->degraded);
 	r5c_update_on_rdev_error(mddev, rdev);
 }
 
-- 
2.26.2

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

[Song Liu]

On Tue, Mar 22, 2022 at 8:24 AM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Raid456 module had allowed to achieve failed state. It was fixed by
> fb73b357fb9 ("raid5: block failing device if raid will be failed").
> This fix introduces a bug, now if raid5 fails during IO, it may result
> with a hung task without completion. Faulty flag on the device is
> necessary to process all requests and is checked many times, mainly in
> analyze_stripe().
[...]

> Cc: stable@vger.kernel.org
> Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>  drivers/md/raid5.c | 48 +++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 7c119208a214..4d76e3a89aa5 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -686,17 +686,20 @@ int raid5_calc_degraded(struct r5conf *conf)
>         return degraded;
>  }
>
> -static int has_failed(struct r5conf *conf)
> +static bool has_failed(struct r5conf *conf)
>  {
> -       int degraded;
> +       int degraded = conf->mddev->degraded;
>
> -       if (conf->mddev->reshape_position == MaxSector)
> -               return conf->mddev->degraded > conf->max_degraded;
> +       if (test_bit(MD_BROKEN, &conf->mddev->flags))
> +               return true;
> +
> +       if (conf->mddev->reshape_position != MaxSector)
> +               degraded = raid5_calc_degraded(conf);
>
> -       degraded = raid5_calc_degraded(conf);
>         if (degraded > conf->max_degraded)

nit: we can just do
   return degraded > conf->max_degraded;

[...]