On Tue, 15 Feb 2022 11:43:34 +0800
Guoqing Jiang [off-list ref] wrote:

quoted

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"?

Yes, now it will be able to impose failed state. This is a regression I
caused and I'm aware of that.

quoted

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.

The device is never marked as failed. I checked write paths (because I
introduced more aggressive policy for writes) and if we are in a
critical array, failfast flag is not added to bio for both raid1 and
radi10, see raid1_write_request().

[ ... ] 

quoted

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.

I will merge errors to one message.

quoted

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.

Got it.

Thanks,
Mariusz