| Message ID | 20200131142219.3264-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | docs: qcow2: introduce compression type feature | expand |
On 1/31/20 8:22 AM, Vladimir Sementsov-Ogievskiy wrote: > The patch adds a new additional field to the qcow2 header: compression_type, > which specifies compression type. If field is absent or zero, default > compression type is set: ZLIB, which corresponds to current behavior. > > New compression type (ZSTD) is to be added in further commit. > > Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > docs/interop/qcow2.txt | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt > index 823cc266e0..3a8d5c0c6e 100644 > --- a/docs/interop/qcow2.txt > +++ b/docs/interop/qcow2.txt > @@ -109,6 +109,11 @@ the next fields through header_length. > An External Data File Name header extension may > be present if this bit is set. > > + Bit 3: Compression type bit. If this bit is set, > + a non-default compression is used for compressed > + clusters. The compression_type field must be > + present and not zero. > + I'm still not sold on this. What's wrong with: Bit 3: Compression type bit. If this bit is set, a non-default compression may be used for compressed clusters, and the compression_type field must be present. > Bits 3-63: Reserved (set to 0) > > 80 - 87: compatible_features > @@ -190,7 +195,19 @@ present*, if not altered by a specific incompatible bit. > to the field's offset. Also, all additional fields are not present for > version 2. > > - < ... No additional fields in the header currently ... > > + 104: compression_type > + > + Defines the compression method used for compressed clusters. > + All compressed clusters in an image use the same compression > + type. > + > + If the incompatible bit "Compression type" is set: the field > + must be present and non-zero (which means non-zlib > + compression type). Otherwise, this field must not be present > + or must be zero (which means zlib). If the incompatible bit "Compression type" is set: the field must be present. Otherwise, this field must not be present or must be zero (which means zlib). It is recommended but not required that the "Compression type" bit be clear if this field is present but zero. > + > + Available compression type values: > + 0: zlib <https://www.zlib.net/> > > > === Header padding === >
On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote: >> + If the incompatible bit "Compression type" is set: the field >> + must be present and non-zero (which means non-zlib >> + compression type). Otherwise, this field must not be present >> + or must be zero (which means zlib). > > If the incompatible bit "Compression type" is set: the field > must be present. Otherwise, this field must not be present > or must be zero (which means zlib). But "not being present" and "being zero" is equivalent (as described in the previous commit). And if the incompatible bit is not present then the field can be safely ignored (i.e. whether it is zero or not is irrelevant). Let's try again: Defines the compression method used for compressed clusters. All compressed clusters in an image use the same type. The value of this field should only be used when the incompatible bit "Compression type" is set. If that bit is unset then this field is not used and the compression method is zlib. Berto
On 1/31/20 11:34 AM, Alberto Garcia wrote: > On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote: >>> + If the incompatible bit "Compression type" is set: the field >>> + must be present and non-zero (which means non-zlib >>> + compression type). Otherwise, this field must not be present >>> + or must be zero (which means zlib). >> >> If the incompatible bit "Compression type" is set: the field >> must be present. Otherwise, this field must not be present >> or must be zero (which means zlib). > > But "not being present" and "being zero" is equivalent (as described in > the previous commit). > > And if the incompatible bit is not present then the field can be safely > ignored (i.e. whether it is zero or not is irrelevant). > > Let's try again: > > Defines the compression method used for compressed clusters. All > compressed clusters in an image use the same type. > > The value of this field should only be used when the incompatible bit > "Compression type" is set. If that bit is unset then this field is > not used and the compression method is zlib. I like that wording.
31.01.2020 20:49, Eric Blake wrote: > On 1/31/20 11:34 AM, Alberto Garcia wrote: >> On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote: >>>> + If the incompatible bit "Compression type" is set: the field >>>> + must be present and non-zero (which means non-zlib >>>> + compression type). Otherwise, this field must not be present >>>> + or must be zero (which means zlib). >>> >>> If the incompatible bit "Compression type" is set: the field >>> must be present. Otherwise, this field must not be present >>> or must be zero (which means zlib). >> >> But "not being present" and "being zero" is equivalent (as described in >> the previous commit). >> >> And if the incompatible bit is not present then the field can be safely >> ignored (i.e. whether it is zero or not is irrelevant). >> >> Let's try again: >> >> Defines the compression method used for compressed clusters. All >> compressed clusters in an image use the same type. >> >> The value of this field should only be used when the incompatible bit >> "Compression type" is set. If that bit is unset then this field is >> not used and the compression method is zlib. > > I like that wording. > I'm OK with it too, as well as I'm OK with the stricter variant, when we don't allow incompatible images with zlib set. I don't see any serious difference. But I need this to land somehow. Max likes stricter variant and he is maintainer of qcow2.. Max, will you merge it as is, or did you change your mind, or should we ask Kevin for his opinion?
On Fri 31 Jan 2020 07:15:33 PM CET, Vladimir Sementsov-Ogievskiy wrote: > I'm OK with it too, as well as I'm OK with the stricter variant, when > we don't allow incompatible images with zlib set. I don't see any > serious difference. I also think both options are fine. Berto
On 31.01.20 19:15, Vladimir Sementsov-Ogievskiy wrote: > 31.01.2020 20:49, Eric Blake wrote: >> On 1/31/20 11:34 AM, Alberto Garcia wrote: >>> On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote: >>>>> + If the incompatible bit "Compression type" is >>>>> set: the field >>>>> + must be present and non-zero (which means >>>>> non-zlib >>>>> + compression type). Otherwise, this field must >>>>> not be present >>>>> + or must be zero (which means zlib). >>>> >>>> If the incompatible bit "Compression type" is set: the >>>> field >>>> must be present. Otherwise, this field must not be present >>>> or must be zero (which means zlib). >>> >>> But "not being present" and "being zero" is equivalent (as described in >>> the previous commit). >>> >>> And if the incompatible bit is not present then the field can be safely >>> ignored (i.e. whether it is zero or not is irrelevant). >>> >>> Let's try again: >>> >>> Defines the compression method used for compressed clusters. All >>> compressed clusters in an image use the same type. >>> >>> The value of this field should only be used when the incompatible >>> bit >>> "Compression type" is set. If that bit is unset then this field is >>> not used and the compression method is zlib. This doesn’t fully make sense to me. Maybe with s/should/must/, because as it is it means that for non-zlib compression methods, you *should* set the compression type bit and add this header extension; but you may also just add the extension and not set the compression bit. All in all, I didn’t see anyone disagreeing on the fact that there are only two cases that make any sense: (1) Have the bit unset and the extension not present or zero: zlib. (2) Have the bit set and an extension present and non-zero: not zlib. If those are the only sensible choices, I don’t see any practical argument for allowing anything else[1]. (However, I do see an argument for forbidding anything else, namely to ensure that everyone follows these sensible guidelines.) >> I like that wording. >> > > I'm OK with it too, as well as I'm OK with the stricter variant, when we > don't allow incompatible images with zlib set. I don't see any serious > difference. > > But I need this to land somehow. Max likes stricter variant and he is > maintainer of qcow2.. > > Max, will you merge it as is, or did you change your mind, or should we > ask Kevin for his opinion? I’m currently preparing a pull request (without this series), but after that I’m planning to merge the stricter variant. As far as I’ve seen, the argument for making it less strict was still accompanied by “Sure, nobody would set this flag for zlib-compressed images because that doesn’t make sense”. So if nobody would do that, we might as well just forbid it and thus ensure that everyone indeed does the sensible thing. Max [1] Besides “The specification would be one restriction shorter”, which I don’t think is a very good argument. Because without that sentence, anyone who implements qcow2 has to think about the problem anyway and figure out that quasi-restriction by themselves. If they don’t, it’d be a bit bad because they’d produce incompatible zlib-compressed images for no reason. Hence why I don’t see the restriction as a burden to the reader but as a helpful guideline (that must be followed).
On 2/6/20 6:38 AM, Max Reitz wrote: >> I'm OK with it too, as well as I'm OK with the stricter variant, when we >> don't allow incompatible images with zlib set. I don't see any serious >> difference. >> >> But I need this to land somehow. Max likes stricter variant and he is >> maintainer of qcow2.. >> >> Max, will you merge it as is, or did you change your mind, or should we >> ask Kevin for his opinion? > > I’m currently preparing a pull request (without this series), but after > that I’m planning to merge the stricter variant. > > As far as I’ve seen, the argument for making it less strict was still > accompanied by “Sure, nobody would set this flag for zlib-compressed > images because that doesn’t make sense”. So if nobody would do that, we > might as well just forbid it and thus ensure that everyone indeed does > the sensible thing. Fair enough; I'm happy to live with your decision as maintainer's prerogative. I _do_ hope that the actual implementation series gets merged soon, though, and that as part of that series, you remember to tweak the optional 'Feature Name' extension header to name the new incompatible bit introduced in this patch. And there's the simultaneous patches from my qcow2 autoclear-all-zeroes bit that touch the same files, so we may have some rebasing fun ahead of us...
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index 823cc266e0..3a8d5c0c6e 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -109,6 +109,11 @@ the next fields through header_length. An External Data File Name header extension may be present if this bit is set. + Bit 3: Compression type bit. If this bit is set, + a non-default compression is used for compressed + clusters. The compression_type field must be + present and not zero. + Bits 3-63: Reserved (set to 0) 80 - 87: compatible_features @@ -190,7 +195,19 @@ present*, if not altered by a specific incompatible bit. to the field's offset. Also, all additional fields are not present for version 2. - < ... No additional fields in the header currently ... > + 104: compression_type + + Defines the compression method used for compressed clusters. + All compressed clusters in an image use the same compression + type. + + If the incompatible bit "Compression type" is set: the field + must be present and non-zero (which means non-zlib + compression type). Otherwise, this field must not be present + or must be zero (which means zlib). + + Available compression type values: + 0: zlib <https://www.zlib.net/> === Header padding ===
The patch adds a new additional field to the qcow2 header: compression_type, which specifies compression type. If field is absent or zero, default compression type is set: ZLIB, which corresponds to current behavior. New compression type (ZSTD) is to be added in further commit. Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- docs/interop/qcow2.txt | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)