mdadm: using ioctl to set disk faulty instead of sysfs

mdadm: using ioctl to set disk faulty instead of sysfs

[wuzhouhui]

Hi, I have a suggest about mdadm to set disk faulty. Since commit 1ca69c4bc4b1ef
(md: avoid taking the mutex on some ioctls) removes lock when set disk faulty,
so we'd better using ioctl(SET_DISK_FAULTY) to set disk faulty, instead of 
echo faulty > /sys/block/<md>/md/dev-<disk>/state, because caller of state's
handler would lock mddev. Like following:

Index: mdadm-4.0/Manage.c
===================================================================
--- mdadm-4.0.orig/Manage.c
+++ mdadm-4.0/Manage.c
@@ -1662,9 +1662,7 @@ int Manage_subdevs(char *devname, int fd
 
 		case 'f': /* set faulty */
 			/* FIXME check current member */
-			if ((sysfd >= 0 && write(sysfd, "faulty", 6) != 6) ||
-			    (sysfd < 0 && ioctl(fd, SET_DISK_FAULTY,
-						rdev))) {
+			if (ioctl(fd, SET_DISK_FAULTY, rdev)) {
 				if (errno == EBUSY)
 					busy = 1;
 				pr_err("set device faulty failed for %s:  %s\n",

Re: mdadm: using ioctl to set disk faulty instead of sysfs

[Shaohua Li]

On Tue, Nov 21, 2017 at 09:09:46PM +0800, wuzhouhui wrote:
> Hi, I have a suggest about mdadm to set disk faulty. Since commit 1ca69c4bc4b1ef
> (md: avoid taking the mutex on some ioctls) removes lock when set disk faulty,
> so we'd better using ioctl(SET_DISK_FAULTY) to set disk faulty, instead of 
> echo faulty > /sys/block/<md>/md/dev-<disk>/state, because caller of state's
> handler would lock mddev. Like following:

While avoiding the lock is good of course, this isn't a hot path, this doesn't
change anything. Did you see lock contention because of this?

Thanks,
Shaohua
 
> Index: mdadm-4.0/Manage.c
> ===================================================================
> --- mdadm-4.0.orig/Manage.c
> +++ mdadm-4.0/Manage.c
> @@ -1662,9 +1662,7 @@ int Manage_subdevs(char *devname, int fd
>  
>  		case 'f': /* set faulty */
>  			/* FIXME check current member */
> -			if ((sysfd >= 0 && write(sysfd, "faulty", 6) != 6) ||
> -			    (sysfd < 0 && ioctl(fd, SET_DISK_FAULTY,
> -						rdev))) {
> +			if (ioctl(fd, SET_DISK_FAULTY, rdev)) {
>  				if (errno == EBUSY)
>  					busy = 1;
>  				pr_err("set device faulty failed for %s:  %s\n",
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mdadm: using ioctl to set disk faulty instead of sysfs

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1356 bytes --]

On Tue, Nov 21 2017, wuzhouhui wrote:

> Hi, I have a suggest about mdadm to set disk faulty. Since commit 1ca69c4bc4b1ef
> (md: avoid taking the mutex on some ioctls) removes lock when set disk faulty,
> so we'd better using ioctl(SET_DISK_FAULTY) to set disk faulty, instead of 
> echo faulty > /sys/block/<md>/md/dev-<disk>/state, because caller of state's
> handler would lock mddev. Like following:
>
> Index: mdadm-4.0/Manage.c
> ===================================================================
> --- mdadm-4.0.orig/Manage.c
> +++ mdadm-4.0/Manage.c
> @@ -1662,9 +1662,7 @@ int Manage_subdevs(char *devname, int fd
>  
>  		case 'f': /* set faulty */
>  			/* FIXME check current member */
> -			if ((sysfd >= 0 && write(sysfd, "faulty", 6) != 6) ||
> -			    (sysfd < 0 && ioctl(fd, SET_DISK_FAULTY,
> -						rdev))) {
> +			if (ioctl(fd, SET_DISK_FAULTY, rdev)) {
>  				if (errno == EBUSY)
>  					busy = 1;
>  				pr_err("set device faulty failed for %s:  %s\n",
>

This isn't just a debatable optimization.  It is wrong and would
introduce a regression.
Please make sure you understand code *before* trying to change it.

Please look at the git history for this code and find the patch where
writing "faulty" was added as an alternative.  That might help you to
understand what is happening here.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]