| Message ID | 20230913085502.17856-1-mariusz.tkaczyk@linux.intel.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Delegated to: | Song Liu |
| Headers | show |
| Series | md: do not require mddev_lock() for all options | expand |
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
在 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 > . >
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
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; }
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(-)