| Message ID | 20190227172256.30368-4-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qcow2: External data files | expand |
On 2/27/19 11:22 AM, Kevin Wolf wrote: > This adds external data file to the qcow2 spec as a new incompatible > feature. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > docs/interop/qcow2.txt | 38 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt > index fb5cb47245..1c1849dc26 100644 > --- a/docs/interop/qcow2.txt > +++ b/docs/interop/qcow2.txt > @@ -97,7 +97,19 @@ in the description of a field. > be written to (unless for regaining > consistency). > > - Bits 2-63: Reserved (set to 0) > + Bit 2: External data file bit. If this bit is set, an > + external data file is used. Guest clusters are > + then stored in the external data file. For such > + images, clusters in the external data file are > + not refcounted. The offset field in the > + Standard Cluster Descriptor must match the > + guest offset and neither compressed clusters > + nor internal snapshots are supported. > + > + An external data file name header extension may > + be present if this bit is set. > + > + Bits 3-63: Reserved (set to 0) > > 80 - 87: compatible_features > Bitmask of compatible features. An implementation can > @@ -126,7 +138,17 @@ in the description of a field. > bit is unset, the bitmaps extension data must be > considered inconsistent. > > - Bits 1-63: Reserved (set to 0) > + Bit 1: If this bit is set, the external data file can > + be read as a consistent standalone raw image > + without looking at the qcow2 metadata. > + > + Setting this bit has a performance impact for > + some operations on the image (e.g. writing > + zeros requires writing to the data file instead > + of only setting the zero flag in the L2 table > + entry) and conflicts with backing files. Is it an error if an image has this bit set but not the 'external data file bit' in the incompatible bits? > + > + Bits 2-63: Reserved (set to 0) > > 96 - 99: refcount_order > Describes the width of a reference count block entry (width > @@ -148,6 +170,7 @@ be stored. Each extension has a structure like the following: > 0x6803f857 - Feature name table > 0x23852875 - Bitmaps extension > 0x0537be77 - Full disk encryption header pointer > + 0x44415441 - External data file name Likewise, should it be an explicit error if this header is present without the 'external data file bit'? > other - Unknown header extension, can be safely > ignored > > @@ -437,6 +460,11 @@ L2 table entry: > This information is only accurate in L2 tables > that are reachable from the active L1 table. > > + With external data files, all guest clusters have an > + implicit refcount of 1 (because of the fixed host = guest > + mapping for guest cluster offsets), so this bit should be 1 > + for all allocated clusters. > + > Standard Cluster Descriptor: > > Bit 0: If set to 1, the cluster reads as all zeros. The host > @@ -450,8 +478,10 @@ Standard Cluster Descriptor: > 1 - 8: Reserved (set to 0) > > 9 - 55: Bits 9-55 of host cluster offset. Must be aligned to a > - cluster boundary. If the offset is 0, the cluster is > - unallocated. > + cluster boundary. If the offset is 0 and bit 63 is clear, > + the cluster is unallocated. The offset may only be 0 with > + bit 63 set (indicating a host cluster offset of 0) when an > + external data file is used. Looks good, modulo any additions you want to make based on answering my questions above.
On Wed, Feb 27, 2019 at 06:22:39PM +0100, Kevin Wolf wrote: > - Bits 2-63: Reserved (set to 0) > + Bit 2: External data file bit. If this bit is set, an > + external data file is used. Guest clusters are > + then stored in the external data file. For such > + images, clusters in the external data file are > + not refcounted. The offset field in the > + Standard Cluster Descriptor must match the > + guest offset and neither compressed clusters > + nor internal snapshots are supported. > + > + An external data file name header extension may > + be present if this bit is set. This sentence is clearer if the header extension name is made prominent: An "external data file name" header extension Or An External Data File Name header extension (In the previous paragraph Standard Cluster Descriptor is also capitalized.) > @@ -148,6 +170,7 @@ be stored. Each extension has a structure like the following: > 0x6803f857 - Feature name table > 0x23852875 - Bitmaps extension > 0x0537be77 - Full disk encryption header pointer > + 0x44415441 - External data file name This new header extension isn't described in this patch?
On 3/1/19 10:17 AM, Stefan Hajnoczi wrote: > On Wed, Feb 27, 2019 at 06:22:39PM +0100, Kevin Wolf wrote: >> - Bits 2-63: Reserved (set to 0) >> + Bit 2: External data file bit. If this bit is set, an >> + external data file is used. Guest clusters are >> + then stored in the external data file. For such >> + images, clusters in the external data file are >> + not refcounted. The offset field in the >> + Standard Cluster Descriptor must match the >> + guest offset and neither compressed clusters >> + nor internal snapshots are supported. >> + >> + An external data file name header extension may >> + be present if this bit is set. > > This sentence is clearer if the header extension name is made prominent: > > An "external data file name" header extension > > Or > > An External Data File Name header extension > > (In the previous paragraph Standard Cluster Descriptor is also > capitalized.) Indeed, so I like the latter suggestion. > >> @@ -148,6 +170,7 @@ be stored. Each extension has a structure like the following: >> 0x6803f857 - Feature name table >> 0x23852875 - Bitmaps extension >> 0x0537be77 - Full disk encryption header pointer >> + 0x44415441 - External data file name > > This new header extension isn't described in this patch? I asked the same on v1, and the answer (which remains valid) is that neither is 0xe2792aca Backing file format name. (In other words, both extensions are simple enough as a single file name to be implicitly described by the reference to the header in the earlier text). Making both explicit wouldn't hurt my feelings, but I don't see it as a showstopper to the patch as-is.
On Fri, Mar 01, 2019 at 10:32:27AM -0600, Eric Blake wrote: > On 3/1/19 10:17 AM, Stefan Hajnoczi wrote: > > On Wed, Feb 27, 2019 at 06:22:39PM +0100, Kevin Wolf wrote: > >> @@ -148,6 +170,7 @@ be stored. Each extension has a structure like the following: > >> 0x6803f857 - Feature name table > >> 0x23852875 - Bitmaps extension > >> 0x0537be77 - Full disk encryption header pointer > >> + 0x44415441 - External data file name > > > > This new header extension isn't described in this patch? > > I asked the same on v1, and the answer (which remains valid) is that > neither is 0xe2792aca Backing file format name. (In other words, both > extensions are simple enough as a single file name to be implicitly > described by the reference to the header in the earlier text). Making > both explicit wouldn't hurt my feelings, but I don't see it as a > showstopper to the patch as-is. The spec should make the representation clear. Is it a NUL-terminated string or is the length dictated by the header extension length field? Otherwise implementors are forced to look at the QEMU source code or guess based on hex dumping example files :(. Stefan
On 3/6/19 3:51 AM, Stefan Hajnoczi wrote: > On Fri, Mar 01, 2019 at 10:32:27AM -0600, Eric Blake wrote: >> On 3/1/19 10:17 AM, Stefan Hajnoczi wrote: >>> On Wed, Feb 27, 2019 at 06:22:39PM +0100, Kevin Wolf wrote: >>>> @@ -148,6 +170,7 @@ be stored. Each extension has a structure like the following: >>>> 0x6803f857 - Feature name table >>>> 0x23852875 - Bitmaps extension >>>> 0x0537be77 - Full disk encryption header pointer >>>> + 0x44415441 - External data file name >>> >>> This new header extension isn't described in this patch? >> >> I asked the same on v1, and the answer (which remains valid) is that >> neither is 0xe2792aca Backing file format name. (In other words, both >> extensions are simple enough as a single file name to be implicitly >> described by the reference to the header in the earlier text). Making >> both explicit wouldn't hurt my feelings, but I don't see it as a >> showstopper to the patch as-is. > > The spec should make the representation clear. Is it a NUL-terminated > string or is the length dictated by the header extension length field? My understanding is length determined by the header field, with optional NUL padding out to the alignment boundary (but that also means that it does NOT necessarily have a trailing NUL on disk if sizing matches alignment). But yes, being explicit never hurts. > > Otherwise implementors are forced to look at the QEMU source code or > guess based on hex dumping example files :(. Indeed, cleaning up the existing Backing file format name is worth doing (at which point this should follow suit). But it still sounds like a separate patch, at which point it becomes a question of ordering - if the cleanup lands first, then this needs to rebase to do the same; if this lands first, then the cleanup does both headers at once.
Am 06.03.2019 um 13:43 hat Eric Blake geschrieben: > On 3/6/19 3:51 AM, Stefan Hajnoczi wrote: > > On Fri, Mar 01, 2019 at 10:32:27AM -0600, Eric Blake wrote: > >> On 3/1/19 10:17 AM, Stefan Hajnoczi wrote: > >>> On Wed, Feb 27, 2019 at 06:22:39PM +0100, Kevin Wolf wrote: > >>>> @@ -148,6 +170,7 @@ be stored. Each extension has a structure like the following: > >>>> 0x6803f857 - Feature name table > >>>> 0x23852875 - Bitmaps extension > >>>> 0x0537be77 - Full disk encryption header pointer > >>>> + 0x44415441 - External data file name > >>> > >>> This new header extension isn't described in this patch? > >> > >> I asked the same on v1, and the answer (which remains valid) is that > >> neither is 0xe2792aca Backing file format name. (In other words, both > >> extensions are simple enough as a single file name to be implicitly > >> described by the reference to the header in the earlier text). Making > >> both explicit wouldn't hurt my feelings, but I don't see it as a > >> showstopper to the patch as-is. > > > > The spec should make the representation clear. Is it a NUL-terminated > > string or is the length dictated by the header extension length field? > > My understanding is length determined by the header field, with optional > NUL padding out to the alignment boundary (but that also means that it > does NOT necessarily have a trailing NUL on disk if sizing matches > alignment). But yes, being explicit never hurts. > > > > > Otherwise implementors are forced to look at the QEMU source code or > > guess based on hex dumping example files :(. > > Indeed, cleaning up the existing Backing file format name is worth doing > (at which point this should follow suit). But it still sounds like a > separate patch, at which point it becomes a question of ordering - if > the cleanup lands first, then this needs to rebase to do the same; if > this lands first, then the cleanup does both headers at once. Maybe add a new section "String header extensions" that covers both? If this remains the only patch in the series that would need a significant change, I'd prefer a follow-up patch indeed. Kevin
On Wed, Mar 06, 2019 at 04:06:33PM +0100, Kevin Wolf wrote: > Maybe add a new section "String header extensions" that covers both? > > If this remains the only patch in the series that would need a > significant change, I'd prefer a follow-up patch indeed. A follow-up patch sounds good. Stefan
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index fb5cb47245..1c1849dc26 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -97,7 +97,19 @@ in the description of a field. be written to (unless for regaining consistency). - Bits 2-63: Reserved (set to 0) + Bit 2: External data file bit. If this bit is set, an + external data file is used. Guest clusters are + then stored in the external data file. For such + images, clusters in the external data file are + not refcounted. The offset field in the + Standard Cluster Descriptor must match the + guest offset and neither compressed clusters + nor internal snapshots are supported. + + An external data file name header extension may + be present if this bit is set. + + Bits 3-63: Reserved (set to 0) 80 - 87: compatible_features Bitmask of compatible features. An implementation can @@ -126,7 +138,17 @@ in the description of a field. bit is unset, the bitmaps extension data must be considered inconsistent. - Bits 1-63: Reserved (set to 0) + Bit 1: If this bit is set, the external data file can + be read as a consistent standalone raw image + without looking at the qcow2 metadata. + + Setting this bit has a performance impact for + some operations on the image (e.g. writing + zeros requires writing to the data file instead + of only setting the zero flag in the L2 table + entry) and conflicts with backing files. + + Bits 2-63: Reserved (set to 0) 96 - 99: refcount_order Describes the width of a reference count block entry (width @@ -148,6 +170,7 @@ be stored. Each extension has a structure like the following: 0x6803f857 - Feature name table 0x23852875 - Bitmaps extension 0x0537be77 - Full disk encryption header pointer + 0x44415441 - External data file name other - Unknown header extension, can be safely ignored @@ -437,6 +460,11 @@ L2 table entry: This information is only accurate in L2 tables that are reachable from the active L1 table. + With external data files, all guest clusters have an + implicit refcount of 1 (because of the fixed host = guest + mapping for guest cluster offsets), so this bit should be 1 + for all allocated clusters. + Standard Cluster Descriptor: Bit 0: If set to 1, the cluster reads as all zeros. The host @@ -450,8 +478,10 @@ Standard Cluster Descriptor: 1 - 8: Reserved (set to 0) 9 - 55: Bits 9-55 of host cluster offset. Must be aligned to a - cluster boundary. If the offset is 0, the cluster is - unallocated. + cluster boundary. If the offset is 0 and bit 63 is clear, + the cluster is unallocated. The offset may only be 0 with + bit 63 set (indicating a host cluster offset of 0) when an + external data file is used. 56 - 61: Reserved (set to 0)
This adds external data file to the qcow2 spec as a new incompatible feature. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- docs/interop/qcow2.txt | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-)