diff mbox

[05/16] block/parallels: support padded Parallels images

Message ID 1418632081-20667-6-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Dec. 15, 2014, 8:27 a.m. UTC 
Unfortunately, old guest OSes do not align partitions to page size by
default. This is true for Windows 2003 and Windows XP.

For the time being Parallels was created an optimization for such OSes
in its desktop product. Desktop users are not qualified enough to create
properly aligned installations. Thus Parallels makes a blind guess
on a customer behalf and creates so-called "padded" images if guest
OS type is specified as WinXP, Win2k and Win2k3.

"Padding" is a value which should be added to guest LBA to obtain
sector number inside the image. This results in a shifted images.
   0123        offset inside image (in 512 byte sectors)
  +-------
  +.012        guest data (512 byte sectors)
  +-------
The information about this is available in DiskDescriptor.xml ONLY. There
is no such data in the image header.

There share of such images could be evaluated as 6-8% according to the
statistics in my hands.

This patch obtains proper value from XML and applies it on reading.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Acked-by: Roman Kagan <rkagan@parallels.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Kevin Wolf Dec. 15, 2014, 11:05 a.m. UTC | #1 
Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> Unfortunately, old guest OSes do not align partitions to page size by
> default. This is true for Windows 2003 and Windows XP.
> 
> For the time being Parallels was created an optimization for such OSes
> in its desktop product. Desktop users are not qualified enough to create
> properly aligned installations. Thus Parallels makes a blind guess
> on a customer behalf and creates so-called "padded" images if guest
> OS type is specified as WinXP, Win2k and Win2k3.
> 
> "Padding" is a value which should be added to guest LBA to obtain
> sector number inside the image. This results in a shifted images.
>    0123        offset inside image (in 512 byte sectors)
>   +-------
>   +.012        guest data (512 byte sectors)
>   +-------
> The information about this is available in DiskDescriptor.xml ONLY. There
> is no such data in the image header.
> 
> There share of such images could be evaluated as 6-8% according to the
> statistics in my hands.
> 
> This patch obtains proper value from XML and applies it on reading.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index c22b91b..fedb009 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -63,6 +63,7 @@ typedef struct BDRVParallelsState {
>      unsigned int tracks;
>  
>      unsigned int off_multiplier;
> +    unsigned int padding;
>  } BDRVParallelsState;
>  
>  
> @@ -235,6 +236,7 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>      const char *data;
>      char image_path[PATH_MAX];
>      Error *local_err = NULL;
> +    BDRVParallelsState *s = bs->opaque;
>  
>      ret = size = bdrv_getlength(bs->file);
>      if (ret < 0) {
> @@ -264,6 +266,19 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>      if (root == NULL) {
>          goto fail;
>      }
> +
> +    data = xml_get_text(root, "Disk_Parameters", "Padding", NULL);
> +    if (data != NULL) {
> +        char *endptr;
> +        unsigned long pad;
> +
> +        pad = strtoul(data, &endptr, 0);
> +        if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) {

Can endptr even be NULL? Also, shouldn't you set errno = 0 before and
check it here?

> +            goto fail;
> +        }
> +        s->padding = (uint32_t)pad;

s->padding is unsigned int, pad is unsigned long. Why the cast to
uint32_t here, which is different from both?

> +    }
> +
>      image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
>      data = ""; /* make gcc happy */
>      for (size = 0; image != NULL; image = image->next) {
> @@ -365,6 +380,10 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>  static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>                      uint8_t *buf, int nb_sectors)
>  {
> +    BDRVParallelsState *s = bs->opaque;
> +
> +    sector_num += s->padding;

No check needed here? bdrv_check_request() has checked sector_num and
nb_sectors against the image size, but now you can't rely on the result
any more.

>      while (nb_sectors > 0) {
>          int64_t position = seek_to_sector(bs, sector_num);
>          if (position >= 0) {

Kevin
Denis V. Lunev Dec. 15, 2014, 11:33 a.m. UTC | #2 
On 15/12/14 14:05, Kevin Wolf wrote:
> Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
>> Unfortunately, old guest OSes do not align partitions to page size by
>> default. This is true for Windows 2003 and Windows XP.
>>
>> For the time being Parallels was created an optimization for such OSes
>> in its desktop product. Desktop users are not qualified enough to create
>> properly aligned installations. Thus Parallels makes a blind guess
>> on a customer behalf and creates so-called "padded" images if guest
>> OS type is specified as WinXP, Win2k and Win2k3.
>>
>> "Padding" is a value which should be added to guest LBA to obtain
>> sector number inside the image. This results in a shifted images.
>>     0123        offset inside image (in 512 byte sectors)
>>    +-------
>>    +.012        guest data (512 byte sectors)
>>    +-------
>> The information about this is available in DiskDescriptor.xml ONLY. There
>> is no such data in the image header.
>>
>> There share of such images could be evaluated as 6-8% according to the
>> statistics in my hands.
>>
>> This patch obtains proper value from XML and applies it on reading.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Acked-by: Roman Kagan <rkagan@parallels.com>
>> Reviewed-by: Jeff Cody <jcody@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   block/parallels.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index c22b91b..fedb009 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -63,6 +63,7 @@ typedef struct BDRVParallelsState {
>>       unsigned int tracks;
>>   
>>       unsigned int off_multiplier;
>> +    unsigned int padding;
>>   } BDRVParallelsState;
>>   
>>   
>> @@ -235,6 +236,7 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>>       const char *data;
>>       char image_path[PATH_MAX];
>>       Error *local_err = NULL;
>> +    BDRVParallelsState *s = bs->opaque;
>>   
>>       ret = size = bdrv_getlength(bs->file);
>>       if (ret < 0) {
>> @@ -264,6 +266,19 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
>>       if (root == NULL) {
>>           goto fail;
>>       }
>> +
>> +    data = xml_get_text(root, "Disk_Parameters", "Padding", NULL);
>> +    if (data != NULL) {
>> +        char *endptr;
>> +        unsigned long pad;
>> +
>> +        pad = strtoul(data, &endptr, 0);
>> +        if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) {
> Can endptr even be NULL? Also, shouldn't you set errno = 0 before and
> check it here?
got it, I think you are right
>> +            goto fail;
>> +        }
>> +        s->padding = (uint32_t)pad;
> s->padding is unsigned int, pad is unsigned long. Why the cast to
> uint32_t here, which is different from both?
ok
>> +    }
>> +
>>       image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
>>       data = ""; /* make gcc happy */
>>       for (size = 0; image != NULL; image = image->next) {
>> @@ -365,6 +380,10 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
>>   static int parallels_read(BlockDriverState *bs, int64_t sector_num,
>>                       uint8_t *buf, int nb_sectors)
>>   {
>> +    BDRVParallelsState *s = bs->opaque;
>> +
>> +    sector_num += s->padding;
> No check needed here? bdrv_check_request() has checked sector_num and
> nb_sectors against the image size, but now you can't rely on the result
> any more.
you are perfectly correct. I'll recheck the situation.
I have seen some dances with image size. Allowed
image size should be extended with 1 block, but
I am not quite sure.

Thank you for pointing this out.

>>       while (nb_sectors > 0) {
>>           int64_t position = seek_to_sector(bs, sector_num);
>>           if (position >= 0) {
> Kevin
diff mbox

Patch

diff --git a/block/parallels.c b/block/parallels.c
index c22b91b..fedb009 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -63,6 +63,7 @@  typedef struct BDRVParallelsState {
     unsigned int tracks;
 
     unsigned int off_multiplier;
+    unsigned int padding;
 } BDRVParallelsState;
 
 
@@ -235,6 +236,7 @@  static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
     const char *data;
     char image_path[PATH_MAX];
     Error *local_err = NULL;
+    BDRVParallelsState *s = bs->opaque;
 
     ret = size = bdrv_getlength(bs->file);
     if (ret < 0) {
@@ -264,6 +266,19 @@  static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
     if (root == NULL) {
         goto fail;
     }
+
+    data = xml_get_text(root, "Disk_Parameters", "Padding", NULL);
+    if (data != NULL) {
+        char *endptr;
+        unsigned long pad;
+
+        pad = strtoul(data, &endptr, 0);
+        if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) {
+            goto fail;
+        }
+        s->padding = (uint32_t)pad;
+    }
+
     image = xml_seek(root, "StorageData", "Storage", "Image", NULL);
     data = ""; /* make gcc happy */
     for (size = 0; image != NULL; image = image->next) {
@@ -365,6 +380,10 @@  static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
 static int parallels_read(BlockDriverState *bs, int64_t sector_num,
                     uint8_t *buf, int nb_sectors)
 {
+    BDRVParallelsState *s = bs->opaque;
+
+    sector_num += s->padding;
+
     while (nb_sectors > 0) {
         int64_t position = seek_to_sector(bs, sector_num);
         if (position >= 0) {