Re: [PATCH 2/2] dm: stay in blk_queue_bypass until queue becomes initialized

["Jun'ichi Nomura" <j-nomura@ce.jp.nec.com>]

On 10/25/12 18:41, Jun'ichi Nomura wrote:
> With 749fefe677 ("block: lift the initial queue bypass mode on
> blk_register_queue() instead of blk_init_allocated_queue()"),
> add_disk() eventually calls blk_queue_bypass_end().
> This change invokes the following warning when multipath is used.
...
> The warning means during queue initialization blk_queue_bypass_start()
> calls sleeping function (synchronize_rcu) while dm holds md->type_lock.
> 
> dm device initialization basically includes the following 3 steps:
>   1. create ioctl, allocates queue and call add_disk()
>   2. table load ioctl, determines device type and initialize queue
>      if request-based
>   3. resume ioctl, device becomes functional
> 
> So it is better to have dm's queue stay in bypass mode until
> the initialization completes in table load ioctl.
> 
> The effect of additional blk_queue_bypass_start():
> 
>   3.7-rc2 (plain)
>     # time for n in $(seq 1000); do dmsetup create --noudevsync --notable a; \
>                                     dmsetup remove a; done
> 
>     real  0m15.434s
>     user  0m0.423s
>     sys   0m7.052s
> 
>   3.7-rc2 (with this patch)
>     # time for n in $(seq 1000); do dmsetup create --noudevsync --notable a; \
>                                     dmsetup remove a; done
>     real  0m19.766s
>     user  0m0.442s
>     sys   0m6.861s
> 
> If this additional cost is not negligible, we need a variant of add_disk()
> that does not end bypassing.

Or call blk_queue_bypass_start() before add_disk():

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ad02761..d14639b 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1868,9 +1868,9 @@ static struct mapped_device *alloc_dev(int minor)
 	md->disk->queue = md->queue;
 	md->disk->private_data = md;
 	sprintf(md->disk->disk_name, "dm-%d", minor);
-	add_disk(md->disk);
 	/* Until md type is determined, put the queue in bypass mode */
 	blk_queue_bypass_start(md->queue);
+	add_disk(md->disk);
 	format_dev_t(md->name, MKDEV(_major, minor));
 
 	md->wq = alloc_workqueue("kdmflush",
---

If the patch is modified like above, we could fix the issue
without incurring additional cost on dm device creation.

# time for n in $(seq 1000); do dmsetup create --noudevsync --notable a; \
                                dmsetup remove a; done

real	0m15.684s
user	0m0.404s
sys	0m7.181s

-- 
Jun'ichi Nomura, NEC Corporation