Re: [PATCH v2 3/3] md: sync blockdev before stopping raid or setting readonly
From: Mariusz Tkaczyk <hidden>
Date: 2024-01-18 08:02:14
Also in:
lkml
On Wed, 17 Jan 2024 17:37:07 +0800 linan666@huaweicloud.com wrote:
quoted hunk ↗ jump to hunk
From: Li Nan <redacted> 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 <redacted> --- 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;
quoted hunk ↗ jump to hunk
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