On Fri, 11/21 11:56, Max Reitz wrote:
So, as for what I think we do need to do when shrinking (and keep in mind:
The offset given to qcow2_truncate() is the guest size! NOT the host image
size!):
(1) Determine the first L2 table and the first entry in the table which will
lie beyond the new guest disk size.
Here is not correct always. Due to the COW, using offset to calculate the
first entry of the first L2 table will be incorrect.
What I have done for this scenario:
(1) if the first entry is the first entry of the L2 table, so will scan "the
previous L2 table"("the previous L2 table" location is in front of "L2 table"
in L1 table). If the entry of previous L2 table is larger than offset, will
discard this entry, too.
(2) If the first entry is not the first entry of the L2 table, still to scan
the whole L2 table to make sure no entry is beyond offset.
(2) Discard all clusters beginning from there.
(3) Discard all L2 tables which are then completely empty.
(4) Update the header size.
For this patch current's realizion, have include above 4 steps I think.
Current patch, also have another step 5.
(5) truncate the file.
Here I think we also should add discard refcount table and refcount block
table when they are completely empty.
And that's it. We can either speed up step (2) by implementing it manually,
or we just use bdrv_discard() on the qcow2 BDS (in the simplest case:
bdrv_discard(bs, DIV_ROUND_UP(offset, BDRV_SECTOR_SIZE), bs->total_sectors -
DIV_ROUND_UP(offset, BDRV_SECTOR_SIZE));.
We can incorporate step (3) by extending qcow2_discard_clusters() to free L2
tables when they are empty after discard_single_l2(). But we don't even have
to that now. It's an optimization we can go about later.
So, we can do (1), (2) and (3) in a single step: Just one bdrv_discard()
call. But it's probably better to use qcow2_discard_clusters() instead and
set the full_discard parameter to true.
So: qcow2_discard_clusters(bs, offset, bs->total_sectors - offset /
BDRV_SECTOR_SIZE, true);. Then update the guest disk size field in the
header. And we're done.
There are four problems with this approach:
- qcow2_discard_clusters() might be slower than optimal. I personally don't
care at all.
- If "bs->total_sectors * BDRV_SECTOR_SIZE - offset" is greater than
INT_MAX, this won't work. Trivially solvable by encapsulating the
qcow2_discard_clusters() call in a loop which limits nb_clusters to INT_MAX
/ BDRV_SECTOR_SIZE.
- The L1 table is not resized. Should not matter in practice at all.
Yes, agree with you.
- The file is not truncated. Does not matter either (because holes in the
file are good enough), and we can't actually solve this problem without
defragmentation anyway.
There is one advantage:
- It's extremely simple. It's literally below ten lines of code.
I think the advantage far outweighs the disadvantage. But I may be wrong.
What do you think?
Hi max,
Sorry for so late to reply as I am so busy recently. I think let's have an
agreement on how to realize qcow2 shrinking first, then type code is better.