Re: [Qemu-devel] [PATCH V9 2/4] raw, qcow2: don't convert file size to sector size

[Hu Tao]

On Thu, May 29, 2014 at 12:24:43AM +0200, Max Reitz wrote:
> On 27.05.2014 21:13, Eric Blake wrote:
> >On 05/27/2014 02:22 AM, Chen Fan wrote:
> >>From: Hu Tao <address@hidden>
> >>
> >>and avoid converting it back later. And round up file size to nearest
> >>sector.
> >The fact that you started a sentence with "And" makes me wonder if this
> >should be two patches.
> >
> >>Signed-off-by: Hu Tao <address@hidden>
> >>---
> >>  block/qcow2.c     | 8 ++++----
> >>  block/raw-posix.c | 5 +++--
> >>  block/raw-win32.c | 5 +++--
> >>  3 files changed, 10 insertions(+), 8 deletions(-)
> >>
> >>@@ -1779,7 +1779,7 @@ static int qcow2_create(const char *filename, 
> >>QEMUOptionParameter *options,
> >>      /* Read out options */
> >>      while (options && options->name) {
> >>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> >>-            sectors = options->value.n / 512;
> >>+            size = (options->value.n + BDRV_SECTOR_SIZE) & 
> >>BDRV_SECTOR_MASK;
> >Probably better to use DIV_ROUND_UP for this.
> 
> Actually, these are not only candidates, but either *need* to use
> DIV_ROUND_UP or "options->value.n + BDRV_SECTOR_SIZE - 1" to be
> correct. Without "- 1", they are not rounding up, but rounding up
> and sometimes adding BDRV_SECTOR_SIZE on top of that.

Indeed. I realized this after seeing Eric's comment.

> 
> Max
> 
> >Also, I have mentioned several times that it appears that the qcow2 file
> >format is perfectly capable of representing a file size that is not a
> >sector or cluster multiple; it's just that the rest of the code base is
> >probably not well prepared to deal with that.  While I think that
> >rounding up instead of truncating is the RIGHT thing to do (and
> >therefore your patch is fixing a valid bug), I wonder if it is further
> >worth trying to represent the EXACT size requested by the user.  There's
> >probably a lot more followups.  If nothing else, I think that this
> >change (which corresponds to the sentence starting with "And" in your
> >commit message) deserves to be in its own patch, and accompanied by a
> >testsuite addition to prove we don't regress (whether we allow exact
> >size or always round up, the testsuite should at least prove that
> >'qemu-img create -f qcow2 foo.img 1234' should not truncate to a 1024
> >byte file, but should be at least 1234 bytes visible to the guest).

I doubt allowing exact size is worth of it since: 1. people usually
creates images way larger than sector, truncating the ending partial
sector won't cause any problem(actually I think people never noticed
the truncation); 2. qemu code operating on sectors dont' expect a
partial sector, it will probably create regressions.

But I agree the last sentence right before the closing bracket so I'd
prefer rounding up.

> >
> >>+++ b/block/raw-posix.c
> >>@@ -1252,7 +1252,8 @@ static int raw_create(const char *filename, 
> >>QEMUOptionParameter *options,
> >>      /* Read out options */
> >>      while (options && options->name) {
> >>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> >>-            total_size = options->value.n / BDRV_SECTOR_SIZE;
> >>+            total_size = (options->value.n + BDRV_SECTOR_SIZE) &
> >>+                BDRV_SECTOR_MASK;
> >Another candidate for DIV_ROUND_UP
> >
> >>+++ b/block/raw-win32.c
> >>@@ -489,7 +489,8 @@ static int raw_create(const char *filename, 
> >>QEMUOptionParameter *options,
> >>      /* Read out options */
> >>      while (options && options->name) {
> >>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> >>-            total_size = options->value.n / 512;
> >>+            total_size = (options->value.n + BDRV_SECTOR_SIZE) &
> >>+                BDRV_SECTOR_MASK;
> >And again.
> >