* [PATCH] md:array cannot be opened again after 'md_set_readonly'
@ 2017-03-27 7:52 Zhilong Liu
2017-03-27 18:22 ` Shaohua Li
2017-03-28 21:13 ` NeilBrown
0 siblings, 2 replies; 6+ messages in thread
From: Zhilong Liu @ 2017-03-27 7:52 UTC (permalink / raw)
To: neilb, shli; +Cc: linux-raid, Zhilong Liu, Guoqing Jiang
This is a bug about array cannot be opened again after 'md_set_readonly',
because the MD_CLOSING bit is still waiting for clear.
MD_CLOSING should only be set for a short period or time to avoid certain
races. After the operation that set it completes, it should be cleared.
Reviewed-by: NeilBrown <neilb@suse.com>
Cc: Guoqing Jiang <gqjiang@suse.com>
Signed-off-by: Zhilong Liu <zlliu@suse.com>
---
drivers/md/md.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index f6ae1d6..7f2db7c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5588,6 +5588,7 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
int err = 0;
int did_freeze = 0;
+ test_and_clear_bit(MD_CLOSING, &mddev->flags);
if (!test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
did_freeze = 1;
set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
--
2.6.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] md:array cannot be opened again after 'md_set_readonly'
2017-03-27 7:52 [PATCH] md:array cannot be opened again after 'md_set_readonly' Zhilong Liu
@ 2017-03-27 18:22 ` Shaohua Li
2017-03-28 13:19 ` Zhilong Liu
2017-03-28 21:13 ` NeilBrown
1 sibling, 1 reply; 6+ messages in thread
From: Shaohua Li @ 2017-03-27 18:22 UTC (permalink / raw)
To: Zhilong Liu; +Cc: neilb, shli, linux-raid, Guoqing Jiang
On Mon, Mar 27, 2017 at 03:52:25PM +0800, Zhilong Liu wrote:
> This is a bug about array cannot be opened again after 'md_set_readonly',
> because the MD_CLOSING bit is still waiting for clear.
> MD_CLOSING should only be set for a short period or time to avoid certain
> races. After the operation that set it completes, it should be cleared.
where is the bit set? Why don't clear it after the operation but clear it in
set_readonly?
> Reviewed-by: NeilBrown <neilb@suse.com>
> Cc: Guoqing Jiang <gqjiang@suse.com>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
> drivers/md/md.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f6ae1d6..7f2db7c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5588,6 +5588,7 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
> int err = 0;
> int did_freeze = 0;
>
> + test_and_clear_bit(MD_CLOSING, &mddev->flags);
I don't understand why this must be a test_and_clear.
Thanks,
Shaohua
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] md:array cannot be opened again after 'md_set_readonly'
2017-03-27 18:22 ` Shaohua Li
@ 2017-03-28 13:19 ` Zhilong Liu
2017-03-28 16:12 ` Shaohua Li
2017-03-28 21:18 ` NeilBrown
0 siblings, 2 replies; 6+ messages in thread
From: Zhilong Liu @ 2017-03-28 13:19 UTC (permalink / raw)
To: Shaohua Li; +Cc: neilb, shli, linux-raid, Guoqing Jiang
On 03/28/2017 02:22 AM, Shaohua Li wrote:
> On Mon, Mar 27, 2017 at 03:52:25PM +0800, Zhilong Liu wrote:
>> This is a bug about array cannot be opened again after 'md_set_readonly',
>> because the MD_CLOSING bit is still waiting for clear.
>> MD_CLOSING should only be set for a short period or time to avoid certain
>> races. After the operation that set it completes, it should be cleared.
> where is the bit set? Why don't clear it after the operation but clear it in
> set_readonly?
>
it goes two paths after set MD_CLOSING bit, do_md_stop and md_set_readonly,
the do_md_stop has done the md_clean to clear the mddev->flags, thus I
put it in
md_set_readonly.
Thanks for your suggestion, is the following changing good for you? here
it has
finished what it needs to do, so clear the MD_CLOSING bit in time.
if looks good, I would send this in new revision patch.
diff --git a/drivers/md/md.c b/drivers/md/md.c
index f6ae1d6..e8c1130 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -6868,6 +6868,7 @@ static int md_ioctl(struct block_device *bdev,
fmode_t mode,
set_bit(MD_CLOSING, &mddev->flags);
mutex_unlock(&mddev->open_mutex);
sync_blockdev(bdev);
+ clear_bit(MD_CLOSING, &mddev->flags);
}
err = mddev_lock(mddev);
if (err) {
Thanks,
-Zhilong
>> Reviewed-by: NeilBrown <neilb@suse.com>
>> Cc: Guoqing Jiang <gqjiang@suse.com>
>> Signed-off-by: Zhilong Liu <zlliu@suse.com>
>> ---
>> drivers/md/md.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index f6ae1d6..7f2db7c 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -5588,6 +5588,7 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
>> int err = 0;
>> int did_freeze = 0;
>>
>> + test_and_clear_bit(MD_CLOSING, &mddev->flags);
> I don't understand why this must be a test_and_clear.
>
> Thanks,
> Shaohua
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] md:array cannot be opened again after 'md_set_readonly'
2017-03-28 13:19 ` Zhilong Liu
@ 2017-03-28 16:12 ` Shaohua Li
2017-03-28 21:18 ` NeilBrown
1 sibling, 0 replies; 6+ messages in thread
From: Shaohua Li @ 2017-03-28 16:12 UTC (permalink / raw)
To: Zhilong Liu; +Cc: neilb, shli, linux-raid, Guoqing Jiang
On Tue, Mar 28, 2017 at 09:19:12PM +0800, Zhilong Liu wrote:
>
> On 03/28/2017 02:22 AM, Shaohua Li wrote:
> > On Mon, Mar 27, 2017 at 03:52:25PM +0800, Zhilong Liu wrote:
> > > This is a bug about array cannot be opened again after 'md_set_readonly',
> > > because the MD_CLOSING bit is still waiting for clear.
> > > MD_CLOSING should only be set for a short period or time to avoid certain
> > > races. After the operation that set it completes, it should be cleared.
> > where is the bit set? Why don't clear it after the operation but clear it in
> > set_readonly?
>
> it goes two paths after set MD_CLOSING bit, do_md_stop and md_set_readonly,
> the do_md_stop has done the md_clean to clear the mddev->flags, thus I put
> it in
> md_set_readonly.
> Thanks for your suggestion, is the following changing good for you? here it
> has
> finished what it needs to do, so clear the MD_CLOSING bit in time.
> if looks good, I would send this in new revision patch.
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f6ae1d6..e8c1130 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6868,6 +6868,7 @@ static int md_ioctl(struct block_device *bdev, fmode_t
> mode,
> set_bit(MD_CLOSING, &mddev->flags);
> mutex_unlock(&mddev->open_mutex);
> sync_blockdev(bdev);
> + clear_bit(MD_CLOSING, &mddev->flags);
> }
> err = mddev_lock(mddev);
> if (err) {
It would be better done at the end of set_readonly to guarantee set ro done.
Thanks,
Shaohua
> Thanks,
> -Zhilong
> > > Reviewed-by: NeilBrown <neilb@suse.com>
> > > Cc: Guoqing Jiang <gqjiang@suse.com>
> > > Signed-off-by: Zhilong Liu <zlliu@suse.com>
> > > ---
> > > drivers/md/md.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > > index f6ae1d6..7f2db7c 100644
> > > --- a/drivers/md/md.c
> > > +++ b/drivers/md/md.c
> > > @@ -5588,6 +5588,7 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
> > > int err = 0;
> > > int did_freeze = 0;
> > > + test_and_clear_bit(MD_CLOSING, &mddev->flags);
> > I don't understand why this must be a test_and_clear.
> >
> > Thanks,
> > Shaohua
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] md:array cannot be opened again after 'md_set_readonly'
2017-03-27 7:52 [PATCH] md:array cannot be opened again after 'md_set_readonly' Zhilong Liu
2017-03-27 18:22 ` Shaohua Li
@ 2017-03-28 21:13 ` NeilBrown
1 sibling, 0 replies; 6+ messages in thread
From: NeilBrown @ 2017-03-28 21:13 UTC (permalink / raw)
To: shli; +Cc: linux-raid, Zhilong Liu, Guoqing Jiang
[-- Attachment #1: Type: text/plain, Size: 1205 bytes --]
On Mon, Mar 27 2017, Zhilong Liu wrote:
> This is a bug about array cannot be opened again after 'md_set_readonly',
> because the MD_CLOSING bit is still waiting for clear.
> MD_CLOSING should only be set for a short period or time to avoid certain
> races. After the operation that set it completes, it should be cleared.
>
> Reviewed-by: NeilBrown <neilb@suse.com>
No I didn't. I never reviewed this patch. I don't agree with this
patch. This patch is wrong.
The flag is set in md_ioctl(), and it should be cleared in md_ioctl().
NeilBrown
> Cc: Guoqing Jiang <gqjiang@suse.com>
> Signed-off-by: Zhilong Liu <zlliu@suse.com>
> ---
> drivers/md/md.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f6ae1d6..7f2db7c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5588,6 +5588,7 @@ static int md_set_readonly(struct mddev *mddev, struct block_device *bdev)
> int err = 0;
> int did_freeze = 0;
>
> + test_and_clear_bit(MD_CLOSING, &mddev->flags);
> if (!test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
> did_freeze = 1;
> set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> --
> 2.6.6
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] md:array cannot be opened again after 'md_set_readonly'
2017-03-28 13:19 ` Zhilong Liu
2017-03-28 16:12 ` Shaohua Li
@ 2017-03-28 21:18 ` NeilBrown
1 sibling, 0 replies; 6+ messages in thread
From: NeilBrown @ 2017-03-28 21:18 UTC (permalink / raw)
To: Zhilong Liu, Shaohua Li; +Cc: shli, linux-raid, Guoqing Jiang
[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]
On Tue, Mar 28 2017, Zhilong Liu wrote:
> On 03/28/2017 02:22 AM, Shaohua Li wrote:
>> On Mon, Mar 27, 2017 at 03:52:25PM +0800, Zhilong Liu wrote:
>>> This is a bug about array cannot be opened again after 'md_set_readonly',
>>> because the MD_CLOSING bit is still waiting for clear.
>>> MD_CLOSING should only be set for a short period or time to avoid certain
>>> races. After the operation that set it completes, it should be cleared.
>> where is the bit set? Why don't clear it after the operation but clear it in
>> set_readonly?
>>
>
> it goes two paths after set MD_CLOSING bit, do_md_stop and md_set_readonly,
> the do_md_stop has done the md_clean to clear the mddev->flags, thus I
> put it in
> md_set_readonly.
> Thanks for your suggestion, is the following changing good for you? here
> it has
> finished what it needs to do, so clear the MD_CLOSING bit in time.
> if looks good, I would send this in new revision patch.
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index f6ae1d6..e8c1130 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6868,6 +6868,7 @@ static int md_ioctl(struct block_device *bdev,
> fmode_t mode,
> set_bit(MD_CLOSING, &mddev->flags);
> mutex_unlock(&mddev->open_mutex);
> sync_blockdev(bdev);
> + clear_bit(MD_CLOSING, &mddev->flags);
No.
MD_CLOSING is used to prevent a race with md_open().
md_open() blocks on open_mutex(), but we cannot hold that here for
complicated reasons.
So we set MD_CLOSING and need to keep it set at least until
md_set_readonly() or do_md_stop() take that lock again.
Clearing it here just opens up the race again.
It needs to be cleared in md_ioctl, after any call to md_set_readonly()
or do_md_stop().
I've already posted a sample patch which has been tested. Please don't
do something completely different.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-28 21:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-27 7:52 [PATCH] md:array cannot be opened again after 'md_set_readonly' Zhilong Liu
2017-03-27 18:22 ` Shaohua Li
2017-03-28 13:19 ` Zhilong Liu
2017-03-28 16:12 ` Shaohua Li
2017-03-28 21:18 ` NeilBrown
2017-03-28 21:13 ` NeilBrown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).