[Qemu-devel] [PATCH] cow: make padding in the header explicit

[Qemu-devel] [PATCH] cow: make padding in the header explicit

[Stefan Hajnoczi]

On-disk structures should be marked packed so the compiler does not
insert padding for field alignment.  Padding should be explicit so
on-disk layout is obvious and we don't rely on the architecture-specific
ABI for alignment rules.

The pahole(1) diff shows that the padding is now explicit and offsets
are unchanged:

 	char                       backing_file[1024];   /*     8  1024 */
 	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
 	int32_t                    mtime;                /*  1032     4 */
-
-	/* XXX 4 bytes hole, try to pack */
-
+	uint32_t                   padding;              /*  1036     4 */
 	uint64_t                   size;                 /*  1040     8 */

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/cow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/cow.c b/block/cow.c
index 6ee4833..5ee9363 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -37,9 +37,10 @@ struct cow_header_v2 {
     uint32_t version;
     char backing_file[1024];
     int32_t mtime;
+    uint32_t padding;
     uint64_t size;
     uint32_t sectorsize;
-};
+} QEMU_PACKED;
 
 typedef struct BDRVCowState {
     CoMutex lock;
-- 
1.9.3

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Kevin Wolf]

Am 04.09.2014 um 10:58 hat Stefan Hajnoczi geschrieben:
> On-disk structures should be marked packed so the compiler does not
> insert padding for field alignment.  Padding should be explicit so
> on-disk layout is obvious and we don't rely on the architecture-specific
> ABI for alignment rules.
> 
> The pahole(1) diff shows that the padding is now explicit and offsets
> are unchanged:
> 
>  	char                       backing_file[1024];   /*     8  1024 */
>  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
>  	int32_t                    mtime;                /*  1032     4 */
> -
> -	/* XXX 4 bytes hole, try to pack */
> -
> +	uint32_t                   padding;              /*  1036     4 */
>  	uint64_t                   size;                 /*  1040     8 */
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Benoît Canet]

The Thursday 04 Sep 2014 à 09:58:41 (+0100), Stefan Hajnoczi wrote :
> On-disk structures should be marked packed so the compiler does not
> insert padding for field alignment.  Padding should be explicit so
> on-disk layout is obvious and we don't rely on the architecture-specific
> ABI for alignment rules.
> 
> The pahole(1) diff shows that the padding is now explicit and offsets
> are unchanged:
> 
>  	char                       backing_file[1024];   /*     8  1024 */
>  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
>  	int32_t                    mtime;                /*  1032     4 */
> -
> -	/* XXX 4 bytes hole, try to pack */
> -
> +	uint32_t                   padding;              /*  1036     4 */
>  	uint64_t                   size;                 /*  1040     8 */
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/cow.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index 6ee4833..5ee9363 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -37,9 +37,10 @@ struct cow_header_v2 {
>      uint32_t version;
>      char backing_file[1024];
>      int32_t mtime;
> +    uint32_t padding;
>      uint64_t size;
>      uint32_t sectorsize;
> -};
> +} QEMU_PACKED;
>  
>  typedef struct BDRVCowState {
>      CoMutex lock;
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote:
> On-disk structures should be marked packed so the compiler does not
> insert padding for field alignment.  Padding should be explicit so
> on-disk layout is obvious and we don't rely on the architecture-specific
> ABI for alignment rules.
> 
> The pahole(1) diff shows that the padding is now explicit and offsets
> are unchanged:
> 
>  	char                       backing_file[1024];   /*     8  1024 */
>  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
>  	int32_t                    mtime;                /*  1032     4 */
> -
> -	/* XXX 4 bytes hole, try to pack */
> -
> +	uint32_t                   padding;              /*  1036     4 */
>  	uint64_t                   size;                 /*  1040     8 */

Was a 32-bit build also inserting this padding, or do we have historical
differences where 32-bit and 64-bit cow files are actually different,
and we may need to be prepared to parse files from both sources?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[shhuiw]

At 2014-09-04 08:07:32, "Eric Blake" <eblake@redhat.com> wrote:
>On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote:
>> On-disk structures should be marked packed so the compiler does not
>> insert padding for field alignment.  Padding should be explicit so
>> on-disk layout is obvious and we don't rely on the architecture-specific
>> ABI for alignment rules.
>> 
>> The pahole(1) diff shows that the padding is now explicit and offsets
>> are unchanged:
>> 
>>  	char                       backing_file[1024];   /*     8  1024 */
>>  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
>>  	int32_t                    mtime;                /*  1032     4 */
>> -
>> -	/* XXX 4 bytes hole, try to pack */
>> -
>> +	uint32_t                   padding;              /*  1036     4 */
>>  	uint64_t                   size;                 /*  1040     8 */
>
>Was a 32-bit build also inserting this padding, or do we have historical
>differences where 32-bit and 64-bit cow files are actually different,
>and we may need to be prepared to parse files from both sources?
>

Tried on my x86 32bits computer, and seems the padding is not needed:

struct cow_header_v2 {
        uint32_t                   	magic;                	/*     0     4 */
        uint32_t                   	version;              	/*     4     4 */
        char                       	backing_file[1024];   	/*     8  1024 */
        /* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
        int32_t                    	mtime;                	/*  1032     4 */
        uint64_t                   	size;                 	/*  1036     8 */
        uint32_t                   	sectorsize;           	/*  1044     4 */
        /* size: 1048, cachelines: 17, members: 6 */
        /* last cacheline: 24 bytes */
};


>-- 
>Eric Blake   eblake redhat com    +1-919-301-3266
>Libvirt virtualization library http://libvirt.org
>

Regards,
shhuiw

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]

