[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V9 2/4] raw, qcow2: don't convert file size to s
From: |
Hu Tao |
Subject: |
Re: [Qemu-devel] [PATCH V9 2/4] raw, qcow2: don't convert file size to sector size |
Date: |
Thu, 29 May 2014 09:52:04 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
> >
[Qemu-devel] [PATCH V9 2/4] raw, qcow2: don't convert file size to sector size, Chen Fan, 2014/05/27
[Qemu-devel] [PATCH V9 4/4] qcow2: Add full image preallocation option, Chen Fan, 2014/05/27