Re: [PATCH 1/5] block/io: introduce bdrv_try_mark_request_serialising

[Vladimir Sementsov-Ogievskiy]

07.07.2020 18:56, Stefan Hajnoczi wrote:
> On Sat, Jun 20, 2020 at 05:36:45PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Introduce a function to mark the request serialising only if there are
> > no conflicting request to wait for.
> >
> > The function is static, so mark it unused. The attribute is to be
> > dropped in the next commit.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/io.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++-------
> >   1 file changed, 51 insertions(+), 7 deletions(-)
>
> I found this patch difficult to understand because there are multiple
> levels of functions passing flags to ultimiately do different things in
> a common function.
>
> Here are some ideas if you have time to rework this patch:
>
> 1. Introduce a bdrv_find_overlapping_request() function that does most
>     of bdrv_wait_serialising_requests_locked() but does not wait. Then
>     bdrv_wait_serialising_requests_locked() can call that function in a
>     loop and wait if an overlapping request is found.

I thought about it, but splitting bdrv_find_overlapping_request is not so clear:
it should include most of the logic inside "if (tracked_request_overlaps(..":
an assertion, and checking !req->waiting_for. So the semantics of new functions
becomes unclear, and it lead to splitting "->waiting_for" logic.. So, I decided
to keep the whole function as is, not splitted. I just can't imagine reasonable
split.

> 2. Pass overlap_offset/overlap_bytes arguments to
>     bdrv_find_overlapping_request() instead of changing and restoring the
>     value in bdrv_do_mark_request_serialising().

I'm not sure that it would be safe to not add a request to the list during the
search.

> 3. Use consistent names for flags: wait/blocking, found/success
>
> I'm not sure if all these ideas will work, but I get the feeling this
> code can be refactored to make it easier to understand. Since I don't
> have a concrete suggestion and the code looks correct:

Hmm. Unfortunately I didn't record the problems I faced on the way to resulting
design, so I just don't remember now the details. So, I'll try to apply your
suggestions, and remember the problems (or we'll get better patch :)

> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks!

--
Best regards,
Vladimir