On 11/10/20 2:15 AM, Mike Snitzer wrote:
On Sat, Nov 07 2020 at 8:09pm -0500,
JeffleXu <jefflexu@xxxxxxxxxxxxxxxxx> wrote:
On 11/7/20 1:45 AM, Mike Snitzer wrote:
On Thu, Nov 05 2020 at 9:51pm -0500,
JeffleXu <jefflexu@xxxxxxxxxxxxxxxxx> 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
