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.
>
> 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);
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.
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.
Thanks,
NeilBrown
>
> Thanks,
> Shaohua
>> @@ -5688,6 +5688,22 @@ 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
>> + */
>> + if (mode == 0) {
>> + 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;
>>
>> @@ -5712,19 +5728,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@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
