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 <[email protected]> > > > --- > > > 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
signature.asc
Description: PGP signature
