From patchwork Fri Jun 22 14:58:39 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jens Axboe X-Patchwork-Id: 10482373 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B29D660388 for ; Fri, 22 Jun 2018 14:58:48 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D82A02892C for ; Fri, 22 Jun 2018 14:58:45 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CCB28289C9; Fri, 22 Jun 2018 14:58:45 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 57D8F2892C for ; Fri, 22 Jun 2018 14:58:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933556AbeFVO6n (ORCPT ); Fri, 22 Jun 2018 10:58:43 -0400 Received: from mail-io0-f193.google.com ([209.85.223.193]:45225 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933346AbeFVO6m (ORCPT ); Fri, 22 Jun 2018 10:58:42 -0400 Received: by mail-io0-f193.google.com with SMTP id l25-v6so6341770ioh.12 for ; Fri, 22 Jun 2018 07:58:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel-dk.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=0iahGQClU5v1iocK81FpLBz5Us4QaFXqdoEi7YuU/9o=; b=ZNLs0Trdsz5Wgc2G9zGLV7y07jcglEN98bzxMymTKD3ByZL9A+EQjr9qFJ2uFPyVXD SjVxtJHQ90lLY4bTvt8/NYF3qj8hIwQLvTK8pXIVHejzlzzwC/vu2csxH0U/eAWGXvej ayRQyTjT3QfB9AVQomsU39JeW4HxmHCAWb3bL7X2TYdZbDAhi2M5hP9XejMGubqQpq3q FRns3xA+jQADzVrGWNZx6UfhZOcNYYrslTs1k+q1X5M4cgwybDHQqBXKSAV0ffpYtUkg w9ZoiX0VAV7rd3Ha9oi+9fBwYew0zyoWmMeoqgwEP4oXPx/tMmg3vp2fIXnKzc5OgVKX jhoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=0iahGQClU5v1iocK81FpLBz5Us4QaFXqdoEi7YuU/9o=; b=EQf01Uy+pStMY+t6t1gqf4UvzNdjP4xRsi503gAYBX+HR96P0KbcRUxrgRUgBI4ToA Qzr0g5XB5XlqTkiLULKoby2Zk59VpbE08RiCQN+LQMOGAXlDkKdrNVa8qkAtlRSNHGQ5 6MdJhgYlOLOHFRJLineVbSamhnHAYjlmS8kFKH7W1d8Ac973K8T7Tb5IuHWw2xN1qrqM fgsD4GB1i/WTXf2ikVLp2uNkEhAd2enueaELyLLf0FIrEymvPDg8IO2ZUvdceOYqPBeq Q8uFGfiLzYMinS7YIaX3psSuJp1PJ58fZrce7+yGZPuowlyB+qTkKRq5n9RAvmFvCuke 53cg== X-Gm-Message-State: APt69E0UzlFcP1ozieIqIozf+K1eiXu1X1OyiqF4kdqWAACKsHtOGcVw 5g9hHkS1bW57AOoohltl1vtzLg== X-Google-Smtp-Source: AAOMgpeGlp9X28JwNnbqxJEbRL6DHjbhej//zaQDA/TF6jBEIqSCKtat/fj2+nZcZ8PYHk0HXHR60g== X-Received: by 2002:a6b:4a15:: with SMTP id w21-v6mr1475976iob.277.1529679521685; Fri, 22 Jun 2018 07:58:41 -0700 (PDT) Received: from [192.168.1.167] ([216.160.245.98]) by smtp.gmail.com with ESMTPSA id p20-v6sm1702228itf.4.2018.06.22.07.58.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 22 Jun 2018 07:58:40 -0700 (PDT) Subject: Re: [PATCH] blk-mq: avoid to synchronize rcu inside blk_cleanup_queue() To: Andrew Jones , Ming Lei Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org, "Martin K. Petersen" , Christoph Hellwig References: <20180620025522.8002-1-ming.lei@redhat.com> <20180622114204.4ohrk53td3iijamc@kamzik.brq.redhat.com> From: Jens Axboe Message-ID: <8024ab9d-ae82-c8f2-83e3-a9d8fa4ba303@kernel.dk> Date: Fri, 22 Jun 2018 08:58:39 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <20180622114204.4ohrk53td3iijamc@kamzik.brq.redhat.com> Content-Language: en-US Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 6/22/18 5:42 AM, Andrew Jones wrote: > On Wed, Jun 20, 2018 at 10:55:22AM +0800, Ming Lei wrote: >> SCSI probing may synchronously create and destroy a lot of request_queues >> for non-existent devices. Any synchronize_rcu() in queue creation or >> destroy path may introduce long latency during booting, see detailed >> description in comment of blk_register_queue(). >> >> This patch removes two synchronize_rcu() inside blk_cleanup_queue() >> for this case: >> >> 1) commit c2856ae2f315d75(blk-mq: quiesce queue before freeing queue) >> need synchronize_rcu() for implementing blk_mq_quiesce_queue(), but >> when queue isn't initialized, it isn't necessary to do that since >> only pass-through requests are involved, no original issue in >> scsi_execute() at all. >> >> 2) when only one request queue is attached to tags, no necessary to >> call synchronize_rcu() too. >> >> Without this patch, it may take more 20+ seconds for virtio-scsi to >> complete disk probe. With this patch, the time becomes less than 100ms. >> >> Reported-by: Andrew Jones >> Cc: Andrew Jones >> Cc: linux-scsi@vger.kernel.org >> Cc: "Martin K. Petersen" >> Cc: Christoph Hellwig >> Signed-off-by: Ming Lei >> --- >> block/blk-core.c | 8 ++++++-- >> block/blk-mq.c | 5 ++++- >> 2 files changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/block/blk-core.c b/block/blk-core.c >> index cf0ee764b908..f0129e20b773 100644 >> --- a/block/blk-core.c >> +++ b/block/blk-core.c >> @@ -766,9 +766,13 @@ void blk_cleanup_queue(struct request_queue *q) >> * make sure all in-progress dispatch are completed because >> * blk_freeze_queue() can only complete all requests, and >> * dispatch may still be in-progress since we dispatch requests >> - * from more than one contexts >> + * from more than one contexts. >> + * >> + * No need to quiesce queue if it isn't initialized yet since >> + * blk_freeze_queue() should be enough for cases of passthrough >> + * request. >> */ >> - if (q->mq_ops) >> + if (q->mq_ops && blk_queue_init_done(q)) >> blk_mq_quiesce_queue(q); >> >> /* for synchronous bio-based driver finish in-flight integrity i/o */ >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 70c65bb6c013..63680b243466 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -2351,6 +2351,7 @@ static void blk_mq_update_tag_set_depth(struct blk_mq_tag_set *set, >> static void blk_mq_del_queue_tag_set(struct request_queue *q) >> { >> struct blk_mq_tag_set *set = q->tag_set; >> + bool shared = true; >> >> mutex_lock(&set->tag_list_lock); >> list_del_rcu(&q->tag_set_list); >> @@ -2359,9 +2360,11 @@ static void blk_mq_del_queue_tag_set(struct request_queue *q) >> set->flags &= ~BLK_MQ_F_TAG_SHARED; >> /* update existing queue */ >> blk_mq_update_tag_set_depth(set, false); >> + shared = true; > > I guess this should be '= false'. > >> } >> mutex_unlock(&set->tag_list_lock); >> - synchronize_rcu(); >> + if (shared) >> + synchronize_rcu(); >> INIT_LIST_HEAD(&q->tag_set_list); >> } >> > > With the '= false' change I tested this and it resolves the issue for me. That logic still doesn't look correct to me. Does the below work? diff --git a/block/blk-core.c b/block/blk-core.c index afd2596ea3d3..222d4fc0e524 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -762,9 +762,13 @@ void blk_cleanup_queue(struct request_queue *q) * make sure all in-progress dispatch are completed because * blk_freeze_queue() can only complete all requests, and * dispatch may still be in-progress since we dispatch requests - * from more than one contexts + * from more than one contexts. + * + * No need to quiesce queue if it isn't initialized yet since + * blk_freeze_queue() should be enough for cases of passthrough + * request. */ - if (q->mq_ops) + if (q->mq_ops && blk_queue_init_done(q)) blk_mq_quiesce_queue(q); /* for synchronous bio-based driver finish in-flight integrity i/o */ diff --git a/block/blk-mq.c b/block/blk-mq.c index 8e57b84e50e9..18ad2b95ff63 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2351,8 +2351,12 @@ static void blk_mq_update_tag_set_depth(struct blk_mq_tag_set *set, static void blk_mq_del_queue_tag_set(struct request_queue *q) { struct blk_mq_tag_set *set = q->tag_set; + bool shared; mutex_lock(&set->tag_list_lock); + + shared = !list_is_singular(&set->tag_list); + list_del_rcu(&q->tag_set_list); if (list_is_singular(&set->tag_list)) { /* just transitioned to unshared */ @@ -2361,7 +2365,8 @@ static void blk_mq_del_queue_tag_set(struct request_queue *q) blk_mq_update_tag_set_depth(set, false); } mutex_unlock(&set->tag_list_lock); - synchronize_rcu(); + if (shared) + synchronize_rcu(); INIT_LIST_HEAD(&q->tag_set_list); }