[Qemu-block] [PATCH] bitmap: Update count after a merge
We need an accurate count of the number of bits set in a bitmap after a merge. In particular, since the merge operation short-circuits a merge from an empty source, if you have bitmaps A, B, and C where B started empty, then merge C into B, and B into A, an inaccurate count meant that A did not get the contents of C. Fixes: be58721db CC: qemu-sta...@nongnu.org Signed-off-by: Eric Blake --- Probably worth some testsuite coverage, but for a late-night one-liner, this is as much as I can do today. util/hbitmap.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/hbitmap.c b/util/hbitmap.c index bcd304041aa..52e12da4b48 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -753,3 +753,4 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b) a->levels[i][j] |= b->levels[i][j]; } } +a->count = hb_count_between(a, 0, a->size - 1); return true; } -- 2.17.1
[Qemu-block] [PULL 1/3] nbd/server: fix bitmap export
From: Vladimir Sementsov-Ogievskiy
bitmap_to_extents function is broken: it switches dirty variable after
every iteration, however it can process only part of dirty (or zero)
area during one iteration in case when this area is too large for one
extent.
Fortunately, the bug doesn't produce wrong extent flags: it just inserts
a zero-length extent between sequential extents representing large dirty
(or zero) area. However, zero-length extents are forbidden by the NBD
protocol. So, a careful client should consider such a reply as a server
fault, while a less-careful will likely ignore zero-length extents.
The bug can only be triggered by a client that requests block status
for nearly 4G at once (a request of 4G and larger is impossible per
the protocol, and requests smaller than 4G less the bitmap granularity
cause the loop to quit iterating rather than revisit the tail of the
large area); it also cannot trigger if the client used the
NBD_CMD_FLAG_REQ_ONE flag. Since qemu 3.0 as client (using the
x-dirty-bitmap extension) always passes the flag, it is immune; and
we are not aware of other open-source clients that know how to request
qemu:dirty-bitmap:FOO contexts. Clients that want to avoid the bug
could cap block status requests to a smaller length, such as 2G or 3G.
Fix this by more careful handling of dirty variable.
Bug was introduced in 3d068aff16
"nbd/server: implement dirty bitmap export", with the whole function.
and is present in v3.0.0 release.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20180914165116.23182-1-vsement...@virtuozzo.com>
CC: qemu-sta...@nongnu.org
Reviewed-by: Eric Blake
[eblake: improved commit message]
Signed-off-by: Eric Blake
---
nbd/server.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/nbd/server.c b/nbd/server.c
index ea5fe0eb336..12f721482dd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1951,6 +1951,8 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap
*bitmap, uint64_t offset,
assert(begin < overall_end && nb_extents);
while (begin < overall_end && i < nb_extents) {
+bool next_dirty = !dirty;
+
if (dirty) {
end = bdrv_dirty_bitmap_next_zero(bitmap, begin);
} else {
@@ -1962,6 +1964,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap
*bitmap, uint64_t offset,
end = MIN(bdrv_dirty_bitmap_size(bitmap),
begin + UINT32_MAX + 1 -
bdrv_dirty_bitmap_granularity(bitmap));
+next_dirty = dirty;
}
if (dont_fragment && end > overall_end) {
end = overall_end;
@@ -1971,7 +1974,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap
*bitmap, uint64_t offset,
extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);
i++;
begin = end;
-dirty = !dirty;
+dirty = next_dirty;
}
bdrv_dirty_iter_free(it);
--
2.17.1
[Qemu-block] [PULL 3/3] nbd/server: send more than one extent of base:allocation context
From: Vladimir Sementsov-Ogievskiy
This is necessary for efficient block-status export, for clients which
support it. (qemu is not yet such a client, but could become one.)
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20180704112302.471456-3-vsement...@virtuozzo.com>
[eblake: grammar tweaks]
Signed-off-by: Eric Blake
---
nbd/server.c | 87 ++--
1 file changed, 64 insertions(+), 23 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 12f721482dd..c3dd402b45e 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1844,37 +1844,68 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
return ret;
}
-static int blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,
-uint64_t bytes, NBDExtent *extent)
+/*
+ * Populate @extents from block status. Update @bytes to be the actual
+ * length encoded (which may be smaller than the original), and update
+ * @nb_extents to the number of extents used.
+ *
+ * Returns zero on success and -errno on bdrv_block_status_above failure.
+ */
+static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
+ uint64_t *bytes, NBDExtent *extents,
+ unsigned int *nb_extents)
{
-uint64_t remaining_bytes = bytes;
+uint64_t remaining_bytes = *bytes;
+NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
+bool first_extent = true;
+assert(*nb_extents);
while (remaining_bytes) {
uint32_t flags;
int64_t num;
int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
&num, NULL, NULL);
+
if (ret < 0) {
return ret;
}
flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
(ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
-
-if (remaining_bytes == bytes) {
-extent->flags = flags;
-}
-
-if (flags != extent->flags) {
-break;
-}
-
offset += num;
remaining_bytes -= num;
+
+if (first_extent) {
+extent->flags = flags;
+extent->length = num;
+first_extent = false;
+continue;
+}
+
+if (flags == extent->flags) {
+/* extend current extent */
+extent->length += num;
+} else {
+if (extent + 1 == extents_end) {
+break;
+}
+
+/* start new extent */
+extent++;
+extent->flags = flags;
+extent->length = num;
+}
+}
+
+extents_end = extent + 1;
+
+for (extent = extents; extent < extents_end; extent++) {
+cpu_to_be32s(&extent->flags);
+cpu_to_be32s(&extent->length);
}
-cpu_to_be32s(&extent->flags);
-extent->length = cpu_to_be32(bytes - remaining_bytes);
+*bytes -= remaining_bytes;
+*nb_extents = extents_end - extents;
return 0;
}
@@ -1910,21 +1941,29 @@ static int nbd_co_send_extents(NBDClient *client,
uint64_t handle,
/* Get block status from the exported device and send it to the client */
static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
BlockDriverState *bs, uint64_t offset,
-uint32_t length, bool last,
-uint32_t context_id, Error **errp)
+uint32_t length, bool dont_fragment,
+bool last, uint32_t context_id,
+Error **errp)
{
int ret;
-NBDExtent extent;
+unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS;
+NBDExtent *extents = g_new(NBDExtent, nb_extents);
+uint64_t final_length = length;
-ret = blockstatus_to_extent_be(bs, offset, length, &extent);
+ret = blockstatus_to_extents(bs, offset, &final_length, extents,
+ &nb_extents);
if (ret < 0) {
+g_free(extents);
return nbd_co_send_structured_error(
client, handle, -ret, "can't get block status", errp);
}
-return nbd_co_send_extents(client, handle, &extent, 1,
- be32_to_cpu(extent.length), last,
- context_id, errp);
+ret = nbd_co_send_extents(client, handle, extents, nb_extents,
+ final_length, last, context_id, errp);
+
+g_free(extents);
+
+return ret;
}
/*
@@ -2231,10 +2270,12 @@ static coroutine_fn int nbd_handle_request(NBDClient
*client,
(client->export_meta.base_allocation ||
client->export_meta.bitmap))
{
+bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
+
if (client->export_meta.base_allocation) {
Re: [Qemu-block] [Qemu-devel] [PATCH] dirty-bitmaps: allow merging to disabled bitmaps
On 9/19/18 4:16 PM, John Snow wrote: On 09/19/2018 05:08 PM, Eric Blake wrote: On 9/19/18 2:58 PM, John Snow wrote: We wish to prohibit merging to read-only bitmaps and frozen bitmaps, but "disabled" bitmaps only preclude their recording of live, new information. It does not prohibit them from manual writes at the behest of the user, as is the case for merge operations. Reported-by: Eric Blake Signed-off-by: John Snow --- block/dirty-bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) I can add this to my NBD queue, if desired. If that makes life easier for you, then please be my guest -- otherwise there's no conflict in taking it through the bitmaps tree I've got. At this point, I'm not queuing this through NBD, as I think it is better served by just folding this directly into your v3 permissions series (or does that need to go up to v4?) Just let me know either way. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH v3 0/5] dirty-bitmaps: fix QMP command permissions
On 9/25/18 6:49 PM, John Snow wrote: based on: jsnow/bitmaps staging branch This series builds on a previous standalone patch and adjusts the permission for all (or most) of the QMP bitmap commands. John Snow (5): block/dirty-bitmaps: add user_modifiable status checker block/dirty-bitmaps: fix merge permissions block/dirty-bitmaps: allow clear on disabled bitmaps block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps block/backup: prohibit backup from using in-use bitmaps block/dirty-bitmap.c | 13 +--- blockdev.c | 75 include/block/dirty-bitmap.h | 1 + 3 files changed, 44 insertions(+), 45 deletions(-) Should there be any testsuite coverage of these changes? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH v3 1/5] block/dirty-bitmaps: add user_modifiable status checker
On 9/26/18 7:19 AM, Vladimir Sementsov-Ogievskiy wrote: 26.09.2018 14:53, Vladimir Sementsov-Ogievskiy wrote: 26.09.2018 02:49, John Snow wrote: Instead of both frozen and qmp_locked checks, wrap it into one check. frozen implies the bitmap is split in two (for backup), and shouldn't be modified. qmp_locked implies it's being used by another operation, like being exported over NBD. In both cases it means we shouldn't allow the user to modify it in any meaningful way. Replace any usages where we check both frozen and qmp_locked with the new check. to reduce number of '!', we may use opposite check, for ex "bdrv_dirty_bitmap_user_locked". bad reason to rewrite the whole series, so, ignore this comment) Then again, if you change 'in-use' to 'in use' in the entire series, you might as well flip the logic (double-negative logic is slightly harder to follow than suitably-named positive logic). At this point, I have not queued this series on my NBD queue, because there may indeed be a reason for v4 to touch up spelling and logic. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH v3 1/5] block/dirty-bitmaps: add user_modifiable status checker
On 9/26/18 6:53 AM, Vladimir Sementsov-Ogievskiy wrote:
26.09.2018 02:49, John Snow wrote:
Instead of both frozen and qmp_locked checks, wrap it into one check.
frozen implies the bitmap is split in two (for backup), and shouldn't
be modified. qmp_locked implies it's being used by another operation,
like being exported over NBD. In both cases it means we shouldn't allow
the user to modify it in any meaningful way.
Replace any usages where we check both frozen and qmp_locked with the
new check.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 6 ++
blockdev.c | 29 -
include/block/dirty-bitmap.h | 1 +
3 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8ac933cf1c..fc10543ab0 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap
*bitmap)
return bitmap->successor;
}
+/* Both conditions disallow user-modification via QMP. */
+bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap) {
+ return !(bdrv_dirty_bitmap_frozen(bitmap) ||
+ bdrv_dirty_bitmap_qmp_locked(bitmap));
+}
to reduce number of '!', we may use opposite check, for ex
"bdrv_dirty_bitmap_user_locked".
Meaning make this function return true if locked for one less negation
in the function body...
anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy
+++ b/blockdev.c
@@ -2009,11 +2009,8 @@ static void
block_dirty_bitmap_clear_prepare(BlkActionState *common,
return;
}
- if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
- error_setg(errp, "Cannot modify a frozen bitmap");
- return;
- } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
- error_setg(errp, "Cannot modify a locked bitmap");
+ if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
+ error_setg(errp, "Cannot modify a bitmap in-use by another
operation");
return;
...and since most callers were negating sense as well?
I'm not sure I'm a fan of "in-use" with the hyphen. It sounds better to
me to just spell it out as two words. (multiple instances)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-block] [PATCH 2/2] nbd/server: send more than one extent of base:allocation context
On 9/17/18 11:15 AM, Vladimir Sementsov-Ogievskiy wrote: 05.07.2018 19:18, Vladimir Sementsov-Ogievskiy wrote: 05.07.2018 18:59, Eric Blake wrote: On 07/04/2018 06:23 AM, Vladimir Sementsov-Ogievskiy wrote: This is necessary for efficient block-status export, for clients which support it. This is a feature, so it is indeed 3.1 material. Reviewed-by: Eric Blake Ok, thank you! hmm, looks like lost patch :( Not lost, just extremely slow getting queued. Now on my NBD queue. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[Qemu-block] [PATCH] util/hbitmaps: recalculate count on merge
We have been neglecting to do so, which results in wrong counts after merge. In the worst case, we may think the bitmap is empty when it has had new writes merged into it. Reported-by: Eric Blake Signed-off-by: John Snow --- util/hbitmap.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/util/hbitmap.c b/util/hbitmap.c index d5aca5159f..28e9c523ab 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -759,6 +759,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result) } } +/* Recompute the dirty count */ +a->count = hb_count_between(a, 0, a->size - 1); + return true; } -- 2.14.4
Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/5] dirty-bitmaps: fix QMP command permissions
On 09/26/2018 08:19 AM, Vladimir Sementsov-Ogievskiy wrote: > 26.09.2018 02:49, John Snow wrote: >> based on: jsnow/bitmaps staging branch >> >> This series builds on a previous standalone patch and adjusts >> the permission for all (or most) of the QMP bitmap commands. >> >> John Snow (5): >> block/dirty-bitmaps: add user_modifiable status checker >> block/dirty-bitmaps: fix merge permissions >> block/dirty-bitmaps: allow clear on disabled bitmaps >> block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps >> block/backup: prohibit backup from using in-use bitmaps >> >> block/dirty-bitmap.c | 13 +--- >> blockdev.c | 75 >> >> include/block/dirty-bitmap.h | 1 + >> 3 files changed, 44 insertions(+), 45 deletions(-) >> > > Great! Thank you for clearing this. I contributed a lot to this mess > with my qmp-locked( > It happens. I should have caught it too, but I think the ramifications here aren't so bad. > PS: I have a draft patch in my current developments to allow set/reset > bits in disabled bitmaps, which is needed to use BdrvDirtyBitmap as a > shared named copy-bitmap between fleecing-hook filter and backup job. So > "disabled" is actually only for use in bdrv_set_dirty(), to disable > automatic bitmap updates on guest writes. > OK, I'll keep an eye out for the series when it comes. Thanks for your reviews, touched up commit message on #2 to reflect that you had already fixed the problem, and staged to my bitmaps branch: Thanks, applied to my bitmaps tree: https://github.com/jnsnow/qemu/commits/bitmaps https://github.com/jnsnow/qemu.git --js
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] block/dirty-bitmaps: fix merge permissions
On 09/26/2018 08:08 AM, Vladimir Sementsov-Ogievskiy wrote:
> 26.09.2018 14:55, Vladimir Sementsov-Ogievskiy wrote:
>> 26.09.2018 02:49, John Snow wrote:
>>> We wish to prohibit merging to read-only bitmaps and frozen bitmaps,
>>> but "disabled" bitmaps only preclude their recording of live, new
>>> information. It does not prohibit them from manual writes at the behest
>>> of the user, as is the case for merge operations.
>>>
>>> Allow the merge to "disabled" bitmaps,
>>> and prohibit merging to "locked" ones.
>>
>> only the second part is here..
>
> Hm, the first one is in first separate patch? With commit message fixed
> to only second part, of course:
>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
Ah, you actually changed it yourself in
b3024909c4b62a989261c288c797f86b150c43fb, for the merge transaction
series -- it just goes unmentioned.
When I rebased on top of the staging branch I lost that change as it was
already made.
I'll update the commit message.
--js
>>
>>>
>>> Reported-by: Eric Blake
>>> Signed-off-by: John Snow
>>> ---
>>> block/dirty-bitmap.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index fc10543ab0..53b7d282c4 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -806,9 +806,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap
>>> *dest, const BdrvDirtyBitmap *src,
>>> qemu_mutex_lock(dest->mutex);
>>> - if (bdrv_dirty_bitmap_frozen(dest)) {
>>> - error_setg(errp, "Bitmap '%s' is frozen and cannot be
>>> modified",
>>> - dest->name);
>>> + if (!bdrv_dirty_bitmap_user_modifiable(dest)) {
>>> + error_setg(errp, "Bitmap '%s' is currently in-use by another"
>>> + " operation and cannot be modified", dest->name);
>>> goto out;
>>> }
>>
>>
>
>
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/5] block/dirty-bitmaps: fix merge permissions
On 09/26/2018 08:08 AM, Vladimir Sementsov-Ogievskiy wrote:
> 26.09.2018 14:55, Vladimir Sementsov-Ogievskiy wrote:
>> 26.09.2018 02:49, John Snow wrote:
>>> We wish to prohibit merging to read-only bitmaps and frozen bitmaps,
>>> but "disabled" bitmaps only preclude their recording of live, new
>>> information. It does not prohibit them from manual writes at the behest
>>> of the user, as is the case for merge operations.
>>>
>>> Allow the merge to "disabled" bitmaps,
>>> and prohibit merging to "locked" ones.
>>
>> only the second part is here..
>
> Hm, the first one is in first separate patch? With commit message fixed
> to only second part, of course:
>
Ah, yeah, I'll make that clearer. Got lost in the patch reordering. Thanks!
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
>>
>>>
>>> Reported-by: Eric Blake
>>> Signed-off-by: John Snow
>>> ---
>>> block/dirty-bitmap.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index fc10543ab0..53b7d282c4 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -806,9 +806,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap
>>> *dest, const BdrvDirtyBitmap *src,
>>> qemu_mutex_lock(dest->mutex);
>>> - if (bdrv_dirty_bitmap_frozen(dest)) {
>>> - error_setg(errp, "Bitmap '%s' is frozen and cannot be
>>> modified",
>>> - dest->name);
>>> + if (!bdrv_dirty_bitmap_user_modifiable(dest)) {
>>> + error_setg(errp, "Bitmap '%s' is currently in-use by another"
>>> + " operation and cannot be modified", dest->name);
>>> goto out;
>>> }
>>
>>
>
>
[Qemu-block] [PATCH v2 1/2] MAINTAINERS: Replace myself with John Snow for block jobs
I'll not be involved with day-to-day qemu development, and John Snow is a block jobs wizard. Have him take over block job maintainership duties. Signed-off-by: Jeff Cody --- MAINTAINERS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index ce7c351afa..0e22b795e6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1455,7 +1455,7 @@ F: include/scsi/* F: scsi/* Block Jobs -M: Jeff Cody +M: John Snow L: qemu-block@nongnu.org S: Supported F: blockjob.c @@ -1468,7 +1468,7 @@ F: block/commit.c F: block/stream.c F: block/mirror.c F: qapi/job.json -T: git git://github.com/codyprime/qemu-kvm-jtc.git block +T: git git://github.com/jnsnow/qemu.git jobs Block QAPI, monitor, command line M: Markus Armbruster -- 2.17.1
[Qemu-block] [PATCH v2 0/2] Maintainership changes
Changes from v1: - cc'ed Peter (meant to for v1, forgot) - added John's 'jobs' branch to his git url - kept myself as maintainer for vhdx - moved sheepdog to 'Odd Fixes' I'm not going to be involved with day-to-day qemu development, so this necessitates some changes. Jeff Cody (2): MAINTAINERS: Replace myself with John Snow for block jobs MAINTAINERS: Remove myself as block maintainer MAINTAINERS | 21 - 1 file changed, 4 insertions(+), 17 deletions(-) -- 2.17.1
[Qemu-block] [PATCH v2 2/2] MAINTAINERS: Remove myself as block maintainer
I'll not be involved in day-to-day qemu development. Remove myself as maintainer from the remainder of the network block drivers, and revert them to the general block layer maintainership. Move 'sheepdog' to the 'Odd Fixes' support level. For VHDX, added my personal email address as a maintainer, as I can answer questions or send the occassional bug fix. Leaving it as 'Supported', instead of 'Odd Fixes', because I think the rest of the block layer maintainers and developers will upkeep it as well, if needed. Signed-off-by: Jeff Cody --- MAINTAINERS | 17 ++--- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 0e22b795e6..7147c44bbc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1988,28 +1988,23 @@ F: block/vmdk.c RBD M: Josh Durgin -M: Jeff Cody L: qemu-block@nongnu.org S: Supported F: block/rbd.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block Sheepdog M: Hitoshi Mitake M: Liu Yuan -M: Jeff Cody L: qemu-block@nongnu.org L: sheep...@lists.wpkg.org -S: Supported +S: Odd Fixes F: block/sheepdog.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block VHDX -M: Jeff Cody +M: Jeff Cody L: qemu-block@nongnu.org S: Supported F: block/vhdx* -T: git git://github.com/codyprime/qemu-kvm-jtc.git block VDI M: Stefan Weil @@ -2040,34 +2035,26 @@ F: docs/interop/nbd.txt T: git git://repo.or.cz/qemu/ericb.git nbd NFS -M: Jeff Cody M: Peter Lieven L: qemu-block@nongnu.org S: Maintained F: block/nfs.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block SSH M: Richard W.M. Jones -M: Jeff Cody L: qemu-block@nongnu.org S: Supported F: block/ssh.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block CURL -M: Jeff Cody L: qemu-block@nongnu.org S: Supported F: block/curl.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block GLUSTER -M: Jeff Cody L: qemu-block@nongnu.org S: Supported F: block/gluster.c -T: git git://github.com/codyprime/qemu-kvm-jtc.git block Null Block Driver M: Fam Zheng -- 2.17.1
Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] MAINTAINERS: Remove myself as block maintainer
On Tue, Sep 25, 2018 at 03:49:36PM +0800, Fam Zheng wrote: > On Tue, 09/25 09:37, Markus Armbruster wrote: > > Do we want to have a dedicated VHDX driver submaintainer again? Fam, > > you're maintaining VMDK, could you cover VHDX as well? > > I don't know a lot VHDX internals. Considering my capacity at the moment I'd > rather not take this one. > > Fam Anyone can feel free to email me at codypr...@gmail.com with any VHDX questions (I am subbed to the qemu mailing list there as well). I'll at least do my best to answer :)
[Qemu-block] [PATCH v12 8/9] qcow2: Set the default cache-clean-interval to 10 minutes
The default cache-clean-interval is set to 10 minutes, in order to lower
the overhead of the qcow2 caches (before the default was 0, i.e.
disabled).
* For non-Linux platforms the default is kept at 0, because
cache-clean-interval is not supported there yet.
Signed-off-by: Leonid Bloch
Reviewed-by: Alberto Garcia
Reviewed-by: Kevin Wolf
---
block/qcow2.c| 2 +-
block/qcow2.h| 4 +++-
docs/qcow2-cache.txt | 4 ++--
qapi/block-core.json | 3 ++-
qemu-options.hx | 2 +-
5 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 20b5093269..95e1c98daa 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -944,7 +944,7 @@ static int qcow2_update_options_prepare(BlockDriverState
*bs,
/* New interval for cache cleanup timer */
r->cache_clean_interval =
qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
-s->cache_clean_interval);
+DEFAULT_CACHE_CLEAN_INTERVAL);
#ifndef CONFIG_LINUX
if (r->cache_clean_interval != 0) {
error_setg(errp, QCOW2_OPT_CACHE_CLEAN_INTERVAL
diff --git a/block/qcow2.h b/block/qcow2.h
index 0f0e3534bf..ba430316b9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -76,13 +76,15 @@
#ifdef CONFIG_LINUX
#define DEFAULT_L2_CACHE_MAX_SIZE S_32MiB
+#define DEFAULT_CACHE_CLEAN_INTERVAL 600 /* seconds */
#else
#define DEFAULT_L2_CACHE_MAX_SIZE S_8MiB
+/* Cache clean interval is currently available only on Linux, so must be 0 */
+#define DEFAULT_CACHE_CLEAN_INTERVAL 0
#endif
#define DEFAULT_CLUSTER_SIZE S_64KiB
-
#define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
#define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
#define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot"
diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 1fcc0658b2..59358b816f 100644
--- a/docs/qcow2-cache.txt
+++ b/docs/qcow2-cache.txt
@@ -210,8 +210,8 @@ This example removes all unused cache entries every 15
minutes:
-drive file=hd.qcow2,cache-clean-interval=900
-If unset, the default value for this parameter is 0 and it disables
-this feature.
+If unset, the default value for this parameter is 600. Setting it to 0
+disables this feature.
Note that this functionality currently relies on the MADV_DONTNEED
argument for madvise() to actually free the memory. This is a
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ac3b48ee54..46dac23d2f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2895,7 +2895,8 @@
#
# @cache-clean-interval: clean unused entries in the L2 and refcount
# caches. The interval is in seconds. The default value
-# is 0 and it disables this feature (since 2.5)
+# is 600, and 0 disables this feature. (since 2.5)
+#
# @encrypt: Image decryption options. Mandatory for
# encrypted images, except when doing a metadata-only
# probe of the image. (since 2.10)
diff --git a/qemu-options.hx b/qemu-options.hx
index 14aee78c6c..52d9d9f06d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -747,7 +747,7 @@ it which is not used for the L2 cache)
@item cache-clean-interval
Clean unused entries in the L2 and refcount caches. The interval is in seconds.
-The default value is 0 and it disables this feature.
+The default value is 600. Setting it to 0 disables this feature.
@item pass-discard-request
Whether discard requests to the qcow2 device should be forwarded to the data
--
2.17.1
[Qemu-block] [PATCH v12 4/9] qcow2: Avoid duplication in setting the refcount cache size
The refcount cache size does not need to be set to its minimum value in
read_cache_sizes(), as it is set to at least its minimum value in
qcow2_update_options_prepare().
Signed-off-by: Leonid Bloch
Reviewed-by: Alberto Garcia
Reviewed-by: Kevin Wolf
---
block/qcow2.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index d2c07ce9fe..cd0053b6ee 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -834,10 +834,9 @@ static void read_cache_sizes(BlockDriverState *bs,
QemuOpts *opts,
(uint64_t)DEFAULT_L2_CACHE_CLUSTERS
* s->cluster_size);
}
-if (!refcount_cache_size_set) {
-*refcount_cache_size = min_refcount_cache;
-}
}
+/* l2_cache_size and refcount_cache_size are ensured to have at least
+ * their minimum values in qcow2_update_options_prepare() */
if (*l2_cache_entry_size < (1 << MIN_CLUSTER_BITS) ||
*l2_cache_entry_size > s->cluster_size ||
--
2.17.1
[Qemu-block] [PATCH v12 7/9] qcow2: Resize the cache upon image resizing
The caches are now recalculated upon image resizing. This is done
because the new default behavior of assigning L2 cache relatively to
the image size, implies that the cache will be adapted accordingly
after an image resize.
Signed-off-by: Leonid Bloch
Reviewed-by: Alberto Garcia
---
block/qcow2.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index 589f6c1b1c..20b5093269 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3418,6 +3418,7 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
uint64_t old_length;
int64_t new_l1_size;
int ret;
+QDict *options;
if (prealloc != PREALLOC_MODE_OFF && prealloc != PREALLOC_MODE_METADATA &&
prealloc != PREALLOC_MODE_FALLOC && prealloc != PREALLOC_MODE_FULL)
@@ -3642,6 +3643,8 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
}
}
+bs->total_sectors = offset / BDRV_SECTOR_SIZE;
+
/* write updated header.size */
offset = cpu_to_be64(offset);
ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
@@ -3652,6 +3655,14 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
}
s->l1_vm_state_index = new_l1_size;
+
+/* Update cache sizes */
+options = qdict_clone_shallow(bs->options);
+ret = qcow2_update_options(bs, options, s->flags, errp);
+qobject_unref(options);
+if (ret < 0) {
+goto fail;
+}
ret = 0;
fail:
qemu_co_mutex_unlock(&s->lock);
--
2.17.1
[Qemu-block] [PATCH v12 6/9] qcow2: Increase the default upper limit on the L2 cache size
The upper limit on the L2 cache size is increased from 1 MB to 32 MB on Linux platforms, and to 8 MB on other platforms (this difference is caused by the ability to set intervals for cache cleaning on Linux platforms only). This is done in order to allow default full coverage with the L2 cache for images of up to 256 GB in size (was 8 GB). Note, that only the needed amount to cover the full image is allocated. The value which is changed here is just the upper limit on the L2 cache size, beyond which it will not grow, even if the size of the image will require it to. Signed-off-by: Leonid Bloch Reviewed-by: Alberto Garcia Reviewed-by: Kevin Wolf --- block/qcow2.h| 6 +- docs/qcow2-cache.txt | 15 +-- qemu-options.hx | 6 +++--- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index 2f8c1fd15c..0f0e3534bf 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -74,7 +74,11 @@ /* Must be at least 4 to cover all cases of refcount table growth */ #define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */ -#define DEFAULT_L2_CACHE_MAX_SIZE S_1MiB +#ifdef CONFIG_LINUX +#define DEFAULT_L2_CACHE_MAX_SIZE S_32MiB +#else +#define DEFAULT_L2_CACHE_MAX_SIZE S_8MiB +#endif #define DEFAULT_CLUSTER_SIZE S_64KiB diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt index 750447ea4f..1fcc0658b2 100644 --- a/docs/qcow2-cache.txt +++ b/docs/qcow2-cache.txt @@ -125,12 +125,15 @@ There are a few things that need to be taken into account: - Both caches must have a size that is a multiple of the cluster size (or the cache entry size: see "Using smaller cache sizes" below). - - The maximum L2 cache size is 1 MB by default (enough for full coverage - of 8 GB images, with the default cluster size). This value can be - modified using the "l2-cache-size" option. QEMU will not use more memory - than needed to hold all of the image's L2 tables, regardless of this max. - value. The minimal L2 cache size is 2 clusters (or 2 cache entries, see - below). + - The maximum L2 cache size is 32 MB by default on Linux platforms (enough + for full coverage of 256 GB images, with the default cluster size). This + value can be modified using the "l2-cache-size" option. QEMU will not use + more memory than needed to hold all of the image's L2 tables, regardless + of this max. value. + On non-Linux platforms the maximal value is smaller by default (8 MB) and + this difference stems from the fact that on Linux the cache can be cleared + periodically if needed, using the "cache-clean-interval" option (see below). + The minimal L2 cache size is 2 clusters (or 2 cache entries, see below). - The default (and minimum) refcount cache size is 4 clusters. diff --git a/qemu-options.hx b/qemu-options.hx index 6eef0f5651..14aee78c6c 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -736,9 +736,9 @@ The maximum total size of the L2 table and refcount block caches in bytes @item l2-cache-size The maximum size of the L2 table cache in bytes -(default: if cache-size is not specified - 1M; otherwise, as large as possible -within the cache-size, while permitting the requested or the minimal refcount -cache size) +(default: if cache-size is not specified - 32M on Linux platforms, and 8M on +non-Linux platforms; otherwise, as large as possible within the cache-size, +while permitting the requested or the minimal refcount cache size) @item refcount-cache-size The maximum size of the refcount block cache in bytes -- 2.17.1
[Qemu-block] [PATCH v12 9/9] qcow2: Explicit number replaced by a constant
Signed-off-by: Leonid Bloch
Reviewed-by: Alberto Garcia
Reviewed-by: Kevin Wolf
---
block/qcow2.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 95e1c98daa..7277feda13 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1324,7 +1324,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
/* 2^(s->refcount_order - 3) is the refcount width in bytes */
s->refcount_block_bits = s->cluster_bits - (s->refcount_order - 3);
s->refcount_block_size = 1 << s->refcount_block_bits;
-bs->total_sectors = header.size / 512;
+bs->total_sectors = header.size / BDRV_SECTOR_SIZE;
s->csize_shift = (62 - (s->cluster_bits - 8));
s->csize_mask = (1 << (s->cluster_bits - 8)) - 1;
s->cluster_offset_mask = (1LL << s->csize_shift) - 1;
@@ -3450,7 +3450,7 @@ static int coroutine_fn
qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
goto fail;
}
-old_length = bs->total_sectors * 512;
+old_length = bs->total_sectors * BDRV_SECTOR_SIZE;
new_l1_size = size_to_l1(s, offset);
if (offset < old_length) {
--
2.17.1
[Qemu-block] [PATCH v12 5/9] qcow2: Assign the L2 cache relatively to the image size
Sufficient L2 cache can noticeably improve the performance when using
large images with frequent I/O.
Previously, unless 'cache-size' was specified and was large enough, the
L2 cache was set to a certain size without taking the virtual image size
into account.
Now, the L2 cache assignment is aware of the virtual size of the image,
and will cover the entire image, unless the cache size needed for that is
larger than a certain maximum. This maximum is set to 1 MB by default
(enough to cover an 8 GB image with the default cluster size) but can
be increased or decreased using the 'l2-cache-size' option. This option
was previously documented as the *maximum* L2 cache size, and this patch
makes it behave as such, instead of as a constant size. Also, the
existing option 'cache-size' can limit the sum of both L2 and refcount
caches, as previously.
Signed-off-by: Leonid Bloch
Reviewed-by: Alberto Garcia
Reviewed-by: Kevin Wolf
---
block/qcow2.c | 21 +
block/qcow2.h | 4 +---
docs/qcow2-cache.txt | 15 ++-
qemu-options.hx| 6 +++---
tests/qemu-iotests/137 | 8 +++-
tests/qemu-iotests/137.out | 4 +++-
6 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index cd0053b6ee..589f6c1b1c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -777,29 +777,35 @@ static void read_cache_sizes(BlockDriverState *bs,
QemuOpts *opts,
uint64_t *refcount_cache_size, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
-uint64_t combined_cache_size;
+uint64_t combined_cache_size, l2_cache_max_setting;
bool l2_cache_size_set, refcount_cache_size_set, combined_cache_size_set;
int min_refcount_cache = MIN_REFCOUNT_CACHE_SIZE * s->cluster_size;
+uint64_t virtual_disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
+uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8);
combined_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_CACHE_SIZE);
l2_cache_size_set = qemu_opt_get(opts, QCOW2_OPT_L2_CACHE_SIZE);
refcount_cache_size_set = qemu_opt_get(opts,
QCOW2_OPT_REFCOUNT_CACHE_SIZE);
combined_cache_size = qemu_opt_get_size(opts, QCOW2_OPT_CACHE_SIZE, 0);
-*l2_cache_size = qemu_opt_get_size(opts, QCOW2_OPT_L2_CACHE_SIZE, 0);
+l2_cache_max_setting = qemu_opt_get_size(opts, QCOW2_OPT_L2_CACHE_SIZE,
+ DEFAULT_L2_CACHE_MAX_SIZE);
*refcount_cache_size = qemu_opt_get_size(opts,
QCOW2_OPT_REFCOUNT_CACHE_SIZE, 0);
*l2_cache_entry_size = qemu_opt_get_size(
opts, QCOW2_OPT_L2_CACHE_ENTRY_SIZE, s->cluster_size);
+*l2_cache_size = MIN(max_l2_cache, l2_cache_max_setting);
+
if (combined_cache_size_set) {
if (l2_cache_size_set && refcount_cache_size_set) {
error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE
" and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set "
"at the same time");
return;
-} else if (*l2_cache_size > combined_cache_size) {
+} else if (l2_cache_size_set &&
+ (l2_cache_max_setting > combined_cache_size)) {
error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
QCOW2_OPT_CACHE_SIZE);
return;
@@ -814,9 +820,6 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts
*opts,
} else if (refcount_cache_size_set) {
*l2_cache_size = combined_cache_size - *refcount_cache_size;
} else {
-uint64_t virtual_disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
-uint64_t max_l2_cache = virtual_disk_size / (s->cluster_size / 8);
-
/* Assign as much memory as possible to the L2 cache, and
* use the remainder for the refcount cache */
if (combined_cache_size >= max_l2_cache + min_refcount_cache) {
@@ -828,12 +831,6 @@ static void read_cache_sizes(BlockDriverState *bs,
QemuOpts *opts,
*l2_cache_size = combined_cache_size - *refcount_cache_size;
}
}
-} else {
-if (!l2_cache_size_set) {
-*l2_cache_size = MAX(DEFAULT_L2_CACHE_SIZE,
- (uint64_t)DEFAULT_L2_CACHE_CLUSTERS
- * s->cluster_size);
-}
}
/* l2_cache_size and refcount_cache_size are ensured to have at least
* their minimum values in qcow2_update_options_prepare() */
diff --git a/block/qcow2.h b/block/qcow2.h
index a8d6f757b1..2f8c1fd15c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -74,9 +74,7 @@
/* Must be at least 4 to cover all cases of refcount table growth */
#define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */
-/* Whichever is more */
-#define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */
-#define DEFAULT_L2_CACHE_SIZE S_1M
[Qemu-block] [PATCH v12 3/9] qcow2: Make sizes more humanly readable
Signed-off-by: Leonid Bloch
Reviewed-by: Alberto Garcia
Reviewed-by: Kevin Wolf
---
block/qcow2.c | 2 +-
block/qcow2.h | 9 +
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c13153735a..d2c07ce9fe 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -830,7 +830,7 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts
*opts,
}
} else {
if (!l2_cache_size_set) {
-*l2_cache_size = MAX(DEFAULT_L2_CACHE_BYTE_SIZE,
+*l2_cache_size = MAX(DEFAULT_L2_CACHE_SIZE,
(uint64_t)DEFAULT_L2_CACHE_CLUSTERS
* s->cluster_size);
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 81b844e936..a8d6f757b1 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -27,6 +27,7 @@
#include "crypto/block.h"
#include "qemu/coroutine.h"
+#include "qemu/units.h"
//#define DEBUG_ALLOC
//#define DEBUG_ALLOC2
@@ -43,11 +44,11 @@
/* 8 MB refcount table is enough for 2 PB images at 64k cluster size
* (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
-#define QCOW_MAX_REFTABLE_SIZE 0x80
+#define QCOW_MAX_REFTABLE_SIZE S_8MiB
/* 32 MB L1 table is enough for 2 PB images at 64k cluster size
* (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */
-#define QCOW_MAX_L1_SIZE 0x200
+#define QCOW_MAX_L1_SIZE S_32MiB
/* Allow for an average of 1k per snapshot table entry, should be plenty of
* space for snapshot names and IDs */
@@ -75,9 +76,9 @@
/* Whichever is more */
#define DEFAULT_L2_CACHE_CLUSTERS 8 /* clusters */
-#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */
+#define DEFAULT_L2_CACHE_SIZE S_1MiB
-#define DEFAULT_CLUSTER_SIZE 65536
+#define DEFAULT_CLUSTER_SIZE S_64KiB
#define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
--
2.17.1
[Qemu-block] [PATCH v12 1/9] qcow2: Options' documentation fixes
Signed-off-by: Leonid Bloch Reviewed-by: Alberto Garcia Reviewed-by: Kevin Wolf --- docs/qcow2-cache.txt | 21 ++--- qemu-options.hx | 9 ++--- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt index 8a09a5cc5f..7e28b41bd3 100644 --- a/docs/qcow2-cache.txt +++ b/docs/qcow2-cache.txt @@ -79,14 +79,14 @@ Choosing the right cache sizes In order to choose the cache sizes we need to know how they relate to the amount of allocated space. -The amount of virtual disk that can be mapped by the L2 and refcount +The part of the virtual disk that can be mapped by the L2 and refcount caches (in bytes) is: disk_size = l2_cache_size * cluster_size / 8 disk_size = refcount_cache_size * cluster_size * 8 / refcount_bits With the default values for cluster_size (64KB) and refcount_bits -(16), that is +(16), this becomes: disk_size = l2_cache_size * 8192 disk_size = refcount_cache_size * 32768 @@ -97,12 +97,16 @@ need: l2_cache_size = disk_size_GB * 131072 refcount_cache_size = disk_size_GB * 32768 -QEMU has a default L2 cache of 1MB (1048576 bytes) and a refcount -cache of 256KB (262144 bytes), so using the formulas we've just seen -we have +For example, 1MB of L2 cache is needed to cover every 8 GB of the virtual +image size (given that the default cluster size is used): - 1048576 / 131072 = 8 GB of virtual disk covered by that cache -262144 / 32768 = 8 GB + 8 GB / 8192 = 1 MB + +The refcount cache is 4 times the cluster size by default. With the default +cluster size of 64 KB, it is 256 KB (262144 bytes). This is sufficient for +8 GB of image size: + + 262144 * 32768 = 8 GB How to configure the cache sizes @@ -130,6 +134,9 @@ There are a few things that need to be taken into account: memory as possible to the L2 cache before increasing the refcount cache size. + - At most two of "l2-cache-size", "refcount-cache-size", and "cache-size" + can be set simultaneously. + Unlike L2 tables, refcount blocks are not used during normal I/O but only during allocations and internal snapshots. In most cases they are accessed sequentially (even during random guest I/O) so increasing the diff --git a/qemu-options.hx b/qemu-options.hx index a642ad297f..2db6247eff 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -732,15 +732,18 @@ image file) @item cache-size The maximum total size of the L2 table and refcount block caches in bytes -(default: 1048576 bytes or 8 clusters, whichever is larger) +(default: the sum of l2-cache-size and refcount-cache-size) @item l2-cache-size The maximum size of the L2 table cache in bytes -(default: 4/5 of the total cache size) +(default: if cache-size is not defined - 1048576 bytes or 8 clusters, whichever +is larger; otherwise, as large as possible or needed within the cache-size, +while permitting the requested or the minimal refcount cache size) @item refcount-cache-size The maximum size of the refcount block cache in bytes -(default: 1/5 of the total cache size) +(default: 4 times the cluster size; or if cache-size is specified, the part of +it which is not used for the L2 cache) @item cache-clean-interval Clean unused entries in the L2 and refcount caches. The interval is in seconds. -- 2.17.1
[Qemu-block] [PATCH v12 2/9] include: Add a lookup table of sizes
Adding a lookup table for the powers of two, with the appropriate size
prefixes. This is needed when a size has to be stringified, in which
case something like '(1 * KiB)' would become a literal '(1 * (1L << 10))'
string. Powers of two are used very often for sizes, so such a table
will also make it easier and more intuitive to write them.
This table is generatred using the following AWK script:
BEGIN {
suffix="KMGTPE";
for(i=10; i<64; i++) {
val=2**i;
s=substr(suffix, int(i/10), 1);
n=2**(i%10);
pad=21-int(log(n)/log(10));
printf("#define S_%d%siB %*d\n", n, s, pad, val);
}
}
Signed-off-by: Leonid Bloch
Reviewed-by: Alberto Garcia
Reviewed-by: Kevin Wolf
---
include/qemu/units.h | 55
1 file changed, 55 insertions(+)
diff --git a/include/qemu/units.h b/include/qemu/units.h
index 692db3fbb2..68a7758650 100644
--- a/include/qemu/units.h
+++ b/include/qemu/units.h
@@ -17,4 +17,59 @@
#define PiB (INT64_C(1) << 50)
#define EiB (INT64_C(1) << 60)
+#define S_1KiB 1024
+#define S_2KiB 2048
+#define S_4KiB 4096
+#define S_8KiB 8192
+#define S_16KiB16384
+#define S_32KiB32768
+#define S_64KiB65536
+#define S_128KiB 131072
+#define S_256KiB 262144
+#define S_512KiB 524288
+#define S_1MiB 1048576
+#define S_2MiB 2097152
+#define S_4MiB 4194304
+#define S_8MiB 8388608
+#define S_16MiB 16777216
+#define S_32MiB 33554432
+#define S_64MiB 67108864
+#define S_128MiB 134217728
+#define S_256MiB 268435456
+#define S_512MiB 536870912
+#define S_1GiB1073741824
+#define S_2GiB2147483648
+#define S_4GiB4294967296
+#define S_8GiB8589934592
+#define S_16GiB 17179869184
+#define S_32GiB 34359738368
+#define S_64GiB 68719476736
+#define S_128GiB137438953472
+#define S_256GiB274877906944
+#define S_512GiB549755813888
+#define S_1TiB 1099511627776
+#define S_2TiB 219902322
+#define S_4TiB 4398046511104
+#define S_8TiB 8796093022208
+#define S_16TiB 17592186044416
+#define S_32TiB 35184372088832
+#define S_64TiB 70368744177664
+#define S_128TiB 140737488355328
+#define S_256TiB 281474976710656
+#define S_512TiB 562949953421312
+#define S_1PiB 1125899906842624
+#define S_2PiB 2251799813685248
+#define S_4PiB 4503599627370496
+#define S_8PiB 9007199254740992
+#define S_16PiB18014398509481984
+#define S_32PiB36028797018963968
+#define S_64PiB72057594037927936
+#define S_128PiB 144115188075855872
+#define S_256PiB 288230376151711744
+#define S_512PiB 576460752303423488
+#define S_1EiB 1152921504606846976
+#define S_2EiB 2305843009213693952
+#define S_4EiB 4611686018427387904
+#define S_8EiB 9223372036854775808
+
#endif
--
2.17.1
[Qemu-block] [PATCH v12 0/9] Take the image size into account when allocating the L2 cache
This series makes the qcow2 L2 cache assignment aware of the image size, with the intention for it to cover the entire image. The importance of this change is in noticeable performance improvement, especially with heavy random I/O. The memory overhead is not big in most cases, as only 1 MB of cache for every 8 GB of image size is used. For cases with very large images and/or small cluster sizes, or systems with limited RAM resources, there is an upper limit on the default L2 cache: 32 MB for Linux systems, and 8 MB for non-Linux systems (the difference is caused by the fact that it is currently impossible to perform scheduled cache cleaning on non-Linux systems). To modify this upper limit one can use the already existing 'l2-cache-size' and 'cache-size' options. Moreover, this fixes the behavior of 'l2-cache-size', as it was documented as the *maximum* L2 cache size, but in practice behaved as the absolute size. To compensate the memory overhead which may (but not necesarily will) be increased following this behavior, the default cache-clean-interval is set to 10 minutes by default (was disabled by default before). The L2 cache is also resized accordingly, by default, if the image is resized. Additionally, few minor changes are made (refactoring and documentation fixes). Differences from v1: * .gitignore modification patch removed (unneeded). * The grammar fix in conflicting cache sizing patch removed (merged). * The update total_sectors when resizing patch squashed with the resizing patch. * L2 cache is now capped at 32 MB. * The default cache-clean-interval is set to 30 seconds. Differences from v2: * Made it clear in the documentation that setting cache-clean-interval to 0 disables this feature. Differences from v3: * The default cache-clean-interval is set to 10 minutes instead of 30 seconds before. * Commit message changes to better explain the patches. * Some refactoring. Differences from v4: * Refactoring. * Documentation and commit message fixes. Differences from v5: * A patch with cosmetic changes is split from the main patch * A patch for avoiding duplication in refcount cache size assignment is split from the main patch. * In the main patch the cap on the L2 cache size is set to only 1 MB (in order to be close to the previous behavior) and a separate patch sets it to 32 MB. * Changes in the documentation fixes patch [1/8]. Differences from v6: * A patch is added to make the defined sizes more humanly readable. Differences from v7: * Documentation and commit message changes. * Fix: when cache-size is specified and is large enough, with the L2 and refcount caches not set, the L2 cache will be as large as needed to cover the entire image, and not as the default max. size. (Return to the previous behavior in this part, which was correct). * Cosmetic changes patch not used. Differences from v8: * Made a lookup table for power-of-two sizes. This will enable writing pretty size values (most of the ones used are powers of two) while allowing correct stringification of these values. * Made the max. L2 cache size 8 MB on non-Linux systems. * Returned the test for too large L2 cache size. * Minor documentation fixes. Differences from v9: * DEFAULT_CACHE_CLEAN_INTERVAL is set to 0 for non-Linux platforms in order to avoid an error, since the cache clean interval feature is currently available only on Linux. Differences from v10: * Rewording documentation. Differences from v11: * Memory leak fixed in "Resize the cache upon image resizing". Leonid Bloch (9): qcow2: Options' documentation fixes include: Add a lookup table of sizes qcow2: Make sizes more humanly readable qcow2: Avoid duplication in setting the refcount cache size qcow2: Assign the L2 cache relatively to the image size qcow2: Increase the default upper limit on the L2 cache size qcow2: Resize the cache upon image resizing qcow2: Set the default cache-clean-interval to 10 minutes qcow2: Explicit number replaced by a constant block/qcow2.c | 43 - block/qcow2.h | 19 - docs/qcow2-cache.txt | 43 +++-- include/qemu/units.h | 55 ++ qapi/block-core.json | 3 ++- qemu-options.hx| 11 +--- tests/qemu-iotests/137 | 8 +- tests/qemu-iotests/137.out | 4 ++- 8 files changed, 140 insertions(+), 46 deletions(-) -- 2.17.1
Re: [Qemu-block] [PATCH v11 7/9] qcow2: Resize the cache upon image resizing
On Wed 26 Sep 2018 04:21:21 PM CEST, Leonid Bloch wrote:
> Oops, sorry. Would something like that be OK:
>
> On 9/26/18 1:55 PM, Alberto Garcia wrote:
>> On Wed 26 Sep 2018 12:43:03 PM CEST, Kevin Wolf wrote:
+/* Update cache sizes */
+options = qdict_clone_shallow(bs->options);
+ret = qcow2_update_options(bs, options, s->flags, errp);
> +qobject_unref(options);
+if (ret < 0) {
+goto fail;
+}
>>>
Yes, I think that should be enough.
Berto
Re: [Qemu-block] [PATCH v11 7/9] qcow2: Resize the cache upon image resizing
Oops, sorry. Would something like that be OK:
On 9/26/18 1:55 PM, Alberto Garcia wrote:
> On Wed 26 Sep 2018 12:43:03 PM CEST, Kevin Wolf wrote:
>>> +/* Update cache sizes */
>>> +options = qdict_clone_shallow(bs->options);
>>> +ret = qcow2_update_options(bs, options, s->flags, errp);
+qobject_unref(options);
>>> +if (ret < 0) {
>>> +goto fail;
>>> +}
>>
Leonid.
>> Isn't options leaked, both in success and error cases?
>
> I'm embarrassed not to have seen that :-!
>
> Berto
>
Re: [Qemu-block] [PATCH v3 0/5] dirty-bitmaps: fix QMP command permissions
26.09.2018 02:49, John Snow wrote: based on: jsnow/bitmaps staging branch This series builds on a previous standalone patch and adjusts the permission for all (or most) of the QMP bitmap commands. John Snow (5): block/dirty-bitmaps: add user_modifiable status checker block/dirty-bitmaps: fix merge permissions block/dirty-bitmaps: allow clear on disabled bitmaps block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps block/backup: prohibit backup from using in-use bitmaps block/dirty-bitmap.c | 13 +--- blockdev.c | 75 include/block/dirty-bitmap.h | 1 + 3 files changed, 44 insertions(+), 45 deletions(-) Great! Thank you for clearing this. I contributed a lot to this mess with my qmp-locked( PS: I have a draft patch in my current developments to allow set/reset bits in disabled bitmaps, which is needed to use BdrvDirtyBitmap as a shared named copy-bitmap between fleecing-hook filter and backup job. So "disabled" is actually only for use in bdrv_set_dirty(), to disable automatic bitmap updates on guest writes. -- Best regards, Vladimir
Re: [Qemu-block] [PATCH v3 1/5] block/dirty-bitmaps: add user_modifiable status checker
26.09.2018 14:53, Vladimir Sementsov-Ogievskiy wrote:
26.09.2018 02:49, John Snow wrote:
Instead of both frozen and qmp_locked checks, wrap it into one check.
frozen implies the bitmap is split in two (for backup), and shouldn't
be modified. qmp_locked implies it's being used by another operation,
like being exported over NBD. In both cases it means we shouldn't allow
the user to modify it in any meaningful way.
Replace any usages where we check both frozen and qmp_locked with the
new check.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 6 ++
blockdev.c | 29 -
include/block/dirty-bitmap.h | 1 +
3 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8ac933cf1c..fc10543ab0 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap
*bitmap)
return bitmap->successor;
}
+/* Both conditions disallow user-modification via QMP. */
+bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap) {
+ return !(bdrv_dirty_bitmap_frozen(bitmap) ||
+ bdrv_dirty_bitmap_qmp_locked(bitmap));
+}
to reduce number of '!', we may use opposite check, for ex
"bdrv_dirty_bitmap_user_locked".
bad reason to rewrite the whole series, so, ignore this comment)
anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy
+
void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool
qmp_locked)
{
qemu_mutex_lock(bitmap->mutex);
diff --git a/blockdev.c b/blockdev.c
index 670ae5bbde..dedcebb0fa 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2009,11 +2009,8 @@ static void
block_dirty_bitmap_clear_prepare(BlkActionState *common,
return;
}
- if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
- error_setg(errp, "Cannot modify a frozen bitmap");
- return;
- } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
- error_setg(errp, "Cannot modify a locked bitmap");
+ if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
+ error_setg(errp, "Cannot modify a bitmap in-use by another
operation");
return;
} else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
error_setg(errp, "Cannot clear a disabled bitmap");
@@ -2882,15 +2879,10 @@ void qmp_block_dirty_bitmap_remove(const char
*node, const char *name,
return;
}
- if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be
removed",
- name);
- return;
- } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
- error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be
removed",
- name);
+ "Bitmap '%s' is currently in-use by another
operation and"
+ " cannot be removed", name);
return;
}
@@ -2920,15 +2912,10 @@ void qmp_block_dirty_bitmap_clear(const
char *node, const char *name,
return;
}
- if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be
modified",
- name);
- return;
- } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
- error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be
modified",
- name);
+ "Bitmap '%s' is currently in-use by another
operation"
+ " and cannot be cleared", name);
return;
} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
error_setg(errp,
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 201ff7f20b..92cf7e39d2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -94,6 +94,7 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap);
bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap);
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v3 2/5] block/dirty-bitmaps: fix merge permissions
26.09.2018 14:55, Vladimir Sementsov-Ogievskiy wrote:
26.09.2018 02:49, John Snow wrote:
We wish to prohibit merging to read-only bitmaps and frozen bitmaps,
but "disabled" bitmaps only preclude their recording of live, new
information. It does not prohibit them from manual writes at the behest
of the user, as is the case for merge operations.
Allow the merge to "disabled" bitmaps,
and prohibit merging to "locked" ones.
only the second part is here..
Hm, the first one is in first separate patch? With commit message fixed
to only second part, of course:
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reported-by: Eric Blake
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index fc10543ab0..53b7d282c4 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -806,9 +806,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap
*dest, const BdrvDirtyBitmap *src,
qemu_mutex_lock(dest->mutex);
- if (bdrv_dirty_bitmap_frozen(dest)) {
- error_setg(errp, "Bitmap '%s' is frozen and cannot be
modified",
- dest->name);
+ if (!bdrv_dirty_bitmap_user_modifiable(dest)) {
+ error_setg(errp, "Bitmap '%s' is currently in-use by another"
+ " operation and cannot be modified", dest->name);
goto out;
}
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v3 5/5] block/backup: prohibit backup from using in-use bitmaps
26.09.2018 02:49, John Snow wrote:
If the bitmap is locked, we shouldn't touch it.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
blockdev.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 751e153557..c998336a43 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3512,10 +3512,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup,
JobTxn *txn,
bdrv_unref(target_bs);
goto out;
}
-if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
+if (!bdrv_dirty_bitmap_user_modifiable(bmap)) {
error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be used for
"
- "backup", backup->bitmap);
+ "Bitmap '%s' is currently in-use by another operation"
+ " and cannot be used for backup", backup->bitmap);
goto out;
}
}
@@ -3620,10 +3620,10 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup,
JobTxn *txn,
error_setg(errp, "Bitmap '%s' could not be found",
backup->bitmap);
goto out;
}
-if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
+if (!bdrv_dirty_bitmap_user_modifiable(bmap)) {
error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be used for
"
- "backup", backup->bitmap);
+ "Bitmap '%s' is currently in-use by another operation"
+ " and cannot be used for backup", backup->bitmap);
goto out;
}
}
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v3 4/5] block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps
26.09.2018 02:49, John Snow wrote:
We're not being consistent about this. If it's in use by an operation,
the user should not be able to change the behavior of that bitmap.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
blockdev.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index e178aae178..751e153557 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2058,6 +2058,13 @@ static void
block_dirty_bitmap_enable_prepare(BlkActionState *common,
return;
}
+if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
+error_setg(errp,
+ "Bitmap '%s' is currently in-use by another operation"
+ " and cannot be enabled", action->name);
+return;
+}
+
state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
bdrv_enable_dirty_bitmap(state->bitmap);
}
@@ -2092,6 +2099,13 @@ static void
block_dirty_bitmap_disable_prepare(BlkActionState *common,
return;
}
+if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
+error_setg(errp,
+ "Bitmap '%s' is currently in-use by another operation"
+ " and cannot be disabled", action->name);
+return;
+}
+
state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
bdrv_disable_dirty_bitmap(state->bitmap);
}
@@ -2933,10 +2947,10 @@ void qmp_x_block_dirty_bitmap_enable(const char *node,
const char *name,
return;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be enabled",
- name);
+ "Bitmap '%s' is currently in-use by another operation"
+ " and cannot be enabled", name);
return;
}
@@ -2954,10 +2968,10 @@ void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,
return;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be disabled",
- name);
+ "Bitmap '%s' is currently in-use by another operation"
+ " and cannot be disabled", name);
return;
}
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v3 3/5] block/dirty-bitmaps: allow clear on disabled bitmaps
26.09.2018 02:49, John Snow wrote:
Similarly to merge, it's OK to allow clear operations on disabled
bitmaps, as this condition only means that they are not recording
new writes. We are free to clear it if the user requests it.
Signed-off-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/dirty-bitmap.c | 1 -
blockdev.c | 8
2 files changed, 9 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 53b7d282c4..ff4a86cea6 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -625,7 +625,6 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
{
-assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
bdrv_dirty_bitmap_lock(bitmap);
if (!out) {
diff --git a/blockdev.c b/blockdev.c
index dedcebb0fa..e178aae178 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2012,9 +2012,6 @@ static void
block_dirty_bitmap_clear_prepare(BlkActionState *common,
if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
error_setg(errp, "Cannot modify a bitmap in-use by another
operation");
return;
-} else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
-error_setg(errp, "Cannot clear a disabled bitmap");
-return;
} else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
error_setg(errp, "Cannot clear a readonly bitmap");
return;
@@ -2917,11 +2914,6 @@ void qmp_block_dirty_bitmap_clear(const char *node,
const char *name,
"Bitmap '%s' is currently in-use by another operation"
" and cannot be cleared", name);
return;
-} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
-error_setg(errp,
- "Bitmap '%s' is currently disabled and cannot be cleared",
- name);
-return;
} else if (bdrv_dirty_bitmap_readonly(bitmap)) {
error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared",
name);
return;
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v3 2/5] block/dirty-bitmaps: fix merge permissions
26.09.2018 02:49, John Snow wrote:
We wish to prohibit merging to read-only bitmaps and frozen bitmaps,
but "disabled" bitmaps only preclude their recording of live, new
information. It does not prohibit them from manual writes at the behest
of the user, as is the case for merge operations.
Allow the merge to "disabled" bitmaps,
and prohibit merging to "locked" ones.
only the second part is here..
Reported-by: Eric Blake
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index fc10543ab0..53b7d282c4 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -806,9 +806,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const
BdrvDirtyBitmap *src,
qemu_mutex_lock(dest->mutex);
-if (bdrv_dirty_bitmap_frozen(dest)) {
-error_setg(errp, "Bitmap '%s' is frozen and cannot be modified",
- dest->name);
+if (!bdrv_dirty_bitmap_user_modifiable(dest)) {
+error_setg(errp, "Bitmap '%s' is currently in-use by another"
+" operation and cannot be modified", dest->name);
goto out;
}
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v3 1/5] block/dirty-bitmaps: add user_modifiable status checker
26.09.2018 02:49, John Snow wrote:
Instead of both frozen and qmp_locked checks, wrap it into one check.
frozen implies the bitmap is split in two (for backup), and shouldn't
be modified. qmp_locked implies it's being used by another operation,
like being exported over NBD. In both cases it means we shouldn't allow
the user to modify it in any meaningful way.
Replace any usages where we check both frozen and qmp_locked with the
new check.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 6 ++
blockdev.c | 29 -
include/block/dirty-bitmap.h | 1 +
3 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8ac933cf1c..fc10543ab0 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
return bitmap->successor;
}
+/* Both conditions disallow user-modification via QMP. */
+bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap) {
+return !(bdrv_dirty_bitmap_frozen(bitmap) ||
+ bdrv_dirty_bitmap_qmp_locked(bitmap));
+}
to reduce number of '!', we may use opposite check, for ex
"bdrv_dirty_bitmap_user_locked".
anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy
+
void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool
qmp_locked)
{
qemu_mutex_lock(bitmap->mutex);
diff --git a/blockdev.c b/blockdev.c
index 670ae5bbde..dedcebb0fa 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2009,11 +2009,8 @@ static void
block_dirty_bitmap_clear_prepare(BlkActionState *common,
return;
}
-if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
-error_setg(errp, "Cannot modify a frozen bitmap");
-return;
-} else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
-error_setg(errp, "Cannot modify a locked bitmap");
+if (!bdrv_dirty_bitmap_user_modifiable(state->bitmap)) {
+error_setg(errp, "Cannot modify a bitmap in-use by another operation");
return;
} else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
error_setg(errp, "Cannot clear a disabled bitmap");
@@ -2882,15 +2879,10 @@ void qmp_block_dirty_bitmap_remove(const char *node,
const char *name,
return;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be removed",
- name);
-return;
-} else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be removed",
- name);
+ "Bitmap '%s' is currently in-use by another operation and"
+ " cannot be removed", name);
return;
}
@@ -2920,15 +2912,10 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
return;
}
-if (bdrv_dirty_bitmap_frozen(bitmap)) {
+if (!bdrv_dirty_bitmap_user_modifiable(bitmap)) {
error_setg(errp,
- "Bitmap '%s' is currently frozen and cannot be modified",
- name);
-return;
-} else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
-error_setg(errp,
- "Bitmap '%s' is currently locked and cannot be modified",
- name);
+ "Bitmap '%s' is currently in-use by another operation"
+ " and cannot be cleared", name);
return;
} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
error_setg(errp,
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 201ff7f20b..92cf7e39d2 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -94,6 +94,7 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_user_modifiable(BdrvDirtyBitmap *bitmap);
bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap);
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v3 10/10] block: Allow changing 'force-share' on reopen
On Wed 26 Sep 2018 01:34:28 PM CEST, Kevin Wolf wrote: >> @@ -3353,6 +3370,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) >> bs->open_flags = reopen_state->flags; >> bs->read_only = !(reopen_state->flags & BDRV_O_RDWR); >> bs->detect_zeroes = reopen_state->detect_zeroes; >> +bs->force_share= reopen_state->force_share; > > Just changing bs->force_share without actually triggering recalculation > of the permissions is kind of pointless, no? As the patch is, you would > have to trigger some graph change for the new setting to take effect. > > The rest of the series looks good to me, so if you like, I could apply > patches 1-9, and then you can either send a v4 of only this one or we'll > just drop it. Apply it without this one then. Thanks! Berto
Re: [Qemu-block] [PATCH v3 10/10] block: Allow changing 'force-share' on reopen
Am 06.09.2018 um 11:37 hat Alberto Garcia geschrieben: > 'force-share' is one of the basic BlockdevOptions available for all > drivers, but it's not handled by bdrv_reopen_prepare() so any attempt > to change it results in a "Cannot change the option" error: > >(qemu) qemu-io virtio0 "reopen -o force-share=on" >Cannot change the option 'force-share' > > Since there's no reason why we shouldn't allow changing it and the > implementation is simple let's just do it. > > It's worth noting that after this patch the above reopen call will > still return an error -although a different one- if the image is not > read-only: > >(qemu) qemu-io virtio0 "reopen -o force-share=on" >force-share=on can only be used with read-only images > > Signed-off-by: Alberto Garcia > Reviewed-by: Max Reitz > @@ -3353,6 +3370,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state) > bs->open_flags = reopen_state->flags; > bs->read_only = !(reopen_state->flags & BDRV_O_RDWR); > bs->detect_zeroes = reopen_state->detect_zeroes; > +bs->force_share= reopen_state->force_share; Just changing bs->force_share without actually triggering recalculation of the permissions is kind of pointless, no? As the patch is, you would have to trigger some graph change for the new setting to take effect. The rest of the series looks good to me, so if you like, I could apply patches 1-9, and then you can either send a v4 of only this one or we'll just drop it. Kevin
Re: [Qemu-block] [PATCH] qapi: drop x- from x-block-latency-histogram-set
20.06.2018 19:07, John Snow wrote:
On 06/20/2018 11:28 AM, Vladimir Sementsov-Ogievskiy wrote:
20.06.2018 18:27, Vladimir Sementsov-Ogievskiy wrote:
Libvirt part is ready, let's drop x- prefix.
libvirt patches will be sent soon I hope.
OK, can you ping this patch with a link to the series when it is posted?
Done:
https://www.redhat.com/archives/libvir-list/2018-September/msg00011.html
Can we merge this patch to Qemu, to continue dialog with Libvirt?
Thank you,
--js
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
qapi/block-core.json | 4 ++--
blockdev.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cc3ede0630..dfaa050651 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -483,7 +483,7 @@
'data': {'boundaries': ['uint64'], 'bins': ['uint64'] } }
##
-# @x-block-latency-histogram-set:
+# @block-latency-histogram-set:
#
# Manage read, write and flush latency histograms for the device.
#
@@ -547,7 +547,7 @@
# "arguments": { "device": "drive0" } }
# <- { "return": {} }
##
-{ 'command': 'x-block-latency-histogram-set',
+{ 'command': 'block-latency-histogram-set',
'data': {'device': 'str',
'*boundaries': ['uint64'],
'*boundaries-read': ['uint64'],
diff --git a/blockdev.c b/blockdev.c
index 58d7570932..6d4ae77041 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4299,7 +4299,7 @@ void qmp_x_blockdev_set_iothread(const char
*node_name, StrOrNull *iothread,
aio_context_release(old_context);
}
-void qmp_x_block_latency_histogram_set(
+void qmp_block_latency_histogram_set(
const char *device,
bool has_boundaries, uint64List *boundaries,
bool has_boundaries_read, uint64List *boundaries_read,
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v11 0/9] Take the image size into account when allocating the L2 cache
On Wed 26 Sep 2018 12:48:18 PM CEST, Kevin Wolf wrote: > By the way, Berto: While reviewing this, I noticed that we don't make > use of the l2-cache-entry-size feature yet by default. I think you > said you wanted to do some more tests to find a good default > (intuitively, I'd expect it around MIN(cluster size, 64k), maybe 128k, > but of course this needs some benchmarking). Obviously, that's for a > separate series, though. Yes, I was thinking about that yesterday after reviewing this series too... I don't know if I'll have a lot of time to test this, but I'll try. MIN(cluster_size, 64k) should be a good default probably. Berto
Re: [Qemu-block] [PATCH v11 7/9] qcow2: Resize the cache upon image resizing
On Wed 26 Sep 2018 12:43:03 PM CEST, Kevin Wolf wrote:
>> +/* Update cache sizes */
>> +options = qdict_clone_shallow(bs->options);
>> +ret = qcow2_update_options(bs, options, s->flags, errp);
>> +if (ret < 0) {
>> +goto fail;
>> +}
>
> Isn't options leaked, both in success and error cases?
I'm embarrassed not to have seen that :-!
Berto
Re: [Qemu-block] [PATCH v11 0/9] Take the image size into account when allocating the L2 cache
Am 25.09.2018 um 00:53 hat Leonid Bloch geschrieben: > This series makes the qcow2 L2 cache assignment aware of the image size, > with the intention for it to cover the entire image. The importance of > this change is in noticeable performance improvement, especially with > heavy random I/O. The memory overhead is not big in most cases, as only > 1 MB of cache for every 8 GB of image size is used. For cases with very > large images and/or small cluster sizes, or systems with limited RAM > resources, there is an upper limit on the default L2 cache: 32 MB for > Linux systems, and 8 MB for non-Linux systems (the difference is caused > by the fact that it is currently impossible to perform scheduled cache > cleaning on non-Linux systems). To modify this upper limit one can use > the already existing 'l2-cache-size' and 'cache-size' options. Moreover, > this fixes the behavior of 'l2-cache-size', as it was documented as the > *maximum* L2 cache size, but in practice behaved as the absolute size. > > To compensate the memory overhead which may (but not necesarily will) be > increased following this behavior, the default cache-clean-interval is set > to 10 minutes by default (was disabled by default before). > > The L2 cache is also resized accordingly, by default, if the image is > resized. > > Additionally, few minor changes are made (refactoring and documentation > fixes). Patches 1-6 and 8-9: Reviewed-by: Kevin Wolf By the way, Berto: While reviewing this, I noticed that we don't make use of the l2-cache-entry-size feature yet by default. I think you said you wanted to do some more tests to find a good default (intuitively, I'd expect it around MIN(cluster size, 64k), maybe 128k, but of course this needs some benchmarking). Obviously, that's for a separate series, though. Kevin
Re: [Qemu-block] [PATCH v11 7/9] qcow2: Resize the cache upon image resizing
Am 25.09.2018 um 00:53 hat Leonid Bloch geschrieben:
> The caches are now recalculated upon image resizing. This is done
> because the new default behavior of assigning L2 cache relatively to
> the image size, implies that the cache will be adapted accordingly
> after an image resize.
>
> Signed-off-by: Leonid Bloch
> Reviewed-by: Alberto Garcia
> ---
> block/qcow2.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 589f6c1b1c..c68f896c66 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3418,6 +3418,7 @@ static int coroutine_fn
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> uint64_t old_length;
> int64_t new_l1_size;
> int ret;
> +QDict *options;
>
> if (prealloc != PREALLOC_MODE_OFF && prealloc != PREALLOC_MODE_METADATA
> &&
> prealloc != PREALLOC_MODE_FALLOC && prealloc != PREALLOC_MODE_FULL)
> @@ -3642,6 +3643,8 @@ static int coroutine_fn
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> }
> }
>
> +bs->total_sectors = offset / BDRV_SECTOR_SIZE;
> +
> /* write updated header.size */
> offset = cpu_to_be64(offset);
> ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
> @@ -3652,6 +3655,13 @@ static int coroutine_fn
> qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> }
>
> s->l1_vm_state_index = new_l1_size;
> +
> +/* Update cache sizes */
> +options = qdict_clone_shallow(bs->options);
> +ret = qcow2_update_options(bs, options, s->flags, errp);
> +if (ret < 0) {
> +goto fail;
> +}
Isn't options leaked, both in success and error cases?
Kevin
Re: [Qemu-block] [PATCH] file-posix: Include filename in locking error message
Am 25.09.2018 um 07:05 hat Fam Zheng geschrieben: > Image locking errors happening at device initialization time doesn't say > which file cannot be locked, for instance, > > -device scsi-disk,drive=drive-1: Failed to get shared "write" lock > Is another process using the image? > > could refer to either the overlay image or its backing image. > > Hoist the error_append_hint to the caller of raw_check_lock_bytes where > file name is known, and include it in the error hint. > > Signed-off-by: Fam Zheng Thanks, applied to the block branch. Kevin
