| Message ID | 1362589321-21402-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 06, 2013 at 06:02:01PM +0100, Paolo Bonzini wrote: > Otherwise, live migration of the top layer will miss zero clusters and > let the backing file show through. This also matches what is done in qed. > > QCOW2_CLUSTER_ZERO clusters are invalid in v2 image files. Check this > directly in qcow2_get_cluster_offset instead of replicating the test > everywhere. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/qcow2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> The qcow2 spec says: Bit 0: If set to 1, the cluster reads as all zeros. The host cluster offset can be used to describe a preallocation, but it won't be used for reading data from this cluster, nor is data read from the backing file if the cluster is unallocated. Your patch makes zero clusters "allocated", which does not violate the preallocation case.
On Wed, Mar 06, 2013 at 06:02:01PM +0100, Paolo Bonzini wrote: > Otherwise, live migration of the top layer will miss zero clusters and > let the backing file show through. This also matches what is done in qed. > > QCOW2_CLUSTER_ZERO clusters are invalid in v2 image files. Check this > directly in qcow2_get_cluster_offset instead of replicating the test > everywhere. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/qcow2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block-next tree: https://github.com/stefanha/qemu/commits/block-next Stefan
Am 06.03.2013 um 18:02 hat Paolo Bonzini geschrieben: > Otherwise, live migration of the top layer will miss zero clusters and > let the backing file show through. This also matches what is done in qed. > > QCOW2_CLUSTER_ZERO clusters are invalid in v2 image files. Check this > directly in qcow2_get_cluster_offset instead of replicating the test > everywhere. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Can you add a test case for this? Also is_allocated() probably is the wrong interface now because it can mean different things. The content of a zero cluster is indeed defined by the image, but it may or may not be fully allocated yet. Have you checked if the callers use it consistently in the former way? > block/qcow2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 56fccf9..bb04432 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -454,6 +454,9 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, > *cluster_offset &= L2E_COMPRESSED_OFFSET_SIZE_MASK; > break; > case QCOW2_CLUSTER_ZERO: > + if (s->qcow_version < 3) { > + return -EIO; > + } This leaks a cache entry. Kevin
Il 13/03/2013 10:14, Kevin Wolf ha scritto: >> > Otherwise, live migration of the top layer will miss zero clusters and >> > let the backing file show through. This also matches what is done in qed. >> > >> > QCOW2_CLUSTER_ZERO clusters are invalid in v2 image files. Check this >> > directly in qcow2_get_cluster_offset instead of replicating the test >> > everywhere. >> > >> > Cc: qemu-stable@nongnu.org >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Can you add a test case for this? Yes, I'll do this. > Also is_allocated() probably is the wrong interface now because it can > mean different things. The content of a zero cluster is indeed defined > by the image, but it may or may not be fully allocated yet. Have you > checked if the callers use it consistently in the former way? Yes, they do. In particular, qemu-img rebase would have the same bug as the live block jobs. Paolo
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 56fccf9..bb04432 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -454,6 +454,9 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, *cluster_offset &= L2E_COMPRESSED_OFFSET_SIZE_MASK; break; case QCOW2_CLUSTER_ZERO: + if (s->qcow_version < 3) { + return -EIO; + } c = count_contiguous_clusters(nb_clusters, s->cluster_size, &l2_table[l2_index], 0, QCOW_OFLAG_COMPRESSED | QCOW_OFLAG_ZERO); Stage this hunk [y,n,q,a,d,/,e,?]? y diff --git a/block/qcow2.c b/block/qcow2.c index 7610e56..b4c7c54 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -584,7 +584,7 @@ static int coroutine_fn qcow2_co_is_allocated(BlockDriverState *bs, *pnum = 0; } - return (cluster_offset != 0); + return (cluster_offset != 0) || (ret == QCOW2_CLUSTER_ZERO); } /* handle reading after the end of the backing file */ @@ -665,10 +665,6 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num, break; case QCOW2_CLUSTER_ZERO: - if (s->qcow_version < 3) { - ret = -EIO; - goto fail; - } qemu_iovec_memset(&hd_qiov, 0, 0, 512 * cur_nr_sectors); break;
Otherwise, live migration of the top layer will miss zero clusters and let the backing file show through. This also matches what is done in qed. QCOW2_CLUSTER_ZERO clusters are invalid in v2 image files. Check this directly in qcow2_get_cluster_offset instead of replicating the test everywhere. Cc: qemu-stable@nongnu.org Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/qcow2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)