From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38907) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQGFf-00042L-MG for qemu-devel@nongnu.org; Tue, 24 Feb 2015 09:14:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQGFZ-0004tj-1a for qemu-devel@nongnu.org; Tue, 24 Feb 2015 09:14:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38302) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQGFY-0004tO-RJ for qemu-devel@nongnu.org; Tue, 24 Feb 2015 09:14:12 -0500 Message-ID: <54EC872C.8000206@redhat.com> Date: Tue, 24 Feb 2015 09:14:04 -0500 From: Max Reitz MIME-Version: 1.0 References: <1424701661-21241-1-git-send-email-pl@kamp.de> <1424701661-21241-5-git-send-email-pl@kamp.de> <54EB7887.7020805@redhat.com> <54EC1EFA.8050005@kamp.de> In-Reply-To: <54EC1EFA.8050005@kamp.de> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] block/vpc: make calculate_geometry spec conform List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , qemu-devel@nongnu.org Cc: kwolf@redhat.com, carnold@suse.com, jcody@redhat.com, stefanha@redhat.com On 2015-02-24 at 01:49, Peter Lieven wrote: > Am 23.02.2015 um 19:59 schrieb Max Reitz: >> On 2015-02-23 at 09:27, Peter Lieven wrote: >>> The VHD spec [1] allows for total_sectors of 65535 x 16 x 255 (~127GB) >>> represented by a CHS geometry. If total_sectors is greater >>> than 65535 x 16 x 255 this geometry is set as a maximum. >>> >>> Qemu, Hyper-V, VirtualBox and disk2vhd use this special geometry as an >>> indicator to use the image current size from the footer as disk size. >>> >>> This patch changes vpc_create to effectively calculate a CxHxS geometry >>> for the given image size if possible while rounding up if necessary. >>> If the image size is too big to be represented in CHS we set the >>> maximum >>> and write the exact requested image size into the footer. >>> >>> This partly reverts commit 258d2edb, but leaves support for >127G disks >>> intact. >>> >>> [1] >>> http://download.microsoft.com/download/f/f/e/ffef50a5-07dd-4cf8-aaa3-442c0673a029/Virtual%20Hard%20Disk%20Format%20Spec_10_18_06.doc >>> >>> Signed-off-by: Peter Lieven >>> --- >>> block/vpc.c | 45 ++++++++++++++++++++++++--------------------- >>> 1 file changed, 24 insertions(+), 21 deletions(-) >>> >>> diff --git a/block/vpc.c b/block/vpc.c >>> index 11d3c86..9c5301b 100644 >>> --- a/block/vpc.c >>> +++ b/block/vpc.c >>> @@ -46,6 +46,7 @@ enum vhd_type { >>> #define VHD_TIMESTAMP_BASE 946684800 >>> #define VHD_MAX_SECTORS (65535LL * 255 * 255) >>> +#define VHD_MAX_GEOMETRY (65535LL * 16 * 255) >>> // always big-endian >>> typedef struct vhd_footer { >>> @@ -218,7 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict >>> *options, int flags, >>> /* Images that have exactly the maximum geometry are probably >>> bigger and >>> * would be truncated if we adhered to the geometry for them. >>> Rely on >>> * footer->size for them. */ >>> - if (bs->total_sectors == 65535ULL * 16 * 255) { >>> + if (bs->total_sectors == VHD_MAX_GEOMETRY) { >>> bs->total_sectors = be64_to_cpu(footer->size) / >>> BDRV_SECTOR_SIZE; >>> } >>> @@ -642,26 +643,20 @@ static int calculate_geometry(int64_t >>> total_sectors, uint16_t* cyls, >>> { >>> uint32_t cyls_times_heads; >>> - /* Allow a maximum disk size of approximately 2 TB */ >>> - if (total_sectors > 65535LL * 255 * 255) { >>> - return -EFBIG; >>> - } >>> + total_sectors = MIN(total_sectors, VHD_MAX_GEOMETRY); >>> - if (total_sectors > 65535 * 16 * 63) { >>> + if (total_sectors > 65535LL * 16 * 63) { >> >> While it is >= in the specification, the "else" branch does work fine >> with the total_sectors = 65535 * 16 * 63 case. So I'm leaving it up >> to you whether to make it really really spec-conform or at least >> functionally spec-conform (although the spec says 16 heads, 16191 >> cylinders, and 255 sectors per cylinder for this case, whereas the >> "else" branch leaves us with 16 heads, 65535 cylinders, and 63 >> sectors per cylinder). > > I will adjust it to match the spec exactly. > >> >>> *secs_per_cyl = 255; >>> - if (total_sectors > 65535 * 16 * 255) { >>> - *heads = 255; >>> - } else { >>> - *heads = 16; >>> - } >>> + *heads = 16; >>> cyls_times_heads = total_sectors / *secs_per_cyl; >>> } else { >>> *secs_per_cyl = 17; >>> cyls_times_heads = total_sectors / *secs_per_cyl; >>> *heads = (cyls_times_heads + 1023) / 1024; >>> - if (*heads < 4) >>> + if (*heads < 4) { >>> *heads = 4; >>> + } >>> if (cyls_times_heads >= (*heads * 1024) || *heads > 16) { >>> *secs_per_cyl = 31; >>> @@ -817,19 +812,27 @@ static int vpc_create(const char *filename, >>> QemuOpts *opts, Error **errp) >>> * Calculate matching total_size and geometry. Increase the >>> number of >>> * sectors requested until we get enough (or fail). This >>> ensures that >>> * qemu-img convert doesn't truncate images, but rather rounds >>> up. >>> + * >>> + * If the image size can't be represented by a spec conform CHS >>> geometry, >>> + * we set the geometry to 65535 x 16 x 255 (CxHxS) sectors and use >>> + * the image size from the VHD footer to calculate total_sectors. >>> */ >>> - total_sectors = total_size / BDRV_SECTOR_SIZE; >>> + total_sectors = MIN(VHD_MAX_GEOMETRY, >>> + DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE)); >> >> DIV_ROUND_UP() is unnecessary, total_size is a multiple of >> BDRV_SECTOR_SIZE anyway. > > ok > >> >> I find the MIN() unnecessary, too, because calculate_geometry() will >> take care of that itself. > > Actually the MIN() is the guarantee that the for-loop terminates for > images with total_sectors > VHD_MAX_GEOMETRY. > Otherwise (int64_t)cyls * heads * secs_per_cyl would always be > VHD_MAX_GEOMETRY which is always > smaller than VHD_MAX_GEOMETRY. This would keep us looping for good. Right. Very well then. >> >>> for (i = 0; total_sectors > (int64_t)cyls * heads * >>> secs_per_cyl; i++) { >>> - if (calculate_geometry(total_sectors + i, &cyls, &heads, >>> - &secs_per_cyl)) >>> - { >>> - ret = -EFBIG; >>> - goto out; >>> - } >>> + calculate_geometry(total_sectors + i, &cyls, &heads, >>> &secs_per_cyl); >>> } >>> - total_sectors = (int64_t) cyls * heads * secs_per_cyl; >>> - total_size = total_sectors * BDRV_SECTOR_SIZE; >>> + if ((int64_t)cyls * heads * secs_per_cyl == VHD_MAX_GEOMETRY) { >>> + total_sectors = DIV_ROUND_UP(total_size, BDRV_SECTOR_SIZE); >> >> Again, DIV_ROUND_UP() is unnecessary. >> >> So this is equivalent to total_sectors = total_size / >> BDRV_SECTOR_SIZE, which, if you omit the MIN() above, is exactly what >> total_sectors already contains, so you can just omit this assignment. >> >>> + /* Allow a maximum disk size of approximately 2 TB */ >>> + if (total_sectors > VHD_MAX_SECTORS) { >>> + return -EFBIG; >> >> No goto out;? > > Oops. > >> >>> + } >>> + } else { >>> + total_sectors = (int64_t)cyls * heads * secs_per_cyl; >>> + total_size = total_sectors * BDRV_SECTOR_SIZE; >>> + } >> >> An alternative way of writing this would be: >> >> if ((int64_t)cyls * heads * secs_per_cyl < VHD_MAX_GEOMETRY) { >> total_sectors = (int64_t)cyls * heads * secs_per_cyl; >> } >> if (total_sectors > VHD_MAX_SECTORS) { >> ret = -EFBIG; >> goto out; >> } >> total_size = total_sectors * BDRV_SECTOR_SIZE; >> >> Or, alternatively, it'd be possible to drop the total_size assignment >> and just use total_sectors * BDRV_SECTOR_SIZE in the assignments of >> footer->orig_size and footer->size. > > Will look at this. Well, it won't work so well with the MIN() being necessary... Max