diff mbox series

[v2] md: do not require mddev_lock() for all options

Message ID 20230928125517.12356-1-mariusz.tkaczyk@linux.intel.com (mailing list archive)
State Accepted, archived
Headers show
Series [v2] md: do not require mddev_lock() for all options | expand

Commit Message

Mariusz Tkaczyk Sept. 28, 2023, 12:55 p.m. UTC 
We don't need to lock device to reject not supported request
in array_state_store(). No functional changes intended.

There are differences between ioctl and sysfs handling during stopping.
With this change, it will be easier to add additional steps which needs
to be completed before mddev_lock() is taken.

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
Song, feel free to remove second chapter if you think it is confusing.

 drivers/md/md.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

Comments

Song Liu Sept. 28, 2023, 8:44 p.m. UTC | #1 
On Thu, Sep 28, 2023 at 5:55 AM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> We don't need to lock device to reject not supported request
> in array_state_store(). No functional changes intended.
>
> There are differences between ioctl and sysfs handling during stopping.
> With this change, it will be easier to add additional steps which needs
> to be completed before mddev_lock() is taken.
>
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Applied to md-next. Thanks!

Song


> ---
> Song, feel free to remove second chapter if you think it is confusing.
>
>  drivers/md/md.c | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index b8f232840f7c..469b1376e66c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4359,6 +4359,18 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
>         int err = 0;
>         enum array_state st = match_word(buf, array_states);
>
> +       /* No lock dependent actions */
> +       switch (st) {
> +       case suspended:         /* not supported yet */
> +       case write_pending:     /* cannot be set */
> +       case active_idle:       /* cannot be set */
> +       case broken:            /* cannot be set */
> +       case bad_word:
> +               return -EINVAL;
> +       default:
> +               break;
> +       }
> +
>         if (mddev->pers && (st == active || st == clean) &&
>             mddev->ro != MD_RDONLY) {
>                 /* don't take reconfig_mutex when toggling between
> @@ -4383,23 +4395,16 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
>         err = mddev_lock(mddev);
>         if (err)
>                 return err;
> -       err = -EINVAL;
> -       switch(st) {
> -       case bad_word:
> -               break;
> -       case clear:
> -               /* stopping an active array */
> -               err = do_md_stop(mddev, 0, NULL);
> -               break;
> +
> +       switch (st) {
>         case inactive:
> -               /* stopping an active array */
> +               /* stop an active array, return 0 otherwise */
>                 if (mddev->pers)
>                         err = do_md_stop(mddev, 2, NULL);
> -               else
> -                       err = 0; /* already inactive */
>                 break;
> -       case suspended:
> -               break; /* not supported yet */
> +       case clear:
> +               err = do_md_stop(mddev, 0, NULL);
> +               break;
>         case readonly:
>                 if (mddev->pers)
>                         err = md_set_readonly(mddev, NULL);
> @@ -4450,10 +4455,8 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
>                         err = do_md_run(mddev);
>                 }
>                 break;
> -       case write_pending:
> -       case active_idle:
> -       case broken:
> -               /* these cannot be set */
> +       default:
> +               err = -EINVAL;
>                 break;
>         }
>
> --
> 2.26.2
>
diff mbox series

Patch

diff --git a/drivers/md/md.c b/drivers/md/md.c
index b8f232840f7c..469b1376e66c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4359,6 +4359,18 @@  array_state_store(struct mddev *mddev, const char *buf, size_t len)
 	int err = 0;
 	enum array_state st = match_word(buf, array_states);
 
+	/* No lock dependent actions */
+	switch (st) {
+	case suspended:		/* not supported yet */
+	case write_pending:	/* cannot be set */
+	case active_idle:	/* cannot be set */
+	case broken:		/* cannot be set */
+	case bad_word:
+		return -EINVAL;
+	default:
+		break;
+	}
+
 	if (mddev->pers && (st == active || st == clean) &&
 	    mddev->ro != MD_RDONLY) {
 		/* don't take reconfig_mutex when toggling between
@@ -4383,23 +4395,16 @@  array_state_store(struct mddev *mddev, const char *buf, size_t len)
 	err = mddev_lock(mddev);
 	if (err)
 		return err;
-	err = -EINVAL;
-	switch(st) {
-	case bad_word:
-		break;
-	case clear:
-		/* stopping an active array */
-		err = do_md_stop(mddev, 0, NULL);
-		break;
+
+	switch (st) {
 	case inactive:
-		/* stopping an active array */
+		/* stop an active array, return 0 otherwise */
 		if (mddev->pers)
 			err = do_md_stop(mddev, 2, NULL);
-		else
-			err = 0; /* already inactive */
 		break;
-	case suspended:
-		break; /* not supported yet */
+	case clear:
+		err = do_md_stop(mddev, 0, NULL);
+		break;
 	case readonly:
 		if (mddev->pers)
 			err = md_set_readonly(mddev, NULL);
@@ -4450,10 +4455,8 @@  array_state_store(struct mddev *mddev, const char *buf, size_t len)
 			err = do_md_run(mddev);
 		}
 		break;
-	case write_pending:
-	case active_idle:
-	case broken:
-		/* these cannot be set */
+	default:
+		err = -EINVAL;
 		break;
 	}