From patchwork Tue Aug 11 09:05:11 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nikanth Karthikesan X-Patchwork-Id: 40549 Received: from hormel.redhat.com (hormel1.redhat.com [209.132.177.33]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n7B93qOj002679 for ; Tue, 11 Aug 2009 09:03:52 GMT Received: from listman.util.phx.redhat.com (listman.util.phx.redhat.com [10.8.4.110]) by hormel.redhat.com (Postfix) with ESMTP id AB552619C46; Tue, 11 Aug 2009 05:03:51 -0400 (EDT) Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by listman.util.phx.redhat.com (8.13.1/8.13.1) with ESMTP id n7B93nc4019170 for ; Tue, 11 Aug 2009 05:03:49 -0400 Received: from mx1.redhat.com (mx1.redhat.com [172.16.48.31]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n7B93mcW031344; Tue, 11 Aug 2009 05:03:48 -0400 Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n7B93Xbx026618; Tue, 11 Aug 2009 05:03:33 -0400 Received: from relay1.suse.de (relay-ext.suse.de [195.135.221.8]) by mx2.suse.de (Postfix) with ESMTP id 8A88E79727; Tue, 11 Aug 2009 11:03:28 +0200 (CEST) From: Nikanth Karthikesan Organization: suse.de To: Kiyoshi Ueda Date: Tue, 11 Aug 2009 14:35:11 +0530 User-Agent: KMail/1.11.1 (Linux/2.6.27.23-0.1-default; KDE/4.2.1; x86_64; ; ) References: <200908081026.01097.knikanth@suse.de> <200908101618.21060.knikanth@suse.de> <4A812680.7040804@ct.jp.nec.com> In-Reply-To: <4A812680.7040804@ct.jp.nec.com> MIME-Version: 1.0 Content-Disposition: inline Message-Id: <200908111435.12020.knikanth@suse.de> X-RedHat-Spam-Score: -3.908 X-Scanned-By: MIMEDefang 2.58 on 172.16.52.254 X-Scanned-By: MIMEDefang 2.63 on 172.16.48.31 X-loop: dm-devel@redhat.com Cc: Mike Snitzer , linux-kernel@vger.kernel.org, dm-devel@redhat.com, Jens Axboe , Alasdair G Kergon Subject: [dm-devel] Re: [PATCH-v2 2/2] Initialize mempool and elevator only for request-based dm devices X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.5 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com On Tuesday 11 August 2009 13:36:24 Kiyoshi Ueda wrote: > Hi Nikanth, > > On 08/10/2009 07:48 PM +0900, Nikanth Karthikesan wrote: > > Intialize the request_queue and elevator only when the device is marked > > as a request-based device. This avoids unnecessary creation of mempool > > for requests. Also we wrongly initialize the elevator even for bio-based > > devices. As the /sys/block/dm-*/queue/scheduler is exported for > > device-mapper devices, it is possible to confuse with scheduler options > > for bio-based devices where scheduler is not at all used. > > Thank you for working on this. > Actually, I had tried this delayed allocation thing before, > but I chose the current implementation since I couldn't solve > some problems, which your patch also has. > Please see my comment below. > Thanks for the review & comments. > > @@ -2203,6 +2199,25 @@ int dm_swap_table(struct mapped_device *md, struct > > dm_table *table) goto out; > > } > > > > + /* new device is being marked as request-based */ > > + if (!md->map && dm_table_request_based(table)) { > > + /* initialize queue for request-based dm */ > > + r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL); > > + if (r) > > + goto out; > > Generally, dm must not allocate memory during resume because > it may cause a deadlock in no memory situation. > However, there is no I/O on this device at this point, > so the allocation should be ok for this special case. > I think some comments are needed here to describe that. > Ok. This comment can be added. > > + > > + /* > > + * reinitialize make_request_fn as it was reset to the > > + * default __make_request by blk_init_allocate_queue > > + */ > > + md->saved_make_request_fn = md->queue->make_request_fn; > > + blk_queue_make_request(md->queue, dm_request); > > + > > + blk_queue_softirq_done(md->queue, dm_softirq_done); > > + blk_queue_prep_rq(md->queue, dm_prep_fn); > > + blk_queue_lld_busy(md->queue, dm_lld_busy); > > + } > > + > > __unbind(md); > > r = __bind(md, table, &limits); > > The queue has been registered at the device creation time by > add_disk() in alloc_dev(). > Since the queue is reconfigured (elevator is attached), you have to > update the queue registration (e.g. unregister, then re-register). > But it may not be easy. At least, there is no exported interface to > unregister/re-register queue. Ah, yes. The scheduler attributes will not be exported in /sys/block/dm*/queue/iosched. Exporting elv_register_queue() and calling it here solves it. Something like.. @@ -2203,6 +2199,29 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table) goto out; } + /* new device is being marked as request-based */ + if (!md->map && dm_table_request_based(table)) { + /* initialize queue for request-based dm */ + r = blk_init_allocated_queue(md->queue, dm_request_fn, NULL); + if (r) + goto out; + + r = elv_register_queue(md->queue); + /* if (r) + * goto out; Better to ignore, just like add_disk does ;-) + */ + /* + * reinitialize make_request_fn as it was reset to the + * default __make_request by blk_init_allocate_queue + */ + md->saved_make_request_fn = md->queue->make_request_fn; + blk_queue_make_request(md->queue, dm_request); + + blk_queue_softirq_done(md->queue, dm_softirq_done); + blk_queue_prep_rq(md->queue, dm_prep_fn); + blk_queue_lld_busy(md->queue, dm_lld_busy); + } + __unbind(md); r = __bind(md, table, &limits); I would post the v3 of the patches with this change. Do you see any problems in this? This can also be solved by initializing the queue for new devices and then unregistering the elevator, if it is a bio-based device at table load time. Like... out: But, I think, delaying the initialization, is the best solution. Thanks Nikanth --- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel diff --git a/block/elevator.c b/block/elevator.c index 2d511f9..864dd29 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -939,9 +939,10 @@ static void __elv_unregister_queue(struct elevator_queue *e) void elv_unregister_queue(struct request_queue *q) { - if (q) + if (q && q->elevator) __elv_unregister_queue(q->elevator); } +EXPORT_SYMBOL(elv_unregister_queue); void elv_register(struct elevator_type *e) { diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 8a311ea..f6f77ea 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -2203,7 +2203,28 @@ int dm_swap_table(struct mapped_device *md, struct dm_table *table) goto out; } - __unbind(md); + if (md->map) + __unbind(md); + else if (!dm_table_request_based(table)) { + /* + * This is a new bio-based device, and doesnt use the elevator + * and requests. + */ + struct request_queue *q; + q = md->queue; + if (q->elevator) { + struct request_list *rl = &q->rq; + elevator_exit(q->elevator); + elv_unregister_queue(q); + q->elevator = 0; + if (rl->rq_pool) { + mempool_destroy(rl->rq_pool); + rl->rq_pool = 0; + } + } + + } + r = __bind(md, table, &limits);