Re: [PATCH v7 00/11] qcow2: fix parallel rewrite and discard (reqlist)

[Vladimir Sementsov-Ogievskiy]

Ping.

Hi Kevin!

Could you look at description in cover-letter and 07-09, and say do you like the approach 
in general? Then I can resend with reqlist included and without "Based-on".

Or do you still prefer the solution with RWLock?

I don't like RWLock-based blocking solution, because:

1. Mirror does discards in parallel with other requests, so blocking discard 
may decrease mirror performance
2. Backup (in push-backup-with-fleecing scheme) will do discards on temporary 
image, again blocking discard is bad in this case
3. block-commit and block-stream will at some moment do discard-after-copy to 
not waste disk space
4. It doesn't seem possible to pass ownership on RWLock to task coroutine (as 
we can't lock it in one coroutine and release in another)


04.09.2021 19:24, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> This is a new (third :)) variant of solving the problem in qcow2 that we
> lack synchronisation in reallocating host clusters with refcount=0 and
> guest reads/writes that may still be in progress on these clusters.
>
> Previous version was
> "[PATCH v6 00/12] qcow2: fix parallel rewrite and discard (lockless)"
> Supersedes: <20210422163046.442932-1-vsementsov@virtuozzo.com>
>
> Key features of new solution:
>
> 1. Simpler data structure: requests list instead of "dynamic refcount"
>     concept. What is good is that request list data structure is
>     implemented and used in other my series. So we can improve and reuse
>     it.
>
> 2. Don't care about discrads: my previous attempts were complicated by
>     trying to postpone discards when we have in-flight operations on
>     cluster which refcounts becomes 0. Don't bother with it anymore.
>     Nothing bad in discard: it similar as when guest does simultaneous
>     writes into same cluster: we don't care to serialize these writes.
>     So keep discards as is.
>
> So now the whole fix may be described in one sentence: don't consider a
> host cluster "free" (for allocation) if there are still some in-flight
> guest operations on it.
>
> What patches are:
>
> 1-2: simple additions to reqlist (see also *)
>
> 3: test. It's unchanged since previous solution
>
> 4-5: simpler refactorings
>
> 6-7: implement guest requests tracking in qcow2
>
> 8: refactoring for [9]
> 9: BUG FIX: use request tracking to avoid reallocating clusters under
>     guest operation
>
> 10-11: improvement if request tracking to avoid extra small critical
> sections that were added in [7]
>
> [*]:
> For this series to work we also need
>   "[PATCH v2 06/19] block: intoduce reqlist":
> Based-on: <20210827181808.311670-7-vsementsov@virtuozzo.com>
>
> Still, note that we use only simple core of reqlist and don't use its
> coroutine-related features. That probably means that I should split a
> kind of BlockReqListBase data structure out of BlockReqList, and then
> will have separate
>
> CoBlockReqList for "[PATCH v2 00/19] Make image fleecing more usable"
> series and Qcow2ReqList for this series..
>
> But that a side question.
>
> Vladimir Sementsov-Ogievskiy (11):
>    block/reqlist: drop extra assertion
>    block/reqlist: add reqlist_new_req() and reqlist_free_req()
>    iotests: add qcow2-discard-during-rewrite
>    qcow2: introduce qcow2_parse_compressed_cluster_descriptor()
>    qcow2: refactor qcow2_co_preadv_task() to have one return
>    qcow2: prepare for tracking guest io requests in data_file
>    qcow2: track guest io requests in data_file
>    qcow2: introduce is_cluster_free() helper
>    qcow2: don't reallocate host clusters under guest operation
>    block/reqlist: implement reqlist_mark_req_invalid()
>    qcow2: use reqlist_mark_req_invalid()
>
>   block/qcow2.h                                 |  15 ++-
>   include/block/reqlist.h                       |  33 ++++++
>   block/qcow2-cluster.c                         |  45 ++++++-
>   block/qcow2-refcount.c                        |  34 +++++-
>   block/qcow2.c                                 | 112 +++++++++++++-----
>   block/reqlist.c                               |  25 +++-
>   .../tests/qcow2-discard-during-rewrite        |  72 +++++++++++
>   .../tests/qcow2-discard-during-rewrite.out    |  21 ++++
>   8 files changed, 310 insertions(+), 47 deletions(-)
>   create mode 100755 tests/qemu-iotests/tests/qcow2-discard-during-rewrite
>   create mode 100644 tests/qemu-iotests/tests/qcow2-discard-during-rewrite.out


--
Best regards,
Vladimir