From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kiyoshi Ueda Subject: Re: [RFC PATCH 2/2] dm: only initialize full request_queue for request-based device Date: Wed, 12 May 2010 17:23:59 +0900 Message-ID: <4BEA659F.9050206@ct.jp.nec.com> References: <1273532139-23043-1-git-send-email-snitzer@redhat.com> <1273532139-23043-2-git-send-email-snitzer@redhat.com> <4BE8DBB0.5060701@ct.jp.nec.com> <20100511131502.GA25211@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20100511131502.GA25211@redhat.com> Sender: linux-kernel-owner@vger.kernel.org To: Mike Snitzer Cc: dm-devel@redhat.com, linux-kernel@vger.kernel.org, Jens Axboe , Jun'ichi Nomura , Vivek Goyal , Nikanth Karthikesan List-Id: dm-devel.ids Hi Mike, On 05/11/2010 10:15 PM +0900, Mike Snitzer wrote: > On Tue, May 11 2010 at 12:23am -0400, > Kiyoshi Ueda wrote: > >> Hi Mike, >> >> On 05/11/2010 07:55 AM +0900, Mike Snitzer wrote: >>> Revert back to only allocating a minimalist request_queue structure >>> initially (needed for both bio and request-based DM). Initialization of >>> a full request_queue (request_fn, elevator, etc) is deferred until it is >>> known that the DM device is request-based. >> >> Thank you for working on this. >> However, I still disagree with this patch as we discussed on this thread: >> http://marc.info/?t=124990138700003&r=1&w=2 >> (Exporting a part of queue's features may cause some maintenance costs >> in future.) > > Thanks for the reference. I completely forgot about that thread (even > though I responded to Nikanth's patches in detail! :) > > It is clear we need to resolve the current full request_queue > initialization that occurs even for bio-based DM devices. > > I believe the 2 patches I posted accomplish this in a stright-forward > way. We can always improve on it (by looking at what you proposed > below) but we need a minimlaist fix that doesn't depend on userspace > LVM2 changes right now. Humm, OK. Indeed, showing iosched directory in bio-based device's sysfs is confusing users actually, and we need something to resolve that soon. So I don't strongly object to your approach as the first step, as long as we can accept the risk of the maintenance cost which I mentioned. By the way, your current patch has a problem below. It needs to be fixed at least. > Similarly, my proposed DM changes are also quite natural. By using > dm_table_set_type() as the hook to initialize the request-based DM > device's elevator we perform allocations during table load. Your patch initializes queue everytime request-based table is loaded. I think that could cause a problem (although I haven't tested your patch yet). Also, as you know, the table load can be canceled. So initializing queue at table loading time may cause some weird behaviors for users. For example, # dmsetup create --notable bio-based # echo "0 100 multipath ..." | dmsetup load bio-based # echo "0 100 linear ..." | dmsetup load bio-based # dmsetup resume bio-based # ls /sys/block/dm-0/queue/ ... iosched ... If you (and Alasdair) think this behavior is acceptable, it might be OK. I just feel it's weird though... > Having just looked at Nikanth's proposed DM patch 2/2 again it shows > that blk_init_allocated_queue(), which allocates memory, was being > called during resume (dm_swap_table). Allocations are not allowed > during resume. Right, in general. However, in this special case, I think initializing queue (allocating memory) during resume should be OK as I mentioned like below in: http://marc.info/?l=linux-kernel&m=124999806420663&w=2 > 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. So you should be able to take this approach. Thanks, Kiyoshi Ueda