| Message ID | 1500993699-19299-11-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On 07/25/2017 09:41 AM, Peter Lieven wrote: > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/qcow2.c | 7 +++++++ > qapi/block-core.json | 6 +++++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > +++ b/qapi/block-core.json > @@ -68,6 +68,9 @@ > # @encrypt: details about encryption parameters; only set if image > # is encrypted (since 2.10) > # > +# @compress: details about parameters for compressed clusters; only set if > +# the compress format header extension is present (since 2.11) Thinking aloud: I still think this is better as "only set if the image uses compressed headers" (similar to encrypt). That is, I would like this output to be present even when opening legacy deflate/0/12 images. But thinking more, what I am REALLY asking for is "if any cluster is compressed, then this information is present; if nothing is compressed, the choice of compression header doesn't matter". But we don't have a quick-and-dirty way to tell if an image has any compressed clusters, short of examining every L2 map (including maps inside internal snapshots). Hmm - we already have 'Dirty bit' and 'Corrupt bit' as incompatible_features that can quickly identify certain image properties; do we want another bit set to 1 for images known to use compressed clusters? Or even make it an auto-clear bit (or even a pair of auto-clear bits: one to designate that this image was written by a new-enough qemu that promises to mark the image if any compressed clusters exist, and the other is set only if such clusters do exist; that way, if both bits are clear, we know we have to walk L2 tables to look for compressed clusters, but if the first bit is set, then the second bit is reliable)? So maybe this means we are still debating spec ideas. Kevin, do you want to chime in?
Am 25.07.2017 um 23:55 schrieb Eric Blake: > On 07/25/2017 09:41 AM, Peter Lieven wrote: >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> block/qcow2.c | 7 +++++++ >> qapi/block-core.json | 6 +++++- >> 2 files changed, 12 insertions(+), 1 deletion(-) >> >> +++ b/qapi/block-core.json >> @@ -68,6 +68,9 @@ >> # @encrypt: details about encryption parameters; only set if image >> # is encrypted (since 2.10) >> # >> +# @compress: details about parameters for compressed clusters; only set if >> +# the compress format header extension is present (since 2.11) > Thinking aloud: > > I still think this is better as "only set if the image uses compressed > headers" (similar to encrypt). That is, I would like this output to be > present even when opening legacy deflate/0/12 images. > > But thinking more, what I am REALLY asking for is "if any cluster is > compressed, then this information is present; if nothing is compressed, > the choice of compression header doesn't matter". But we don't have a > quick-and-dirty way to tell if an image has any compressed clusters, > short of examining every L2 map (including maps inside internal snapshots). > > Hmm - we already have 'Dirty bit' and 'Corrupt bit' as > incompatible_features that can quickly identify certain image > properties; do we want another bit set to 1 for images known to use > compressed clusters? Or even make it an auto-clear bit (or even a pair > of auto-clear bits: one to designate that this image was written by a > new-enough qemu that promises to mark the image if any compressed > clusters exist, and the other is set only if such clusters do exist; > that way, if both bits are clear, we know we have to walk L2 tables to > look for compressed clusters, but if the first bit is set, then the > second bit is reliable)? > > So maybe this means we are still debating spec ideas. Kevin, do you > want to chime in? > I would also vote for adding an information to the image if it contains compressed clusters. 2 bits sounds reasonable. One to promise we set the compressed bit if we write any compressed cluster and one to tell that there are actually compressed cluster. If we set the "promise" bit at create time we can simply hook up in qcow2_co_pwritev_comperssed and set the bit there as soon as we write any compressed cluster. The compression info I would display unconditionally. Because the compress settings are made at create time and are there even if we don't have compressed clusters yet. Peter
Am 25.07.2017 um 23:55 schrieb Eric Blake: > On 07/25/2017 09:41 AM, Peter Lieven wrote: >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> block/qcow2.c | 7 +++++++ >> qapi/block-core.json | 6 +++++- >> 2 files changed, 12 insertions(+), 1 deletion(-) >> >> +++ b/qapi/block-core.json >> @@ -68,6 +68,9 @@ >> # @encrypt: details about encryption parameters; only set if image >> # is encrypted (since 2.10) >> # >> +# @compress: details about parameters for compressed clusters; only set if >> +# the compress format header extension is present (since 2.11) > Thinking aloud: > > I still think this is better as "only set if the image uses compressed > headers" (similar to encrypt). That is, I would like this output to be > present even when opening legacy deflate/0/12 images. > > But thinking more, what I am REALLY asking for is "if any cluster is > compressed, then this information is present; if nothing is compressed, > the choice of compression header doesn't matter". But we don't have a > quick-and-dirty way to tell if an image has any compressed clusters, > short of examining every L2 map (including maps inside internal snapshots). > > Hmm - we already have 'Dirty bit' and 'Corrupt bit' as > incompatible_features that can quickly identify certain image > properties; do we want another bit set to 1 for images known to use > compressed clusters? Or even make it an auto-clear bit (or even a pair > of auto-clear bits: one to designate that this image was written by a > new-enough qemu that promises to mark the image if any compressed > clusters exist, and the other is set only if such clusters do exist; > that way, if both bits are clear, we know we have to walk L2 tables to > look for compressed clusters, but if the first bit is set, then the > second bit is reliable)? > > So maybe this means we are still debating spec ideas. Kevin, do you > want to chime in? > Hi all, I would really like to move this forward and get at least the spec for the QCOW2 Extension Header fixed. I internally need this feature and would like not to produce incompatible images. Maybe we can even decide later if we add further bits for tracking if there are compressed cluster and keep this indepented of this series. Thanks, Peter
Am 25.07.2017 um 23:55 hat Eric Blake geschrieben: > On 07/25/2017 09:41 AM, Peter Lieven wrote: > > Signed-off-by: Peter Lieven <pl@kamp.de> > > --- > > block/qcow2.c | 7 +++++++ > > qapi/block-core.json | 6 +++++- > > 2 files changed, 12 insertions(+), 1 deletion(-) > > > > +++ b/qapi/block-core.json > > @@ -68,6 +68,9 @@ > > # @encrypt: details about encryption parameters; only set if image > > # is encrypted (since 2.10) > > # > > +# @compress: details about parameters for compressed clusters; only set if > > +# the compress format header extension is present (since 2.11) > > Thinking aloud: > > I still think this is better as "only set if the image uses compressed > headers" (similar to encrypt). That is, I would like this output to be > present even when opening legacy deflate/0/12 images. > > But thinking more, what I am REALLY asking for is "if any cluster is > compressed, then this information is present; if nothing is compressed, > the choice of compression header doesn't matter". But we don't have a > quick-and-dirty way to tell if an image has any compressed clusters, > short of examining every L2 map (including maps inside internal snapshots). > > Hmm - we already have 'Dirty bit' and 'Corrupt bit' as > incompatible_features that can quickly identify certain image > properties; do we want another bit set to 1 for images known to use > compressed clusters? Or even make it an auto-clear bit (or even a pair > of auto-clear bits: one to designate that this image was written by a > new-enough qemu that promises to mark the image if any compressed > clusters exist, and the other is set only if such clusters do exist; > that way, if both bits are clear, we know we have to walk L2 tables to > look for compressed clusters, but if the first bit is set, then the > second bit is reliable)? > > So maybe this means we are still debating spec ideas. Kevin, do you > want to chime in? I'm not sure if this would be an useful extension. Adding feature bits is a relatively big change and I don't see what it actually buys us here. First of all, what use is the information that an image contains compressed clusters? For any practical use that I could imagine, there is a big difference between an image that contains one compressed cluster and an image that has 100% compressed clusters - much bigger than between the image with one compressed cluster and an image that is fully uncompressed. But if we assume for a moment that the information is useful: For old images, we would still be in the same situation as today and couldn't tell whether an image contains compressed clusters or not. For new images that are meant for new qemu versions, we can have the compression header, which is probably as close as we can get (at least I don't think you suggested something that would unset the compressed bit as soon as the last compressed cluster gets uncompressed through copy-on-write). That leaves us with images created by new qemu versions that are meant to be consumed by old versions so that we can't set the incompatible feature bit. If we are concerned about these, couldn't we simply specify that a compression header that contains the current defaults MAY exist even without the incompatible feature bit? Kevin
diff --git a/block/qcow2.c b/block/qcow2.c index 0e9c2b8..7e03877 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3968,6 +3968,13 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs) spec_info->u.qcow2.data->encrypt = qencrypt; } + if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESS) { + spec_info->u.qcow2.data->compress = g_new0(Qcow2Compress, 1); + memcpy(spec_info->u.qcow2.data->compress, &s->compress, + sizeof(Qcow2Compress)); + spec_info->u.qcow2.data->has_compress = true; + } + return spec_info; } diff --git a/qapi/block-core.json b/qapi/block-core.json index d655bb1..3b338d5 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -68,6 +68,9 @@ # @encrypt: details about encryption parameters; only set if image # is encrypted (since 2.10) # +# @compress: details about parameters for compressed clusters; only set if +# the compress format header extension is present (since 2.11) +# # Since: 1.7 ## { 'struct': 'ImageInfoSpecificQCow2', @@ -76,7 +79,8 @@ '*lazy-refcounts': 'bool', '*corrupt': 'bool', 'refcount-bits': 'int', - '*encrypt': 'ImageInfoSpecificQCow2Encryption' + '*encrypt': 'ImageInfoSpecificQCow2Encryption', + '*compress': 'Qcow2Compress' } } ##
Signed-off-by: Peter Lieven <pl@kamp.de> --- block/qcow2.c | 7 +++++++ qapi/block-core.json | 6 +++++- 2 files changed, 12 insertions(+), 1 deletion(-)