2024-02-01 06:40:54

by Li Nan

Subject: [PATCH v5 7/8] md: sync blockdev before stopping raid or setting readonly

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 4c7a0225f77d..86becf0015f5 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4493,6 +4493,16 @@ 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;
+ break;
default:
break;
}
@@ -4518,6 +4528,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;
@@ -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

2024-02-02 02:12:36

by Yu Kuai

Subject: Re: [PATCH v5 7/8] md: sync blockdev before stopping raid or setting readonly

Hi,

?? 2024/02/01 14:34, [email protected] д??:
> 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.

You're not just adding sync_blockdev() here. Please rewrite the tittle
and commit message.

>
> 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 4c7a0225f77d..86becf0015f5 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4493,6 +4493,16 @@ 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);

In this context, mddev->openers should be zero, and such check is in
do_md_stop() and md_set_readonly():

if (atomic_read(&mddev->openers) > !!bdev).

Thanks,
Kuai

> + if (err)
> + return err;
> + break;
> default:
> break;
> }
> @@ -4518,6 +4528,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;
> @@ -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 =
>

2024-02-02 09:29:36

by Li Nan

Subject: Re: [PATCH v5 7/8] md: sync blockdev before stopping raid or setting readonly

在 2024/2/2 10:12, Yu Kuai 写道:
> Hi,
>
> 在 2024/02/01 14:34, [email protected] 写道:
>> 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.
>
> You're not just adding sync_blockdev() here. Please rewrite the tittle
> and commit message.
>
>>
>> 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 4c7a0225f77d..86becf0015f5 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -4493,6 +4493,16 @@ 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);
>
> In this context, mddev->openers should be zero, and such check is in
> do_md_stop() and md_set_readonly():
>

Yeah, the checks in do_md_stop() and md_set_readonly() can be removed after
this patch. However, 'mddev->open_metux' is used to protect MD_CLOSING and
'mddev->openers', it can be removed in these functions, too.

I will fix it later. Thanks for your review.

> if (atomic_read(&mddev->openers) > !!bdev).
>
> Thanks,
> Kuai
>



--
Thanks,
Nan