On Thu, Sep 04, 2014 at 06:07:32AM -0600, Eric Blake wrote:
> On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote:
> > On-disk structures should be marked packed so the compiler does not
> > insert padding for field alignment.  Padding should be explicit so
> > on-disk layout is obvious and we don't rely on the architecture-specific
> > ABI for alignment rules.
> > 
> > The pahole(1) diff shows that the padding is now explicit and offsets
> > are unchanged:
> > 
> >  	char                       backing_file[1024];   /*     8  1024 */
> >  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
> >  	int32_t                    mtime;                /*  1032     4 */
> > -
> > -	/* XXX 4 bytes hole, try to pack */
> > -
> > +	uint32_t                   padding;              /*  1036     4 */
> >  	uint64_t                   size;                 /*  1040     8 */
> 
> Was a 32-bit build also inserting this padding, or do we have historical
> differences where 32-bit and 64-bit cow files are actually different,
> and we may need to be prepared to parse files from both sources?

Good point.  Let's not merge this patch since it breaks 32-bit hosts.

The fact that no one hit problems when exchanging files between 32-bit
and 64-bit machines shows that the cow format is rarely used.

At this point we have 2 different formats: one without padding
(i386-style) and one with padding (x86_64-style).  The chance of more
variants is small but who knows, maybe some other host architecture ABI
has yet another alignment rule for uint64_t.

I'd like to git rm block/cow.c but I suppose the backwards-compatible
thing to do is to introduce subformats to support both variants.
Opinions?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

Am 04.09.2014 um 15:51 hat Stefan Hajnoczi geschrieben:
> On Thu, Sep 04, 2014 at 06:07:32AM -0600, Eric Blake wrote:
> > On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote:
> > > On-disk structures should be marked packed so the compiler does not
> > > insert padding for field alignment.  Padding should be explicit so
> > > on-disk layout is obvious and we don't rely on the architecture-specific
> > > ABI for alignment rules.
> > > 
> > > The pahole(1) diff shows that the padding is now explicit and offsets
> > > are unchanged:
> > > 
> > >  	char                       backing_file[1024];   /*     8  1024 */
> > >  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
> > >  	int32_t                    mtime;                /*  1032     4 */
> > > -
> > > -	/* XXX 4 bytes hole, try to pack */
> > > -
> > > +	uint32_t                   padding;              /*  1036     4 */
> > >  	uint64_t                   size;                 /*  1040     8 */
> > 
> > Was a 32-bit build also inserting this padding, or do we have historical
> > differences where 32-bit and 64-bit cow files are actually different,
> > and we may need to be prepared to parse files from both sources?
> 
> Good point.  Let's not merge this patch since it breaks 32-bit hosts.
> 
> The fact that no one hit problems when exchanging files between 32-bit
> and 64-bit machines shows that the cow format is rarely used.
> 
> At this point we have 2 different formats: one without padding
> (i386-style) and one with padding (x86_64-style).  The chance of more
> variants is small but who knows, maybe some other host architecture ABI
> has yet another alignment rule for uint64_t.
> 
> I'd like to git rm block/cow.c but I suppose the backwards-compatible
> thing to do is to introduce subformats to support both variants.
> Opinions?

