Re: [dm-devel] [RFC 0/3] Add support of iopoll for dm device

[JeffleXu <jefflexu@linux.alibaba.com>]

On 11/10/20 2:15 AM, Mike Snitzer wrote:
> On Sat, Nov 07 2020 at  8:09pm -0500,
> JeffleXu <jefflexu@linux.alibaba.com> wrote:
>
>> On 11/7/20 1:45 AM, Mike Snitzer wrote:
>>> On Thu, Nov 05 2020 at  9:51pm -0500,
>>> JeffleXu <jefflexu@linux.alibaba.com> wrote:
>>>
>>>> blk-mq.c: blk_mq_submit_bio
>>>>
>>>>      if (bio->orig)
>>>>
>>>>          init blk_poll_data and insert it into bio->orig's @cookies list
>>>>
>>>> ```
>>> If you feel that is doable: certainly give it a shot.
>> Make sense.
>>
>>> But it is not clear to me how you intend to translate from cookie passed
>>> in to ->blk_poll hook (returned from submit_bio) to the saved off
>>> bio->orig.
>>>
>>> What is your cookie strategy in this non-recursive implementation?  What
>>> will you be returning?  Where will you be storing it?
>> Actually I think it's a common issue to design the cookie returned
>> by submit_bio() whenever it's implemented in a recursive or
>> non-recursive way. After all you need to translate the returned cookie
>> to the original bio even if it's implemented in a recursive way as you
>> described.
> Yes.
>
>> Or how could you walk through the bio graph when the returned cookie
>> is only 'unsigned int' type?
> You could store a pointer (to orig bio, or per-bio-data stored in split
> clone, or whatever makes sense for how you're associating poll data with
> split bios) in 'unsigned int' type but that's very clumsy -- as I
> discovered when trying to do that ;)

Fine, that's also what I thought.


>
>> How about this:
>>
>>
>> ```
>>
>> typedef uintptr_t blk_qc_t;
>>
>> ```
>>
>>
>> or something like union
>>
>> ```
>>
>> typedef union {
>>
>>      unsigned int cookie;
>>
>>      struct bio *orig; // the original bio of submit_bio()
>>
>> } blk_qc_t;
>>
>> ```
>> When serving for blk-mq, the integer part of blk_qc_t is used and it
>> stores the valid cookie, while it stores a pointer to the original bio
>> when serving bio-based device.
> Union looks ideal, but maybe make it a void *?  Initial implementation
> may store bio pointer but it _could_ evolve to be 'struct blk_poll_data
> *' or whatever.  But not a big deal either way.

Of course you could define blk_qc_t as a pointer type (e.g. void *), or 
integer type (e.g. unsigned int),

but you will get a gcc warning in each case. For example, if it's 
defined as "void *", then gcc will warn

'return makes pointer from integer without a cast' in request_to_qc_t() 
as cookie returned by mq

device is actually integer type. Vice versa. So we need a type cast in 
request_to_qc_t().


The union is also not perfect though, as we also need type cast.


So both these two designs are quite equal to me, though 'void *' may be 
more concise though.

But one annoying issue is that the request_to_qc_t() and blk_poll() 
should be revised somehow

if it's defined as a union or 'void *'. For example if it's defined as 
'void *', then in request_to_qc_t()

integer need to be cast to pointer and in blk_poll() pointer need to be 
cast to integer.


The benefit of uintptr_t is that, it's still integer type which means 
the original request_to_qc_t()/

blk_poll() routine for blk-mq can retain unchanged, while the size of 
the data type can be large

enough to contain a pointer type. So we only need  type cast in 
bio-based routine, while keeping

the request-based routine unchanged.


And yes it's a trivial issue though.


>> By the way, would you mind sharing your plan and progress on this
>> work, I mean, supporting iopoll for dm device. To be honest, I don't
>> want to re-invent the wheel as you have started on this work, but I do
>> want to participate in somehow. Please let me know if there's something
>> I could do here.
> I thought I said as much before but: I really don't have anything
> meaningful to share.  My early exploration was more rough pseudo code
> that served to try to get my arms around the scope of the design
> problem.
>
> Please feel free to own all aspects of this work.
>
> I will gladly help analyze/test/refine your approach once you reach the
> point of sharing RFC patches.

Got it. Thanks for that. Really. I will continue working on this.


-- 
Thanks,
Jeffle