* [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-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
* 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
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).