Can we safely detect which of the subformats we have? But I'm not sure
if it's even worth fixing.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2374 bytes --]

On Thu, Sep 04, 2014 at 04:10:14PM +0200, Kevin Wolf wrote:
> Am 04.09.2014 um 15:51 hat Stefan Hajnoczi geschrieben:
> > On Thu, Sep 04, 2014 at 06:07:32AM -0600, Eric Blake wrote:
> > > On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote:
> > > > On-disk structures should be marked packed so the compiler does not
> > > > insert padding for field alignment.  Padding should be explicit so
> > > > on-disk layout is obvious and we don't rely on the architecture-specific
> > > > ABI for alignment rules.
> > > > 
> > > > The pahole(1) diff shows that the padding is now explicit and offsets
> > > > are unchanged:
> > > > 
> > > >  	char                       backing_file[1024];   /*     8  1024 */
> > > >  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
> > > >  	int32_t                    mtime;                /*  1032     4 */
> > > > -
> > > > -	/* XXX 4 bytes hole, try to pack */
> > > > -
> > > > +	uint32_t                   padding;              /*  1036     4 */
> > > >  	uint64_t                   size;                 /*  1040     8 */
> > > 
> > > Was a 32-bit build also inserting this padding, or do we have historical
> > > differences where 32-bit and 64-bit cow files are actually different,
> > > and we may need to be prepared to parse files from both sources?
> > 
> > Good point.  Let's not merge this patch since it breaks 32-bit hosts.
> > 
> > The fact that no one hit problems when exchanging files between 32-bit
> > and 64-bit machines shows that the cow format is rarely used.
> > 
> > At this point we have 2 different formats: one without padding
> > (i386-style) and one with padding (x86_64-style).  The chance of more
> > variants is small but who knows, maybe some other host architecture ABI
> > has yet another alignment rule for uint64_t.
> > 
> > I'd like to git rm block/cow.c but I suppose the backwards-compatible
> > thing to do is to introduce subformats to support both variants.
> > Opinions?
> 
> Can we safely detect which of the subformats we have? But I'm not sure
> if it's even worth fixing.

I think it would default to the subformat depending on the host
architecture but allow overriding with -o subformat=i386|x86_64.

I'm also not sure if it's worth fixing.  The cow file format is so
rarely used I wonder if we'd be better off without it.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3679 bytes --]

Am 04.09.2014 um 17:43 hat Stefan Hajnoczi geschrieben:
> On Thu, Sep 04, 2014 at 04:10:14PM +0200, Kevin Wolf wrote:
> > Am 04.09.2014 um 15:51 hat Stefan Hajnoczi geschrieben:
> > > On Thu, Sep 04, 2014 at 06:07:32AM -0600, Eric Blake wrote:
> > > > On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote:
> > > > > On-disk structures should be marked packed so the compiler does not
> > > > > insert padding for field alignment.  Padding should be explicit so
> > > > > on-disk layout is obvious and we don't rely on the architecture-specific
> > > > > ABI for alignment rules.
> > > > > 
> > > > > The pahole(1) diff shows that the padding is now explicit and offsets
> > > > > are unchanged:
> > > > > 
> > > > >  	char                       backing_file[1024];   /*     8  1024 */
> > > > >  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
> > > > >  	int32_t                    mtime;                /*  1032     4 */
> > > > > -
> > > > > -	/* XXX 4 bytes hole, try to pack */
> > > > > -
> > > > > +	uint32_t                   padding;              /*  1036     4 */
> > > > >  	uint64_t                   size;                 /*  1040     8 */
> > > > 
> > > > Was a 32-bit build also inserting this padding, or do we have historical
> > > > differences where 32-bit and 64-bit cow files are actually different,
> > > > and we may need to be prepared to parse files from both sources?
> > > 
> > > Good point.  Let's not merge this patch since it breaks 32-bit hosts.
> > > 
> > > The fact that no one hit problems when exchanging files between 32-bit
> > > and 64-bit machines shows that the cow format is rarely used.
> > > 
> > > At this point we have 2 different formats: one without padding
> > > (i386-style) and one with padding (x86_64-style).  The chance of more
> > > variants is small but who knows, maybe some other host architecture ABI
> > > has yet another alignment rule for uint64_t.
> > > 
> > > I'd like to git rm block/cow.c but I suppose the backwards-compatible
> > > thing to do is to introduce subformats to support both variants.
> > > Opinions?
> > 
> > Can we safely detect which of the subformats we have? But I'm not sure
> > if it's even worth fixing.
> 
> I think it would default to the subformat depending on the host
> architecture but allow overriding with -o subformat=i386|x86_64.

