* [PATCH v4 1/4] md: Don't clear MD_CLOSING when the raid is about to stop
2024-01-26 9:22 [PATCH v4 0/4] md: Don't clear MD_CLOSING when the raid is about to stop linan666
@ 2024-01-26 9:22 ` linan666
2024-01-27 8:38 ` Yu Kuai
2024-01-26 9:22 ` [PATCH v4 2/4] md: factor out a helper to sync mddev linan666
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: linan666 @ 2024-01-26 9:22 UTC (permalink / raw)
To: song, neilb, shli, mariusz.tkaczyk
Cc: linux-raid, linux-kernel, linan666, yukuai3, yi.zhang, houtao1,
yangerkun
From: Li Nan <linan122@huawei.com>
The raid should not be opened anymore when it is about to be stopped.
However, other processes can open it again if the flag MD_CLOSING is
cleared before exiting. From now on, this flag will not be cleared when
the raid will be stopped.
Fixes: 065e519e71b2 ("md: MD_CLOSING needs to be cleared after called md_set_readonly or do_md_stop")
Signed-off-by: Li Nan <linan122@huawei.com>
Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
drivers/md/md.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 9bdd57324c37..06550fe34aa1 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6254,7 +6254,15 @@ static void md_clean(struct mddev *mddev)
mddev->persistent = 0;
mddev->level = LEVEL_NONE;
mddev->clevel[0] = 0;
- mddev->flags = 0;
+ /*
+ * Don't clear MD_CLOSING, or mddev can be opened again.
+ * 'hold_active != 0' means mddev is still in the creation
+ * process and will be used later.
+ */
+ if (mddev->hold_active)
+ mddev->flags = 0;
+ else
+ mddev->flags &= BIT_ULL_MASK(MD_CLOSING);
mddev->sb_flags = 0;
mddev->ro = MD_RDWR;
mddev->metadata_type[0] = 0;
@@ -7600,7 +7608,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
int err = 0;
void __user *argp = (void __user *)arg;
struct mddev *mddev = NULL;
- bool did_set_md_closing = false;
+ bool clear_md_closing = false;
if (!md_ioctl_valid(cmd))
return -ENOTTY;
@@ -7684,7 +7692,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
err = -EBUSY;
goto out;
}
- did_set_md_closing = true;
+ clear_md_closing = true;
mutex_unlock(&mddev->open_mutex);
sync_blockdev(bdev);
}
@@ -7728,6 +7736,12 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
case STOP_ARRAY:
err = do_md_stop(mddev, 0, bdev);
+ if (!err)
+ /*
+ * mddev has been stopped, keep the flag
+ * MD_CLOSING to prevent reuse.
+ */
+ clear_md_closing = false;
goto unlock;
case STOP_ARRAY_RO:
@@ -7826,7 +7840,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
mddev_unlock(mddev);
out:
- if(did_set_md_closing)
+ if (clear_md_closing)
clear_bit(MD_CLOSING, &mddev->flags);
return err;
}
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v4 1/4] md: Don't clear MD_CLOSING when the raid is about to stop
2024-01-26 9:22 ` [PATCH v4 1/4] " linan666
@ 2024-01-27 8:38 ` Yu Kuai
0 siblings, 0 replies; 6+ messages in thread
From: Yu Kuai @ 2024-01-27 8:38 UTC (permalink / raw)
To: linan666, song, neilb, shli, mariusz.tkaczyk
Cc: linux-raid, linux-kernel, yi.zhang, houtao1, yangerkun, yukuai (C)
Hi,
在 2024/01/26 17:22, linan666@huaweicloud.com 写道:
> From: Li Nan <linan122@huawei.com>
>
> The raid should not be opened anymore when it is about to be stopped.
> However, other processes can open it again if the flag MD_CLOSING is
> cleared before exiting. From now on, this flag will not be cleared when
> the raid will be stopped.
This patch looks good, just one nit below:
>
> Fixes: 065e519e71b2 ("md: MD_CLOSING needs to be cleared after called md_set_readonly or do_md_stop")
> Signed-off-by: Li Nan <linan122@huawei.com>
> Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
> drivers/md/md.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 9bdd57324c37..06550fe34aa1 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6254,7 +6254,15 @@ static void md_clean(struct mddev *mddev)
> mddev->persistent = 0;
> mddev->level = LEVEL_NONE;
> mddev->clevel[0] = 0;
> - mddev->flags = 0;
> + /*
> + * Don't clear MD_CLOSING, or mddev can be opened again.
> + * 'hold_active != 0' means mddev is still in the creation
> + * process and will be used later.
> + */
> + if (mddev->hold_active)
> + mddev->flags = 0;
> + else
> + mddev->flags &= BIT_ULL_MASK(MD_CLOSING);
> mddev->sb_flags = 0;
> mddev->ro = MD_RDWR;
> mddev->metadata_type[0] = 0;
> @@ -7600,7 +7608,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
> int err = 0;
> void __user *argp = (void __user *)arg;
> struct mddev *mddev = NULL;
> - bool did_set_md_closing = false;
> + bool clear_md_closing = false;
>
> if (!md_ioctl_valid(cmd))
> return -ENOTTY;
> @@ -7684,7 +7692,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
> err = -EBUSY;
> goto out;
> }
> - did_set_md_closing = true;
> + clear_md_closing = true;
> mutex_unlock(&mddev->open_mutex);
> sync_blockdev(bdev);
> }
> @@ -7728,6 +7736,12 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
>
> case STOP_ARRAY:
> err = do_md_stop(mddev, 0, bdev);
> + if (!err)
> + /*
> + * mddev has been stopped, keep the flag
> + * MD_CLOSING to prevent reuse.
> + */
> + clear_md_closing = false;
> goto unlock;
>
> case STOP_ARRAY_RO:
> @@ -7826,7 +7840,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
> mddev_unlock(mddev);
>
> out:
> - if(did_set_md_closing)
> + if (clear_md_closing)
I think code will be simplier if we just remove this local variable, and
replace this with:
if (test_bit(MD_CLOSING, &mddev->flags) && (err || cmd == STOP_ARRAY_RO))
And the same for patch 3.
Thanks,
Kuai
> clear_bit(MD_CLOSING, &mddev->flags);
> return err;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 2/4] md: factor out a helper to sync mddev
2024-01-26 9:22 [PATCH v4 0/4] md: Don't clear MD_CLOSING when the raid is about to stop linan666
2024-01-26 9:22 ` [PATCH v4 1/4] " linan666
@ 2024-01-26 9:22 ` linan666
2024-01-26 9:22 ` [PATCH v4 3/4] md: sync blockdev before stopping raid or setting readonly linan666
2024-01-26 9:22 ` [PATCH v4 4/4] md: check mddev->pers before calling md_set_readonly() linan666
3 siblings, 0 replies; 6+ messages in thread
From: linan666 @ 2024-01-26 9:22 UTC (permalink / raw)
To: song, neilb, shli, mariusz.tkaczyk
Cc: linux-raid, linux-kernel, linan666, yukuai3, yi.zhang, houtao1,
yangerkun
From: Li Nan <linan122@huawei.com>
There are no functional changes, prepare to sync mddev in
array_state_store().
Signed-off-by: Li Nan <linan122@huawei.com>
Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
drivers/md/md.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 06550fe34aa1..40ab5c7ce394 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -529,6 +529,24 @@ void mddev_resume(struct mddev *mddev)
}
EXPORT_SYMBOL_GPL(mddev_resume);
+/* sync bdev before setting device to readonly or stopping raid*/
+static int mddev_set_closing_and_sync_blockdev(struct mddev *mddev)
+{
+ mutex_lock(&mddev->open_mutex);
+ if (mddev->pers && atomic_read(&mddev->openers) > 1) {
+ mutex_unlock(&mddev->open_mutex);
+ return -EBUSY;
+ }
+ if (test_and_set_bit(MD_CLOSING, &mddev->flags)) {
+ mutex_unlock(&mddev->open_mutex);
+ return -EBUSY;
+ }
+ mutex_unlock(&mddev->open_mutex);
+
+ sync_blockdev(mddev->gendisk->part0);
+ return 0;
+}
+
/*
* Generic flush handling for md
*/
@@ -7681,20 +7699,10 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
/* Need to flush page cache, and ensure no-one else opens
* and writes
*/
- mutex_lock(&mddev->open_mutex);
- if (mddev->pers && atomic_read(&mddev->openers) > 1) {
- mutex_unlock(&mddev->open_mutex);
- err = -EBUSY;
- goto out;
- }
- if (test_and_set_bit(MD_CLOSING, &mddev->flags)) {
- mutex_unlock(&mddev->open_mutex);
- err = -EBUSY;
+ err = mddev_set_closing_and_sync_blockdev(mddev);
+ if (err)
goto out;
- }
clear_md_closing = true;
- mutex_unlock(&mddev->open_mutex);
- sync_blockdev(bdev);
}
if (!md_is_rdwr(mddev))
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v4 3/4] md: sync blockdev before stopping raid or setting readonly
2024-01-26 9:22 [PATCH v4 0/4] md: Don't clear MD_CLOSING when the raid is about to stop linan666
2024-01-26 9:22 ` [PATCH v4 1/4] " linan666
2024-01-26 9:22 ` [PATCH v4 2/4] md: factor out a helper to sync mddev linan666
@ 2024-01-26 9:22 ` linan666
2024-01-26 9:22 ` [PATCH v4 4/4] md: check mddev->pers before calling md_set_readonly() linan666
3 siblings, 0 replies; 6+ messages in thread
From: linan666 @ 2024-01-26 9:22 UTC (permalink / raw)
To: song, neilb, shli, mariusz.tkaczyk
Cc: linux-raid, linux-kernel, linan666, yukuai3, yi.zhang, houtao1,
yangerkun
From: Li Nan <linan122@huawei.com>
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@huawei.com>
---
drivers/md/md.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 40ab5c7ce394..24e33b65c6ff 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4478,6 +4478,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) {
@@ -4487,6 +4488,17 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
case broken: /* cannot be set */
case bad_word:
return -EINVAL;
+ case clear:
+ case readonly:
+ case inactive:
+ case read_auto:
+ if (!mddev->pers || !md_is_rdwr(mddev))
+ break;
+ err = mddev_set_closing_and_sync_blockdev(mddev);
+ if (err)
+ return err;
+ clear_md_closing = true;
+ break;
default:
break;
}
@@ -4512,6 +4524,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
spin_unlock(&mddev->lock);
return err ?: len;
}
+
err = mddev_lock(mddev);
if (err)
return err;
@@ -4524,6 +4537,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)
@@ -4586,6 +4601,10 @@ 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);
+
return err ?: len;
}
static struct md_sysfs_entry md_array_state =
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v4 4/4] md: check mddev->pers before calling md_set_readonly()
2024-01-26 9:22 [PATCH v4 0/4] md: Don't clear MD_CLOSING when the raid is about to stop linan666
` (2 preceding siblings ...)
2024-01-26 9:22 ` [PATCH v4 3/4] md: sync blockdev before stopping raid or setting readonly linan666
@ 2024-01-26 9:22 ` linan666
3 siblings, 0 replies; 6+ messages in thread
From: linan666 @ 2024-01-26 9:22 UTC (permalink / raw)
To: song, neilb, shli, mariusz.tkaczyk
Cc: linux-raid, linux-kernel, linan666, yukuai3, yi.zhang, houtao1,
yangerkun
From: Li Nan <linan122@huawei.com>
If 'mddev->pers' is NULL, there is nothing to do in md_set_readonly().
To simplify the code, move the check of 'mddev->pers' to the caller of
md_set_readonly().
Signed-off-by: Li Nan <linan122@huawei.com>
---
drivers/md/md.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 24e33b65c6ff..ece6eddb2748 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6412,6 +6412,7 @@ void md_stop(struct mddev *mddev)
EXPORT_SYMBOL_GPL(md_stop);
+/* ensure 'mddev->pers' exist before calling md_set_readonly() */
static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
{
int err = 0;
@@ -6432,28 +6433,25 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
mddev_lock_nointr(mddev);
mutex_lock(&mddev->open_mutex);
- if ((mddev->pers && atomic_read(&mddev->openers) > !!bdev) ||
- mddev->sync_thread ||
+ if (atomic_read(&mddev->openers) > !!bdev || mddev->sync_thread ||
test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
pr_warn("md: %s still in use.\n",mdname(mddev));
err = -EBUSY;
goto out;
}
- if (mddev->pers) {
- __md_stop_writes(mddev);
-
- if (mddev->ro == MD_RDONLY) {
- err = -ENXIO;
- goto out;
- }
+ __md_stop_writes(mddev);
- mddev->ro = MD_RDONLY;
- set_disk_ro(mddev->gendisk, 1);
+ if (mddev->ro == MD_RDONLY) {
+ err = -ENXIO;
+ goto out;
}
+ mddev->ro = MD_RDONLY;
+ set_disk_ro(mddev->gendisk, 1);
+
out:
- if ((mddev->pers && !err) || did_freeze) {
+ if (!err || did_freeze) {
clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
md_wakeup_thread(mddev->thread);
@@ -7772,7 +7770,8 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
goto unlock;
case STOP_ARRAY_RO:
- err = md_set_readonly(mddev, bdev);
+ if (mddev->pers)
+ err = md_set_readonly(mddev, bdev);
goto unlock;
case HOT_REMOVE_DISK:
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread