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

[Song Liu <song@kernel.org>]

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
>