Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Something is likely wrong when calculating the minimum block count #10

Closed
romainguinot opened this issue Jul 28, 2012 · 4 comments
Closed
Labels

Comments

@romainguinot
Copy link

Hi,

Thanks for the tool it is useful !
I noticed however that the minimum block count was wrong in my case, it wouldn't allow me to shrink a virtual disk size.
I commented out that check and everything worked out OK. See description below :

-- tests on Jul 28,2012 :

romain@host:[/opt/vboxes]$ vidma test.vdi 8448
Recognized file format:
        Virtual Disk Image (vdi)

Requested disk resize
from                130047 block(s)
to                    8448 block(s)
(each block has    1048576 bytes)
But minimal possible block count equals
                4294967295 block(s)
Resize aborted.

comment out that check in vdi.c :

diff --git a/vdi.c b/vdi.c
index 5cfc600..652c795 100644
--- a/vdi.c
+++ b/vdi.c
@@ -322,12 +322,12 @@ static int resize_confirmation(vdi_start_t *vdi, int fin, int fout,
        if (vdi->header.type == VDI_DYNAMIC) {
                find_last_blocks(vdi, fin, &last_blk_no, &last_blk_pos);
                min_blk_count = max_u32(last_blk_no, last_blk_pos) + 1;
-               if (new_blk_count < min_blk_count) {
-                       ui->log("But minimal possible block count equals\n"
-                               "     %21u block(s)\n",
-                               min_blk_count);
-                       return FAILURE;
-               }
+//             if (new_blk_count < min_blk_count) {
+//                     ui->log("But minimal possible block count equals\n"
+//                             "     %21u block(s)\n",
+//                             min_blk_count);
+//                     return FAILURE;
+//             }
        }

        ui->log("\nDisk size will change\n"

works out OK :

Recognized file format:
        Virtual Disk Image (vdi)

Requested disk resize
from                130047 block(s)
to                    8448 block(s)
(each block has    1048576 bytes)

Disk size will change
from          136364163072 bytes (         130047 MB)
to              8858370048 bytes (           8448 MB)

Image size will change
from            1453850624 bytes (           1386 MB)
to              1453850624 bytes (           1386 MB)

Resize operation in fact will create resized copy of the image.
NOTE    UUID of the new image will be the same as old one.
NOTE    Input file is safe and won't be modified.

Are you sure you want to continue? (y/N) y

Operation: Resize
[1/4] Copying blocks: 1386/1386 Done
[1/4] Syncing
[1/4] Data copied (1386 blocks in 4734 ms = ~306 B/us)
[2/4] Updating block allocation map: Done
[2/4] Syncing
[3/4] Updating file size: Done
[3/4] Syncing
[4/4] Updating header: Done
[4/4] Syncing
Operation finished

header.offset.data               = 00080000 524288
header.disk.size                 = 0000000210000000 8858370048
header.disk.blk_count            = 00002100 8448
header.disk.blk_count_alloc      = 0000056a 1386
header.lchs.cylinders            = 00000000 0
header.lchs.heads                = 00000000 0
header.lchs.sectors              = 00000000 0

The VM worked fine afterwards.
I had prior to the resize defragmented and shrunk the vm partition of the windows guest down with gparted live CD, to the size that i then requested with vidma.

Something must be wrong in find_last_blocks, but i have not investigated further.
Let me know what you think,

Regards,
Romain.

przemoc added a commit that referenced this issue Jul 28, 2012
Bug discovered thanks to Romain Guinot (issue #10).
@przemoc
Copy link
Owner

przemoc commented Jul 28, 2012

You're right, Romain. Thanks for detailed report (*) that allowed me to find a blatant omission in the code!

I've already committed a fix: 7903b07.

You have to know that it can still don't allow to shrink as much as you did, because zero blocks are data too, so I cannot treat them as garbage, because I have no way to know the real origin of these zeroes. And vidma doesn't allow you to resize dynamic VDI in a way that could destroy part your data.

In future there will be a way to enforce such dangerous operation.

(*) Still, could be better. ;-)
You didn't show all the information I asked in README file. Especially you didn't show the original file header information.

@przemoc przemoc closed this as completed Jul 28, 2012
@romainguinot
Copy link
Author

Hi,

Thanks for the update.
As for the additional info, for reference here it is :

distribution :

Linux host 3.4.4-5.fc17.x86_64 #1 SMP Thu Jul 5 20:20:59 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
Description:    Fedora release 17 (Beefy Miracle)
Release:        17
Codename:       BeefyMiracle

cc -v :

Using built-in specs.
COLLECT_GCC=cc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.7.0/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --disable-build-with-cxx --disable-build-poststage1-with-cxx --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 4.7.0 20120507 (Red Hat 4.7.0-5) (GCC) 

For vidma versions i tried the latest release 0.0.3b and git.9478881.

Thanks for your explanations.
I tried it again on the same .vdi with your updates and seems OK now :

vidma - Virtual Disks Manipulator, v0.0.3b-9-ga48c5a5
(C) 2009-2011 Przemyslaw Pawelczyk

By the way here you may want to add 2012 in the copyright notice :)

Recognized file format:
        Virtual Disk Image (vdi)

Requested disk resize
from                 16384 block(s)
to                    8192 block(s)
(each block has    1048576 bytes + 0 extra bytes)
But minimal possible block count equals
                     16371 block(s)
Resize aborted.

Do you think it would make sense to add a --force option here, if the user knows they are only zeroes ?
Just out of curiosity, do you know why there is a 13 (16384 - 16371) block difference in what is allowed ? in that .vdi, zeroes currently start at roughly 60% of that 8GB virtual partition.

Let me know what you think,
Thanks,
Romain.

@przemoc
Copy link
Owner

przemoc commented Jul 29, 2012

By the way here you may want to add 2012 in the copyright notice :)