Hm, okay. If we can't do it automatically, that's an option, too.

> I'm also not sure if it's worth fixing.  The cow file format is so
> rarely used I wonder if we'd be better off without it.

It has never been used as a native qemu image format. As I understand
it, its use case is compatibility with existing images, and one of the
great things about qemu-img is that it can open more or less any random
image that you get from somewhere. The exotic formats are part of this.

If we wanted to simplify our code, we could probably make it read-only
(at the cost of losing qemu-iotests support), but then it's so simple
that leaving r/w support around won't hurt us.


Hm, actually, looking at the kernel (arch/um/drivers/cow_user.c), it
doesn't look as if we were compatible at all for v2, at least in the
current kernel version, we use a different backing file name length...
And they have the struct packed today, so we should probably follow
their example.

On the other hand, out of three versions, we only support v2, and we
don't use the same format as the kernel. Yeah, I guess we might just
drop the support then, it's not helpful for compatibility. The only
other way would be to update it to handle all three versions the same as
the kernel code.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Thu, Sep 04, 2014 at 04:10:14PM +0200, Kevin Wolf wrote:
>> Am 04.09.2014 um 15:51 hat Stefan Hajnoczi geschrieben:
>> > On Thu, Sep 04, 2014 at 06:07:32AM -0600, Eric Blake wrote:
>> > > On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote:
>> > > > On-disk structures should be marked packed so the compiler does not
>> > > > insert padding for field alignment.  Padding should be explicit so
>> > > > on-disk layout is obvious and we don't rely on the architecture-specific
>> > > > ABI for alignment rules.
>> > > > 
>> > > > The pahole(1) diff shows that the padding is now explicit and offsets
>> > > > are unchanged:
>> > > > 
>> > > >  	char backing_file[1024]; /* 8 1024 */
>> > > >  	/* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */
>> > > >  	int32_t mtime; /* 1032 4 */
>> > > > -
>> > > > -	/* XXX 4 bytes hole, try to pack */
>> > > > -
>> > > > + uint32_t padding; /* 1036 4 */
>> > > >  	uint64_t size; /* 1040 8 */
>> > > 
>> > > Was a 32-bit build also inserting this padding, or do we have historical
>> > > differences where 32-bit and 64-bit cow files are actually different,
>> > > and we may need to be prepared to parse files from both sources?
>> > 
>> > Good point.  Let's not merge this patch since it breaks 32-bit hosts.
>> > 
>> > The fact that no one hit problems when exchanging files between 32-bit
>> > and 64-bit machines shows that the cow format is rarely used.
>> > 
>> > At this point we have 2 different formats: one without padding
>> > (i386-style) and one with padding (x86_64-style).  The chance of more
>> > variants is small but who knows, maybe some other host architecture ABI
>> > has yet another alignment rule for uint64_t.
>> > 
>> > I'd like to git rm block/cow.c but I suppose the backwards-compatible
>> > thing to do is to introduce subformats to support both variants.
>> > Opinions?
>> 
>> Can we safely detect which of the subformats we have? But I'm not sure
>> if it's even worth fixing.
>
> I think it would default to the subformat depending on the host
> architecture but allow overriding with -o subformat=i386|x86_64.

Admirable dedication to bug-compatibility, but...

> I'm also not sure if it's worth fixing.  The cow file format is so
> rarely used I wonder if we'd be better off without it.

... good grief, nuke it already :)