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@xxxxxxxxxxxxxxx>
---
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
