On 2/14/22 5:37 PM, Mariusz Tkaczyk wrote:

On Sat, 12 Feb 2022 09:12:00 +0800
Guoqing Jiang [off-list ref] wrote:

quoted

On 1/27/22 11:39 PM, Mariusz Tkaczyk wrote:

quoted

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.

I think the purpose of them are quite different, as said before,
error_handler
is mostly against rdev while is_mddev_broken is for mddev though it
needs to test rdev first.

I changed is_mddev_broken to is_rdev_broken, because it checks the
device now. On error it calls md_error and later error_handler.
I unified error handling for each level. Do you consider it as wrong?

I am neutral to the change

quoted

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.
What is the safe issue here? It would betterr to post mdadm fix
together.

We can and should block user from damaging raid even if it is
recoverable. It is a regression.

I don't follow, did you mean --set-fault from mdadm could "damaging raid"?

I will fix mdadm. I don't consider it as a big risk (because it is
recoverable) so I focused on kernel part first.

quoted

quoted

It is correctable because failed
state is not recorded in the metadata. After next assembly array
will be read-write again.

I don't think it is a problem, care to explain why it can't be RW
again?

failed state is not recoverable in runtime, so you need to recreate
array.

IIUC, the failfast flag is supposed to be set during transient error not
permanent failure, the rdev (marked as failfast) need to be revalidated
and readded to array.


[ ... ]

quoted

quoted

+		char *md_name = mdname(mddev);
+
+		pr_crit("md/linear%s: Disk failure on %pg
detected.\n"
+			"md/linear:%s: Cannot continue, failing
array.\n",
+			md_name, rdev->bdev, md_name);

The second md_name is not needed.

Could you elaborate here more? Do you want to skip device name in
second message?

Yes, we printed two md_name here, seems unnecessary.

[ ... ]

quoted

quoted

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

The above only valid for raid0 and linear, I guess it is fine if DM
don't create LV on top
of them. But the new checking deserves some comment above.

Will do, could you propose comment?

Or, just check if it is raid0 or linear directly instead of implies 
level with
redundancy.

Thanks,
Guoqing