On 03/03/2017 06:15 AM, NeilBrown wrote:
On Thu, Mar 02 2017, Shaohua Li wrote:
On Wed, Mar 01, 2017 at 04:42:37PM +0800, Guoqing Jiang wrote:
Since we have switched to sync way to handle METADATA_UPDATED
msg for md-cluster, then process_metadata_update is depended
on mddev->thread->wqueue.
With the new change, clustered raid could possible hang if
array received a METADATA_UPDATED msg after array unregistered
mddev->thread, so we need to stop clustered raid earlier
than before.
And this change should be safe for non-clustered raid since
all writes are stopped before the destroy. Also in md_run,
we activate the personality (pers->run()) before activating
the bitmap (bitmap_create()). So it is pleasingly symmetric
to stop the bitmap (bitmap_destroy()) before stopping the
personality (__md_stop() calls pers->free()).
Reviewed-by: NeilBrown <neilb@xxxxxxxx>
Signed-off-by: Guoqing Jiang <gqjiang@xxxxxxxx>
---
drivers/md/md.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 44206bc6e3aa..e1d9116044ae 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5574,8 +5574,8 @@ void md_stop(struct mddev *mddev)
/* stop the array and free an attached data structures.
* This is called from dm-raid
*/
- __md_stop(mddev);
bitmap_destroy(mddev);
+ __md_stop(mddev);
if (mddev->bio_set)
bioset_free(mddev->bio_set);
}
Applied other 4 patches. But this one I still have concerns.
For raid1, if a bio is behind IO, we return the bio to upper layer but don't
wait behind IO completion. So even there are no writes running, there might be
behind IO running. mddev_detach will do the wait which checks bitmap. If we
bitmap_destroy before __md_stop, mddev_detach doesn't do the wait.
Got it, thanks for explanation!
Probably we should move mddev_detach out of __md_stop and always do:
mddev_detach()
bitmap_destroy()
__md_stop()
This looks safer to me.
Thanks for catching that. I agree - mddev_detach should come before
bitmap_destroy.
I might be best to change __md_stop() to start
static void __md_stop(struct mddev *mddev)
{
struct md_personality *pers = mddev->pers;
mddev_detach(mddev);
+ bitmap_destroy(mddev);
/* Ensure ->event_work is done */
flush_workqueue(md_misc_wq);
With this change, does it mean we destroy bitmap unconditionally even
if the mode is 2? Thanks.
That would make the remainder of the patch (below) unnecessary.
I think it is wrong anyway.
We were correct to move the "bitmap_destroy() call up to the
"if (mddev->pers)" case, but we were not correct to move the
closing of bitmap_info.file.
If a file is added to an array but that array is not started
(so ->pers is not set), then stopping the array will not close the file
with the change below, and that isn't good.
Hmm, thanks for reminder. Though I have some questions about above.
I guess the file is pointed to bitmap file, from mdadm manpage, I see
"-b, --bitmap=" is used under create, build, grow and assemble mode.
But how to add a file to a array which is not started? Please correct me
for my wrong understanding.
So if we move bitmap_destroy() into __md_stop() and remove it from
do_md_stop and md_stop(), that might be exactly what we need.
How about the below changes? It may addresses both your concern and
Shaohua's concern, but not sure ...
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 79a99a1..a0e79d6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5555,7 +5555,6 @@ static void mddev_detach(struct mddev *mddev)
static void __md_stop(struct mddev *mddev)
{
struct md_personality *pers = mddev->pers;
- mddev_detach(mddev);
/* Ensure ->event_work is done */
flush_workqueue(md_misc_wq);
spin_lock(&mddev->lock);
@@ -5574,8 +5573,9 @@ void md_stop(struct mddev *mddev)
/* stop the array and free an attached data structures.
* This is called from dm-raid
*/
- __md_stop(mddev);
+ mddev_detach(mddev);
bitmap_destroy(mddev);
+ __md_stop(mddev);
if (mddev->bio_set)
bioset_free(mddev->bio_set);
}
@@ -5688,6 +5688,10 @@ static int do_md_stop(struct mddev *mddev, int mode,
set_disk_ro(disk, 0);
__md_stop_writes(mddev);
+ mddev_detach(mddev);
+ if (mode == 0)
+ bitmap_destroy(mddev);
+
__md_stop(mddev);
mddev->queue->backing_dev_info->congested_fn = NULL;
@@ -5713,7 +5717,8 @@ static int do_md_stop(struct mddev *mddev, int mode,
if (mode == 0) {
pr_info("md: %s stopped.\n", mdname(mddev));
- bitmap_destroy(mddev);
+ if (!mddev->pers)
+ bitmap_destroy(mddev);
if (mddev->bitmap_info.file) {
struct file *f = mddev->bitmap_info.file;
spin_lock(&mddev->lock);
Thanks,
Guoqing
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
