[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 1/5] block/io: introduce bdrv_try_mark_request_serialising
Date: Wed, 8 Jul 2020 18:51:18 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

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

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

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>


Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]