Yeah, I should.

Do you think it would make sense to add a --force option here, if the user knows they are only zeroes ?

TODOs #4 and #5 roughly show what I am aiming for in terms of flexibility. So yes, resize operation will have an optional --force parameter to allow performing of dangerous shrinking and losing some data in the end.

But again, it's not about knowing that there are only zeroes, because vidma can know it too. It's about knowing that these zeroes (or whatever is at the end) are something we can get rid of without any harm. And only user can know it. Even assuming that I would implement understanding of partition table in vidma, and I'm really reluctant to do so (actually I'm not even considering it, because vidma is not intended to fiddle with partitions, as it's other level of abstraction), it wouldn't change a thing, because user can keep some data beyond last partition and there is no way to detect whether these data are important or e.g. leftovers from deleting a partition no longer needed.

It's worth to add that relying solely on blocks with data (allocated or zeroes) is not enough, because filesystem format (rarely modifying whole partition nowadays) could simply not modify the last sector of the partition and similarly user's files could not reach that point either yet. In such case you can shrink the image more than you should, making last partition(s) geometry no longer correct, breaking file system size metadata (if stored), and so on... literally asking for lots of trouble. But unless you use such image and make the damage running it under VM, this image image is perfectly fine in terms of data stored in it and can be "cured" by enlarging it to the point where last partition ends.

Just out of curiosity, do you know why there is a 13 (16384 - 16371) block difference in what is allowed ?

Because 16370th block (counting goes from 0) has some data (possibly leftovers from before partition was resized, e.g. some file systems store super-blocks/metadata in many copies and one of them can be near the end of the file system).

Below you have a patch that treats unallocated zero-filled blocks just like other unallocated blocks. If you must, use this, but with extreme care, remembering about all the things I mentioned here.

diff --git a/vdi.c b/vdi.c
index 77782b3..73c8a17 100644
--- a/vdi.c
+++ b/vdi.c
@@ -288,9 +288,9 @@ static void find_last_blocks(vdi_start_t *vdi, int fd,
                         min_u64(VDI_BAM_SIZE((uint64_t)blocks), _1MB)) /
                    VDI_BAM_ENTRY_SIZE;
                for (i = 0; i < n; i++)
-                       if (bam[i] != VDI_BLK_NONE) {
+                       if (bam[i] != VDI_BLK_NONE && bam[i] != VDI_BLK_ZERO) {
                                no = i;
-                               if (bam[i] != VDI_BLK_ZERO && pos < bam[i])
+                               if (pos < bam[i])
                                        pos = bam[i];
                        }
        }

P.S. You still haven't shown your image header information (even though I mentioned it explicitly in my previous comment), i.e. the output of vidma YOUR_IMAGE_FILE. Not that it's important at this point anymore, just saying. :-)

@romainguinot
Copy link
Author

Hi,

Many thanks for the details, speed of response, and thanks again for writing the tool.

For reference, here is the output of vidma test.vdi :

vidma test.vdi :

Recognized file format:
        Virtual Disk Image (vdi)

pre.file_info                    = <<< Oracle VM VirtualBox Disk Image >>>

pre.signature                    = beda107f 3201962111
version                          = 00010001 65537
header.size                      = 00000190 400
header.type                      = 00000001 1 (dynamic)
header.flags                     = 00000000 0
header.offset.bam                = 00001000 4096
header.offset.data               = 00080000 524288
header.pchs.cylinders            = 00000000 0
header.pchs.heads                = 00000000 0
header.pchs.sectors              = 00000000 0
header.pchs.sector_size          = 00000200 512
header.dummy                     = 00000000 0
header.disk.size                 = 0000000200000000 8589934592
header.disk.blk_size             = 00100000 1048576
header.disk.blk_extra_data       = 00000000 0
header.disk.blk_count            = 00002000 8192
header.disk.blk_count_alloc      = 000006ae 1710
header.uuid.create               = 6deeec3f-fa09-4648-8d1e-f4de28d72c7e
header.uuid.modify               = 2876ed9d-3a66-4bd8-839a-eb640e04240b
header.uuid.linkage              = 00000000-0000-0000-0000-000000000000
header.uuid.parent_modify        = 00000000-0000-0000-0000-000000000000
header.lchs.cylinders            = 00000400 1024
header.lchs.heads                = 000000ff 255
header.lchs.sectors              = 0000003f 63
header.lchs.sector_size          = 00000200 512

fake test resize, just to test the minimal detected block count :

Recognized file format:
        Virtual Disk Image (vdi)

Requested disk resize
from                  8192 block(s)
to                       1 block(s)
(each block has    1048576 bytes + 0 extra bytes)
But minimal possible block count equals
                      5452 block(s)
Resize aborted.

The minimum block count seems correct (same value with or without the latest patch), and seems indeed to correspond to the last data block. also seems to be confirmed when viewing the defrag view.

(btw the resize output from the previous comment was from another test VM. the two outputs in this comment are for the same VM, the one that i resized in the original comment).

Cheers,
Romain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants