On Wed, 17 Jan 2024 17:37:07 +0800
linan666@xxxxxxxxxxxxxxx wrote:
> From: Li Nan <linan122@xxxxxxxxxx>
>
> Commit a05b7ea03d72 ("md: avoid crash when stopping md array races
> with closing other open fds.") added sync_block before stopping raid and
> setting readonly. Later in commit 260fa034ef7a ("md: avoid deadlock when
> dirty buffers during md_stop.") it is moved to ioctl. array_state_store()
> was ignored. Add sync blockdev to array_state_store() now.
>
> Signed-off-by: Li Nan <linan122@xxxxxxxxxx>
> ---
> drivers/md/md.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 2c793992a604..aea39598457c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4477,6 +4477,7 @@ array_state_store(struct mddev *mddev, const char *buf,
> size_t len) {
> int err = 0;
> enum array_state st = match_word(buf, array_states);
> + bool clear_md_closing = false;
>
> /* No lock dependent actions */
> switch (st) {
> @@ -4511,6 +4512,16 @@ array_state_store(struct mddev *mddev, const char
> *buf, size_t len) spin_unlock(&mddev->lock);
> return err ?: len;
> }
> +
> + /* we will call set readonly or stop raid, sync blockdev */
> + if (st == clear || (mddev->pers && (st == readonly ||
> + st == inactive || (st == read_auto && md_is_rdwr(mddev))))) {
> + err = mddev_sync_blockdev(mddev);
> + if (err)
> + return err;
> + clear_md_closing = true;
> + }
> +
Please reorganize it a little for readability:
I think if no mddev->pers we don't need to consider sync_blockdev at all. If
personality is there we can probably check for read-write. If it is not
read-write then nothing to sync. What about that:
if (mddev->pers && md_is_rdwr(mddev) &&
(st == clear || st == readonly || st == inactive || st == read_auto))
Please note that I didn't test it so please let me know if you see issue in
proposed logic.
I think that we may be able to include it in "/* No lock dependent actions */"
switch. Please consider it too:
case clear:
case readonly:
case inactive:
case read_auto:
if(!mddev->pers || !md_is_rdwr(mddev))
break;
err = mddev_sync_blockdev(mddev);
if (err)
return err;
clear_md_closing = true;
> err = mddev_lock(mddev);
> if (err)
> return err;
> @@ -4523,6 +4534,8 @@ array_state_store(struct mddev *mddev, const char *buf,
> size_t len) break;
> case clear:
> err = do_md_stop(mddev, 0, NULL);
> + if (!err)
> + clear_md_closing = false;
> break;
> case readonly:
> if (mddev->pers)
> @@ -4585,6 +4598,8 @@ array_state_store(struct mddev *mddev, const char *buf,
> size_t len) sysfs_notify_dirent_safe(mddev->sysfs_state);
> }
> mddev_unlock(mddev);
> + if (clear_md_closing)
> + clear_bit(MD_CLOSING, &mddev->flags);
Please add spaces before and after if.
> return err ?: len;
> }
> static struct md_sysfs_entry md_array_state =
Thanks,
Mariusz
