From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shaohua Li Subject: Re: [PATCH 07/14] md: move bitmap_destroy before __md_stop Date: Tue, 28 Feb 2017 10:20:09 -0800 Message-ID: <20170228182009.zfsi2epb24ihjudp@kernel.org> References: <1487906124-20107-1-git-send-email-gqjiang@suse.com> <1487906124-20107-8-git-send-email-gqjiang@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1487906124-20107-8-git-send-email-gqjiang@suse.com> Sender: linux-raid-owner@vger.kernel.org To: Guoqing Jiang Cc: linux-raid@vger.kernel.org, shli@fb.com, neilb@suse.de List-Id: linux-raid.ids On Fri, Feb 24, 2017 at 11:15:17AM +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()). There is no patch 6. So I can't judge the purpose of the patch. The patch changed behaviors. We only destroy bitmap with mode == 0, now we do it even for mode == 2. Please specify why it's safe. The __md_stop will wait for behind writes for example only with bitmap set, but the patch makes us not do the wait. > Reviewed-by: NeilBrown > Signed-off-by: Guoqing Jiang > --- > drivers/md/md.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 6e910d99c9c1..7bcc757386e1 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -5582,8 +5582,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); > } > @@ -5696,6 +5696,20 @@ static int do_md_stop(struct mddev *mddev, int mode, > set_disk_ro(disk, 0); > > __md_stop_writes(mddev); > + > + /* > + * Destroy bitmap after all writes are stopped > + */ > + bitmap_destroy(mddev); > + if (mddev->bitmap_info.file) { > + struct file *f = mddev->bitmap_info.file; > + spin_lock(&mddev->lock); > + mddev->bitmap_info.file = NULL; > + spin_unlock(&mddev->lock); > + fput(f); > + } > + mddev->bitmap_info.offset = 0; > + > __md_stop(mddev); > mddev->queue->backing_dev_info.congested_fn = NULL; > > @@ -5720,19 +5734,7 @@ 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->bitmap_info.file) { > - struct file *f = mddev->bitmap_info.file; > - spin_lock(&mddev->lock); > - mddev->bitmap_info.file = NULL; > - spin_unlock(&mddev->lock); > - fput(f); > - } > - mddev->bitmap_info.offset = 0; > - > export_array(mddev); > - > md_clean(mddev); > if (mddev->hold_active == UNTIL_STOP) > mddev->hold_active = 0; > -- > 2.6.2 > > -- > 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