Re: [RFC PATCH 2/2] dm: only initialize full request_queue for request-based device

[Kiyoshi Ueda <k-ueda@ct.jp.nec.com>]

Hi Mike,

On 05/11/2010 10:15 PM +0900, Mike Snitzer wrote:
> On Tue, May 11 2010 at 12:23am -0400,
> Kiyoshi Ueda <k-ueda@ct.jp.nec.com> 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