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

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

[Mariusz Tkaczyk]

We don't need to lock device to reject not supported request
in array_state_store().
Main motivation is to make a room for action does not require lock yet,
like prepare to stop (see md_ioctl()).

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
Code refactor, submitting it now because work in this area will be
postponed- we have the issue root caused.

 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 bb654ff62765..3b1a28a753e5 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4365,6 +4365,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
@@ -4389,23 +4401,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);
@@ -4456,10 +4461,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

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

[Song Liu]

Hi Mariusz,

Sorry for the late reply.

On Wed, Sep 13, 2023 at 1: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().
> Main motivation is to make a room for action does not require lock yet,
> like prepare to stop (see md_ioctl()).

I made some changes to the commit log:

    md: do not require mddev_lock() for all options

    We don't need to lock device to reject not supported request
    in array_state_store().
    Main motivation is to make a room for action does not require lock yet,
    like prepare to stop (see md_ioctl()).

But I am not sure what you meant by "make a room for action does not
require lock yet". Could you please explain?

Otherwise, the code looks reasonable to me.

Thanks,
Song

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

[Yu Kuai]

在 2023/09/23 5:04, Song Liu 写道:
> Hi Mariusz,
> 
> Sorry for the late reply.
> 
> On Wed, Sep 13, 2023 at 1: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().
>> Main motivation is to make a room for action does not require lock yet,
>> like prepare to stop (see md_ioctl()).
> 
> I made some changes to the commit log:
> 
>      md: do not require mddev_lock() for all options
> 
>      We don't need to lock device to reject not supported request
>      in array_state_store().
>      Main motivation is to make a room for action does not require lock yet,
>      like prepare to stop (see md_ioctl()).
> 
> But I am not sure what you meant by "make a room for action does not
> require lock yet". Could you please explain?

Yes, this sounds confusing, if 'action does not require lock', then it
shound not be blocked by array_state_store() with or without this patch.

> 
> Otherwise, the code looks reasonable to me.

Changes look good to me, after clarify commit message, feel free to add

Reviewed-by: Yu Kuai <yukuai3@huawei.com>
> 
> Thanks,
> Song
> .
> 

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

[Mariusz Tkaczyk]

On Mon, 25 Sep 2023 11:05:42 +0800
Yu Kuai <yukuai1@huaweicloud.com> wrote:

> 在 2023/09/23 5:04, Song Liu 写道:
> > Hi Mariusz,
> > 
> > Sorry for the late reply.
> > 
> > On Wed, Sep 13, 2023 at 1: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().
> >> Main motivation is to make a room for action does not require lock yet,
> >> like prepare to stop (see md_ioctl()).  
> > 
> > I made some changes to the commit log:
> > 
> >      md: do not require mddev_lock() for all options
> > 
> >      We don't need to lock device to reject not supported request
> >      in array_state_store().
> >      Main motivation is to make a room for action does not require lock yet,
> >      like prepare to stop (see md_ioctl()).
> > 
> > But I am not sure what you meant by "make a room for action does not
> > require lock yet". Could you please explain?  
> 
> Yes, this sounds confusing, if 'action does not require lock', then it
> shound not be blocked by array_state_store() with or without this patch.

In md_ioctl() we do some actions before stopping. We are verifying
how many holders are there (mddev->openers), we are setting MD_CLOSING and
sync_blockdev() is executed. I see that it is omitted in array_state_store().
https://elixir.bootlin.com/linux/latest/source/drivers/md/md.c#L7580

I meant that with this separated switch before locking mddev it is now easy to
add other actions, like mentioned code above for stopping.
> 
> > 
> > Otherwise, the code looks reasonable to me.  
> 
> Changes look good to me, after clarify commit message, feel free to add
> 
> Reviewed-by: Yu Kuai <yukuai3@huawei.com>

Thanks!
I will send v2.

Mariusz