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

[Mike Snitzer <snitzer@redhat.com>]

On Thu, Nov 05 2020 at  9:51pm -0500,
JeffleXu <jefflexu@linux.alibaba.com> wrote:

> 
> 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.

I feel we need the ability to walk the entire graph and call down to
next level.  The lowest level would be what you call a "valid cookie"
that blk-mq returned.  But the preceding cookies need to be made valid
and used when walking the graph (from highest to lowest) and calling
down to the underlying layers.

> 
> 
> 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
> 
> ```

If you feel that is doable: certainly give it a shot.

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?

Mike