* [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED @ 2018-04-10 16:05 Alberto Garcia 2018-04-10 16:05 ` [Qemu-devel] [PATCH 1/2] Fix error message " Alberto Garcia ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Alberto Garcia @ 2018-04-10 16:05 UTC (permalink / raw) To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Eric Blake, Kevin Wolf, Max Reitz Hi, while reviewing one previous patch about data corruption and compressed clusters we discussed that the documentation doesn't clarify that L2 entries for compressed clusters are not supposed to have the OFLAG_COPIED bit set. Here's a patch to update the documentation and another one to fix the error message that "qemu-img check" produces. Regards, Berto Alberto Garcia (2): Fix error message about compressed clusters with OFLAG_COPIED specs/qcow2: Clarify that compressed clusters have the COPIED bit reset block/qcow2-refcount.c | 4 ++-- docs/interop/qcow2.txt | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 1/2] Fix error message about compressed clusters with OFLAG_COPIED 2018-04-10 16:05 [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED Alberto Garcia @ 2018-04-10 16:05 ` Alberto Garcia 2018-04-10 16:18 ` Eric Blake 2018-04-10 16:05 ` [Qemu-devel] [PATCH 2/2] specs/qcow2: Clarify that compressed clusters have the COPIED bit reset Alberto Garcia 2018-04-13 14:38 ` [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED Max Reitz 2 siblings, 1 reply; 7+ messages in thread From: Alberto Garcia @ 2018-04-10 16:05 UTC (permalink / raw) To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Eric Blake, Kevin Wolf, Max Reitz Compressed clusters are not supposed to have the COPIED bit set. "qemu-img check" detects that and prints an error message reporting the number of the affected host cluster. This doesn't make much sense because compressed clusters are not aligned to host clusters, so it would be better to report the offset instead. Plus, the calculation is wrong and it uses the raw L2 entry as if it was simply an offset. This patch fixes the error message and reports the offset of the compressed cluster. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2-refcount.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 6b8b63514a..2dc23005b7 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -1577,9 +1577,9 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, case QCOW2_CLUSTER_COMPRESSED: /* Compressed clusters don't have QCOW_OFLAG_COPIED */ if (l2_entry & QCOW_OFLAG_COPIED) { - fprintf(stderr, "ERROR: cluster %" PRId64 ": " + fprintf(stderr, "ERROR: coffset=0x%" PRIx64 ": " "copied flag must never be set for compressed " - "clusters\n", l2_entry >> s->cluster_bits); + "clusters\n", l2_entry & s->cluster_offset_mask); l2_entry &= ~QCOW_OFLAG_COPIED; res->corruptions++; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] Fix error message about compressed clusters with OFLAG_COPIED 2018-04-10 16:05 ` [Qemu-devel] [PATCH 1/2] Fix error message " Alberto Garcia @ 2018-04-10 16:18 ` Eric Blake 2018-04-10 16:45 ` Alberto Garcia 0 siblings, 1 reply; 7+ messages in thread From: Eric Blake @ 2018-04-10 16:18 UTC (permalink / raw) To: Alberto Garcia, qemu-devel; +Cc: qemu-block, Kevin Wolf, Max Reitz [-- Attachment #1: Type: text/plain, Size: 2852 bytes --] On 04/10/2018 11:05 AM, Alberto Garcia wrote: > Compressed clusters are not supposed to have the COPIED bit set. > "qemu-img check" detects that and prints an error message reporting > the number of the affected host cluster. This doesn't make much sense > because compressed clusters are not aligned to host clusters, so it > would be better to report the offset instead. Plus, the calculation is > wrong and it uses the raw L2 entry as if it was simply an offset. > > This patch fixes the error message and reports the offset of the > compressed cluster. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2-refcount.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Do we have iotests coverage of this? Should the qcow2 spec be explicit that OFLAG_COPIED should never be set on a compressed cluster? The current documentation for L2 table entry mentions that bit 63 is '1' for a cluster with a refcount of exactly 1 if it is an L2 table reachable from the active L1 table, with no mention of a restriction that it must not be set when bit 62 is set. Or is it feasible that although qemu itself (should) never set OFLAG_COPIED on a compressed cluster, that some third-party tool could still validly set the bit for a compressed cluster that happens to occupy a host cluster with a refcount of exactly 1 (and where writing to that cluster could be done by replacing the cluster in-place with uncompressed data, or by again writing compressed data - compression is rather wasteful in that sense as the tail of the host cluster is unused, and the only way to use the tail of the cluster is with another compressed cluster at which point the refcount is no longer 1). > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 6b8b63514a..2dc23005b7 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1577,9 +1577,9 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, > case QCOW2_CLUSTER_COMPRESSED: > /* Compressed clusters don't have QCOW_OFLAG_COPIED */ > if (l2_entry & QCOW_OFLAG_COPIED) { > - fprintf(stderr, "ERROR: cluster %" PRId64 ": " > + fprintf(stderr, "ERROR: coffset=0x%" PRIx64 ": " > "copied flag must never be set for compressed " > - "clusters\n", l2_entry >> s->cluster_bits); > + "clusters\n", l2_entry & s->cluster_offset_mask); > l2_entry &= ~QCOW_OFLAG_COPIED; At any rate, regardless of the answers to my questions, the error message cleanup makes sense. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 619 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] Fix error message about compressed clusters with OFLAG_COPIED 2018-04-10 16:18 ` Eric Blake @ 2018-04-10 16:45 ` Alberto Garcia 0 siblings, 0 replies; 7+ messages in thread From: Alberto Garcia @ 2018-04-10 16:45 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: qemu-block, Kevin Wolf, Max Reitz On Tue 10 Apr 2018 06:18:28 PM CEST, Eric Blake wrote: > On 04/10/2018 11:05 AM, Alberto Garcia wrote: >> Compressed clusters are not supposed to have the COPIED bit set. >> "qemu-img check" detects that and prints an error message reporting >> the number of the affected host cluster. This doesn't make much sense >> because compressed clusters are not aligned to host clusters, so it >> would be better to report the offset instead. Plus, the calculation is >> wrong and it uses the raw L2 entry as if it was simply an offset. >> >> This patch fixes the error message and reports the offset of the >> compressed cluster. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> >> --- >> block/qcow2-refcount.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> > > Do we have iotests coverage of this? I have one half-written, but I was thinking to put it in 214, so I'll wait until Max's patch is merged. Berto ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 2/2] specs/qcow2: Clarify that compressed clusters have the COPIED bit reset 2018-04-10 16:05 [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED Alberto Garcia 2018-04-10 16:05 ` [Qemu-devel] [PATCH 1/2] Fix error message " Alberto Garcia @ 2018-04-10 16:05 ` Alberto Garcia 2018-04-10 16:31 ` Eric Blake 2018-04-13 14:38 ` [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED Max Reitz 2 siblings, 1 reply; 7+ messages in thread From: Alberto Garcia @ 2018-04-10 16:05 UTC (permalink / raw) To: qemu-devel; +Cc: Alberto Garcia, qemu-block, Eric Blake, Kevin Wolf, Max Reitz Compressed clusters are not supposed to have the COPIED bit set, but this is not made explicit in the specs, so let's document it. Signed-off-by: Alberto Garcia <berto@igalia.com> --- docs/interop/qcow2.txt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index feb711fb6a..8e1547ded2 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -400,10 +400,10 @@ L2 table entry: 62: 0 for standard clusters 1 for compressed clusters - 63: 0 for a cluster that is unused or requires COW, 1 if its - refcount is exactly one. This information is only accurate - in L2 tables that are reachable from the active L1 - table. + 63: 0 for clusters that are unused, compressed or require COW. + 1 for standard clusters whose refcount is exactly one. + This information is only accurate in L2 tables + that are reachable from the active L1 table. Standard Cluster Descriptor: -- 2.11.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] specs/qcow2: Clarify that compressed clusters have the COPIED bit reset 2018-04-10 16:05 ` [Qemu-devel] [PATCH 2/2] specs/qcow2: Clarify that compressed clusters have the COPIED bit reset Alberto Garcia @ 2018-04-10 16:31 ` Eric Blake 0 siblings, 0 replies; 7+ messages in thread From: Eric Blake @ 2018-04-10 16:31 UTC (permalink / raw) To: Alberto Garcia, qemu-devel; +Cc: qemu-block, Kevin Wolf, Max Reitz [-- Attachment #1: Type: text/plain, Size: 2039 bytes --] On 04/10/2018 11:05 AM, Alberto Garcia wrote: > Compressed clusters are not supposed to have the COPIED bit set, but > this is not made explicit in the specs, so let's document it. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > docs/interop/qcow2.txt | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt > index feb711fb6a..8e1547ded2 100644 > --- a/docs/interop/qcow2.txt > +++ b/docs/interop/qcow2.txt > @@ -400,10 +400,10 @@ L2 table entry: > 62: 0 for standard clusters > 1 for compressed clusters > > - 63: 0 for a cluster that is unused or requires COW, 1 if its > - refcount is exactly one. This information is only accurate > - in L2 tables that are reachable from the active L1 > - table. > + 63: 0 for clusters that are unused, compressed or require COW. > + 1 for standard clusters whose refcount is exactly one. > + This information is only accurate in L2 tables > + that are reachable from the active L1 table. This matches what qemu outputs, so the question becomes whether it is technically necessary to make this requirement mandatory for 3rd-party implementations. But I'm in favor of the tighter wording, as it gets rather hairy to check whether exactly one compressed cluster is occupying a host cluster, plus I don't want to think about what happens if a compressed cluster with the bit set crosses a host cluster boundary (does it mean that compressed cluster is the only [remaining] source of data for BOTH host clusters at once, where both the head of the first host cluster and tail of the second host cluster is unused?) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 619 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED 2018-04-10 16:05 [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED Alberto Garcia 2018-04-10 16:05 ` [Qemu-devel] [PATCH 1/2] Fix error message " Alberto Garcia 2018-04-10 16:05 ` [Qemu-devel] [PATCH 2/2] specs/qcow2: Clarify that compressed clusters have the COPIED bit reset Alberto Garcia @ 2018-04-13 14:38 ` Max Reitz 2 siblings, 0 replies; 7+ messages in thread From: Max Reitz @ 2018-04-13 14:38 UTC (permalink / raw) To: Alberto Garcia, qemu-devel; +Cc: qemu-block, Eric Blake, Kevin Wolf On 2018-04-10 18:05, Alberto Garcia wrote: > Hi, > > while reviewing one previous patch about data corruption and > compressed clusters we discussed that the documentation doesn't > clarify that L2 entries for compressed clusters are not supposed to > have the OFLAG_COPIED bit set. > > Here's a patch to update the documentation and another one to fix the > error message that "qemu-img check" produces. > > Regards, > > Berto > > Alberto Garcia (2): > Fix error message about compressed clusters with OFLAG_COPIED > specs/qcow2: Clarify that compressed clusters have the COPIED bit > reset > > block/qcow2-refcount.c | 4 ++-- > docs/interop/qcow2.txt | 8 ++++---- > 2 files changed, 6 insertions(+), 6 deletions(-) Thanks, applied to my block-next tree: https://github.com/XanClic/qemu/commits/block-next Max ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-13 14:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-10 16:05 [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED Alberto Garcia 2018-04-10 16:05 ` [Qemu-devel] [PATCH 1/2] Fix error message " Alberto Garcia 2018-04-10 16:18 ` Eric Blake 2018-04-10 16:45 ` Alberto Garcia 2018-04-10 16:05 ` [Qemu-devel] [PATCH 2/2] specs/qcow2: Clarify that compressed clusters have the COPIED bit reset Alberto Garcia 2018-04-10 16:31 ` Eric Blake 2018-04-13 14:38 ` [Qemu-devel] [PATCH 0/2] Minor fixes about compressed clusters with OFLAG_COPIED Max Reitz
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.