[03/20] qcow2: Extend spec for external data files
diff mbox series

Message ID 20190227172256.30368-4-kwolf@redhat.com
State New
Headers show
Series
  • qcow2: External data files
Related show

Commit Message

Kevin Wolf Feb. 27, 2019, 5:22 p.m. UTC
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(-)

Comments

Eric Blake Feb. 27, 2019, 5:59 p.m. UTC | #1
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.
Stefan Hajnoczi March 1, 2019, 4:17 p.m. UTC | #2
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?
Eric Blake March 1, 2019, 4:32 p.m. UTC | #3
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.
Stefan Hajnoczi March 6, 2019, 9:51 a.m. UTC | #4
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
Eric Blake March 6, 2019, 12:43 p.m. UTC | #5
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.
Kevin Wolf March 6, 2019, 3:06 p.m. UTC | #6
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
Stefan Hajnoczi March 7, 2019, 10:07 a.m. UTC | #7
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

Patch
diff mbox series

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)