* [Qemu-devel] [PATCH] [RFC] qcow2: group refcount updates during cow
@ 2011-07-28 13:50 Frediano Ziglio
2011-07-28 14:15 ` Kevin Wolf
0 siblings, 1 reply; 3+ messages in thread
From: Frediano Ziglio @ 2011-07-28 13:50 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, Frediano Ziglio
Well, I think this is the first real improve patch.
Is more a RFC than a patch. Yes, some lines are terrible!
It collapses refcount decrement during cow.
>From a first check time executing 015 test passed from about 600 seconds
to 70.
This at least prove that refcount updates counts!
Some doubt:
1- place the code in qcow2-refcount.c as it update only refcount and not
cluster?
2- allow some sort of "begin transaction" / "commit" / "rollback" like
databases instead?
3- allow changing tables from different coroutines?
1) If you have a sequence like (1, 2, 4) probably these clusters are all in
the same l2 table but with this code you get two write instead of one.
I'm thinking about a function in qcow2-refcount.c that accept an array of cluster
instead of a start + len.
Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
---
block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++++--
1 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 81cf77d..da17365 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -675,10 +675,42 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
* Also flush bs->file to get the right order for L2 and refcount update.
*/
if (j != 0) {
+ int64_t old_start = 0, old_end = -2;
+ int count = 0;
for (i = 0; i < j; i++) {
- qcow2_free_any_clusters(bs,
- be64_to_cpu(old_cluster[i]) & ~QCOW_OFLAG_COPIED, 1);
+ old_cluster[i] = be64_to_cpu(old_cluster[i]) & ~QCOW_OFLAG_COPIED;
}
+ // XXX sort old_cluster
+ for (i = 0; i < j; i++) {
+ int64_t cluster = old_cluster[i];
+
+ /* group if contiguos */
+ if (old_end + 1 == (cluster >> s->cluster_bits)) {
+ ++old_end;
+ continue;
+ }
+
+ /* handle */
+ if (old_end > 0) {
+ qcow2_free_any_clusters(bs, old_start << s->cluster_bits, old_end - old_start + 1);
+ count += old_end - old_start + 1;
+ }
+ old_end = -2;
+
+ /* handle compressed separately */
+ if ((cluster & QCOW_OFLAG_COMPRESSED)) {
+ qcow2_free_any_clusters(bs, cluster, 1);
+ continue;
+ }
+
+ /* start a new group */
+ old_start = old_end = cluster >> s->cluster_bits;
+ }
+ if (old_end > 0) {
+ qcow2_free_any_clusters(bs, old_start << s->cluster_bits, old_end - old_start + 1);
+ count += old_end - old_start + 1;
+ }
+ assert(count == j);
}
ret = 0;
--
1.7.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] [RFC] qcow2: group refcount updates during cow
2011-07-28 13:50 [Qemu-devel] [PATCH] [RFC] qcow2: group refcount updates during cow Frediano Ziglio
@ 2011-07-28 14:15 ` Kevin Wolf
2011-07-29 10:54 ` Kevin Wolf
0 siblings, 1 reply; 3+ messages in thread
From: Kevin Wolf @ 2011-07-28 14:15 UTC (permalink / raw)
To: Frediano Ziglio; +Cc: qemu-devel
Am 28.07.2011 15:50, schrieb Frediano Ziglio:
> Well, I think this is the first real improve patch.
> Is more a RFC than a patch. Yes, some lines are terrible!
> It collapses refcount decrement during cow.
> From a first check time executing 015 test passed from about 600 seconds
> to 70.
> This at least prove that refcount updates counts!
> Some doubt:
> 1- place the code in qcow2-refcount.c as it update only refcount and not
> cluster?
> 2- allow some sort of "begin transaction" / "commit" / "rollback" like
> databases instead?
> 3- allow changing tables from different coroutines?
>
> 1) If you have a sequence like (1, 2, 4) probably these clusters are all in
> the same l2 table but with this code you get two write instead of one.
> I'm thinking about a function in qcow2-refcount.c that accept an array of cluster
> instead of a start + len.
>
> Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
I think what you're seeing is actually just one special case of a more
general problem. The problem is that we're interpreting writethrough
stricter than required.
The semantics that we really need is that on completion of a request,
all of its data and metadata must be flushed to disk. There is no
requirement that we flush all intermediate states.
My recent update to qcow2_update_snapshot_refcount() is just another
case of the same problem. I think the solution should be similar to what
I did there, i.e. switch the cache to writeback mode while we're
operating on it and switch back when we're done. We should probably have
functions that make both of this a one-liner (I think here we have some
similarity to your begin/commit idea).
With the right functions, this could become as easy as this (might need
better function names, but you get the idea):
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 882f50a..45b67b1 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -612,6 +612,8 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState
*bs, QCowL2Meta *m)
if (m->nb_clusters == 0)
return 0;
+ qcow2_cache_disable_writethrough(bs);
+
old_cluster = qemu_malloc(m->nb_clusters * sizeof(uint64_t));
/* copy content of unmodified sectors */
@@ -683,6 +685,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState
*bs, QCowL2Meta *m)
ret = 0;
err:
+ qcow2_cache_restore_writethrough(bs);
qemu_free(old_cluster);
return ret;
}
Kevin
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] [RFC] qcow2: group refcount updates during cow
2011-07-28 14:15 ` Kevin Wolf
@ 2011-07-29 10:54 ` Kevin Wolf
0 siblings, 0 replies; 3+ messages in thread
From: Kevin Wolf @ 2011-07-29 10:54 UTC (permalink / raw)
To: Frediano Ziglio; +Cc: qemu-devel
Am 28.07.2011 16:15, schrieb Kevin Wolf:
> Am 28.07.2011 15:50, schrieb Frediano Ziglio:
>> Well, I think this is the first real improve patch.
>> Is more a RFC than a patch. Yes, some lines are terrible!
>> It collapses refcount decrement during cow.
>> From a first check time executing 015 test passed from about 600 seconds
>> to 70.
>> This at least prove that refcount updates counts!
>> Some doubt:
>> 1- place the code in qcow2-refcount.c as it update only refcount and not
>> cluster?
>> 2- allow some sort of "begin transaction" / "commit" / "rollback" like
>> databases instead?
>> 3- allow changing tables from different coroutines?
>>
>> 1) If you have a sequence like (1, 2, 4) probably these clusters are all in
>> the same l2 table but with this code you get two write instead of one.
>> I'm thinking about a function in qcow2-refcount.c that accept an array of cluster
>> instead of a start + len.
>>
>> Signed-off-by: Frediano Ziglio <freddy77@gmail.com>
>
> I think what you're seeing is actually just one special case of a more
> general problem. The problem is that we're interpreting writethrough
> stricter than required.
>
> The semantics that we really need is that on completion of a request,
> all of its data and metadata must be flushed to disk. There is no
> requirement that we flush all intermediate states.
>
> My recent update to qcow2_update_snapshot_refcount() is just another
> case of the same problem. I think the solution should be similar to what
> I did there, i.e. switch the cache to writeback mode while we're
> operating on it and switch back when we're done. We should probably have
> functions that make both of this a one-liner (I think here we have some
> similarity to your begin/commit idea).
>
> With the right functions, this could become as easy as this (might need
> better function names, but you get the idea):
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 882f50a..45b67b1 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -612,6 +612,8 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState
> *bs, QCowL2Meta *m)
> if (m->nb_clusters == 0)
> return 0;
>
> + qcow2_cache_disable_writethrough(bs);
> +
> old_cluster = qemu_malloc(m->nb_clusters * sizeof(uint64_t));
>
> /* copy content of unmodified sectors */
> @@ -683,6 +685,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState
> *bs, QCowL2Meta *m)
>
> ret = 0;
> err:
> + qcow2_cache_restore_writethrough(bs);
> qemu_free(old_cluster);
> return ret;
> }
Maybe that's a bit too easy for a solution. With coroutines this
requires running under a CoMutex in order to avoid influencing other
requests and possibly missing a cache flush. This is contrary to our
goal of running requests in parallel, so maybe some more changes to how
the cache handles cache=writethrough are required.
Kevin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-07-29 10:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-28 13:50 [Qemu-devel] [PATCH] [RFC] qcow2: group refcount updates during cow Frediano Ziglio
2011-07-28 14:15 ` Kevin Wolf
2011-07-29 10:54 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.