From: Li Nan <[email protected]>
Changes in v6:
- in patch 2, return directly.
- in patch 4, return directly in case GET_DISK_INFO and GET_ARRAY_INFO.
- in patch 7, rewrite commit message.
- add patch 8, clean up openers check.
Changes in v5:
- add patches 1-4 to clean up md_ioct(), pathc 4 can help us clean up
local variable 'clear_md_closing'.
- in patches 5 and 7, clean up local variable 'clear_md_closing'.
By the way, md_ioctl() is not readable now, I wanna to re-write it later
to make it only have one 'switch' like other drivers.
Li Nan (9):
md: merge the check of capabilities into md_ioctl_valid()
md: changed the switch of RAID_VERSION to if
md: clean up invalid BUG_ON in md_ioctl
md: return directly before setting did_set_md_closing
md: Don't clear MD_CLOSING when the raid is about to stop
md: factor out a helper to sync mddev
md: sync blockdev before stopping raid or setting readonly
md: clean up openers check in do_md_stop() and md_set_readonly()
md: check mddev->pers before calling md_set_readonly()
drivers/md/md.c | 190 ++++++++++++++++++++++++------------------------
1 file changed, 94 insertions(+), 96 deletions(-)
--
2.39.2
From: Li Nan <[email protected]>
'disk->private_data' is set to mddev in md_alloc() and never set to NULL,
and users need to open mddev before submitting ioctl. So mddev must not
have been freed during ioctl, and there is no need to check mddev here.
Clean up it.
Signed-off-by: Li Nan <[email protected]>
Reviewed-by: Yu Kuai <[email protected]>
---
drivers/md/md.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index ea147348bd24..fb4e80a0aa9a 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7645,11 +7645,6 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
mddev = bdev->bd_disk->private_data;
- if (!mddev) {
- BUG();
- goto out;
- }
-
/* Some actions do not requires the mutex */
switch (cmd) {
case GET_ARRAY_INFO:
--
2.39.2
From: Li Nan <[email protected]>
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 <[email protected]>
Reviewed-by: Yu Kuai <[email protected]>
---
drivers/md/md.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 02d6197dc944..c72af9fa3d7f 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6247,7 +6247,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;
@@ -7626,7 +7634,6 @@ 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;
err = md_ioctl_valid(cmd);
if (err)
@@ -7683,7 +7690,6 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
mutex_unlock(&mddev->open_mutex);
return -EBUSY;
}
- did_set_md_closing = true;
mutex_unlock(&mddev->open_mutex);
sync_blockdev(bdev);
}
@@ -7825,7 +7831,7 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
mddev_unlock(mddev);
out:
- if(did_set_md_closing)
+ if (cmd == STOP_ARRAY_RO || (err && cmd == STOP_ARRAY))
clear_bit(MD_CLOSING, &mddev->flags);
return err;
}
--
2.39.2
From: Li Nan <[email protected]>
There are no functional changes, prepare to sync mddev in
array_state_store().
Signed-off-by: Li Nan <[email protected]>
---
drivers/md/md.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index c72af9fa3d7f..04826431d3c6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -515,6 +515,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, int opener_num)
+{
+ mutex_lock(&mddev->open_mutex);
+ if (mddev->pers && atomic_read(&mddev->openers) > opener_num) {
+ 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,17 +7699,9 @@ 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);
- 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(bdev);
+ err = mddev_set_closing_and_sync_blockdev(mddev, 1);
+ if (err)
+ return err;
}
if (!md_is_rdwr(mddev))
--
2.39.2
From: Li Nan <[email protected]>
Before stopping or setting readonly, mddev_set_closing_and_sync_blockdev()
is always called to check the openers. So no longer need to check it again
in do_md_stop() and md_set_readonly(). Clean it up.
Signed-off-by: Li Nan <[email protected]>
---
drivers/md/md.c | 38 ++++++++++++++------------------------
1 file changed, 14 insertions(+), 24 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 200410a8adf3..adb821626d66 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4475,8 +4475,8 @@ array_state_show(struct mddev *mddev, char *page)
return sprintf(page, "%s\n", array_states[st]);
}
-static int do_md_stop(struct mddev *mddev, int ro, struct block_device *bdev);
-static int md_set_readonly(struct mddev *mddev, struct block_device *bdev);
+static int do_md_stop(struct mddev *mddev, int ro);
+static int md_set_readonly(struct mddev *mddev);
static int restart_array(struct mddev *mddev);
static ssize_t
@@ -4537,14 +4537,14 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
case inactive:
/* stop an active array, return 0 otherwise */
if (mddev->pers)
- err = do_md_stop(mddev, 2, NULL);
+ err = do_md_stop(mddev, 2);
break;
case clear:
- err = do_md_stop(mddev, 0, NULL);
+ err = do_md_stop(mddev, 0);
break;
case readonly:
if (mddev->pers)
- err = md_set_readonly(mddev, NULL);
+ err = md_set_readonly(mddev);
else {
mddev->ro = MD_RDONLY;
set_disk_ro(mddev->gendisk, 1);
@@ -4554,7 +4554,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
case read_auto:
if (mddev->pers) {
if (md_is_rdwr(mddev))
- err = md_set_readonly(mddev, NULL);
+ err = md_set_readonly(mddev);
else if (mddev->ro == MD_RDONLY)
err = restart_array(mddev);
if (err == 0) {
@@ -6412,7 +6412,7 @@ void md_stop(struct mddev *mddev)
EXPORT_SYMBOL_GPL(md_stop);
-static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
+static int md_set_readonly(struct mddev *mddev)
{
int err = 0;
int did_freeze = 0;
@@ -6440,9 +6440,7 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
!test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags));
mddev_lock_nointr(mddev);
- mutex_lock(&mddev->open_mutex);
- if ((mddev->pers && atomic_read(&mddev->openers) > !!bdev) ||
- mddev->sync_thread ||
+ if (mddev->sync_thread ||
test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
pr_warn("md: %s still in use.\n",mdname(mddev));
if (did_freeze) {
@@ -6468,7 +6466,6 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
err = 0;
}
out:
- mutex_unlock(&mddev->open_mutex);
return err;
}
@@ -6476,8 +6473,7 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
* 0 - completely stop and dis-assemble array
* 2 - stop but do not disassemble array
*/
-static int do_md_stop(struct mddev *mddev, int mode,
- struct block_device *bdev)
+static int do_md_stop(struct mddev *mddev, int mode)
{
struct gendisk *disk = mddev->gendisk;
struct md_rdev *rdev;
@@ -6503,13 +6499,9 @@ static int do_md_stop(struct mddev *mddev, int mode,
&mddev->recovery)));
mddev_lock_nointr(mddev);
- mutex_lock(&mddev->open_mutex);
- if ((mddev->pers && atomic_read(&mddev->openers) > !!bdev) ||
- mddev->sysfs_active ||
- mddev->sync_thread ||
+ if (mddev->sysfs_active || mddev->sync_thread ||
test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
pr_warn("md: %s still in use.\n",mdname(mddev));
- mutex_unlock(&mddev->open_mutex);
if (did_freeze) {
clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
@@ -6532,13 +6524,11 @@ static int do_md_stop(struct mddev *mddev, int mode,
sysfs_unlink_rdev(mddev, rdev);
set_capacity_and_notify(disk, 0);
- mutex_unlock(&mddev->open_mutex);
mddev->changed = 1;
if (!md_is_rdwr(mddev))
mddev->ro = MD_RDWR;
- } else
- mutex_unlock(&mddev->open_mutex);
+ }
/*
* Free resources if final stop
*/
@@ -6584,7 +6574,7 @@ static void autorun_array(struct mddev *mddev)
err = do_md_run(mddev);
if (err) {
pr_warn("md: do_md_run() returned %d\n", err);
- do_md_stop(mddev, 0, NULL);
+ do_md_stop(mddev, 0);
}
}
@@ -7758,11 +7748,11 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
goto unlock;
case STOP_ARRAY:
- err = do_md_stop(mddev, 0, bdev);
+ err = do_md_stop(mddev, 0);
goto unlock;
case STOP_ARRAY_RO:
- err = md_set_readonly(mddev, bdev);
+ err = md_set_readonly(mddev);
goto unlock;
case HOT_REMOVE_DISK:
--
2.39.2
From: Li Nan <[email protected]>
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 <[email protected]>
---
drivers/md/md.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 04826431d3c6..200410a8adf3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4493,6 +4493,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;
+ /* write sysfs will not open mddev and opener should be 0 */
+ err = mddev_set_closing_and_sync_blockdev(mddev, 0);
+ if (err)
+ return err;
+ break;
default:
break;
}
@@ -4592,6 +4603,11 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
sysfs_notify_dirent_safe(mddev->sysfs_state);
}
mddev_unlock(mddev);
+
+ if (st == readonly || st == read_auto || st == inactive ||
+ (err && st == clear))
+ clear_bit(MD_CLOSING, &mddev->flags);
+
return err ?: len;
}
static struct md_sysfs_entry md_array_state =
--
2.39.2
From: Li Nan <[email protected]>
If 'mddev->pers' is NULL, there is nothing to do in md_set_readonly().
Except for md_ioctl(), the other two callers of md_set_readonly() have
already checked 'mddev->pers'. To simplify the code, move the check of
'mddev->pers' to the caller.
Signed-off-by: Li Nan <[email protected]>
---
drivers/md/md.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index adb821626d66..987851db98c1 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)
{
int err = 0;
@@ -6451,20 +6452,18 @@ static int md_set_readonly(struct mddev *mddev)
err = -EBUSY;
goto out;
}
- if (mddev->pers) {
- __md_stop_writes(mddev);
+ __md_stop_writes(mddev);
- err = -ENXIO;
- if (mddev->ro == MD_RDONLY)
- goto out;
- mddev->ro = MD_RDONLY;
- set_disk_ro(mddev->gendisk, 1);
- clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
- set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
- md_wakeup_thread(mddev->thread);
- sysfs_notify_dirent_safe(mddev->sysfs_state);
- err = 0;
- }
+ err = -ENXIO;
+ if (mddev->ro == MD_RDONLY)
+ goto out;
+ mddev->ro = MD_RDONLY;
+ set_disk_ro(mddev->gendisk, 1);
+ clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
+ set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
+ md_wakeup_thread(mddev->thread);
+ sysfs_notify_dirent_safe(mddev->sysfs_state);
+ err = 0;
out:
return err;
}
@@ -7752,7 +7751,8 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
goto unlock;
case STOP_ARRAY_RO:
- err = md_set_readonly(mddev);
+ if (mddev->pers)
+ err = md_set_readonly(mddev);
goto unlock;
case HOT_REMOVE_DISK:
--
2.39.2
Hi Li Nan,
On Tue, Feb 6, 2024 at 1:00 AM <[email protected]> wrote:
>
> From: Li Nan <[email protected]>
>
> Changes in v6:
> - in patch 2, return directly.
> - in patch 4, return directly in case GET_DISK_INFO and GET_ARRAY_INFO.
> - in patch 7, rewrite commit message.
> - add patch 8, clean up openers check.
>
> Changes in v5:
> - add patches 1-4 to clean up md_ioct(), pathc 4 can help us clean up
> local variable 'clear_md_closing'.
> - in patches 5 and 7, clean up local variable 'clear_md_closing'.
>
> By the way, md_ioctl() is not readable now, I wanna to re-write it later
> to make it only have one 'switch' like other drivers.
Thanks for the patchset, and sorry for the delay.
The patchset looks good to me. However, it doesn't apply to md-6.9
branch. Could you please resend or let me know the base branch/commit
of the set?
Thanks,
Song
在 2024/2/13 7:36, Song Liu 写道:
> Hi Li Nan,
>
> On Tue, Feb 6, 2024 at 1:00 AM <[email protected]> wrote:
>>
>> From: Li Nan <[email protected]>
>>
>> Changes in v6:
>> - in patch 2, return directly.
>> - in patch 4, return directly in case GET_DISK_INFO and GET_ARRAY_INFO.
>> - in patch 7, rewrite commit message.
>> - add patch 8, clean up openers check.
>>
>> Changes in v5:
>> - add patches 1-4 to clean up md_ioct(), pathc 4 can help us clean up
>> local variable 'clear_md_closing'.
>> - in patches 5 and 7, clean up local variable 'clear_md_closing'.
>>
>> By the way, md_ioctl() is not readable now, I wanna to re-write it later
>> to make it only have one 'switch' like other drivers.
>
> Thanks for the patchset, and sorry for the delay.
>
> The patchset looks good to me. However, it doesn't apply to md-6.9
> branch. Could you please resend or let me know the base branch/commit
> of the set?
>
> Thanks,
> Song
>
> .
Thanks for your review. I will resend it later.
--
Thanks,
Nan