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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2502 bytes --]

On Wed, Jul 08, 2020 at 06:51:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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 :)

Thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]