| Message ID | 1409821121-20645-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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>
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?
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
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
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
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
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
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 :)
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;
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(-)