linux-kernel - Re: [patch, v2] blk-mq: avoid excessive boot delays with large lun counts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACVXFVPwfv0-T0v9CseqrkmZEyVwUwBFBer_yoTEwxFKrBLLjw@mail.gmail.com>
Date:	Sun, 1 Nov 2015 08:32:46 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Jeff Moyer <jmoyer@...hat.com>
Cc:	Jens Axboe <axboe@...nel.dk>, Jason Luo <zhangqing.luo@...cle.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Guru Anbalagane <guru.anbalagane@...cle.com>,
	Feng Jin <joe.jin@...cle.com>, Tejun Heo <tj@...nel.org>
Subject: Re: [patch, v2] blk-mq: avoid excessive boot delays with large lun counts

On Sat, Oct 31, 2015 at 1:22 AM, Jeff Moyer <jmoyer@...hat.com> wrote:
> Hi,
>
> Zhangqing Luo reported long boot times on a system with thousands of
> LUNs when scsi-mq was enabled.  He narrowed the problem down to
> blk_mq_add_queue_tag_set, where every queue is frozen in order to set
> the BLK_MQ_F_TAG_SHARED flag.  Each added device will freeze all queues
> added before it in sequence, which involves waiting for an RCU grace
> period for each one.  We don't need to do this.  After the second queue
> is added, only new queues need to be initialized with the shared tag.
> We can do that by percolating the flag up to the blk_mq_tag_set, and
> updating the newly added queue's hctxs if the flag is set.
>
> This problem was introduced by commit 0d2602ca30e41 (blk-mq: improve
> support for shared tags maps).
>
> Reported-and-tested-by: Jason Luo <zhangqing.luo@...cle.com>
> Signed-off-by: Jeff Moyer <jmoyer@...hat.com>

You can add
         Reviewed-by: Ming Lei <ming.lei@...onical.com>
if the following trivial issues(especially the 2nd one) are addressed.

>
> ---
> Changes from v1:
> - addressed review comments from Ming, which simplified the patch
>
> Jason, if you could sanity test this patch to make sure it still solves
> your problem, that would be greatly appreciated.
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 85f0143..12f79af 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1673,7 +1673,7 @@ static int blk_mq_init_hctx(struct request_queue *q,
>         INIT_LIST_HEAD(&hctx->dispatch);
>         hctx->queue = q;
>         hctx->queue_num = hctx_idx;
> -       hctx->flags = set->flags;
> +       hctx->flags = set->flags & ~BLK_MQ_F_TAG_SHARED;
>
>         blk_mq_init_cpu_notifier(&hctx->cpu_notifier,
>                                         blk_mq_hctx_notify, hctx);
> @@ -1860,27 +1860,26 @@ static void blk_mq_map_swqueue(struct request_queue *q,
>         }
>  }
>
> -static void blk_mq_update_tag_set_depth(struct blk_mq_tag_set *set)
> +static void queue_set_hctx_shared(struct request_queue *q, bool shared)
>  {
>         struct blk_mq_hw_ctx *hctx;
> -       struct request_queue *q;
> -       bool shared;
>         int i;
>
> -       if (set->tag_list.next == set->tag_list.prev)
> -               shared = false;
> -       else
> -               shared = true;
> +       queue_for_each_hw_ctx(q, hctx, i) {
> +               if (shared)
> +                       hctx->flags |= BLK_MQ_F_TAG_SHARED;
> +               else
> +                       hctx->flags &= ~BLK_MQ_F_TAG_SHARED;
> +       }
> +}
> +
> +static void blk_mq_update_tag_set_depth(struct blk_mq_tag_set *set, bool shared)
> +{
> +       struct request_queue *q;
>
>         list_for_each_entry(q, &set->tag_list, tag_set_list) {
>                 blk_mq_freeze_queue(q);
> -
> -               queue_for_each_hw_ctx(q, hctx, i) {
> -                       if (shared)
> -                               hctx->flags |= BLK_MQ_F_TAG_SHARED;
> -                       else
> -                               hctx->flags &= ~BLK_MQ_F_TAG_SHARED;
> -               }
> +               queue_set_hctx_shared(q, shared);
>                 blk_mq_unfreeze_queue(q);
>         }
>  }
> @@ -1891,7 +1890,12 @@ static void blk_mq_del_queue_tag_set(struct request_queue *q)
>
>         mutex_lock(&set->tag_list_lock);
>         list_del_init(&q->tag_set_list);
> -       blk_mq_update_tag_set_depth(set);
> +       if (set->tag_list.next == set->tag_list.prev) {

list_is_singular() should be better.

> +               /* just transitioned to unshared */
> +               set->flags &= ~BLK_MQ_F_TAG_SHARED;
> +               /* update existing queue */
> +               blk_mq_update_tag_set_depth(set, false);
> +       }
>         mutex_unlock(&set->tag_list_lock);
>  }
>
> @@ -1901,8 +1905,17 @@ static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set,
>         q->tag_set = set;
>
>         mutex_lock(&set->tag_list_lock);
> +
> +       /* Check to see if we're transitioning to shared (from 1 to 2 queues). */
> +       if (!list_empty(&set->tag_list) && !(set->flags & BLK_MQ_F_TAG_SHARED)) {
> +               set->flags |= BLK_MQ_F_TAG_SHARED;
> +               /* update existing queue */
> +               blk_mq_update_tag_set_depth(set, true);
> +       }
> +       if (set->flags & BLK_MQ_F_TAG_SHARED)

The above should be 'else if', otherwise the current queue will be set
twice.

> +               queue_set_hctx_shared(q, true);
>         list_add_tail(&q->tag_set_list, &set->tag_list);
> -       blk_mq_update_tag_set_depth(set);
> +
>         mutex_unlock(&set->tag_list_lock);
>  }
>



-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/