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

Message

Vladimir Sementsov-Ogievskiy Sept. 4, 2021, 4:24 p.m. UTC

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: <[e-mail]>

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: <[e-mail]>

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

Comments

Vladimir Sementsov-Ogievskiy Sept. 22, 2021, 8:24 a.m. UTC | #1

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: <[e-mail]>
> 
> 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: <[e-mail]>
> 
> 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
>