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

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

On 11/4/20 11:08 PM, Mike Snitzer wrote:
>> I'm doubted if this should be implemented in block layer like:
>>
>> ```
>>
>> struct bio {
>>
>>      ...
>>
>>      struct list_head  cookies;
>>
>> };
>>
>> ```
>>
>> After all it's only used by bio-based queue, or more specifically
>> only dm device currently.
> I do think this line of work really should be handled in block core
> because I cannot see any reason why MD or bcache or whatever bio-based
> device wouldn't want the ability to better support io_uring (with IO
> poll).
>
>> Another design I can come up with is to maintain a global data
>> structure for the very beginning
>> original bio. Currently the blocking point is that now one original
>> bio to the dm device (@bio of dm_submit()) can correspond to multiple
>> dm_io and thus we have nowhere to place the @cookies list.
> Yes, and that will always be the case.  We need the design to handle an
> arbitrary sprawl of splitting from a given bio.  The graph of bios
> resulting from that fan-out needs to be walked at various levels -- be
> it the top-level original bio's submit_bio() returned cookie or some
> intermediate point in the chain of bios.
>
> The problem is the lifetime of the data structure created for a given
> split bio versus layering boundaries (that come from block core's
> simplistic recursion via bio using submit_bio).
>
>> Now we have to maintain one data structure for every original bio,
>> something like
>>
>> ```
>>
>> struct dm_poll_instance {
>>
>>      ...
>>
>>      struct list_head cookies;
>>
>> };
>>
>> ```
> I do think we need a hybrid where at the point of recursion we're able
> to make the associated data structure available across the recursion
> boundary so that modeling the association in a chain of split bios is
> possible. (e.g. struct dm_poll_data or dm_poll_instance as you named it,
> _but_ that struct definition would live in block core, but would be part
> of per-bio-data; so 'struct blk_poll_data' is more logical name when
> elevated to block core).
>
> It _might_ be worthwhile to see if a new BIO_ flag could be added to
> allow augmenting the bio_split + bio_chain pattern to also track this
> additional case of carrying additional data per-bio while creating
> bio-chains.  I may not be clear yet, said differently: augmenting
> bio_chain to not only chain bios, but to _also_ thread/chain together
> per-bio-data that lives within those chained bios.  SO you have the
> chain of bios _and_ the chain of potentially opaque void * that happens
> to point to a list head for a list of 'struct blk_poll_data'.
>
> Does that make sense?


I'm doubted if it really makes sense to maintain a *complete* bio chain 
across the recursive

call boundary.


Considering the following device stack:

```

                                   dm0

         dm1                   dm2           dm3

nvme0  nvme1          ....               ....

```


We have the following bio graph (please let me know if it's wrong or the 
image can't display)


For example, for dm1 there are three bios containing valid cookie in the 
end, that is

bio 9/10/11. We only need to insert the corresponding blk_poll_data 
(containing

request_queue, cookie, etc) of these three bios into the very beginning 
original

bio (that is bio0). Of course we can track all the sub-bios down through 
the device

stack, layer by layer, e.g.,

- get bio 1/2/3 from bio 0

- get bio 4 from bio 3

- finally get bio 9 from bio 4

But I'm doubted if it's overkill to just implement the iopoll.


Another issue still unclear is that, if we should implement the iopoll 
in a recursive way.

In a recursive way, to poll dm 0, we should poll all its sub-devices, 
that is, bio 4/5/7/8.

Oppositely we can insert only the bottom bio (bio 9/10/11 which have 
valid cookie) at

the very beginning (at submit_bio() phase), and to poll dm 0, we only 
need to poll bio

9/10/11.


To implement this non-recursive design, we can add a field in struct bio

```

struct bio {

     ...

     struct bio *orig;

}

```

@orig points to the original bio inputted into submit_bio(), that is, bio 0.


@orig field is transmitted through bio splitting.

```

bio_split()

     split->orig = bio->orig ? : bio


dm.c: __clone_and_map_data_bio

     clone->orig = bio->orig ? : bio

```


Finally bio 9/10/11 can be inserted into bio 0.

```

blk-mq.c: blk_mq_submit_bio

     if (bio->orig)

         init blk_poll_data and insert it into bio->orig's @cookies list

```

>
>> We can transfer this dm_poll_instance between split bios by
>> bio->bi_private, like
>>
>> ```
>>
>> dm_submit_bio(...) {
>>
>>      struct dm_poll_instance *ins;
>>
>>      if (bio->bi_private)
>>
>>          ins = bio->bi_private;
>>
>>      else {
>>
>>          ins = alloc_poll_instance();
>>
>>          bio->bi_private = ins;
>>
>>      }
>>
>>      ...
>>
>> }
>>
>> ```
> Sadly, we cannot (ab)use bi_private for this given its (ab)used via the
> bio_chain() interface.  It's almost like we need to add a new pointer in
> the bio that isn't left for block core to hijack.
>
> There is the well-worn pattern of saving off the original bi_private,
> hooking a new endio method and then when that endio is called restoring
> bi_private but we really want to avoid excessive indirect function calls
> for this usecase.  The entire point of implementing blk_poll support is
> for performance after all.
>
> Mike
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

-- 
Thanks,
Jeffle